shared/specifier: fix %u/%U/%g/%G when called as unprivileged user

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.

diff --git a/src/core/unit-printf.c b/src/core/unit-printf.c
index 4a86a3c0d6c02dc15911541f8b35a8f44ed5e656..0cdfcd6efdb60d250c063e1234e38e7589863d31 100644 (file)
--- a/src/core/unit-printf.c
+++ b/src/core/unit-printf.c
@@ -183,7 +183,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),
                 {}
         };
 
@@ -253,7 +253,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 9c121cbaf7993ff3596d09d758689ddb6db68439..571c4818e2b71b39c3e75e34211fa8814ff5d93d 100644 (file)
--- a/src/shared/install-printf.c
+++ b/src/shared/install-printf.c
@@ -97,7 +97,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[] = {
@@ -109,7 +114,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 5ca9406797a793b3fea90c0f4758fe46a272ed24..d2cccdf66d999ae07a468bc90490e57b427be272 100644 (file)
--- 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 bff1930828ef63a99b1480e33ef643c11e38a87e..4f43f190d615166842f4b77fa933434d0ee3d2c2 100644 (file)
--- a/src/shared/install.c
+++ b/src/shared/install.c
@@ -1157,7 +1157,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);
@@ -1188,6 +1188,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;
@@ -1205,7 +1206,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);
@@ -1772,7 +1773,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;
@@ -1858,7 +1859,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 e994884196d2355457f4b8b35f984ba46b02c11f..777881b325f57415134f587cdfa28792598047e7 100644 (file)
--- a/src/shared/specifier.c
+++ b/src/shared/specifier.c
@@ -22,6 +22,7 @@
 #include "specifier.h"
 #include "string-util.h"
 #include "strv.h"
+#include "unit-file.h"
 #include "user-util.h"
 
 /*
@@ -309,11 +310,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;
 
         assert(ret);
 
-        t = gid_to_name(getgid());
+        if (scope == UNIT_FILE_GLOBAL)
+                return -EINVAL;
+
+        t = gid_to_name(scope == UNIT_FILE_USER ? getgid() : 0);
         if (!t)
                 return -ENOMEM;
 
@@ -322,27 +327,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) {
+        UnitFileScope scope = PTR_TO_INT(data);
+        gid_t gid;
+
         assert(ret);
 
-        if (asprintf(ret, UID_FMT, getgid()) < 0)
+        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;
 
         assert(ret);
 
-        /* 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.
+        if (scope == UNIT_FILE_GLOBAL)
+                return -EINVAL;
+
+        uid = scope == UNIT_FILE_USER ? getuid() : 0;
 
-         * 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 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;
 
@@ -351,9 +371,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) {
+        UnitFileScope scope = PTR_TO_INT(data);
+        uid_t uid;
+
         assert(ret);
 
-        if (asprintf(ret, UID_FMT, getuid()) < 0)
+        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 2f2553cfdc0e13cb900c396c083ef63eefcba1a4..78b10f846717258e77b01d1f5e1ac128a3a3a92d 100644 (file)
--- a/src/shared/specifier.h
+++ b/src/shared/specifier.h
@@ -84,11 +84,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 52f6096eee14d127fa9b0d165113e4d82b67a576..40cd7ea7e627c60cc07d8d66fd787e9e6ea6a7b6 100644 (file)
--- a/src/test/test-load-fragment.c
+++ b/src/test/test-load-fragment.c
@@ -510,59 +510,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 bb1c651c2abd284bab4d27c6bb342f3b1ee83d8c..947a32acf75ba46e3dcfd78bc0ce07206ba0e6d2 100644 (file)
--- a/src/test/test-specifier.c
+++ b/src/test/test-specifier.c
@@ -8,6 +8,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;
@@ -46,7 +47,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 f0190f9b4cd5f7f7b9fd54ebcef6f7cb746d86ed..17b9c6ab9ae8650d5616607601c21761e9fb4d85 100644 (file)
--- 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;
 
@@ -2737,7 +2712,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);
@@ -2944,6 +2919,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,
@@ -3148,7 +3147,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 da1fffe9444abcaad46a56a62eea415037779cc4..8ac1342b91f194a172c84827adab1f5e625a527b 100644 (file)
--- 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 3376029463a133fd0b78f181b6975986700a15bb..ba42b3fa3702833cada8fc530807aa2d7b95d499 100755 (executable)
--- 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,