From 9154bd57db6aed4823830638126b95bf7f63f25a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 12 Oct 2022 07:58:54 +0200 Subject: [PATCH 1/2] basic/chase-symlinks: add note that CHASE_WARN is not for PID 1 This should be enough to make us remember that constraint. A more heavy-weight approach like renaming the option to CHASE_WARN_NSS_LOOKUP would be possible, but it'd make the callers quite verbose and doesn't seem worth it. --- src/basic/chase-symlinks.c | 5 +++-- src/basic/chase-symlinks.h | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/basic/chase-symlinks.c b/src/basic/chase-symlinks.c index ad1c92900b..afab54f067 100644 --- a/src/basic/chase-symlinks.c +++ b/src/basic/chase-symlinks.c @@ -125,11 +125,12 @@ int chase_symlinks( * * 4. With CHASE_SAFE: in this case the path must not contain unsafe transitions, i.e. transitions from * unprivileged to privileged files or directories. In such cases the return value is -ENOLINK. If - * CHASE_WARN is also set, a warning describing the unsafe transition is emitted. + * CHASE_WARN is also set, a warning describing the unsafe transition is emitted. CHASE_WARN cannot + * be used in PID 1. * * 5. With CHASE_NO_AUTOFS: in this case if an autofs mount point is encountered, path normalization * is aborted and -EREMOTE is returned. If CHASE_WARN is also set, a warning showing the path of - * the mount point is emitted. + * the mount point is emitted. CHASE_WARN cannot be used in PID 1. */ /* A root directory of "/" or "" is identical to none */ diff --git a/src/basic/chase-symlinks.h b/src/basic/chase-symlinks.h index 7e45b0cbab..a9ee58f9f7 100644 --- a/src/basic/chase-symlinks.h +++ b/src/basic/chase-symlinks.h @@ -15,7 +15,8 @@ typedef enum ChaseSymlinksFlags { CHASE_STEP = 1 << 5, /* Just execute a single step of the normalization */ CHASE_NOFOLLOW = 1 << 6, /* Do not follow the path's right-most component. With ret_fd, when the path's * right-most component refers to symlink, return O_PATH fd of the symlink. */ - CHASE_WARN = 1 << 7, /* Emit an appropriate warning when an error is encountered */ + CHASE_WARN = 1 << 7, /* Emit an appropriate warning when an error is encountered. + * Note: this may do an NSS lookup, hence this flag cannot be used in PID 1. */ } ChaseSymlinksFlags; bool unsafe_transition(const struct stat *a, const struct stat *b); From 734f96b8490a2c48712ff6754a84fcaeac3d53c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 7 Oct 2022 17:34:53 +0200 Subject: [PATCH 2/2] shared/condition: avoid nss lookup in PID1 PID 1 is not allowed to do nss lookups because this may take a long time or even deadlock. While at it, the comparisons are reordered to do the "easy" comparisons which only require a string comparison first. Delay parsing of the UID until it is really necessary. The result is the same, because we know that "root" and "nobody" parse as valid. --- src/shared/condition.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/shared/condition.c b/src/shared/condition.c index aa34e1e285..310ffcbdc6 100644 --- a/src/shared/condition.c +++ b/src/shared/condition.c @@ -338,31 +338,36 @@ static int condition_test_cpus(Condition *c, char **env) { static int condition_test_user(Condition *c, char **env) { uid_t id; int r; - _cleanup_free_ char *username = NULL; - const char *u; assert(c); assert(c->parameter); assert(c->type == CONDITION_USER); + /* Do the quick&easy comparisons first, and only parse the UID later. */ + if (streq(c->parameter, "root")) + return getuid() == 0 || geteuid() == 0; + if (streq(c->parameter, NOBODY_USER_NAME)) + return getuid() == UID_NOBODY || geteuid() == UID_NOBODY; + if (streq(c->parameter, "@system")) + return uid_is_system(getuid()) || uid_is_system(geteuid()); + r = parse_uid(c->parameter, &id); if (r >= 0) return id == getuid() || id == geteuid(); - if (streq("@system", c->parameter)) - return uid_is_system(getuid()) || uid_is_system(geteuid()); + if (getpid_cached() == 1) /* We already checked for "root" above, and we know that + * PID 1 is running as root, hence we know it cannot match. */ + return false; - username = getusername_malloc(); + /* getusername_malloc() may do an nss lookup, which is not allowed in PID 1. */ + _cleanup_free_ char *username = getusername_malloc(); if (!username) return -ENOMEM; if (streq(username, c->parameter)) return 1; - if (getpid_cached() == 1) - return streq(c->parameter, "root"); - - u = c->parameter; + const char *u = c->parameter; r = get_user_creds(&u, &id, NULL, NULL, NULL, USER_CREDS_ALLOW_MISSING); if (r < 0) return 0;