From c73a55faa14f73fdf3c09a0ab0d3394a94325091 Mon Sep 17 00:00:00 2001 From: Richard Hughes Date: Wed, 21 Sep 2022 19:43:51 +0100 Subject: [PATCH] Remove a potential foot-gun when saving config values If a specific plugin calls fu_plugin_set_secure_config_value() and then fu_plugin_set_config_value() then we'll save the file with the world-readable permissions. Set a plugin flag to say that 'this plugin is storing sensitive details' which allows us to use the same entrypoint and also fix up any files at startup that do not have the correct permissions. --- contrib/fwupd.spec.in | 3 - libfwupd/fwupd-enums.c | 4 + libfwupd/fwupd-enums.h | 9 ++ libfwupd/fwupd-self-test.c | 2 +- libfwupdplugin/README.md | 1 + libfwupdplugin/fu-plugin.c | 158 ++++++++++++++++++---------- libfwupdplugin/fu-plugin.h | 1 + libfwupdplugin/fu-self-test.c | 5 +- plugins/redfish/fu-plugin-redfish.c | 11 +- src/fu-util-common.c | 5 + 10 files changed, 130 insertions(+), 69 deletions(-) diff --git a/contrib/fwupd.spec.in b/contrib/fwupd.spec.in index 42ea2024a..b011292b1 100644 --- a/contrib/fwupd.spec.in +++ b/contrib/fwupd.spec.in @@ -326,9 +326,6 @@ 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/libfwupd/fwupd-enums.c b/libfwupd/fwupd-enums.c index b46533e74..46df3d261 100644 --- a/libfwupd/fwupd-enums.c +++ b/libfwupd/fwupd-enums.c @@ -440,6 +440,8 @@ fwupd_plugin_flag_to_string(FwupdPluginFlags plugin_flag) return "unknown"; if (plugin_flag == FWUPD_PLUGIN_FLAG_AUTH_REQUIRED) return "auth-required"; + if (plugin_flag == FWUPD_PLUGIN_FLAG_SECURE_CONFIG) + return "secure-config"; return NULL; } @@ -484,6 +486,8 @@ fwupd_plugin_flag_from_string(const gchar *plugin_flag) return FWUPD_PLUGIN_FLAG_KERNEL_TOO_OLD; if (g_strcmp0(plugin_flag, "auth-required") == 0) return FWUPD_PLUGIN_FLAG_AUTH_REQUIRED; + if (g_strcmp0(plugin_flag, "secure-config") == 0) + return FWUPD_PLUGIN_FLAG_SECURE_CONFIG; return FWUPD_DEVICE_FLAG_UNKNOWN; } diff --git a/libfwupd/fwupd-enums.h b/libfwupd/fwupd-enums.h index 2828294bf..3502cf831 100644 --- a/libfwupd/fwupd-enums.h +++ b/libfwupd/fwupd-enums.h @@ -854,6 +854,15 @@ typedef enum { * Since: 1.6.2 */ #define FWUPD_PLUGIN_FLAG_AUTH_REQUIRED (1u << 12) +/** + * FWUPD_PLUGIN_FLAG_SECURE_CONFIG: + * + * The plugin requires the config file to be saved with permissions that only allow the root user + * to read. + * + * Since: 1.8.5 + */ +#define FWUPD_PLUGIN_FLAG_SECURE_CONFIG (1u << 13) /** * FWUPD_PLUGIN_FLAG_UNKNOWN: * diff --git a/libfwupd/fwupd-self-test.c b/libfwupd/fwupd-self-test.c index 649db344a..379e8e22b 100644 --- a/libfwupd/fwupd-self-test.c +++ b/libfwupd/fwupd-self-test.c @@ -170,7 +170,7 @@ fwupd_enums_func(void) g_assert_cmpstr(tmp, !=, NULL); g_assert_cmpint(fwupd_device_problem_from_string(tmp), ==, i); } - for (guint64 i = 1; i <= FWUPD_PLUGIN_FLAG_AUTH_REQUIRED; i *= 2) { + for (guint64 i = 1; i <= FWUPD_PLUGIN_FLAG_SECURE_CONFIG; i *= 2) { const gchar *tmp = fwupd_plugin_flag_to_string(i); if (tmp == NULL) g_warning("missing plugin flag 0x%x", (guint)i); diff --git a/libfwupdplugin/README.md b/libfwupdplugin/README.md index 5f288cbc4..857d17d22 100644 --- a/libfwupdplugin/README.md +++ b/libfwupdplugin/README.md @@ -98,3 +98,4 @@ Remember: Plugins should be upstream! ## 1.8.5 * `fu_volume_new_esp_default()`: Use `fu_context_get_esp_volumes()` instead. +* `fu_plugin_set_secure_config_value()`: Set `FWUPD_PLUGIN_FLAG_SECURE_CONFIG` and use `fu_plugin_set_config_value()` diff --git a/libfwupdplugin/fu-plugin.c b/libfwupdplugin/fu-plugin.c index b431f6d41..4feee4c05 100644 --- a/libfwupdplugin/fu-plugin.c +++ b/libfwupdplugin/fu-plugin.c @@ -84,6 +84,9 @@ typedef gboolean (*FuPluginFlaggedDeviceFunc)(FuPlugin *self, GError **error); typedef gboolean (*FuPluginDeviceArrayFunc)(FuPlugin *self, GPtrArray *devices, GError **error); +#define FU_PLUGIN_FILE_MODE_NONSECURE 0644 +#define FU_PLUGIN_FILE_MODE_SECURE 0640 + /** * fu_plugin_is_open: * @self: a #FuPlugin @@ -299,6 +302,14 @@ fu_plugin_guess_name_from_fn(const gchar *filename) return name; } +static gchar * +fu_plugin_get_config_filename(FuPlugin *self) +{ + g_autofree gchar *conf_dir = fu_path_from_kind(FU_PATH_KIND_SYSCONFDIR_PKG); + g_autofree gchar *conf_file = g_strdup_printf("%s.conf", fu_plugin_get_name(self)); + return g_build_filename(conf_dir, conf_file, NULL); +} + /** * fu_plugin_open: * @self: a #FuPlugin @@ -355,6 +366,23 @@ fu_plugin_open(FuPlugin *self, const gchar *filename, GError **error) fu_plugin_set_name(self, str); } + /* ensure the configure file is set to the correct permission */ + if (fu_plugin_has_flag(self, FWUPD_PLUGIN_FLAG_SECURE_CONFIG)) { + g_autofree gchar *conf_path = fu_plugin_get_config_filename(self); + if (g_file_test(conf_path, G_FILE_TEST_EXISTS)) { + gint rc = g_chmod(conf_path, FU_PLUGIN_FILE_MODE_SECURE); + if (rc != 0) { + g_set_error(error, + G_IO_ERROR, + G_IO_ERROR_FAILED, + "failed to change permission of %s", + filename); + fu_plugin_add_flag(self, FWUPD_PLUGIN_FLAG_FAILED_OPEN); + return FALSE; + } + } + } + /* optional */ if (vfuncs->load != NULL) { FuContext *ctx = fu_plugin_get_context(self); @@ -502,14 +530,6 @@ fu_plugin_config_monitor_changed_cb(GFileMonitor *monitor, g_signal_emit(self, signals[SIGNAL_CONFIG_CHANGED], 0); } -static gchar * -fu_plugin_get_config_filename(FuPlugin *self) -{ - g_autofree gchar *conf_dir = fu_path_from_kind(FU_PATH_KIND_SYSCONFDIR_PKG); - g_autofree gchar *conf_file = g_strdup_printf("%s.conf", fu_plugin_get_name(self)); - return g_build_filename(conf_dir, conf_file, NULL); -} - /** * fu_plugin_device_add: * @self: a #FuPlugin @@ -2388,36 +2408,6 @@ fu_plugin_security_attr_new(FuPlugin *self, const gchar *appstream_id) return g_steal_pointer(&attr); } -/** - * fu_plugin_set_config_value: - * @self: a #FuPlugin - * @key: a settings key - * @value: (nullable): a settings value - * @error: (nullable): optional return location for an error - * - * Sets a plugin config value. - * - * Returns: %TRUE for success - * - * Since: 1.7.0 - **/ -gboolean -fu_plugin_set_config_value(FuPlugin *self, const gchar *key, const gchar *value, GError **error) -{ - g_autofree gchar *conf_path = fu_plugin_get_config_filename(self); - g_autoptr(GKeyFile) keyfile = NULL; - - g_return_val_if_fail(FU_IS_PLUGIN(self), FALSE); - g_return_val_if_fail(key != NULL, FALSE); - g_return_val_if_fail(error == NULL || *error == NULL, FALSE); - - keyfile = g_key_file_new(); - if (!g_key_file_load_from_file(keyfile, conf_path, G_KEY_FILE_KEEP_COMMENTS, error)) - 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 @@ -2458,25 +2448,12 @@ g_file_set_contents_full(const gchar *filename, } #endif -/** - * fu_plugin_set_secure_config_value: - * @self: a #FuPlugin - * @key: a settings key - * @value: (nullable): a settings value - * @error: (nullable): optional return location for an error - * - * Sets a plugin config file value and updates file so that non-privileged users - * cannot read it. - * - * Returns: %TRUE for success - * - * Since: 1.7.4 - **/ -gboolean -fu_plugin_set_secure_config_value(FuPlugin *self, - const gchar *key, - const gchar *value, - GError **error) +static gboolean +fu_plugin_set_config_value_internal(FuPlugin *self, + const gchar *key, + const gchar *value, + guint32 mode, + GError **error) { g_autofree gchar *conf_path = fu_plugin_get_config_filename(self); g_autofree gchar *data = NULL; @@ -2499,10 +2476,75 @@ fu_plugin_set_secure_config_value(FuPlugin *self, data, -1, G_FILE_SET_CONTENTS_CONSISTENT, - 0660, + mode, error); } +/** + * fu_plugin_set_secure_config_value: + * @self: a #FuPlugin + * @key: a settings key + * @value: (nullable): a settings value + * @error: (nullable): optional return location for an error + * + * Sets a plugin config file value and updates file so that non-privileged users + * cannot read it. + * + * This function is deprecated, and you should use `FWUPD_PLUGIN_FLAG_SECURE_CONFIG` and + * fu_plugin_set_config_value() instead. + * + * Returns: %TRUE for success + * + * Since: 1.7.4 + **/ +gboolean +fu_plugin_set_secure_config_value(FuPlugin *self, + const gchar *key, + const gchar *value, + GError **error) +{ + g_return_val_if_fail(FU_IS_PLUGIN(self), FALSE); + g_return_val_if_fail(key != NULL, FALSE); + g_return_val_if_fail(error == NULL || *error == NULL, FALSE); + fu_plugin_add_flag(self, FWUPD_PLUGIN_FLAG_SECURE_CONFIG); + return fu_plugin_set_config_value(self, key, value, error); +} + +/** + * fu_plugin_set_config_value: + * @self: a #FuPlugin + * @key: a settings key + * @value: (nullable): a settings value + * @error: (nullable): optional return location for an error + * + * Sets a plugin config value. + * + * Returns: %TRUE for success + * + * Since: 1.7.0 + **/ +gboolean +fu_plugin_set_config_value(FuPlugin *self, const gchar *key, const gchar *value, GError **error) +{ + g_return_val_if_fail(FU_IS_PLUGIN(self), FALSE); + g_return_val_if_fail(key != NULL, FALSE); + g_return_val_if_fail(error == NULL || *error == NULL, FALSE); + + /* when removing fu_plugin_set_secure_config_value() just remove this instead */ + if (fu_plugin_has_flag(self, FWUPD_PLUGIN_FLAG_SECURE_CONFIG)) { + return fu_plugin_set_config_value_internal(self, + key, + value, + FU_PLUGIN_FILE_MODE_SECURE, + error); + } + return fu_plugin_set_config_value_internal(self, + key, + value, + FU_PLUGIN_FILE_MODE_NONSECURE, + error); +} + /** * fu_plugin_get_config_value_boolean: * @self: a #FuPlugin diff --git a/libfwupdplugin/fu-plugin.h b/libfwupdplugin/fu-plugin.h index 1221ee049..cfe18f1c6 100644 --- a/libfwupdplugin/fu-plugin.h +++ b/libfwupdplugin/fu-plugin.h @@ -460,6 +460,7 @@ void fu_plugin_add_report_metadata(FuPlugin *self, const gchar *key, const gchar *value); gchar * fu_plugin_get_config_value(FuPlugin *self, const gchar *key); +G_DEPRECATED_FOR(fu_plugin_set_config_value) gboolean fu_plugin_set_secure_config_value(FuPlugin *self, const gchar *key, diff --git a/libfwupdplugin/fu-self-test.c b/libfwupdplugin/fu-self-test.c index aaf49c172..ab2d6550e 100644 --- a/libfwupdplugin/fu-self-test.c +++ b/libfwupdplugin/fu-self-test.c @@ -721,8 +721,9 @@ fu_plugin_config_func(void) 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); + /* write it private, i.e. only readable by the user/group */ + fu_plugin_add_flag(plugin, FWUPD_PLUGIN_FLAG_SECURE_CONFIG); + ret = fu_plugin_set_config_value(plugin, "Key", "False", &error); g_assert_no_error(error); g_assert_true(ret); rc = g_stat(fn, &statbuf); diff --git a/plugins/redfish/fu-plugin-redfish.c b/plugins/redfish/fu-plugin-redfish.c index ef6618c65..7004ceb67 100644 --- a/plugins/redfish/fu-plugin-redfish.c +++ b/plugins/redfish/fu-plugin-redfish.c @@ -63,7 +63,7 @@ fu_plugin_redfish_change_expired(FuPlugin *plugin, GError **error) uri = fu_plugin_get_config_value(plugin, "UserUri"); if (uri == NULL) { uri = g_strdup("/redfish/v1/AccountService/Accounts/2"); - if (!fu_plugin_set_secure_config_value(plugin, "UserUri", uri, error)) + if (!fu_plugin_set_config_value(plugin, "UserUri", uri, error)) return FALSE; } @@ -83,7 +83,7 @@ fu_plugin_redfish_change_expired(FuPlugin *plugin, GError **error) fu_redfish_backend_set_password(priv->backend, password_new); /* success */ - return fu_plugin_set_secure_config_value(plugin, "Password", password_new, error); + return fu_plugin_set_config_value(plugin, "Password", password_new, error); } static gboolean @@ -352,11 +352,11 @@ fu_redfish_plugin_ipmi_create_user(FuPlugin *plugin, GError **error) fu_redfish_backend_set_password(priv->backend, password_new); /* success */ - if (!fu_plugin_set_secure_config_value(plugin, "UserUri", uri, error)) + if (!fu_plugin_set_config_value(plugin, "UserUri", uri, error)) return FALSE; - if (!fu_plugin_set_secure_config_value(plugin, "Username", username_fwupd, error)) + if (!fu_plugin_set_config_value(plugin, "Username", username_fwupd, error)) return FALSE; - if (!fu_plugin_set_secure_config_value(plugin, "Password", password_new, error)) + if (!fu_plugin_set_config_value(plugin, "Password", password_new, error)) return FALSE; return TRUE; @@ -594,6 +594,7 @@ fu_plugin_redfish_init(FuPlugin *plugin) FuPluginData *priv = fu_plugin_alloc_data(plugin, sizeof(FuPluginData)); priv->backend = fu_redfish_backend_new(ctx); fu_plugin_add_firmware_gtype(plugin, NULL, FU_TYPE_REDFISH_SMBIOS); + fu_plugin_add_flag(plugin, FWUPD_PLUGIN_FLAG_SECURE_CONFIG); } static void diff --git a/src/fu-util-common.c b/src/fu-util-common.c index b63ef7939..29d49ada2 100644 --- a/src/fu-util-common.c +++ b/src/fu-util-common.c @@ -1690,6 +1690,10 @@ fu_util_plugin_flag_to_string(FwupdPluginFlags plugin_flag) /* TRANSLATORS: user needs to run a command */ return _("Authentication details are required"); } + if (plugin_flag == FWUPD_PLUGIN_FLAG_SECURE_CONFIG) { + /* TRANSLATORS: no peeking */ + return _("Configuration is only readable by the system administrator"); + } if (plugin_flag == FWUPD_PLUGIN_FLAG_EFIVAR_NOT_MOUNTED) { /* TRANSLATORS: the user is using Gentoo/Arch and has screwed something up */ return _("Required efivarfs filesystem was not found"); @@ -1731,6 +1735,7 @@ fu_util_plugin_flag_to_cli_text(FwupdPluginFlags plugin_flag) case FWUPD_PLUGIN_FLAG_CAPSULES_UNSUPPORTED: case FWUPD_PLUGIN_FLAG_UNLOCK_REQUIRED: case FWUPD_PLUGIN_FLAG_AUTH_REQUIRED: + case FWUPD_PLUGIN_FLAG_SECURE_CONFIG: case FWUPD_PLUGIN_FLAG_EFIVAR_NOT_MOUNTED: case FWUPD_PLUGIN_FLAG_ESP_NOT_FOUND: case FWUPD_PLUGIN_FLAG_KERNEL_TOO_OLD: