From 41575afd93ca0e68bced78ca43a4488f124906a1 Mon Sep 17 00:00:00 2001 From: Richard Hughes Date: Wed, 21 Sep 2022 14:56:10 +0100 Subject: [PATCH 3/3] 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. --- contrib/fwupd.spec.in | 3 ++ libfwupdplugin/fu-plugin.c | 65 +++++++++++++++++++++++++++++------ libfwupdplugin/fu-self-test.c | 57 ++++++++++++++++++++++++++++++ 3 files changed, 114 insertions(+), 11 deletions(-) diff --git a/contrib/fwupd.spec.in b/contrib/fwupd.spec.in index a50e30a9c..0854fcf4f 100644 --- a/contrib/fwupd.spec.in +++ b/contrib/fwupd.spec.in @@ -313,6 +313,9 @@ for fn in /etc/fwupd/remotes.d/*.conf; do fi done +# ensure this is private +chmod 0660 /etc/fwupd/redfish.conf + %preun %systemd_preun fwupd.service diff --git a/libfwupdplugin/fu-plugin.c b/libfwupdplugin/fu-plugin.c index 18042a028..04951de85 100644 --- a/libfwupdplugin/fu-plugin.c +++ b/libfwupdplugin/fu-plugin.c @@ -9,6 +9,7 @@ #include "config.h" #include +#include #include #include #include @@ -2256,6 +2257,46 @@ fu_plugin_set_config_value(FuPlugin *self, const gchar *key, const gchar *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 @@ -2277,7 +2318,8 @@ fu_plugin_set_secure_config_value(FuPlugin *self, 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); @@ -2286,17 +2328,18 @@ fu_plugin_set_secure_config_value(FuPlugin *self, 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); } /** -- 2.39.1