Skip to content
Permalink
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
hughsie committed Sep 22, 2022
1 parent be2311c commit ea67685
Show file tree
Hide file tree
Showing 3 changed files with 114 additions and 11 deletions.
3 changes: 3 additions & 0 deletions contrib/fwupd.spec.in
Expand Up @@ -326,6 +326,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

Expand Down
65 changes: 54 additions & 11 deletions libfwupdplugin/fu-plugin.c
Expand Up @@ -9,6 +9,7 @@
#include "config.h"

#include <errno.h>
#include <fcntl.h>
#include <fwupd.h>
#include <glib/gstdio.h>
#include <gmodule.h>
Expand Down Expand Up @@ -2417,6 +2418,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
Expand All @@ -2438,7 +2479,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);
Expand All @@ -2447,17 +2489,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);
}

/**
Expand Down
57 changes: 57 additions & 0 deletions libfwupdplugin/fu-self-test.c
Expand Up @@ -674,6 +674,62 @@ _plugin_device_added_cb(FuPlugin *plugin, FuDevice *device, gpointer user_data)
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)
{
Expand Down Expand Up @@ -3598,6 +3654,7 @@ main(int argc, char **argv)
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);
Expand Down

0 comments on commit ea67685

Please sign in to comment.