From 87df0601074b024ab591534e0e78b2d1e7013a8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 9 Mar 2022 22:29:19 +0100 Subject: [PATCH] shared/specifier: fix %u/%U/%g/%G when called as unprivileged user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We would resolve those specifiers to the calling user/group. This is mostly OK when done in the manager, because the manager generally operates as root in system mode, and a non-root in user mode. It would still be wrong if called with --test though. But in systemctl, this would be generally wrong, since we can call 'systemctl --system' as a normal user, either for testing or even for actual operation with '--root=…'. When operating in --global mode, %u/%U/%g/%G should return an error. The information whether we're operating in system mode, user mode, or global mode is passed as the data pointer to specifier_group_name(), specifier_user_name(), specifier_group_id(), specifier_user_id(). We can't use userdata, because it's already used for other things. (cherry picked from commit 172e9cc3ee3dcca288d04c744984a9a3b2a0d008) Related: #2082131 --- src/core/unit-printf.c | 4 +- src/shared/install-printf.c | 9 ++- src/shared/install-printf.h | 7 ++- src/shared/install.c | 9 +-- src/shared/specifier.c | 52 ++++++++++++++--- src/shared/specifier.h | 10 ++-- src/test/test-load-fragment.c | 101 +++++++++++++++++++--------------- src/test/test-specifier.c | 3 +- src/tmpfiles/tmpfiles.c | 53 +++++++++--------- test/test-systemctl-enable.sh | 11 ++-- test/test-systemd-tmpfiles.py | 8 +-- 11 files changed, 163 insertions(+), 104 deletions(-) diff --git a/src/core/unit-printf.c b/src/core/unit-printf.c index 46c383b841..3086356419 100644 --- a/src/core/unit-printf.c +++ b/src/core/unit-printf.c @@ -190,7 +190,7 @@ int unit_name_printf(const Unit *u, const char* format, char **ret) { COMMON_SYSTEM_SPECIFIERS, - COMMON_CREDS_SPECIFIERS, + COMMON_CREDS_SPECIFIERS(u->manager->unit_file_scope), {} }; @@ -256,7 +256,7 @@ int unit_full_printf_full(const Unit *u, const char *format, size_t max_length, COMMON_SYSTEM_SPECIFIERS, - COMMON_CREDS_SPECIFIERS, + COMMON_CREDS_SPECIFIERS(u->manager->unit_file_scope), COMMON_TMP_SPECIFIERS, {} diff --git a/src/shared/install-printf.c b/src/shared/install-printf.c index 6ff4198ac9..963102674b 100644 --- a/src/shared/install-printf.c +++ b/src/shared/install-printf.c @@ -103,7 +103,12 @@ static int specifier_last_component(char specifier, const void *data, const char return 0; } -int install_name_printf(const UnitFileInstallInfo *i, const char *format, const char *root, char **ret) { +int install_name_printf( + UnitFileScope scope, + const UnitFileInstallInfo *i, + const char *format, + const char *root, + char **ret) { /* This is similar to unit_name_printf() */ const Specifier table[] = { @@ -115,7 +120,7 @@ int install_name_printf(const UnitFileInstallInfo *i, const char *format, const COMMON_SYSTEM_SPECIFIERS, - COMMON_CREDS_SPECIFIERS, + COMMON_CREDS_SPECIFIERS(scope), {} }; diff --git a/src/shared/install-printf.h b/src/shared/install-printf.h index 5ca9406797..d2cccdf66d 100644 --- a/src/shared/install-printf.h +++ b/src/shared/install-printf.h @@ -4,4 +4,9 @@ #include "install.h" #include "unit-name.h" -int install_name_printf(const UnitFileInstallInfo *i, const char *format, const char *root, char **ret); +int install_name_printf( + UnitFileScope scope, + const UnitFileInstallInfo *i, + const char *format, + const char *root, + char **ret); diff --git a/src/shared/install.c b/src/shared/install.c index bfdeee48bf..8d82cb6b16 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1160,7 +1160,7 @@ static int config_parse_also( if (r == 0) break; - r = install_name_printf(info, word, info->root, &printed); + r = install_name_printf(ctx->scope, info, word, info->root, &printed); if (r < 0) return log_syntax(unit, LOG_WARNING, filename, line, r, "Failed to resolve unit name in Also=\"%s\": %m", word); @@ -1191,6 +1191,7 @@ static int config_parse_default_instance( void *data, void *userdata) { + InstallContext *ctx = ASSERT_PTR(data); UnitFileInstallInfo *info = ASSERT_PTR(userdata); _cleanup_free_ char *printed = NULL; int r; @@ -1208,7 +1209,7 @@ static int config_parse_default_instance( return log_syntax(unit, LOG_WARNING, filename, line, 0, "DefaultInstance= only makes sense for template units, ignoring."); - r = install_name_printf(info, rvalue, info->root, &printed); + r = install_name_printf(ctx->scope, info, rvalue, info->root, &printed); if (r < 0) return log_syntax(unit, LOG_WARNING, filename, line, r, "Failed to resolve instance name in DefaultInstance=\"%s\": %m", rvalue); @@ -1780,7 +1781,7 @@ static int install_info_symlink_alias( STRV_FOREACH(s, info->aliases) { _cleanup_free_ char *alias_path = NULL, *dst = NULL, *dst_updated = NULL; - q = install_name_printf(info, *s, info->root, &dst); + q = install_name_printf(scope, info, *s, info->root, &dst); if (q < 0) { unit_file_changes_add(changes, n_changes, q, *s, NULL); return q; @@ -1867,7 +1868,7 @@ static int install_info_symlink_wants( STRV_FOREACH(s, list) { _cleanup_free_ char *path = NULL, *dst = NULL; - q = install_name_printf(info, *s, info->root, &dst); + q = install_name_printf(scope, info, *s, info->root, &dst); if (q < 0) { unit_file_changes_add(changes, n_changes, q, *s, NULL); return q; diff --git a/src/shared/specifier.c b/src/shared/specifier.c index a917378427..ac353a651c 100644 --- a/src/shared/specifier.c +++ b/src/shared/specifier.c @@ -21,6 +21,7 @@ #include "specifier.h" #include "string-util.h" #include "strv.h" +#include "unit-file.h" #include "user-util.h" /* @@ -253,9 +254,15 @@ int specifier_os_image_version(char specifier, const void *data, const char *roo } int specifier_group_name(char specifier, const void *data, const char *root, const void *userdata, char **ret) { + UnitFileScope scope = PTR_TO_INT(data); char *t; - t = gid_to_name(getgid()); + assert(ret); + + if (scope == UNIT_FILE_GLOBAL) + return -EINVAL; + + t = gid_to_name(scope == UNIT_FILE_USER ? getgid() : 0); if (!t) return -ENOMEM; @@ -264,23 +271,42 @@ int specifier_group_name(char specifier, const void *data, const char *root, con } int specifier_group_id(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - if (asprintf(ret, UID_FMT, getgid()) < 0) + UnitFileScope scope = PTR_TO_INT(data); + gid_t gid; + + assert(ret); + + if (scope == UNIT_FILE_GLOBAL) + return -EINVAL; + + gid = scope == UNIT_FILE_USER ? getgid() : 0; + + if (asprintf(ret, UID_FMT, gid) < 0) return -ENOMEM; return 0; } int specifier_user_name(char specifier, const void *data, const char *root, const void *userdata, char **ret) { + UnitFileScope scope = PTR_TO_INT(data); + uid_t uid; char *t; - /* If we are UID 0 (root), this will not result in NSS, otherwise it might. This is good, as we want to be able - * to run this in PID 1, where our user ID is 0, but where NSS lookups are not allowed. + assert(ret); - * We don't use getusername_malloc() here, because we don't want to look at $USER, to remain consistent with - * specifer_user_id() below. + if (scope == UNIT_FILE_GLOBAL) + return -EINVAL; + + uid = scope == UNIT_FILE_USER ? getuid() : 0; + + /* If we are UID 0 (root), this will not result in NSS, otherwise it might. This is good, as we want + * to be able to run this in PID 1, where our user ID is 0, but where NSS lookups are not allowed. + + * We don't use getusername_malloc() here, because we don't want to look at $USER, to remain + * consistent with specifer_user_id() below. */ - t = uid_to_name(getuid()); + t = uid_to_name(uid); if (!t) return -ENOMEM; @@ -289,7 +315,17 @@ int specifier_user_name(char specifier, const void *data, const char *root, cons } int specifier_user_id(char specifier, const void *data, const char *root, const void *userdata, char **ret) { - if (asprintf(ret, UID_FMT, getuid()) < 0) + UnitFileScope scope = PTR_TO_INT(data); + uid_t uid; + + assert(ret); + + if (scope == UNIT_FILE_GLOBAL) + return -EINVAL; + + uid = scope == UNIT_FILE_USER ? getuid() : 0; + + if (asprintf(ret, UID_FMT, uid) < 0) return -ENOMEM; return 0; diff --git a/src/shared/specifier.h b/src/shared/specifier.h index c433ee2d63..3c619a6c8f 100644 --- a/src/shared/specifier.h +++ b/src/shared/specifier.h @@ -80,11 +80,11 @@ int specifier_var_tmp_dir(char specifier, const void *data, const char *root, co { 'w', specifier_os_version_id, NULL }, \ { 'W', specifier_os_variant_id, NULL } -#define COMMON_CREDS_SPECIFIERS \ - { 'g', specifier_group_name, NULL }, \ - { 'G', specifier_group_id, NULL }, \ - { 'u', specifier_user_name, NULL }, \ - { 'U', specifier_user_id, NULL } +#define COMMON_CREDS_SPECIFIERS(scope) \ + { 'g', specifier_group_name, INT_TO_PTR(scope) }, \ + { 'G', specifier_group_id, INT_TO_PTR(scope) }, \ + { 'u', specifier_user_name, INT_TO_PTR(scope) }, \ + { 'U', specifier_user_id, INT_TO_PTR(scope) } #define COMMON_TMP_SPECIFIERS \ { 'T', specifier_tmp_dir, NULL }, \ diff --git a/src/test/test-load-fragment.c b/src/test/test-load-fragment.c index c579be4150..9df53cec2b 100644 --- a/src/test/test-load-fragment.c +++ b/src/test/test-load-fragment.c @@ -526,59 +526,74 @@ TEST(install_printf, .sd_booted = true) { assert_se(user = uid_to_name(getuid())); assert_se(asprintf(&uid, UID_FMT, getuid()) >= 0); -#define expect(src, pattern, result) \ +#define expect(scope, src, pattern, result) \ do { \ - _cleanup_free_ char *t = NULL; \ - _cleanup_free_ char \ - *d1 = strdup(i.name), \ - *d2 = strdup(i.path); \ - assert_se(install_name_printf(&src, pattern, NULL, &t) >= 0 || !result); \ + _cleanup_free_ char *t = NULL, \ + *d1 = ASSERT_PTR(strdup(i.name)), \ + *d2 = ASSERT_PTR(strdup(i.path)); \ + int r = install_name_printf(scope, &src, pattern, NULL, &t); \ + assert_se(result ? r >= 0 : r < 0); \ memzero(i.name, strlen(i.name)); \ memzero(i.path, strlen(i.path)); \ - assert_se(d1 && d2); \ if (result) { \ printf("%s\n", t); \ assert_se(streq(t, result)); \ - } else assert_se(t == NULL); \ + } else \ + assert_se(!t); \ strcpy(i.name, d1); \ strcpy(i.path, d2); \ } while (false) - expect(i, "%n", "name.service"); - expect(i, "%N", "name"); - expect(i, "%p", "name"); - expect(i, "%i", ""); - expect(i, "%j", "name"); - expect(i, "%g", group); - expect(i, "%G", gid); - expect(i, "%u", user); - expect(i, "%U", uid); - - expect(i, "%m", mid); - expect(i, "%b", bid); - expect(i, "%H", host); - - expect(i2, "%g", group); - expect(i2, "%G", gid); - expect(i2, "%u", user); - expect(i2, "%U", uid); - - expect(i3, "%n", "name@inst.service"); - expect(i3, "%N", "name@inst"); - expect(i3, "%p", "name"); - expect(i3, "%g", group); - expect(i3, "%G", gid); - expect(i3, "%u", user); - expect(i3, "%U", uid); - - expect(i3, "%m", mid); - expect(i3, "%b", bid); - expect(i3, "%H", host); - - expect(i4, "%g", group); - expect(i4, "%G", gid); - expect(i4, "%u", user); - expect(i4, "%U", uid); + expect(UNIT_FILE_SYSTEM, i, "%n", "name.service"); + expect(UNIT_FILE_SYSTEM, i, "%N", "name"); + expect(UNIT_FILE_SYSTEM, i, "%p", "name"); + expect(UNIT_FILE_SYSTEM, i, "%i", ""); + expect(UNIT_FILE_SYSTEM, i, "%j", "name"); + expect(UNIT_FILE_SYSTEM, i, "%g", "root"); + expect(UNIT_FILE_SYSTEM, i, "%G", "0"); + expect(UNIT_FILE_SYSTEM, i, "%u", "root"); + expect(UNIT_FILE_SYSTEM, i, "%U", "0"); + + expect(UNIT_FILE_SYSTEM, i, "%m", mid); + expect(UNIT_FILE_SYSTEM, i, "%b", bid); + expect(UNIT_FILE_SYSTEM, i, "%H", host); + + expect(UNIT_FILE_SYSTEM, i2, "%g", "root"); + expect(UNIT_FILE_SYSTEM, i2, "%G", "0"); + expect(UNIT_FILE_SYSTEM, i2, "%u", "root"); + expect(UNIT_FILE_SYSTEM, i2, "%U", "0"); + + expect(UNIT_FILE_USER, i2, "%g", group); + expect(UNIT_FILE_USER, i2, "%G", gid); + expect(UNIT_FILE_USER, i2, "%u", user); + expect(UNIT_FILE_USER, i2, "%U", uid); + + /* gcc-12.0.1-0.9.fc36.x86_64 insist that streq(…, NULL) is called, + * even though the call is inside of a conditional where the pointer is checked. :( */ +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wnonnull" + expect(UNIT_FILE_GLOBAL, i2, "%g", NULL); + expect(UNIT_FILE_GLOBAL, i2, "%G", NULL); + expect(UNIT_FILE_GLOBAL, i2, "%u", NULL); + expect(UNIT_FILE_GLOBAL, i2, "%U", NULL); +#pragma GCC diagnostic pop + + expect(UNIT_FILE_SYSTEM, i3, "%n", "name@inst.service"); + expect(UNIT_FILE_SYSTEM, i3, "%N", "name@inst"); + expect(UNIT_FILE_SYSTEM, i3, "%p", "name"); + expect(UNIT_FILE_USER, i3, "%g", group); + expect(UNIT_FILE_USER, i3, "%G", gid); + expect(UNIT_FILE_USER, i3, "%u", user); + expect(UNIT_FILE_USER, i3, "%U", uid); + + expect(UNIT_FILE_SYSTEM, i3, "%m", mid); + expect(UNIT_FILE_SYSTEM, i3, "%b", bid); + expect(UNIT_FILE_SYSTEM, i3, "%H", host); + + expect(UNIT_FILE_USER, i4, "%g", group); + expect(UNIT_FILE_USER, i4, "%G", gid); + expect(UNIT_FILE_USER, i4, "%u", user); + expect(UNIT_FILE_USER, i4, "%U", uid); } static uint64_t make_cap(int cap) { diff --git a/src/test/test-specifier.c b/src/test/test-specifier.c index a45d1bd0b9..dd47f0285e 100644 --- a/src/test/test-specifier.c +++ b/src/test/test-specifier.c @@ -7,6 +7,7 @@ #include "string-util.h" #include "strv.h" #include "tests.h" +#include "unit-file.h" static void test_specifier_escape_one(const char *a, const char *b) { _cleanup_free_ char *x = NULL; @@ -45,7 +46,7 @@ TEST(specifier_escape_strv) { static const Specifier specifier_table[] = { COMMON_SYSTEM_SPECIFIERS, - COMMON_CREDS_SPECIFIERS, + COMMON_CREDS_SPECIFIERS(UNIT_FILE_USER), { 'h', specifier_user_home, NULL }, COMMON_TMP_SPECIFIERS, diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index fcab51c208..aa7ff73a36 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -204,31 +204,6 @@ STATIC_DESTRUCTOR_REGISTER(arg_image, freep); static int specifier_machine_id_safe(char specifier, const void *data, const char *root, const void *userdata, char **ret); static int specifier_directory(char specifier, const void *data, const char *root, const void *userdata, char **ret); -static const Specifier specifier_table[] = { - { 'a', specifier_architecture, NULL }, - { 'b', specifier_boot_id, NULL }, - { 'B', specifier_os_build_id, NULL }, - { 'H', specifier_host_name, NULL }, - { 'l', specifier_short_host_name, NULL }, - { 'm', specifier_machine_id_safe, NULL }, - { 'o', specifier_os_id, NULL }, - { 'v', specifier_kernel_release, NULL }, - { 'w', specifier_os_version_id, NULL }, - { 'W', specifier_os_variant_id, NULL }, - - { 'h', specifier_user_home, NULL }, - - { 'C', specifier_directory, UINT_TO_PTR(DIRECTORY_CACHE) }, - { 'L', specifier_directory, UINT_TO_PTR(DIRECTORY_LOGS) }, - { 'S', specifier_directory, UINT_TO_PTR(DIRECTORY_STATE) }, - { 't', specifier_directory, UINT_TO_PTR(DIRECTORY_RUNTIME) }, - - COMMON_CREDS_SPECIFIERS, - - COMMON_TMP_SPECIFIERS, - {} -}; - static int specifier_machine_id_safe(char specifier, const void *data, const char *root, const void *userdata, char **ret) { int r; @@ -2743,7 +2718,7 @@ static bool should_include_path(const char *path) { return false; } -static int specifier_expansion_from_arg(Item *i) { +static int specifier_expansion_from_arg(const Specifier *specifier_table, Item *i) { int r; assert(i); @@ -2951,6 +2926,30 @@ static int parse_line( assert(line >= 1); assert(buffer); + const Specifier specifier_table[] = { + { 'a', specifier_architecture, NULL }, + { 'b', specifier_boot_id, NULL }, + { 'B', specifier_os_build_id, NULL }, + { 'H', specifier_host_name, NULL }, + { 'l', specifier_short_host_name, NULL }, + { 'm', specifier_machine_id_safe, NULL }, + { 'o', specifier_os_id, NULL }, + { 'v', specifier_kernel_release, NULL }, + { 'w', specifier_os_version_id, NULL }, + { 'W', specifier_os_variant_id, NULL }, + + { 'h', specifier_user_home, NULL }, + + { 'C', specifier_directory, UINT_TO_PTR(DIRECTORY_CACHE) }, + { 'L', specifier_directory, UINT_TO_PTR(DIRECTORY_LOGS) }, + { 'S', specifier_directory, UINT_TO_PTR(DIRECTORY_STATE) }, + { 't', specifier_directory, UINT_TO_PTR(DIRECTORY_RUNTIME) }, + + COMMON_CREDS_SPECIFIERS(arg_user ? UNIT_FILE_USER : UNIT_FILE_SYSTEM), + COMMON_TMP_SPECIFIERS, + {} + }; + r = extract_many_words( &buffer, NULL, @@ -3155,7 +3154,7 @@ static int parse_line( if (!should_include_path(i.path)) return 0; - r = specifier_expansion_from_arg(&i); + r = specifier_expansion_from_arg(specifier_table, &i); if (r == -ENXIO) return log_unresolvable_specifier(fname, line); if (r < 0) { diff --git a/test/test-systemctl-enable.sh b/test/test-systemctl-enable.sh index da1fffe944..8ac1342b91 100644 --- a/test/test-systemctl-enable.sh +++ b/test/test-systemctl-enable.sh @@ -500,9 +500,10 @@ check_alias t '' && { echo "Expected failure" >&2; exit 1; } check_alias T '' && { echo "Expected failure" >&2; exit 1; } check_alias V '' && { echo "Expected failure" >&2; exit 1; } -# FIXME: we use the calling user instead of root :( -check_alias g root || : -check_alias G 0 || : +check_alias g root +check_alias G 0 +check_alias u root +check_alias U 0 check_alias i "" @@ -521,10 +522,6 @@ check_alias N 'some-some-link6@' check_alias p 'some-some-link6' -# FIXME: we use the calling user instead of root :( -check_alias u root || : -check_alias U 0 || : - check_alias v "$(uname -r)" check_alias % '%' && { echo "Expected failure because % is not legal in unit name" >&2; exit 1; } diff --git a/test/test-systemd-tmpfiles.py b/test/test-systemd-tmpfiles.py index 3376029463..ba42b3fa37 100755 --- a/test/test-systemd-tmpfiles.py +++ b/test/test-systemd-tmpfiles.py @@ -98,13 +98,13 @@ def test_valid_specifiers(*, user): test_content('f {} - - - - %b', '{}'.format(id128.get_boot().hex), user=user) test_content('f {} - - - - %H', '{}'.format(socket.gethostname()), user=user) test_content('f {} - - - - %v', '{}'.format(os.uname().release), user=user) - test_content('f {} - - - - %U', '{}'.format(os.getuid()), user=user) - test_content('f {} - - - - %G', '{}'.format(os.getgid()), user=user) + test_content('f {} - - - - %U', '{}'.format(os.getuid() if user else 0), user=user) + test_content('f {} - - - - %G', '{}'.format(os.getgid() if user else 0), user=user) - puser = pwd.getpwuid(os.getuid()) + puser = pwd.getpwuid(os.getuid() if user else 0) test_content('f {} - - - - %u', '{}'.format(puser.pw_name), user=user) - pgroup = grp.getgrgid(os.getgid()) + pgroup = grp.getgrgid(os.getgid() if user else 0) test_content('f {} - - - - %g', '{}'.format(pgroup.gr_name), user=user) # Note that %h is the only specifier in which we look the environment,