From 905ec0c0af3459477a922db8c314537659fc0b50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 19 Aug 2022 13:44:54 +0200 Subject: [PATCH 1/9] sysusers: rename output params with 'ret' --- src/sysusers/sysusers.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/sysusers/sysusers.c b/src/sysusers/sysusers.c index 9c1abf984e..eacb063349 100644 --- a/src/sysusers/sysusers.c +++ b/src/sysusers/sysusers.c @@ -981,7 +981,7 @@ static int root_stat(const char *p, struct stat *st) { return RET_NERRNO(stat(fix, st)); } -static int read_id_from_file(Item *i, uid_t *_uid, gid_t *_gid) { +static int read_id_from_file(Item *i, uid_t *ret_uid, gid_t *ret_gid) { struct stat st; bool found_uid = false, found_gid = false; uid_t uid = 0; @@ -990,13 +990,13 @@ static int read_id_from_file(Item *i, uid_t *_uid, gid_t *_gid) { assert(i); /* First, try to get the GID directly */ - if (_gid && i->gid_path && root_stat(i->gid_path, &st) >= 0) { + if (ret_gid && i->gid_path && root_stat(i->gid_path, &st) >= 0) { gid = st.st_gid; found_gid = true; } /* Then, try to get the UID directly */ - if ((_uid || (_gid && !found_gid)) + if ((ret_uid || (ret_gid && !found_gid)) && i->uid_path && root_stat(i->uid_path, &st) >= 0) { @@ -1004,14 +1004,14 @@ static int read_id_from_file(Item *i, uid_t *_uid, gid_t *_gid) { found_uid = true; /* If we need the gid, but had no success yet, also derive it from the UID path */ - if (_gid && !found_gid) { + if (ret_gid && !found_gid) { gid = st.st_gid; found_gid = true; } } /* If that didn't work yet, then let's reuse the GID as UID */ - if (_uid && !found_uid && i->gid_path) { + if (ret_uid && !found_uid && i->gid_path) { if (found_gid) { uid = (uid_t) gid; @@ -1022,18 +1022,18 @@ static int read_id_from_file(Item *i, uid_t *_uid, gid_t *_gid) { } } - if (_uid) { + if (ret_uid) { if (!found_uid) return 0; - *_uid = uid; + *ret_uid = uid; } - if (_gid) { + if (ret_gid) { if (!found_gid) return 0; - *_gid = gid; + *ret_gid = gid; } return 1; From 87c696f24707d0785a0626164d0b15a376a6c586 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 19 Aug 2022 15:45:22 +0200 Subject: [PATCH 2/9] sysusers: use log_syntax (changes prefix from '[%s:%u]' to '%s:%u:') This makes the sysusers use the same message convention as other tools. Also adds the prefix in a few places. --- src/sysusers/sysusers.c | 134 ++++++++++------------ test/test-sysusers.sh.in | 2 +- test/test-sysusers/unhappy-1.expected-err | 2 +- 3 files changed, 65 insertions(+), 73 deletions(-) diff --git a/src/sysusers/sysusers.c b/src/sysusers/sysusers.c index eacb063349..36d86fb6cb 100644 --- a/src/sysusers/sysusers.c +++ b/src/sysusers/sysusers.c @@ -1506,22 +1506,22 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) { r = extract_many_words(&p, NULL, EXTRACT_UNQUOTE, &action, &name, &id, &description, &home, &shell, NULL); if (r < 0) - return log_error_errno(r, "[%s:%u] Syntax error.", fname, line); + return log_syntax(NULL, LOG_ERR, fname, line, r, "Syntax error."); if (r < 2) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "[%s:%u] Missing action and name columns.", fname, line); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), + "Missing action and name columns."); if (!isempty(p)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "[%s:%u] Trailing garbage.", fname, line); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), + "Trailing garbage."); /* Verify action */ if (strlen(action) != 1) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "[%s:%u] Unknown modifier '%s'", fname, line, action); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), + "Unknown modifier '%s'.", action); if (!IN_SET(action[0], ADD_USER, ADD_GROUP, ADD_MEMBER, ADD_RANGE)) - return log_error_errno(SYNTHETIC_ERRNO(EBADMSG), - "[%s:%u] Unknown command type '%c'.", fname, line, action[0]); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), + "Unknown command type '%c'.", action[0]); /* Verify name */ if (empty_or_dash(name)) @@ -1530,12 +1530,11 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) { if (name) { r = specifier_printf(name, NAME_MAX, system_and_tmp_specifier_table, arg_root, NULL, &resolved_name); if (r < 0) - return log_error_errno(r, "[%s:%u] Failed to replace specifiers in '%s': %m", fname, line, name); + return log_syntax(NULL, LOG_ERR, fname, line, r, "Failed to replace specifiers in '%s': %m", name); if (!valid_user_group_name(resolved_name, 0)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "[%s:%u] '%s' is not a valid user or group name.", - fname, line, resolved_name); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), + "'%s' is not a valid user or group name.", resolved_name); } /* Verify id */ @@ -1545,8 +1544,8 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) { if (id) { r = specifier_printf(id, PATH_MAX-1, system_and_tmp_specifier_table, arg_root, NULL, &resolved_id); if (r < 0) - return log_error_errno(r, "[%s:%u] Failed to replace specifiers in '%s': %m", - fname, line, name); + return log_syntax(NULL, LOG_ERR, fname, line, r, + "Failed to replace specifiers in '%s': %m", name); } /* Verify description */ @@ -1556,13 +1555,12 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) { if (description) { r = specifier_printf(description, LONG_LINE_MAX, system_and_tmp_specifier_table, arg_root, NULL, &resolved_description); if (r < 0) - return log_error_errno(r, "[%s:%u] Failed to replace specifiers in '%s': %m", - fname, line, description); + return log_syntax(NULL, LOG_ERR, fname, line, r, + "Failed to replace specifiers in '%s': %m", description); if (!valid_gecos(resolved_description)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "[%s:%u] '%s' is not a valid GECOS field.", - fname, line, resolved_description); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), + "'%s' is not a valid GECOS field.", resolved_description); } /* Verify home */ @@ -1572,13 +1570,12 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) { if (home) { r = specifier_printf(home, PATH_MAX-1, system_and_tmp_specifier_table, arg_root, NULL, &resolved_home); if (r < 0) - return log_error_errno(r, "[%s:%u] Failed to replace specifiers in '%s': %m", - fname, line, home); + return log_syntax(NULL, LOG_ERR, fname, line, r, + "Failed to replace specifiers in '%s': %m", home); if (!valid_home(resolved_home)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "[%s:%u] '%s' is not a valid home directory field.", - fname, line, resolved_home); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), + "'%s' is not a valid home directory field.", resolved_home); } /* Verify shell */ @@ -1588,63 +1585,57 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) { if (shell) { r = specifier_printf(shell, PATH_MAX-1, system_and_tmp_specifier_table, arg_root, NULL, &resolved_shell); if (r < 0) - return log_error_errno(r, "[%s:%u] Failed to replace specifiers in '%s': %m", - fname, line, shell); + return log_syntax(NULL, LOG_ERR, fname, line, r, + "Failed to replace specifiers in '%s': %m", shell); if (!valid_shell(resolved_shell)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "[%s:%u] '%s' is not a valid login shell field.", - fname, line, resolved_shell); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), + "'%s' is not a valid login shell field.", resolved_shell); } switch (action[0]) { case ADD_RANGE: if (resolved_name) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "[%s:%u] Lines of type 'r' don't take a name field.", - fname, line); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), + "Lines of type 'r' don't take a name field."); if (!resolved_id) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "[%s:%u] Lines of type 'r' require an ID range in the third field.", - fname, line); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), + "Lines of type 'r' require an ID range in the third field."); if (description || home || shell) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "[%s:%u] Lines of type '%c' don't take a %s field.", - fname, line, action[0], - description ? "GECOS" : home ? "home directory" : "login shell"); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), + "Lines of type '%c' don't take a %s field.", + action[0], + description ? "GECOS" : home ? "home directory" : "login shell"); r = uid_range_add_str(&uid_range, &n_uid_range, resolved_id); if (r < 0) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "[%s:%u] Invalid UID range %s.", fname, line, resolved_id); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), + "Invalid UID range %s.", resolved_id); return 0; case ADD_MEMBER: { /* Try to extend an existing member or group item */ if (!name) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "[%s:%u] Lines of type 'm' require a user name in the second field.", - fname, line); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), + "Lines of type 'm' require a user name in the second field."); if (!resolved_id) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "[%s:%u] Lines of type 'm' require a group name in the third field.", - fname, line); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), + "Lines of type 'm' require a group name in the third field."); if (!valid_user_group_name(resolved_id, 0)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "[%s:%u] '%s' is not a valid user or group name.", - fname, line, resolved_id); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), + "'%s' is not a valid user or group name.", resolved_id); if (description || home || shell) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "[%s:%u] Lines of type '%c' don't take a %s field.", - fname, line, action[0], - description ? "GECOS" : home ? "home directory" : "login shell"); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), + "Lines of type '%c' don't take a %s field.", + action[0], + description ? "GECOS" : home ? "home directory" : "login shell"); r = string_strv_ordered_hashmap_put(&members, resolved_id, resolved_name); if (r < 0) @@ -1655,9 +1646,8 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) { case ADD_USER: if (!name) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "[%s:%u] Lines of type 'u' require a user name in the second field.", - fname, line); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), + "Lines of type 'u' require a user name in the second field."); r = ordered_hashmap_ensure_allocated(&users, &item_hash_ops); if (r < 0) @@ -1679,7 +1669,8 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) { if (valid_user_group_name(gid, 0)) i->group_name = TAKE_PTR(gid); else - return log_error_errno(r, "Failed to parse GID: '%s': %m", id); + return log_syntax(NULL, LOG_ERR, fname, line, r, + "Failed to parse GID: '%s': %m", id); } else { i->gid_set = true; i->id_set_strict = true; @@ -1689,7 +1680,8 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) { if (!streq(resolved_id, "-")) { r = parse_uid(resolved_id, &i->uid); if (r < 0) - return log_error_errno(r, "Failed to parse UID: '%s': %m", id); + return log_syntax(NULL, LOG_ERR, fname, line, r, + "Failed to parse UID: '%s': %m", id); i->uid_set = true; } } @@ -1704,15 +1696,14 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) { case ADD_GROUP: if (!name) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "[%s:%u] Lines of type 'g' require a user name in the second field.", - fname, line); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), + "Lines of type 'g' require a user name in the second field."); if (description || home || shell) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "[%s:%u] Lines of type '%c' don't take a %s field.", - fname, line, action[0], - description ? "GECOS" : home ? "home directory" : "login shell"); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), + "Lines of type '%c' don't take a %s field.", + action[0], + description ? "GECOS" : home ? "home directory" : "login shell"); r = ordered_hashmap_ensure_allocated(&groups, &item_hash_ops); if (r < 0) @@ -1729,7 +1720,8 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) { } else { r = parse_gid(resolved_id, &i->gid); if (r < 0) - return log_error_errno(r, "Failed to parse GID: '%s': %m", id); + return log_syntax(NULL, LOG_ERR, fname, line, r, + "Failed to parse GID: '%s': %m", id); i->gid_set = true; } @@ -1749,9 +1741,9 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) { if (existing) { /* Two identical items are fine */ if (!item_equal(existing, i)) - log_warning("%s:%u: conflict with earlier configuration for %s '%s', ignoring line.", - fname, line, - item_type_to_string(i->type), i->name); + log_syntax(NULL, LOG_WARNING, fname, line, SYNTHETIC_ERRNO(EUCLEAN), + "Conflict with earlier configuration for %s '%s', ignoring line.", + item_type_to_string(i->type), i->name); return 0; } diff --git a/test/test-sysusers.sh.in b/test/test-sysusers.sh.in index c9d9bd993b..950dc297d8 100755 --- a/test/test-sysusers.sh.in +++ b/test/test-sysusers.sh.in @@ -152,7 +152,7 @@ for f in $(ls -1 $SOURCE/unhappy-*.input | sort -V); do echo "*** Running test $f" prepare_testdir ${f%.input} cp $f $TESTDIR/usr/lib/sysusers.d/test.conf - $SYSUSERS --root=$TESTDIR 2>&1 | tail -n1 > $TESTDIR/err + $SYSUSERS --root=$TESTDIR 2>&1 | tail -n1 | sed -r 's/^[^:]+:[^:]+://' >$TESTDIR/err if ! diff -u $TESTDIR/err ${f%.*}.expected-err; then echo "**** Unexpected error output for $f" cat $TESTDIR/err diff --git a/test/test-sysusers/unhappy-1.expected-err b/test/test-sysusers/unhappy-1.expected-err index d3342402e9..f6b1b3c5e6 100644 --- a/test/test-sysusers/unhappy-1.expected-err +++ b/test/test-sysusers/unhappy-1.expected-err @@ -1 +1 @@ -Failed to parse UID: '9999999999': Numerical result out of range + Failed to parse UID: '9999999999': Numerical result out of range From eef74f912528764be3cb1925b8f72ad98f0d99c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 19 Aug 2022 15:49:16 +0200 Subject: [PATCH 3/9] sysusers: do not reject non-simplified paths for shell/home /home/zbyszek/src/systemd-work/testcase.conf:3: '//sbin//nologin' is not a valid login shell field. This isn't very useful. The usual argument holds: people use templates to construct config, so paths may have doubled slashes and similar. Let's simplify paths so that the value that is pushed to /etc/passwd is nice and clean. --- src/sysusers/sysusers.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/sysusers/sysusers.c b/src/sysusers/sysusers.c index 36d86fb6cb..4bb173da46 100644 --- a/src/sysusers/sysusers.c +++ b/src/sysusers/sysusers.c @@ -1573,6 +1573,8 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) { return log_syntax(NULL, LOG_ERR, fname, line, r, "Failed to replace specifiers in '%s': %m", home); + path_simplify(resolved_home); + if (!valid_home(resolved_home)) return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), "'%s' is not a valid home directory field.", resolved_home); @@ -1588,6 +1590,8 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) { return log_syntax(NULL, LOG_ERR, fname, line, r, "Failed to replace specifiers in '%s': %m", shell); + path_simplify(resolved_shell); + if (!valid_shell(resolved_shell)) return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), "'%s' is not a valid login shell field.", resolved_shell); From 0336c23e98caf9a8e374ef3823858a9926077ec8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 19 Aug 2022 15:56:36 +0200 Subject: [PATCH 4/9] man: fix description of the config file argument It's any relative path, not just "basename", as was stated before. --- man/systemd-sysusers.xml | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/man/systemd-sysusers.xml b/man/systemd-sysusers.xml index 9011cdb755..b399b3b04c 100644 --- a/man/systemd-sysusers.xml +++ b/man/systemd-sysusers.xml @@ -35,23 +35,22 @@ Description - systemd-sysusers creates system users and - groups, based on the file format and location specified in + systemd-sysusers creates system users and groups, based on files in the format + described in sysusers.d5. - If invoked with no arguments, it applies all directives from all files - found in the directories specified by - sysusers.d5. - When invoked with positional arguments, if option - is specified, arguments - specified on the command line are used instead of the configuration file - PATH. Otherwise, just the configuration specified by - the command line arguments is executed. The string - may be - specified instead of a filename to instruct systemd-sysusers - to read the configuration from standard input. If only the basename of a file is - specified, all configuration directories are searched for a matching file and - the file found that has the highest priority is executed. + If invoked with no arguments, it applies all directives from all files found in the directories + specified by + sysusers.d5. When + invoked with positional arguments, if option + is specified, arguments specified on the command line are used instead of the configuration file + PATH. Otherwise, just the configuration specified by the command line + arguments is executed. The string - may be specified instead of a filename to instruct + systemd-sysusers to read the configuration from standard input. If the argument is a + relative path, all configuration directories are searched for a matching file and the file found that has + the highest priority is executed. If the argument is an absolute path, that file is used directly without + searching of the configuration directories. From 5f465fda4ec9f1e70a1bb993944ea92b2469b0db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 19 Aug 2022 15:38:56 +0200 Subject: [PATCH 5/9] sysusers: do not warn about values that equivalent We'd warn that "-" and "/sbin/nologin" are different, even even though "/sbin/nologin" is the default we'd use. So let's stop warning in all cases where the config would lead to the same file, also under different paths, or when both shells are nologin shells. The general idea is to avoid warnings when sysusers config is moved between packages (and not exactly the same), or when it is generated from some template and the details change in an unimportant way. We try to chase symlinks. This means that on unmerged-usr systems we'll find that e.g. /usr/bin/bash and /bin/bash are equivalent if the basic fs structure is already in place (bash doesn't actually have to be installed, enough that the /bin symlink exists). I think this is a good result: after all, /bin/bash and /usr/bin/bash *may* be different things on an unmerged-usr system. Fixes #24215. --- src/sysusers/sysusers.c | 60 +++++++++++++++++++++++++++++++++++------ 1 file changed, 52 insertions(+), 8 deletions(-) diff --git a/src/sysusers/sysusers.c b/src/sysusers/sysusers.c index 4bb173da46..5eb67ea084 100644 --- a/src/sysusers/sysusers.c +++ b/src/sysusers/sysusers.c @@ -4,6 +4,7 @@ #include #include "alloc-util.h" +#include "chase-symlinks.h" #include "conf-files.h" #include "copy.h" #include "creds-util.h" @@ -27,6 +28,7 @@ #include "set.h" #include "smack-util.h" #include "specifier.h" +#include "stat-util.h" #include "string-util.h" #include "strv.h" #include "sync-util.h" @@ -390,8 +392,14 @@ static int putsgent_with_members(const struct sgrp *sg, FILE *gshadow) { } #endif -static const char* default_shell(uid_t uid) { - return uid == 0 ? "/bin/sh" : NOLOGIN; +static const char* pick_shell(const Item *i) { + if (i->type != ADD_USER) + return NULL; + if (i->shell) + return i->shell; + if (i->uid_set && i->uid == 0) + return "/bin/sh"; + return NOLOGIN; } static int write_temporary_passwd(const char *passwd_path, FILE **tmpfile, char **tmpfile_path) { @@ -473,7 +481,7 @@ static int write_temporary_passwd(const char *passwd_path, FILE **tmpfile, char /* Initialize the shell to nologin, with one exception: * for root we patch in something special */ - .pw_shell = i->shell ?: (char*) default_shell(i->uid), + .pw_shell = (char*) pick_shell(i), }; /* Try to pick up the shell for this account via the credentials logic */ @@ -1444,7 +1452,9 @@ static int add_implicit(void) { return 0; } -static bool item_equal(Item *a, Item *b) { +static int item_equivalent(Item *a, Item *b) { + int r; + assert(a); assert(b); @@ -1454,6 +1464,7 @@ static bool item_equal(Item *a, Item *b) { if (!streq_ptr(a->name, b->name)) return false; + /* Paths were simplified previously, so we can use streq. */ if (!streq_ptr(a->uid_path, b->uid_path)) return false; @@ -1478,8 +1489,38 @@ static bool item_equal(Item *a, Item *b) { if (!streq_ptr(a->home, b->home)) return false; - if (!streq_ptr(a->shell, b->shell)) - return false; + /* Check if the two paths refer to the same file. + * If the paths are equal (after normalization), it's obviously the same file. + * If both paths specify a nologin shell, treat them as the same (e.g. /bin/true and /bin/false). + * Otherwise, try to resolve the paths, and see if we get the same result, (e.g. /sbin/nologin and + * /usr/sbin/nologin). + * If we can't resolve something, treat different paths as different. */ + + const char *a_shell = pick_shell(a), + *b_shell = pick_shell(b); + if (!path_equal_ptr(a_shell, b_shell) && + !(is_nologin_shell(a_shell) && is_nologin_shell(b_shell))) { + _cleanup_free_ char *pa = NULL, *pb = NULL; + + r = chase_symlinks(a_shell, arg_root, CHASE_PREFIX_ROOT | CHASE_NONEXISTENT, &pa, NULL); + if (r < 0) { + log_full_errno(ERRNO_IS_RESOURCE(r) ? LOG_ERR : LOG_DEBUG, + r, "Failed to look up path '%s%s%s': %m", + strempty(arg_root), arg_root ? "/" : "", a_shell); + return ERRNO_IS_RESOURCE(r) ? r : false; + } + + r = chase_symlinks(b_shell, arg_root, CHASE_PREFIX_ROOT | CHASE_NONEXISTENT, &pb, NULL); + if (r < 0) { + log_full_errno(ERRNO_IS_RESOURCE(r) ? LOG_ERR : LOG_DEBUG, + r, "Failed to look up path '%s%s%s': %m", + strempty(arg_root), arg_root ? "/" : "", b_shell); + return ERRNO_IS_RESOURCE(r) ? r : false; + } + + if (!path_equal(pa, pb)) + return false; + } return true; } @@ -1743,8 +1784,11 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) { existing = ordered_hashmap_get(h, i->name); if (existing) { - /* Two identical items are fine */ - if (!item_equal(existing, i)) + /* Two functionally-equivalent items are fine */ + r = item_equivalent(existing, i); + if (r < 0) + return r; + if (r == 0) log_syntax(NULL, LOG_WARNING, fname, line, SYNTHETIC_ERRNO(EUCLEAN), "Conflict with earlier configuration for %s '%s', ignoring line.", item_type_to_string(i->type), i->name); From 8a7adccbdb23ae6fee82840ef41d17d5e568a8f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 19 Aug 2022 16:43:45 +0200 Subject: [PATCH 6/9] various: try to use DEFAULT_USER_SHELL for root too /bin/sh as a shell is punishing. There is no good reason to make the occasional root login unpleasant. Since /bin/sh is usually /bin/bash in compat mode, i.e. if one is available, the other will be too, /bin/bash is almost as good as a default. But to avoid a regression in the situation where /bin/bash (or DEFAULT_USER_SHELL) is not installed, we check with access() and fall back to /bin/sh. This should make this change in behaviour less risky. (FWIW, e.g. Fedora/RHEL use /bin/bash as default for root.) This is a follow-up of sorts for 53350c7bbade8c5f357aa3d1029ef9b2208ea675, which added the default-user-shell option, but most likely with the idea of using /bin/bash less ;) Fixes #24369. --- man/systemd.unit.xml | 2 +- src/basic/user-util.c | 21 ++++++-- src/basic/user-util.h | 1 + src/firstboot/firstboot.c | 2 +- src/nss-systemd/nss-systemd.c | 60 +++++++++++++---------- src/sysusers/sysusers.c | 2 +- src/test/test-user-util.c | 4 +- test/test-execute/exec-specifier.service | 2 +- test/test-execute/exec-specifier@.service | 2 +- 9 files changed, 59 insertions(+), 37 deletions(-) diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml index ea95ba8869..c4c6be98a5 100644 --- a/man/systemd.unit.xml +++ b/man/systemd.unit.xml @@ -2118,7 +2118,7 @@ Note that this setting is not influenced by the Us %s User shell - This is the shell of the user running the service manager instance. In case of the system manager this resolves to /bin/sh. + This is the shell of the user running the service manager instance. %S diff --git a/src/basic/user-util.c b/src/basic/user-util.c index 16185332f9..37ccb667b2 100644 --- a/src/basic/user-util.c +++ b/src/basic/user-util.c @@ -13,6 +13,7 @@ #include "sd-messages.h" #include "alloc-util.h" +#include "chase-symlinks.h" #include "errno-util.h" #include "fd-util.h" #include "fileio.h" @@ -136,7 +137,6 @@ char *getusername_malloc(void) { } bool is_nologin_shell(const char *shell) { - return PATH_IN_SET(shell, /* 'nologin' is the friendliest way to disable logins for a user account. It prints a nice * message and exits. Different distributions place the binary at different places though, @@ -154,6 +154,21 @@ bool is_nologin_shell(const char *shell) { "/usr/bin/true"); } +const char* default_root_shell(const char *root) { + /* We want to use the preferred shell, i.e. DEFAULT_USER_SHELL, which usually + * will be /bin/bash. Fall back to /bin/sh if DEFAULT_USER_SHELL is not found, + * or any access errors. */ + + int r = chase_symlinks(DEFAULT_USER_SHELL, root, CHASE_PREFIX_ROOT, NULL, NULL); + if (r < 0 && r != -ENOENT) + log_debug_errno(r, "Failed to look up shell '%s%s%s': %m", + strempty(root), root ? "/" : "", DEFAULT_USER_SHELL); + if (r > 0) + return DEFAULT_USER_SHELL; + + return "/bin/sh"; +} + static int synthesize_user_creds( const char **username, uid_t *uid, gid_t *gid, @@ -176,7 +191,7 @@ static int synthesize_user_creds( *home = "/root"; if (shell) - *shell = "/bin/sh"; + *shell = default_root_shell(NULL); return 0; } @@ -635,7 +650,7 @@ int get_shell(char **_s) { /* Hardcode shell for root and nobody to avoid NSS */ u = getuid(); if (u == 0) { - s = strdup("/bin/sh"); + s = strdup(default_root_shell(NULL)); if (!s) return -ENOMEM; diff --git a/src/basic/user-util.h b/src/basic/user-util.h index e1692c4f66..c8f0758541 100644 --- a/src/basic/user-util.h +++ b/src/basic/user-util.h @@ -130,6 +130,7 @@ int putsgent_sane(const struct sgrp *sg, FILE *stream); #endif bool is_nologin_shell(const char *shell); +const char* default_root_shell(const char *root); int is_this_me(const char *username); diff --git a/src/firstboot/firstboot.c b/src/firstboot/firstboot.c index 9169129a68..fd9954b54d 100644 --- a/src/firstboot/firstboot.c +++ b/src/firstboot/firstboot.c @@ -755,7 +755,7 @@ static int write_root_passwd(const char *passwd_path, const char *password, cons .pw_gid = 0, .pw_gecos = (char *) "Super User", .pw_dir = (char *) "/root", - .pw_shell = (char *) (shell ?: "/bin/sh"), + .pw_shell = (char *) (shell ?: default_root_shell(arg_root)), }; if (errno != ENOENT) diff --git a/src/nss-systemd/nss-systemd.c b/src/nss-systemd/nss-systemd.c index e24828450f..75d749e736 100644 --- a/src/nss-systemd/nss-systemd.c +++ b/src/nss-systemd/nss-systemd.c @@ -26,7 +26,7 @@ static const struct passwd root_passwd = { .pw_gid = 0, .pw_gecos = (char*) "Super User", .pw_dir = (char*) "/root", - .pw_shell = (char*) "/bin/sh", + .pw_shell = NULL, }; static const struct spwd root_spwd = { @@ -142,24 +142,25 @@ NSS_INITGROUPS_PROTOTYPE(systemd); static enum nss_status copy_synthesized_passwd( struct passwd *dest, const struct passwd *src, + const char *fallback_shell, char *buffer, size_t buflen, int *errnop) { - size_t required; - assert(dest); assert(src); assert(src->pw_name); assert(src->pw_passwd); assert(src->pw_gecos); assert(src->pw_dir); - assert(src->pw_shell); - required = strlen(src->pw_name) + 1; - required += strlen(src->pw_passwd) + 1; - required += strlen(src->pw_gecos) + 1; - required += strlen(src->pw_dir) + 1; - required += strlen(src->pw_shell) + 1; + const char *shell = ASSERT_PTR(src->pw_shell ?: fallback_shell); + + size_t required = + strlen(src->pw_name) + 1 + + strlen(src->pw_passwd) + 1 + + strlen(src->pw_gecos) + 1 + + strlen(src->pw_dir) + 1 + + strlen(shell) + 1; if (buflen < required) { *errnop = ERANGE; @@ -176,7 +177,7 @@ static enum nss_status copy_synthesized_passwd( dest->pw_gecos = stpcpy(dest->pw_passwd, src->pw_passwd) + 1; dest->pw_dir = stpcpy(dest->pw_gecos, src->pw_gecos) + 1; dest->pw_shell = stpcpy(dest->pw_dir, src->pw_dir) + 1; - strcpy(dest->pw_shell, src->pw_shell); + strcpy(dest->pw_shell, shell); return NSS_STATUS_SUCCESS; } @@ -187,15 +188,14 @@ static enum nss_status copy_synthesized_spwd( char *buffer, size_t buflen, int *errnop) { - size_t required; - assert(dest); assert(src); assert(src->sp_namp); assert(src->sp_pwdp); - required = strlen(src->sp_namp) + 1; - required += strlen(src->sp_pwdp) + 1; + size_t required = + strlen(src->sp_namp) + 1 + + strlen(src->sp_pwdp) + 1; if (buflen < required) { *errnop = ERANGE; @@ -220,8 +220,6 @@ static enum nss_status copy_synthesized_group( char *buffer, size_t buflen, int *errnop) { - size_t required; - assert(dest); assert(src); assert(src->gr_name); @@ -229,9 +227,10 @@ static enum nss_status copy_synthesized_group( assert(src->gr_mem); assert(!*src->gr_mem); /* Our synthesized records' gr_mem is always just NULL... */ - required = strlen(src->gr_name) + 1; - required += strlen(src->gr_passwd) + 1; - required += sizeof(char*); /* ...but that NULL still needs to be stored into the buffer! */ + size_t required = + strlen(src->gr_name) + 1 + + strlen(src->gr_passwd) + 1 + + sizeof(char*); /* ...but that NULL still needs to be stored into the buffer! */ if (buflen < ALIGN(required)) { *errnop = ERANGE; @@ -257,15 +256,14 @@ static enum nss_status copy_synthesized_sgrp( char *buffer, size_t buflen, int *errnop) { - size_t required; - assert(dest); assert(src); assert(src->sg_namp); assert(src->sg_passwd); - required = strlen(src->sg_namp) + 1; - required += strlen(src->sg_passwd) + 1; + size_t required = + strlen(src->sg_namp) + 1 + + strlen(src->sg_passwd) + 1; if (buflen < required) { *errnop = ERANGE; @@ -310,13 +308,17 @@ enum nss_status _nss_systemd_getpwnam_r( if (getenv_bool_secure("SYSTEMD_NSS_BYPASS_SYNTHETIC") <= 0) { if (streq(name, root_passwd.pw_name)) - return copy_synthesized_passwd(pwd, &root_passwd, buffer, buflen, errnop); + return copy_synthesized_passwd(pwd, &root_passwd, + default_root_shell(NULL), + buffer, buflen, errnop); if (streq(name, nobody_passwd.pw_name)) { if (!synthesize_nobody()) return NSS_STATUS_NOTFOUND; - return copy_synthesized_passwd(pwd, &nobody_passwd, buffer, buflen, errnop); + return copy_synthesized_passwd(pwd, &nobody_passwd, + NULL, + buffer, buflen, errnop); } } else if (STR_IN_SET(name, root_passwd.pw_name, nobody_passwd.pw_name)) @@ -354,13 +356,17 @@ enum nss_status _nss_systemd_getpwuid_r( if (getenv_bool_secure("SYSTEMD_NSS_BYPASS_SYNTHETIC") <= 0) { if (uid == root_passwd.pw_uid) - return copy_synthesized_passwd(pwd, &root_passwd, buffer, buflen, errnop); + return copy_synthesized_passwd(pwd, &root_passwd, + default_root_shell(NULL), + buffer, buflen, errnop); if (uid == nobody_passwd.pw_uid) { if (!synthesize_nobody()) return NSS_STATUS_NOTFOUND; - return copy_synthesized_passwd(pwd, &nobody_passwd, buffer, buflen, errnop); + return copy_synthesized_passwd(pwd, &nobody_passwd, + NULL, + buffer, buflen, errnop); } } else if (uid == root_passwd.pw_uid || uid == nobody_passwd.pw_uid) diff --git a/src/sysusers/sysusers.c b/src/sysusers/sysusers.c index 5eb67ea084..393d2cc0fc 100644 --- a/src/sysusers/sysusers.c +++ b/src/sysusers/sysusers.c @@ -398,7 +398,7 @@ static const char* pick_shell(const Item *i) { if (i->shell) return i->shell; if (i->uid_set && i->uid == 0) - return "/bin/sh"; + return default_root_shell(arg_root); return NOLOGIN; } diff --git a/src/test/test-user-util.c b/src/test/test-user-util.c index 907de54eaa..48d9b1e0fb 100644 --- a/src/test/test-user-util.c +++ b/src/test/test-user-util.c @@ -347,8 +347,8 @@ static void test_get_user_creds_one(const char *id, const char *name, uid_t uid, } TEST(get_user_creds) { - test_get_user_creds_one("root", "root", 0, 0, "/root", "/bin/sh"); - test_get_user_creds_one("0", "root", 0, 0, "/root", "/bin/sh"); + test_get_user_creds_one("root", "root", 0, 0, "/root", DEFAULT_USER_SHELL); + test_get_user_creds_one("0", "root", 0, 0, "/root", DEFAULT_USER_SHELL); test_get_user_creds_one(NOBODY_USER_NAME, NOBODY_USER_NAME, UID_NOBODY, GID_NOBODY, "/", NOLOGIN); test_get_user_creds_one("65534", NOBODY_USER_NAME, UID_NOBODY, GID_NOBODY, "/", NOLOGIN); } diff --git a/test/test-execute/exec-specifier.service b/test/test-execute/exec-specifier.service index ae8b2428bc..321d0e338a 100644 --- a/test/test-execute/exec-specifier.service +++ b/test/test-execute/exec-specifier.service @@ -26,7 +26,7 @@ ExecStart=sh -c 'test %U = $$(id -u)' ExecStart=sh -c 'test %g = $$(id -gn)' ExecStart=sh -c 'test %G = $$(id -g)' ExecStart=test %h = /root -ExecStart=sh -c 'test %s = /bin/sh' +ExecStart=sh -c 'test -x %s' ExecStart=sh -c 'test %m = $$(cat /etc/machine-id)' ExecStart=sh -c 'test %b = $$(cat /proc/sys/kernel/random/boot_id | sed -e 's/-//g')' ExecStart=sh -c 'test %H = $$(uname -n)' diff --git a/test/test-execute/exec-specifier@.service b/test/test-execute/exec-specifier@.service index 5e30efce4c..46c8503f1d 100644 --- a/test/test-execute/exec-specifier@.service +++ b/test/test-execute/exec-specifier@.service @@ -23,7 +23,7 @@ ExecStart=sh -c 'test %U = $$(id -u)' ExecStart=sh -c 'test %g = $$(id -gn)' ExecStart=sh -c 'test %G = $$(id -g)' ExecStart=test %h = /root -ExecStart=sh -c 'test %s = /bin/sh' +ExecStart=sh -c 'test -x %s' ExecStart=sh -c 'test %m = $$(cat /etc/machine-id)' ExecStart=sh -c 'test %b = $$(cat /proc/sys/kernel/random/boot_id | sed -e 's/-//g')' ExecStart=sh -c 'test %H = $$(uname -n)' From 4c795066b63a15fc20913469df6a926c7aeff1f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 19 Aug 2022 18:04:47 +0200 Subject: [PATCH 7/9] sysusers: report the original error when writing fails We have fairly nice error messages for specific operations, but only at debug level. Instead, we'd print a fairly useless generic message: Before: Failed to write files: Invalid argument After: Failed to add existing group "users" to temporary group file: Invalid argument Fixes #10241. --- src/sysusers/sysusers.c | 56 +++++++++++++++++++---------------------- 1 file changed, 26 insertions(+), 30 deletions(-) diff --git a/src/sysusers/sysusers.c b/src/sysusers/sysusers.c index 393d2cc0fc..14f5ac3246 100644 --- a/src/sysusers/sysusers.c +++ b/src/sysusers/sysusers.c @@ -667,14 +667,14 @@ static int write_temporary_group(const char *group_path, FILE **tmpfile, char ** r = fopen_temporary_label("/etc/group", group_path, &group, &group_tmp); if (r < 0) - return log_debug_errno(r, "Failed to open temporary copy of %s: %m", group_path); + return log_error_errno(r, "Failed to open temporary copy of %s: %m", group_path); original = fopen(group_path, "re"); if (original) { r = copy_rights_with_fallback(fileno(original), fileno(group), group_tmp); if (r < 0) - return log_debug_errno(r, "Failed to copy permissions from %s to %s: %m", + return log_error_errno(r, "Failed to copy permissions from %s to %s: %m", group_path, group_tmp); while ((r = fgetgrent_sane(original, &gr)) > 0) { @@ -700,19 +700,19 @@ static int write_temporary_group(const char *group_path, FILE **tmpfile, char ** r = putgrent_with_members(gr, group); if (r < 0) - return log_debug_errno(r, "Failed to add existing group \"%s\" to temporary group file: %m", + return log_error_errno(r, "Failed to add existing group \"%s\" to temporary group file: %m", gr->gr_name); if (r > 0) group_changed = true; } if (r < 0) - return log_debug_errno(r, "Failed to read %s: %m", group_path); + return log_error_errno(r, "Failed to read %s: %m", group_path); } else { if (errno != ENOENT) - return log_debug_errno(errno, "Failed to open %s: %m", group_path); + return log_error_errno(errno, "Failed to open %s: %m", group_path); if (fchmod(fileno(group), 0644) < 0) - return log_debug_errno(errno, "Failed to fchmod %s: %m", group_tmp); + return log_error_errno(errno, "Failed to fchmod %s: %m", group_tmp); } ORDERED_HASHMAP_FOREACH(i, todo_gids) { @@ -724,7 +724,7 @@ static int write_temporary_group(const char *group_path, FILE **tmpfile, char ** r = putgrent_with_members(&n, group); if (r < 0) - return log_debug_errno(r, "Failed to add new group \"%s\" to temporary group file: %m", + return log_error_errno(r, "Failed to add new group \"%s\" to temporary group file: %m", gr->gr_name); group_changed = true; @@ -734,19 +734,19 @@ static int write_temporary_group(const char *group_path, FILE **tmpfile, char ** while (gr) { r = putgrent_sane(gr, group); if (r < 0) - return log_debug_errno(r, "Failed to add existing group \"%s\" to temporary group file: %m", + return log_error_errno(r, "Failed to add existing group \"%s\" to temporary group file: %m", gr->gr_name); r = fgetgrent_sane(original, &gr); if (r < 0) - return log_debug_errno(r, "Failed to read %s: %m", group_path); + return log_error_errno(r, "Failed to read %s: %m", group_path); if (r == 0) break; } r = fflush_sync_and_check(group); if (r < 0) - return log_debug_errno(r, "Failed to flush %s: %m", group_tmp); + return log_error_errno(r, "Failed to flush %s: %m", group_tmp); if (group_changed) { *tmpfile = TAKE_PTR(group); @@ -773,7 +773,7 @@ static int write_temporary_gshadow(const char * gshadow_path, FILE **tmpfile, ch r = fopen_temporary_label("/etc/gshadow", gshadow_path, &gshadow, &gshadow_tmp); if (r < 0) - return log_debug_errno(r, "Failed to open temporary copy of %s: %m", gshadow_path); + return log_error_errno(r, "Failed to open temporary copy of %s: %m", gshadow_path); original = fopen(gshadow_path, "re"); if (original) { @@ -781,7 +781,7 @@ static int write_temporary_gshadow(const char * gshadow_path, FILE **tmpfile, ch r = copy_rights_with_fallback(fileno(original), fileno(gshadow), gshadow_tmp); if (r < 0) - return log_debug_errno(r, "Failed to copy permissions from %s to %s: %m", + return log_error_errno(r, "Failed to copy permissions from %s to %s: %m", gshadow_path, gshadow_tmp); while ((r = fgetsgent_sane(original, &sg)) > 0) { @@ -794,7 +794,7 @@ static int write_temporary_gshadow(const char * gshadow_path, FILE **tmpfile, ch r = putsgent_with_members(sg, gshadow); if (r < 0) - return log_debug_errno(r, "Failed to add existing group \"%s\" to temporary gshadow file: %m", + return log_error_errno(r, "Failed to add existing group \"%s\" to temporary gshadow file: %m", sg->sg_namp); if (r > 0) group_changed = true; @@ -804,9 +804,9 @@ static int write_temporary_gshadow(const char * gshadow_path, FILE **tmpfile, ch } else { if (errno != ENOENT) - return log_debug_errno(errno, "Failed to open %s: %m", gshadow_path); + return log_error_errno(errno, "Failed to open %s: %m", gshadow_path); if (fchmod(fileno(gshadow), 0000) < 0) - return log_debug_errno(errno, "Failed to fchmod %s: %m", gshadow_tmp); + return log_error_errno(errno, "Failed to fchmod %s: %m", gshadow_tmp); } ORDERED_HASHMAP_FOREACH(i, todo_gids) { @@ -817,7 +817,7 @@ static int write_temporary_gshadow(const char * gshadow_path, FILE **tmpfile, ch r = putsgent_with_members(&n, gshadow); if (r < 0) - return log_debug_errno(r, "Failed to add new group \"%s\" to temporary gshadow file: %m", + return log_error_errno(r, "Failed to add new group \"%s\" to temporary gshadow file: %m", n.sg_namp); group_changed = true; @@ -825,7 +825,7 @@ static int write_temporary_gshadow(const char * gshadow_path, FILE **tmpfile, ch r = fflush_sync_and_check(gshadow); if (r < 0) - return log_debug_errno(r, "Failed to flush %s: %m", gshadow_tmp); + return log_error_errno(r, "Failed to flush %s: %m", gshadow_tmp); if (group_changed) { *tmpfile = TAKE_PTR(gshadow); @@ -866,30 +866,30 @@ static int write_files(void) { if (group) { r = make_backup("/etc/group", group_path); if (r < 0) - return log_debug_errno(r, "Failed to make backup %s: %m", group_path); + return log_error_errno(r, "Failed to make backup %s: %m", group_path); } if (gshadow) { r = make_backup("/etc/gshadow", gshadow_path); if (r < 0) - return log_debug_errno(r, "Failed to make backup %s: %m", gshadow_path); + return log_error_errno(r, "Failed to make backup %s: %m", gshadow_path); } if (passwd) { r = make_backup("/etc/passwd", passwd_path); if (r < 0) - return log_debug_errno(r, "Failed to make backup %s: %m", passwd_path); + return log_error_errno(r, "Failed to make backup %s: %m", passwd_path); } if (shadow) { r = make_backup("/etc/shadow", shadow_path); if (r < 0) - return log_debug_errno(r, "Failed to make backup %s: %m", shadow_path); + return log_error_errno(r, "Failed to make backup %s: %m", shadow_path); } /* And make the new files count */ if (group) { r = rename_and_apply_smack_floor_label(group_tmp, group_path); if (r < 0) - return log_debug_errno(r, "Failed to rename %s to %s: %m", + return log_error_errno(r, "Failed to rename %s to %s: %m", group_tmp, group_path); group_tmp = mfree(group_tmp); @@ -899,7 +899,7 @@ static int write_files(void) { if (gshadow) { r = rename_and_apply_smack_floor_label(gshadow_tmp, gshadow_path); if (r < 0) - return log_debug_errno(r, "Failed to rename %s to %s: %m", + return log_error_errno(r, "Failed to rename %s to %s: %m", gshadow_tmp, gshadow_path); gshadow_tmp = mfree(gshadow_tmp); @@ -908,7 +908,7 @@ static int write_files(void) { if (passwd) { r = rename_and_apply_smack_floor_label(passwd_tmp, passwd_path); if (r < 0) - return log_debug_errno(r, "Failed to rename %s to %s: %m", + return log_error_errno(r, "Failed to rename %s to %s: %m", passwd_tmp, passwd_path); passwd_tmp = mfree(passwd_tmp); @@ -919,7 +919,7 @@ static int write_files(void) { if (shadow) { r = rename_and_apply_smack_floor_label(shadow_tmp, shadow_path); if (r < 0) - return log_debug_errno(r, "Failed to rename %s to %s: %m", + return log_error_errno(r, "Failed to rename %s to %s: %m", shadow_tmp, shadow_path); shadow_tmp = mfree(shadow_tmp); @@ -2186,11 +2186,7 @@ static int run(int argc, char *argv[]) { ORDERED_HASHMAP_FOREACH(i, users) (void) process_item(i); - r = write_files(); - if (r < 0) - return log_error_errno(r, "Failed to write files: %m"); - - return 0; + return write_files(); } DEFINE_MAIN_FUNCTION(run); From 36bac2dcba7ee77d80900af6ddba347242ffe7a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 19 Aug 2022 16:45:08 +0200 Subject: [PATCH 8/9] basic/user-util: avoid filesystem access check The check of u==UID_NOBODY is just a register comparison, but synthesize_nobody() requires a system call, so let's invert the order in the condition. Since most calls into this module are not for nobody, we should save one syscall in the common case. --- src/basic/user-util.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/basic/user-util.c b/src/basic/user-util.c index 37ccb667b2..e705f703ea 100644 --- a/src/basic/user-util.c +++ b/src/basic/user-util.c @@ -196,8 +196,8 @@ static int synthesize_user_creds( return 0; } - if (synthesize_nobody() && - STR_IN_SET(*username, NOBODY_USER_NAME, "65534")) { + if (STR_IN_SET(*username, NOBODY_USER_NAME, "65534") && + synthesize_nobody()) { *username = NOBODY_USER_NAME; if (uid) @@ -341,8 +341,8 @@ int get_group_creds(const char **groupname, gid_t *gid, UserCredsFlags flags) { return 0; } - if (synthesize_nobody() && - STR_IN_SET(*groupname, NOBODY_GROUP_NAME, "65534")) { + if (STR_IN_SET(*groupname, NOBODY_GROUP_NAME, "65534") && + synthesize_nobody()) { *groupname = NOBODY_GROUP_NAME; if (gid) @@ -388,8 +388,7 @@ char* uid_to_name(uid_t uid) { /* Shortcut things to avoid NSS lookups */ if (uid == 0) return strdup("root"); - if (synthesize_nobody() && - uid == UID_NOBODY) + if (uid == UID_NOBODY && synthesize_nobody()) return strdup(NOBODY_USER_NAME); if (uid_is_valid(uid)) { @@ -432,8 +431,7 @@ char* gid_to_name(gid_t gid) { if (gid == 0) return strdup("root"); - if (synthesize_nobody() && - gid == GID_NOBODY) + if (gid == GID_NOBODY && synthesize_nobody()) return strdup(NOBODY_GROUP_NAME); if (gid_is_valid(gid)) { @@ -600,8 +598,8 @@ int get_home_dir(char **_h) { *_h = h; return 0; } - if (synthesize_nobody() && - u == UID_NOBODY) { + + if (u == UID_NOBODY && synthesize_nobody()) { h = strdup("/"); if (!h) return -ENOMEM; @@ -657,8 +655,7 @@ int get_shell(char **_s) { *_s = s; return 0; } - if (synthesize_nobody() && - u == UID_NOBODY) { + if (u == UID_NOBODY && synthesize_nobody()) { s = strdup(NOLOGIN); if (!s) return -ENOMEM; From 8795d9bacdea7074b023c9f8836720002e39adc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 19 Aug 2022 17:03:36 +0200 Subject: [PATCH 9/9] basic/user-util: rename output param to ret, shorten code --- src/basic/user-util.c | 78 +++++++++++++++---------------------------- src/basic/user-util.h | 2 +- 2 files changed, 27 insertions(+), 53 deletions(-) diff --git a/src/basic/user-util.c b/src/basic/user-util.c index e705f703ea..80f9ff144b 100644 --- a/src/basic/user-util.c +++ b/src/basic/user-util.c @@ -569,43 +569,29 @@ int getgroups_alloc(gid_t** gids) { return ngroups; } -int get_home_dir(char **_h) { +int get_home_dir(char **ret) { struct passwd *p; const char *e; char *h; uid_t u; - assert(_h); + assert(ret); /* Take the user specified one */ e = secure_getenv("HOME"); - if (e && path_is_valid(e) && path_is_absolute(e)) { - h = strdup(e); - if (!h) - return -ENOMEM; - - *_h = path_simplify(h); - return 0; - } + if (e && path_is_valid(e) && path_is_absolute(e)) + goto found; /* Hardcode home directory for root and nobody to avoid NSS */ u = getuid(); if (u == 0) { - h = strdup("/root"); - if (!h) - return -ENOMEM; - - *_h = h; - return 0; + e = "/root"; + goto found; } if (u == UID_NOBODY && synthesize_nobody()) { - h = strdup("/"); - if (!h) - return -ENOMEM; - - *_h = h; - return 0; + e = "/"; + goto found; } /* Check the database... */ @@ -613,55 +599,42 @@ int get_home_dir(char **_h) { p = getpwuid(u); if (!p) return errno_or_else(ESRCH); + e = p->pw_dir; - if (!path_is_valid(p->pw_dir) || - !path_is_absolute(p->pw_dir)) + if (!path_is_valid(e) || !path_is_absolute(e)) return -EINVAL; - h = strdup(p->pw_dir); + found: + h = strdup(e); if (!h) return -ENOMEM; - *_h = path_simplify(h); + *ret = path_simplify(h); return 0; } -int get_shell(char **_s) { +int get_shell(char **ret) { struct passwd *p; const char *e; char *s; uid_t u; - assert(_s); + assert(ret); /* Take the user specified one */ e = secure_getenv("SHELL"); - if (e && path_is_valid(e) && path_is_absolute(e)) { - s = strdup(e); - if (!s) - return -ENOMEM; - - *_s = path_simplify(s); - return 0; - } + if (e && path_is_valid(e) && path_is_absolute(e)) + goto found; /* Hardcode shell for root and nobody to avoid NSS */ u = getuid(); if (u == 0) { - s = strdup(default_root_shell(NULL)); - if (!s) - return -ENOMEM; - - *_s = s; - return 0; + e = default_root_shell(NULL); + goto found; } if (u == UID_NOBODY && synthesize_nobody()) { - s = strdup(NOLOGIN); - if (!s) - return -ENOMEM; - - *_s = s; - return 0; + e = NOLOGIN; + goto found; } /* Check the database... */ @@ -669,16 +642,17 @@ int get_shell(char **_s) { p = getpwuid(u); if (!p) return errno_or_else(ESRCH); + e = p->pw_shell; - if (!path_is_valid(p->pw_shell) || - !path_is_absolute(p->pw_shell)) + if (!path_is_valid(e) || !path_is_absolute(e)) return -EINVAL; - s = strdup(p->pw_shell); + found: + s = strdup(e); if (!s) return -ENOMEM; - *_s = path_simplify(s); + *ret = path_simplify(s); return 0; } diff --git a/src/basic/user-util.h b/src/basic/user-util.h index c8f0758541..614dec2fde 100644 --- a/src/basic/user-util.h +++ b/src/basic/user-util.h @@ -55,7 +55,7 @@ int merge_gid_lists(const gid_t *list1, size_t size1, const gid_t *list2, size_t int getgroups_alloc(gid_t** gids); int get_home_dir(char **ret); -int get_shell(char **_ret); +int get_shell(char **ret); int reset_uid_gid(void);