CVE-2022-3287: Files or Directories Accessible to External Parties (CWE-552)
value: When creating an OPERATOR user account on the BMC, the redfish plugin saved the auto-generated password to /etc/fwupd/redfish.conf without proper restriction, allowing any user on the system to read the same configuration file.
CWE description: The product makes files or directories accessible to unauthorized actors, even though they should not be.
CVSS v3 base score: 6.5
Published: 2022-09-28T20:15Z
Repository: https://github.com/fwupd/fwupd
Language: C
Files changed: contrib/fwupd.spec.in, libfwupdplugin/fu-plugin.c, libfwupdplugin/fu-self-test.c
Commit date: 2022-09-22 14:46:44 +0100
Commit message: Never save the Redfish passwords to a file readable by users

When the redfish plugin automatically creates an OPERATOR user account on the
BMC we save the autogenerated password to /etc/fwupd/redfish.conf, ensuring it
is chmod'ed to 0660 before writing the file with g_key_file_save_to_file().

Under the covers, g_key_file_save_to_file() calls g_file_set_contents() with
the keyfile string data.
I was under the impression that G_FILE_CREATE_REPLACE_DESTINATION was being
used to copy permissions, but alas not.

GLib instead calls g_file_set_contents_full() with the mode hardcoded to 0666,
which undoes the previous chmod().

Use g_file_set_contents_full() with the correct mode for newer GLib versions,
and provide a fallback with the same semantics for older versions.

