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

@@ -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
 

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;
 }
 

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:
  *

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);

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()`

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

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,

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);

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

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: