zkey: Protect from symlink-following attacks

Files in the zkey repository can be created by any member of the
'zkeyadm' group as well as 'root'. Such files are owned by the creator
and the 'zkeyadm' group, and allow read and write for the owner user
and owner group.

When creating or writing files inside the zkey repository, make sure
that the file is not a sysmlink. That way, only files within the zkey
repository are set to be read/write for the owner user and members of
the 'zkeyadm' group. Make sure to open such files with the 'O_NOFOLLOW'
flag, and use 'lstat()' to check for files and directories.

Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>

zkey/keystore.c

@@ -259,21 +259,30 @@ static void _keystore_free_key_filenames(struct key_filenames *names)
 static int _keystore_set_file_permission(struct keystore *keystore,
 					 const char *filename)
 {
-	int rc;
+	int fd, rc = 0;
 
-	if (chmod(filename, keystore->mode) != 0) {
+	fd = open(filename, O_RDONLY | O_NOFOLLOW);
+	if (fd < 0) {
+		rc = -errno;
+		warnx("Failed to open '%s': %s", filename, strerror(-rc));
+		return rc;
+	}
+
+	if (fchmod(fd, keystore->mode) != 0) {
 		rc = -errno;
 		warnx("chmod failed on file '%s': %s", filename, strerror(-rc));
-		return rc;
+		goto out;
 	}
 
-	if (chown(filename, geteuid(), keystore->owner) != 0) {
+	if (fchown(fd, geteuid(), keystore->owner) != 0) {
 		rc = -errno;
 		warnx("chown failed on file '%s': %s", filename, strerror(-rc));
-		return rc;
+		goto out;
 	}
 
-	return 0;
+out:
+	close(fd);
+	return rc;
 }
 
 /**
@@ -1476,8 +1485,8 @@ static int _keystore_lock_repository(struct keystore *keystore)
 	util_asprintf(&lock_file_name, "%s/%s", keystore->directory,
 		      LOCK_FILE_NAME);
 
-	if (stat(lock_file_name, &sb) == 0) {
-		keystore->lock_fd = open(lock_file_name, O_RDONLY);
+	if (lstat(lock_file_name, &sb) == 0) {
+		keystore->lock_fd = open(lock_file_name, O_RDONLY | O_NOFOLLOW);
 		if (keystore->lock_fd == -1) {
 			rc = -errno;
 			warnx("Failed to open lock file '%s': %s",
@@ -1486,7 +1495,8 @@ static int _keystore_lock_repository(struct keystore *keystore)
 			goto out;
 		}
 	} else {
-		keystore->lock_fd = open(lock_file_name, O_CREAT | O_RDONLY,
+		keystore->lock_fd = open(lock_file_name,
+					 O_CREAT | O_RDONLY | O_NOFOLLOW,
 					 keystore->mode);
 		if (keystore->lock_fd == -1) {
 			rc = -errno;
@@ -1561,7 +1571,7 @@ struct keystore *keystore_new(const char *directory,
 
 	util_assert(directory != NULL, "Internal error: directory is NULL");
 
-	if (stat(directory, &sb) != 0) {
+	if (lstat(directory, &sb) != 0) {
 		warnx("Can not access '%s': %s", directory, strerror(errno));
 		return NULL;
 	}

zkey/kms.c

@@ -382,7 +382,7 @@ static int _save_kms_properties(const struct keystore *keystore,
 				struct properties *kms_props, bool verbose)
 {
 	char *filename = NULL;
-	int rc;
+	int fd = -1, rc = 0;
 
 	util_assert(keystore != NULL, "Internal error: keystore is NULL");
 	util_assert(kms_props != NULL, "Internal error: kms_props is NULL");
@@ -398,19 +398,28 @@ static int _save_kms_properties(const struct keystore *keystore,
 		goto out;
 	}
 
-	if (chmod(filename, keystore->mode) != 0) {
+	fd = open(filename, O_RDONLY | O_NOFOLLOW);
+	if (fd < 0) {
+		rc = -errno;
+		warnx("Failed to open '%s': %s", filename, strerror(-rc));
+		return rc;
+	}
+
+	if (fchmod(fd, keystore->mode) != 0) {
 		rc = -errno;
 		warnx("chmod failed on file '%s': %s", filename, strerror(-rc));
-		return rc;
+		goto out;
 	}
 
-	if (chown(filename, geteuid(), keystore->owner) != 0) {
+	if (fchown(fd, geteuid(), keystore->owner) != 0) {
 		rc = -errno;
 		warnx("chown failed on file '%s': %s", filename, strerror(-rc));
-		return rc;
+		goto out;
 	}
-
 out:
+
+	if (fd >= 0)
+		close(fd);
 	if (filename != NULL)
 		free(filename);
 
@@ -609,7 +618,7 @@ int bind_kms_plugin(struct keystore *keystore, const char *plugin,
 	char *plugin_name = NULL;
 	void *plugin_lib = NULL;
 	bool created = false;
-	int rc;
+	int rc, fd;
 
 	util_assert(keystore != NULL, "Internal error: keystore is NULL");
 	util_assert(plugin != NULL, "Internal error: plugin is NULL");
@@ -649,20 +658,31 @@ int bind_kms_plugin(struct keystore *keystore, const char *plugin,
 	}
 	created = true;
 
-	if (chmod(config_dir, keystore->mode) != 0) {
+	fd = open(config_dir, O_RDONLY | O_NOFOLLOW);
+	if (fd < 0) {
+		rc = -errno;
+		warnx("Failed to open '%s': %s", config_dir, strerror(-rc));
+		goto out;
+	}
+
+	if (fchmod(fd, keystore->mode) != 0) {
 		rc = -errno;
 		warnx("chmod failed on directory '%s': %s", config_dir,
 		      strerror(-rc));
-		return rc;
+		close(fd);
+		goto out;
 	}
 
-	if (chown(config_dir, geteuid(), keystore->owner) != 0) {
+	if (fchown(fd, geteuid(), keystore->owner) != 0) {
 		rc = -errno;
 		warnx("chown failed on directory '%s': %s", config_dir,
 		      strerror(-rc));
-		return rc;
+		close(fd);
+		goto out;
 	}
 
+	close(fd);
+
 	if (funcs->kms_bind != NULL) {
 		rc = funcs->kms_bind(config_dir);
 		if (rc != 0) {

zkey/pkey.c

@@ -150,7 +150,7 @@ int write_secure_key(const char *keyfile, const u8 *secure_key,
 	util_assert(secure_key_size > 0,
 		    "Internal error: secure_key_size is zero");
 
-	fp = fopen(keyfile, "w");
+	fp = fopen_nofollow(keyfile, "w");
 	if (fp == NULL) {
 		warnx("File '%s': %s", keyfile, strerror(errno));
 		return -EIO;

zkey/plugin-utils.c

@@ -73,7 +73,7 @@ int plugin_init(struct plugin_data *pd, const char *plugin_name,
 
 	pr_verbose(pd, "Plugin initializing, config_path: '%s'", config_path);
 
-	if (stat(config_path, &sb) != 0) {
+	if (lstat(config_path, &sb) != 0) {
 		rc = -errno;
 		warnx("Can not access '%s': %s", config_path, strerror(-rc));
 		goto error;
@@ -231,23 +231,33 @@ out:
  */
 int plugin_set_file_permission(struct plugin_data *pd, const char *filename)
 {
-	int rc;
+	int fd, rc = 0;
 
-	if (chmod(filename, pd->config_path_mode) != 0) {
+	fd = open(filename, O_RDONLY | O_NOFOLLOW);
+	if (fd < 0) {
+		rc = -errno;
+		plugin_set_error(pd, "Failed to open '%s': %s", filename,
+				 strerror(-rc));
+		return rc;
+	}
+
+	if (fchmod(fd, pd->config_path_mode) != 0) {
 		rc = -errno;
 		plugin_set_error(pd, "chmod failed on file '%s': %s", filename,
 				 strerror(-rc));
-		return rc;
+		goto out;
 	}
 
-	if (chown(filename, geteuid(), pd->config_path_owner) != 0) {
+	if (fchown(fd, geteuid(), pd->config_path_owner) != 0) {
 		rc = -errno;
 		plugin_set_error(pd, "chown failed on file '%s': %s", filename,
 				 strerror(-rc));
-		return rc;
+		goto out;
 	}
 
-	return 0;
+out:
+	close(fd);
+	return rc;
 }
 
 /**

zkey/properties.c

@@ -22,6 +22,7 @@
 #include "lib/util_panic.h"
 
 #include "properties.h"
+#include "utils.h"
 
 struct properties {
 	struct util_list list;
@@ -312,7 +313,7 @@ int properties_save(struct properties *properties, const char *filename,
 	util_assert(properties != NULL, "Internal error: properties is NULL");
 	util_assert(filename != NULL, "Internal error: filename is NULL");
 
-	fp = fopen(filename, "w");
+	fp = fopen_nofollow(filename, "w");
 	if (fp == NULL)
 		return -EIO;
 

zkey/utils.c

@@ -1225,7 +1225,7 @@ int copy_file(const char *in_file_name, const char *out_file_name,
 		goto out;
 	}
 
-	fp_out = fopen(out_file_name, "w");
+	fp_out = fopen_nofollow(out_file_name, "w");
 	if (fp_out == NULL) {
 		rc = -errno;
 		warnx("Failed to open '%s': %s", out_file_name, strerror(-rc));
@@ -1366,7 +1366,7 @@ int store_passphrase_from_base64(const char *b64_string, const char *filename,
 		return -ENOMEM;
 	}
 
-	fp = fopen(filename, "w");
+	fp = fopen_nofollow(filename, "w");
 	if (fp == NULL) {
 		pr_verbose(verbose, "Open of file '%s' failed: %s", filename,
 			   strerror(errno));
@@ -1393,3 +1393,32 @@ out:
 	return rc;
 }
 
+FILE *fopen_nofollow(const char *path, const char *mode)
+{
+	int flags = O_NOFOLLOW;
+	int fd;
+	FILE *fp;
+
+	/* Determine flags based on mode */
+	if (mode[0] == 'r')
+		flags |= (mode[1] == '+') ? O_RDWR : O_RDONLY;
+	else if (mode[0] == 'w')
+		flags |= O_CREAT | O_TRUNC |
+				((mode[1] == '+') ? O_RDWR : O_WRONLY);
+	else if (mode[0] == 'a')
+		flags |= O_CREAT | O_APPEND |
+				((mode[1] == '+') ? O_RDWR : O_WRONLY);
+	else
+		return NULL;
+
+	fd = open(path, flags, 0600);
+	if (fd < 0)
+		return NULL;
+
+	fp = fdopen(fd, mode);
+	if (fp == NULL) {
+		close(fd);
+		return NULL;
+	}
+	return fp;
+}

zkey/utils.h

@@ -84,4 +84,6 @@ char *read_passphrase_as_base64(const char *filename, bool verbose);
 int store_passphrase_from_base64(const char *hex_string, const char *filename,
 				 bool verbose);
 
+FILE *fopen_nofollow(const char *path, const char *mode);
+
 #endif