Diff: diff --git a/contrib/fwupd.spec.in b/contrib/fwupd.spec.in
index b011292b1..42ea2024a 100644
--- a/contrib/fwupd.spec.in
+++ b/contrib/fwupd.spec.in
@@ -324,10 +324,13 @@ for fn in /etc/fwupd/remotes.d/*.conf; do
     if grep -q "Keyring=gpg" "$fn"; then
         sed -i 's/Keyring=gpg/#Keyring=pkcs/g' "$fn";
     fi
 done
 
+# ensure this is private
+chmod 0660 /etc/fwupd/redfish.conf
+
 %preun
 %systemd_preun fwupd.service
 
 %postun
 %systemd_postun_with_restart fwupd.service
diff --git a/libfwupdplugin/fu-plugin.c b/libfwupdplugin/fu-plugin.c
index 9744af9d6..b431f6d41 100644
--- a/libfwupdplugin/fu-plugin.c
+++ b/libfwupdplugin/fu-plugin.c
@@ -7,10 +7,11 @@
 #define G_LOG_DOMAIN "FuPlugin"
 
 #include "config.h"
 
 #include <errno.h>
+#include <fcntl.h>
 #include <fwupd.h>
 #include <glib/gstdio.h>
 #include <gmodule.h>
 #include <string.h>
 #include <unistd.h>
@@ -2415,10 +2416,50 @@ fu_plugin_set_config_value(FuPlugin *self, const gchar *key, const gchar *value,
 		return FALSE;
 	g_key_file_set_string(keyfile, fu_plugin_get_name(self), key, value);
 	return g_key_file_save_to_file(keyfile, conf_path, error);
 }
 
+#if !GLIB_CHECK_VERSION(2, 66, 0)
+
+#define G_FILE_SET_CONTENTS_CONSISTENT 0
+typedef guint GFileSetContentsFlags;
+static gboolean
+g_file_set_contents_full(const gchar *filename,
+			 const gchar *contents,
+			 gssize length,
+			 GFileSetContentsFlags flags,
+			 int mode,
+			 GError **error)
+{
+	gint fd;
+	gssize wrote;
+
+	if (length < 0)
+		length = strlen(contents);
+	fd = g_open(filename, O_CREAT, mode);
+	if (fd <= 0) {
+		g_set_error(error,
+			    G_IO_ERROR,
+			    G_IO_ERROR_FAILED,
+			    "could not open %s file",
+			    filename);
+		return FALSE;
+	}
+	wrote = write(fd, contents, length);
+	if (wrote != length) {
+		g_set_error(error,
+			    G_IO_ERROR,
+			    G_IO_ERROR_FAILED,
+			    "did not write %s file",
+			    filename);
+		g_close(fd, NULL);
+		return FALSE;
+	}
+	return g_close(fd, error);
+}
+#endif
+
 /**
  * fu_plugin_set_secure_config_value:
  * @self: a #FuPlugin
  * @key: a settings key
  * @value: (nullable): a settings value
@@ -2436,30 +2477,32 @@ fu_plugin_set_secure_config_value(FuPlugin *self,
 				  const gchar *key,
 				  const gchar *value,
 				  GError **error)
 {
 	g_autofree gchar *conf_path = fu_plugin_get_config_filename(self);
-	gint ret;
+	g_autofree gchar *data = NULL;
+	g_autoptr(GKeyFile) keyfile = g_key_file_new();
 
 	g_return_val_if_fail(FU_IS_PLUGIN(self), FALSE);
 	g_return_val_if_fail(error == NULL || *error == NULL, FALSE);
 
 	if (!g_file_test(conf_path, G_FILE_TEST_EXISTS)) {
 		g_set_error(error, FWUPD_ERROR, FWUPD_ERROR_NOT_FOUND, "%s is missing", conf_path);
 		return FALSE;
 	}
-	ret = g_chmod(conf_path, 0660);
-	if (ret == -1) {
-		g_set_error(error,
-			    FWUPD_ERROR,
-			    FWUPD_ERROR_INTERNAL,
-			    "failed to set permissions on %s",
-			    conf_path);
+	if (!g_key_file_load_from_file(keyfile, conf_path, G_KEY_FILE_KEEP_COMMENTS, error))
 		return FALSE;
-	}
-
-	return fu_plugin_set_config_value(self, key, value, error);
+	g_key_file_set_string(keyfile, fu_plugin_get_name(self), key, value);
+	data = g_key_file_to_data(keyfile, NULL, error);
+	if (data == NULL)
+		return FALSE;
+	return g_file_set_contents_full(conf_path,
+					data,
+					-1,
+					G_FILE_SET_CONTENTS_CONSISTENT,
+					0660,
+					error);
 }
 
 /**
  * fu_plugin_get_config_value_boolean:
  * @self: a #FuPlugin
diff --git a/libfwupdplugin/fu-self-test.c b/libfwupdplugin/fu-self-test.c
index 2dbc9c94f..aaf49c172 100644
--- a/libfwupdplugin/fu-self-test.c
+++ b/libfwupdplugin/fu-self-test.c
@@ -672,10 +672,66 @@ _plugin_device_added_cb(FuPlugin *plugin, FuDevice *device, gpointer user_data)
 	FuDevice **dev = (FuDevice **)user_data;
 	*dev = g_object_ref(device);
 	fu_test_loop_quit();
 }
 
+static void
+fu_plugin_config_func(void)
+{
+	GStatBuf statbuf = {0};
+	gboolean ret;
+	gint rc;
+	g_autofree gchar *conf_dir = NULL;
+	g_autofree gchar *conf_file = NULL;
+	g_autofree gchar *fn = NULL;
+	g_autofree gchar *testdatadir = NULL;
+	g_autofree gchar *value = NULL;
+	g_autoptr(FuPlugin) plugin = fu_plugin_new(NULL);
+	g_autoptr(GError) error = NULL;
+
+	/* this is a build file */
+	testdatadir = g_test_build_filename(G_TEST_BUILT, "tests", NULL);
+	(void)g_setenv("FWUPD_SYSCONFDIR", testdatadir, TRUE);
+	conf_dir = fu_path_from_kind(FU_PATH_KIND_SYSCONFDIR_PKG);
+
+	/* remove existing file */
+	fu_plugin_set_name(plugin, "test");
+	conf_file = g_strdup_printf("%s.conf", fu_plugin_get_name(plugin));
+	fn = g_build_filename(conf_dir, conf_file, NULL);
+	ret = fu_path_mkdir_parent(fn, &error);
+	g_assert_no_error(error);
+	g_assert_true(ret);
+	g_remove(fn);
+	ret = g_file_set_contents(fn, "", -1, &error);
+	g_assert_no_error(error);
+	g_assert_true(ret);
+
+	/* set a value */
+	ret = fu_plugin_set_config_value(plugin, "Key", "True", &error);
+	g_assert_no_error(error);
+	g_assert_true(ret);
+	g_assert_true(g_file_test(fn, G_FILE_TEST_EXISTS));
+
+	/* check it is world readable */
+	rc = g_stat(fn, &statbuf);
+	g_assert_cmpint(rc, ==, 0);
+	g_assert_cmpint(statbuf.st_mode & 0777, ==, 0644);
+
+	/* read back the value */
+	value = fu_plugin_get_config_value(plugin, "Key");
+	g_assert_cmpstr(value, ==, "True");
+	g_assert_true(fu_plugin_get_config_value_boolean(plugin, "Key"));
+
+	/* check it is private, i.e. only readable by the user/group */
+	ret = fu_plugin_set_secure_config_value(plugin, "Key", "False", &error);
+	g_assert_no_error(error);
+	g_assert_true(ret);
+	rc = g_stat(fn, &statbuf);
+	g_assert_cmpint(rc, ==, 0);
+	g_assert_cmpint(statbuf.st_mode & 0777, ==, 0640);
+}
+
 static void
 fu_plugin_devices_func(void)
 {
 	g_autoptr(FuDevice) device = fu_device_new(NULL);
 	g_autoptr(FuDevice) child = fu_device_new(NULL);
@@ -3596,10 +3652,11 @@ main(int argc, char **argv)
 	g_test_add_func("/fwupd/progress{parent-1-step}", fu_progress_parent_one_step_proxy_func);
 	g_test_add_func("/fwupd/progress{no-equal}", fu_progress_non_equal_steps_func);
 	g_test_add_func("/fwupd/progress{finish}", fu_progress_finish_func);
 	g_test_add_func("/fwupd/bios-attrs{load}", fu_bios_settings_load_func);
 	g_test_add_func("/fwupd/security-attrs{hsi}", fu_security_attrs_hsi_func);
+	g_test_add_func("/fwupd/plugin{config}", fu_plugin_config_func);
 	g_test_add_func("/fwupd/plugin{devices}", fu_plugin_devices_func);
 	g_test_add_func("/fwupd/plugin{device-inhibit-children}",
 			fu_plugin_device_inhibit_children_func);
 	g_test_add_func("/fwupd/plugin{delay}", fu_plugin_delay_func);
 	g_test_add_func("/fwupd/plugin{quirks}", fu_plugin_quirks_func);

Vulnerable code: 	gint ret;
	ret = g_chmod(conf_path, 0660);
	if (ret == -1) {
		g_set_error(error,
			    FWUPD_ERROR,
			    FWUPD_ERROR_INTERNAL,
			    "failed to set permissions on %s",
			    conf_path);
	}

	return fu_plugin_set_config_value(self, key, value, error);
Fixed code: # ensure this is private
chmod 0660 /etc/fwupd/redfish.conf

#include <fcntl.h>
#if !GLIB_CHECK_VERSION(2, 66, 0)

#define G_FILE_SET_CONTENTS_CONSISTENT 0
typedef guint GFileSetContentsFlags;
static gboolean
g_file_set_contents_full(const gchar *filename,
			 const gchar *contents,
			 gssize length,
			 GFileSetContentsFlags flags,
			 int mode,
			 GError **error)
{
	gint fd;
	gssize wrote;

	if (length < 0)
		length = strlen(contents);
	fd = g_open(filename, O_CREAT, mode);
	if (fd <= 0) {
		g_set_error(error,
			    G_IO_ERROR,
			    G_IO_ERROR_FAILED,
			    "could not open %s file",
			    filename);
		return FALSE;
	}
	wrote = write(fd, contents, length);
	if (wrote != length) {
		g_set_error(error,
			    G_IO_ERROR,
			    G_IO_ERROR_FAILED,
			    "did not write %s file",
			    filename);
		g_close(fd, NULL);
		return FALSE;
	}
	return g_close(fd, error);
}
#endif

	g_autofree gchar *data = NULL;
	g_autoptr(GKeyFile) keyfile = g_key_file_new();
	if (!g_key_file_load_from_file(keyfile, conf_path, G_KEY_FILE_KEEP_COMMENTS, error))
	g_key_file_set_string(keyfile, fu_plugin_get_name(self), key, value);
	data = g_key_file_to_data(keyfile, NULL, error);
	if (data == NULL)
		return FALSE;
	return g_file_set_contents_full(conf_path,
					data,
					-1,
					G_FILE_SET_CONTENTS_CONSISTENT,
					0660,
					error);
static void
fu_plugin_config_func(void)
{
	GStatBuf statbuf = {0};
	gboolean ret;
	gint rc;
	g_autofree gchar *conf_dir = NULL;
	g_autofree gchar *conf_file = NULL;
	g_autofree gchar *fn = NULL;
	g_autofree gchar *testdatadir = NULL;
	g_autofree gchar *value = NULL;
	g_autoptr(FuPlugin) plugin = fu_plugin_new(NULL);
	g_autoptr(GError) error = NULL;

	/* this is a build file */
	testdatadir = g_test_build_filename(G_TEST_BUILT, "tests", NULL);
	(void)g_setenv("FWUPD_SYSCONFDIR", testdatadir, TRUE);
	conf_dir = fu_path_from_kind(FU_PATH_KIND_SYSCONFDIR_PKG);

	/* remove existing file */
	fu_plugin_set_name(plugin, "test");
	conf_file = g_strdup_printf("%s.conf", fu_plugin_get_name(plugin));
	fn = g_build_filename(conf_dir, conf_file, NULL);
	ret = fu_path_mkdir_parent(fn, &error);
	g_assert_no_error(error);
	g_assert_true(ret);
	g_remove(fn);
	ret = g_file_set_contents(fn, "", -1, &error);
	g_assert_no_error(error);
	g_assert_true(ret);

	/* set a value */
	ret = fu_plugin_set_config_value(plugin, "Key", "True", &error);
	g_assert_no_error(error);
	g_assert_true(ret);
	g_assert_true(g_file_test(fn, G_FILE_TEST_EXISTS));

	/* check it is world readable */
	rc = g_stat(fn, &statbuf);
	g_assert_cmpint(rc, ==, 0);
	g_assert_cmpint(statbuf.st_mode & 0777, ==, 0644);

	/* read back the value */
	value = fu_plugin_get_config_value(plugin, "Key");
	g_assert_cmpstr(value, ==, "True");
	g_assert_true(fu_plugin_get_config_value_boolean(plugin, "Key"));

	/* check it is private, i.e. only readable by the user/group */
	ret = fu_plugin_set_secure_config_value(plugin, "Key", "False", &error);
	g_assert_no_error(error);
	g_assert_true(ret);
	rc = g_stat(fn, &statbuf);
	g_assert_cmpint(rc, ==, 0);
	g_assert_cmpint(statbuf.st_mode & 0777, ==, 0640);
}

	g_test_add_func("/fwupd/plugin{config}", fu_plugin_config_func);