From 87df0601074b024ab591534e0e78b2d1e7013a8d Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>

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,