From 2593ae2522cc26286bed5e9dd69fda62ec7b84ad Mon Sep 17 00:00:00 2001 From: Justin Spencer Date: Mon, 10 Jan 2022 17:41:55 -0500 Subject: [PATCH 1/8] Remove check for running as root, but keep translations --- src/core/main.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 50f70a9923..e39a961416 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -328,8 +328,6 @@ main(int argc, char *argv[]) exit(result); } - nm_main_utils_ensure_root(); - nm_main_utils_ensure_not_running_pidfile(global_opt.pidfile); nm_main_utils_ensure_statedir(); From 3783494dfec9d90ea5cd2685bb252a4b7e63514d Mon Sep 17 00:00:00 2001 From: Justin Spencer Date: Mon, 10 Jan 2022 17:49:02 -0500 Subject: [PATCH 2/8] Add helper functions to find + remember EUID / EGID, avoid many syscalls --- src/core/main-utils.c | 36 ++++++++++++++++++++++++++++++++++++ src/core/main-utils.h | 4 ++++ 2 files changed, 40 insertions(+) diff --git a/src/core/main-utils.c b/src/core/main-utils.c index 7fcffcbc08..14685fc3b4 100644 --- a/src/core/main-utils.c +++ b/src/core/main-utils.c @@ -218,6 +218,42 @@ nm_main_utils_ensure_root() } } +uid_t nm_uid; +/** + * nm_main_utils_get_nm_uid: + * + * Checks what EUID NetworkManager is running as. + * Saves the EUID so it can be reused without making many syscalls. + */ +uid_t +nm_main_utils_get_nm_uid(void) +{ + static uint8_t nm_uid_flag = 0; + if (!nm_uid_flag) { + nm_uid = geteuid(); + nm_uid_flag = 1; + } + return nm_uid; +} + +gid_t nm_gid; +/** + * nm_main_utils_get_nm_gid: + * + * Checks what EGID NetworkManager is running as. + * Saves the EGID so it can be reused without making many syscalls. + */ +gid_t +nm_main_utils_get_nm_gid(void) +{ + static uint8_t nm_gid_flag = 0; + if (!nm_gid_flag) { + nm_gid = getegid(); + nm_gid_flag = 1; + } + return nm_gid; +} + gboolean nm_main_utils_early_setup(const char *progname, int *argc, diff --git a/src/core/main-utils.h b/src/core/main-utils.h index e3a3b490a9..d947dcaebd 100644 --- a/src/core/main-utils.h +++ b/src/core/main-utils.h @@ -8,6 +8,10 @@ void nm_main_utils_ensure_root(void); +uid_t nm_main_utils_get_nm_uid(void); + +gid_t nm_main_utils_get_nm_gid(void); + void nm_main_utils_setup_signals(GMainLoop *main_loop); void nm_main_utils_ensure_statedir(void); From 41db8c75638483a599cd142e7c3d4becf2a450d9 Mon Sep 17 00:00:00 2001 From: Justin Spencer Date: Thu, 25 Nov 2021 12:24:26 -0500 Subject: [PATCH 3/8] Write keyfiles as euid / egid instead of 0 / 0 (root / root) --- src/core/settings/plugins/keyfile/nms-keyfile-writer.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/core/settings/plugins/keyfile/nms-keyfile-writer.c b/src/core/settings/plugins/keyfile/nms-keyfile-writer.c index 73788e891e..2311d0dd9d 100644 --- a/src/core/settings/plugins/keyfile/nms-keyfile-writer.c +++ b/src/core/settings/plugins/keyfile/nms-keyfile-writer.c @@ -10,6 +10,7 @@ #include #include +#include #include #include "libnm-core-intern/nm-keyfile-internal.h" @@ -18,6 +19,7 @@ #include "nms-keyfile-reader.h" #include "libnm-glib-aux/nm-io-utils.h" +#include "src/core/main-utils.h" /*****************************************************************************/ @@ -444,8 +446,8 @@ nms_keyfile_writer_connection(NMConnection *connection, keyfile_dir, profile_dir, TRUE, - 0, - 0, + nm_main_utils_get_nm_uid(), + nm_main_utils_get_nm_gid(), existing_path, existing_path_read_only, force_rename, From 604260c6ea1286582c72531c7689cd681189aaf7 Mon Sep 17 00:00:00 2001 From: Justin Spencer Date: Thu, 25 Nov 2021 12:24:51 -0500 Subject: [PATCH 4/8] Assert keyfiles are owned by euid, not root --- src/core/settings/plugins/keyfile/nms-keyfile-utils.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/core/settings/plugins/keyfile/nms-keyfile-utils.c b/src/core/settings/plugins/keyfile/nms-keyfile-utils.c index 68ceee5d26..c675df94e2 100644 --- a/src/core/settings/plugins/keyfile/nms-keyfile-utils.c +++ b/src/core/settings/plugins/keyfile/nms-keyfile-utils.c @@ -9,6 +9,7 @@ #include #include +#include #include "libnm-glib-aux/nm-uuid.h" #include "libnm-glib-aux/nm-io-utils.h" @@ -18,6 +19,7 @@ #include "nm-setting-wireless.h" #include "nm-setting-wireless-security.h" #include "nm-config.h" +#include "src/core/main-utils.h" /*****************************************************************************/ @@ -337,7 +339,7 @@ nms_keyfile_utils_check_file_permissions_stat(NMSKeyfileFiletype filetype, g_return_val_if_reached(FALSE); if (!NM_FLAGS_HAS(nm_utils_get_testing(), NM_UTILS_TEST_NO_KEYFILE_OWNER_CHECK)) { - if (st->st_uid != 0) { + if (st->st_uid != nm_main_utils_get_nm_uid()) { g_set_error(error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, From f755c7b1db44194fdbd8a0d9beded3c1a5604a76 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Jan 2022 17:14:23 +0100 Subject: [PATCH 5/8] core: remove unused function nm_main_utils_ensure_root() --- src/core/main-utils.c | 9 --------- src/core/main-utils.h | 2 -- 2 files changed, 11 deletions(-) diff --git a/src/core/main-utils.c b/src/core/main-utils.c index 14685fc3b4..0e1f5b89a8 100644 --- a/src/core/main-utils.c +++ b/src/core/main-utils.c @@ -209,15 +209,6 @@ nm_main_utils_ensure_not_running_pidfile(const char *pidfile) } } -void -nm_main_utils_ensure_root() -{ - if (getuid() != 0) { - fprintf(stderr, _("You must be root to run %s!\n"), g_get_prgname() ?: ""); - exit(1); - } -} - uid_t nm_uid; /** * nm_main_utils_get_nm_uid: diff --git a/src/core/main-utils.h b/src/core/main-utils.h index d947dcaebd..0f3ee50dc0 100644 --- a/src/core/main-utils.h +++ b/src/core/main-utils.h @@ -6,8 +6,6 @@ #ifndef __MAIN_UTILS_H__ #define __MAIN_UTILS_H__ -void nm_main_utils_ensure_root(void); - uid_t nm_main_utils_get_nm_uid(void); gid_t nm_main_utils_get_nm_gid(void); From b1a14e3398a2734c6ffc99b37b7e8c605086861d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Jan 2022 17:30:14 +0100 Subject: [PATCH 6/8] core: move nm_main_utils_get_nm_[ug]id() to "nm-core-utils.h" There is a hierarchy of how files include each other. "main-utils.h" is pretty much at the bottom (one above "main.c"), in the sense that it only includes other headers, but is not included itself (aside "main.c"). Move the utils function to a place where its accessible from everywhere and rename. --- src/core/main-utils.c | 36 ----------------- src/core/main-utils.h | 4 -- src/core/nm-core-utils.c | 40 +++++++++++++++++++ src/core/nm-core-utils.h | 7 ++++ .../plugins/keyfile/nms-keyfile-utils.c | 4 +- .../plugins/keyfile/nms-keyfile-writer.c | 6 +-- 6 files changed, 50 insertions(+), 47 deletions(-) diff --git a/src/core/main-utils.c b/src/core/main-utils.c index 0e1f5b89a8..444e122e8a 100644 --- a/src/core/main-utils.c +++ b/src/core/main-utils.c @@ -209,42 +209,6 @@ nm_main_utils_ensure_not_running_pidfile(const char *pidfile) } } -uid_t nm_uid; -/** - * nm_main_utils_get_nm_uid: - * - * Checks what EUID NetworkManager is running as. - * Saves the EUID so it can be reused without making many syscalls. - */ -uid_t -nm_main_utils_get_nm_uid(void) -{ - static uint8_t nm_uid_flag = 0; - if (!nm_uid_flag) { - nm_uid = geteuid(); - nm_uid_flag = 1; - } - return nm_uid; -} - -gid_t nm_gid; -/** - * nm_main_utils_get_nm_gid: - * - * Checks what EGID NetworkManager is running as. - * Saves the EGID so it can be reused without making many syscalls. - */ -gid_t -nm_main_utils_get_nm_gid(void) -{ - static uint8_t nm_gid_flag = 0; - if (!nm_gid_flag) { - nm_gid = getegid(); - nm_gid_flag = 1; - } - return nm_gid; -} - gboolean nm_main_utils_early_setup(const char *progname, int *argc, diff --git a/src/core/main-utils.h b/src/core/main-utils.h index 0f3ee50dc0..1c3e7a1cbe 100644 --- a/src/core/main-utils.h +++ b/src/core/main-utils.h @@ -6,10 +6,6 @@ #ifndef __MAIN_UTILS_H__ #define __MAIN_UTILS_H__ -uid_t nm_main_utils_get_nm_uid(void); - -gid_t nm_main_utils_get_nm_gid(void); - void nm_main_utils_setup_signals(GMainLoop *main_loop); void nm_main_utils_ensure_statedir(void); diff --git a/src/core/nm-core-utils.c b/src/core/nm-core-utils.c index 908cfd8912..1db74e5eb9 100644 --- a/src/core/nm-core-utils.c +++ b/src/core/nm-core-utils.c @@ -5145,3 +5145,43 @@ nm_utils_spawn_helper_finish(GAsyncResult *result, GError **error) return g_task_propagate_pointer(task, error); } + +/*****************************************************************************/ + +/** + * nm_utils_get_nm_uid: + * + * Checks what EUID NetworkManager is running as. + * Saves the EUID so it can be reused without making many syscalls. + */ +uid_t +nm_utils_get_nm_uid(void) +{ + static uint8_t nm_uid_flag = 0; + static uid_t nm_uid; + + if (!nm_uid_flag) { + nm_uid = geteuid(); + nm_uid_flag = 1; + } + return nm_uid; +} + +/** + * nm_utils_get_nm_gid: + * + * Checks what EGID NetworkManager is running as. + * Saves the EGID so it can be reused without making many syscalls. + */ +gid_t +nm_utils_get_nm_gid(void) +{ + static uint8_t nm_gid_flag = 0; + static gid_t nm_gid; + + if (!nm_gid_flag) { + nm_gid = getegid(); + nm_gid_flag = 1; + } + return nm_gid; +} diff --git a/src/core/nm-core-utils.h b/src/core/nm-core-utils.h index c3ce85fa04..7079ab2f25 100644 --- a/src/core/nm-core-utils.h +++ b/src/core/nm-core-utils.h @@ -9,6 +9,7 @@ #include #include +#include #include "nm-connection.h" @@ -470,4 +471,10 @@ void nm_utils_spawn_helper(const char *const *args, char *nm_utils_spawn_helper_finish(GAsyncResult *result, GError **error); +/*****************************************************************************/ + +uid_t nm_utils_get_nm_uid(void); + +gid_t nm_utils_get_nm_gid(void); + #endif /* __NM_CORE_UTILS_H__ */ diff --git a/src/core/settings/plugins/keyfile/nms-keyfile-utils.c b/src/core/settings/plugins/keyfile/nms-keyfile-utils.c index c675df94e2..b31370b15a 100644 --- a/src/core/settings/plugins/keyfile/nms-keyfile-utils.c +++ b/src/core/settings/plugins/keyfile/nms-keyfile-utils.c @@ -9,7 +9,6 @@ #include #include -#include #include "libnm-glib-aux/nm-uuid.h" #include "libnm-glib-aux/nm-io-utils.h" @@ -19,7 +18,6 @@ #include "nm-setting-wireless.h" #include "nm-setting-wireless-security.h" #include "nm-config.h" -#include "src/core/main-utils.h" /*****************************************************************************/ @@ -339,7 +337,7 @@ nms_keyfile_utils_check_file_permissions_stat(NMSKeyfileFiletype filetype, g_return_val_if_reached(FALSE); if (!NM_FLAGS_HAS(nm_utils_get_testing(), NM_UTILS_TEST_NO_KEYFILE_OWNER_CHECK)) { - if (st->st_uid != nm_main_utils_get_nm_uid()) { + if (st->st_uid != nm_utils_get_nm_uid()) { g_set_error(error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, diff --git a/src/core/settings/plugins/keyfile/nms-keyfile-writer.c b/src/core/settings/plugins/keyfile/nms-keyfile-writer.c index 2311d0dd9d..ad6f277ce3 100644 --- a/src/core/settings/plugins/keyfile/nms-keyfile-writer.c +++ b/src/core/settings/plugins/keyfile/nms-keyfile-writer.c @@ -10,7 +10,6 @@ #include #include -#include #include #include "libnm-core-intern/nm-keyfile-internal.h" @@ -19,7 +18,6 @@ #include "nms-keyfile-reader.h" #include "libnm-glib-aux/nm-io-utils.h" -#include "src/core/main-utils.h" /*****************************************************************************/ @@ -446,8 +444,8 @@ nms_keyfile_writer_connection(NMConnection *connection, keyfile_dir, profile_dir, TRUE, - nm_main_utils_get_nm_uid(), - nm_main_utils_get_nm_gid(), + nm_utils_get_nm_uid(), + nm_utils_get_nm_gid(), existing_path, existing_path_read_only, force_rename, From b2660b7012693bd2a171b9f237950c225cf4d123 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Jan 2022 17:47:17 +0100 Subject: [PATCH 7/8] keyfile: for keyfile owner check allow root and euid This partly restores the previous behavior. The point of the file owner check is to ensure that the file cannot be read by unpriviledged processes as it may contain secrets. If the file is owned by root, that is considered secure (even if our euid is different). Possibly, if our euid is not root, then we couldn't read the file, but that is a different problem. --- src/core/settings/plugins/keyfile/nms-keyfile-utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/settings/plugins/keyfile/nms-keyfile-utils.c b/src/core/settings/plugins/keyfile/nms-keyfile-utils.c index b31370b15a..7c0e329e2d 100644 --- a/src/core/settings/plugins/keyfile/nms-keyfile-utils.c +++ b/src/core/settings/plugins/keyfile/nms-keyfile-utils.c @@ -337,7 +337,7 @@ nms_keyfile_utils_check_file_permissions_stat(NMSKeyfileFiletype filetype, g_return_val_if_reached(FALSE); if (!NM_FLAGS_HAS(nm_utils_get_testing(), NM_UTILS_TEST_NO_KEYFILE_OWNER_CHECK)) { - if (st->st_uid != nm_utils_get_nm_uid()) { + if (!NM_IN_SET(st->st_uid, 0, nm_utils_get_nm_uid())) { g_set_error(error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, From 31dbcb81fe7ac5e718bdcaba461eb01c1d73b600 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Jan 2022 18:21:01 +0100 Subject: [PATCH 8/8] core: make nm_utils_get_nm_[ug]id() thread safe While NetworkManager is of course (mostly) single threaded, our static functions really should be thread safe. "mostly" single threaded because we have GDBus's worker thread, we use a thread for writing non-blocking SR-IOV sysctl, in the past (or still?) we used a thread for async glibc resolver. Anyway, a low-level function like must be thread safe, when it uses global data. Granted, the initialize-once pattern with the flag and the int variable, is probably in many cases good enough. But it makes me unhappy, the thought of accessing static data without a synchronized operation. --- src/core/nm-core-utils.c | 40 ++++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/src/core/nm-core-utils.c b/src/core/nm-core-utils.c index 1db74e5eb9..e05638e43c 100644 --- a/src/core/nm-core-utils.c +++ b/src/core/nm-core-utils.c @@ -5157,14 +5157,22 @@ nm_utils_spawn_helper_finish(GAsyncResult *result, GError **error) uid_t nm_utils_get_nm_uid(void) { - static uint8_t nm_uid_flag = 0; - static uid_t nm_uid; + static int u_static = -1; + int u; - if (!nm_uid_flag) { - nm_uid = geteuid(); - nm_uid_flag = 1; +again: + if ((u = g_atomic_int_get(&u_static)) == -1) { + uid_t u2; + + u2 = geteuid(); + u = u2; + nm_assert(u == u2); + nm_assert(u >= 0); + if (!g_atomic_int_compare_and_exchange(&u_static, -1, u)) + goto again; } - return nm_uid; + + return u; } /** @@ -5176,12 +5184,20 @@ nm_utils_get_nm_uid(void) gid_t nm_utils_get_nm_gid(void) { - static uint8_t nm_gid_flag = 0; - static gid_t nm_gid; + static int g_static = -1; + int g; - if (!nm_gid_flag) { - nm_gid = getegid(); - nm_gid_flag = 1; +again: + if ((g = g_atomic_int_get(&g_static)) == -1) { + gid_t g2; + + g2 = geteuid(); + g = g2; + nm_assert(g == g2); + nm_assert(g >= 0); + if (!g_atomic_int_compare_and_exchange(&g_static, -1, g)) + goto again; } - return nm_gid; + + return g; }