From e4631b48e17e63a3859456df639482063a0276fd Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 14 Jul 2016 12:23:39 +0200 Subject: sysusers: move various user credential validity checks to src/basic/ This way we can reuse them for validating User=/Group= settings in unit files (to be added in a later commit). Also, add some tests for them. --- src/basic/user-util.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) (limited to 'src/basic/user-util.c') diff --git a/src/basic/user-util.c b/src/basic/user-util.c index e9d668ddfc..122d9a0c7c 100644 --- a/src/basic/user-util.c +++ b/src/basic/user-util.c @@ -29,6 +29,7 @@ #include #include #include +#include #include "missing.h" #include "alloc-util.h" @@ -39,6 +40,7 @@ #include "path-util.h" #include "string-util.h" #include "user-util.h" +#include "utf8.h" bool uid_is_valid(uid_t uid) { @@ -479,3 +481,94 @@ int take_etc_passwd_lock(const char *root) { return fd; } + +bool valid_user_group_name(const char *u) { + const char *i; + long sz; + + /* Checks if the specified name is a valid user/group name. */ + + if (isempty(u)) + return false; + + if (!(u[0] >= 'a' && u[0] <= 'z') && + !(u[0] >= 'A' && u[0] <= 'Z') && + u[0] != '_') + return false; + + for (i = u+1; *i; i++) { + if (!(*i >= 'a' && *i <= 'z') && + !(*i >= 'A' && *i <= 'Z') && + !(*i >= '0' && *i <= '9') && + *i != '_' && + *i != '-') + return false; + } + + sz = sysconf(_SC_LOGIN_NAME_MAX); + assert_se(sz > 0); + + if ((size_t) (i-u) > (size_t) sz) + return false; + + if ((size_t) (i-u) > UT_NAMESIZE - 1) + return false; + + return true; +} + +bool valid_user_group_name_or_id(const char *u) { + + /* Similar as above, but is also fine with numeric UID/GID specifications, as long as they are in the right + * range, and not the invalid user ids. */ + + if (isempty(u)) + return false; + + if (valid_user_group_name(u)) + return true; + + return parse_uid(u, NULL) >= 0; +} + +bool valid_gecos(const char *d) { + + if (!d) + return false; + + if (!utf8_is_valid(d)) + return false; + + if (string_has_cc(d, NULL)) + return false; + + /* Colons are used as field separators, and hence not OK */ + if (strchr(d, ':')) + return false; + + return true; +} + +bool valid_home(const char *p) { + + if (isempty(p)) + return false; + + if (!utf8_is_valid(p)) + return false; + + if (string_has_cc(p, NULL)) + return false; + + if (!path_is_absolute(p)) + return false; + + if (!path_is_safe(p)) + return false; + + /* Colons are used as field separators, and hence not OK */ + if (strchr(p, ':')) + return false; + + return true; +} -- cgit v1.2.3-54-g00ecf From be39ccf3a0d4d15324af1de4d8552a1d65f40808 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 25 Aug 2016 10:24:10 +0200 Subject: execute: move suppression of HOME=/ and SHELL=/bin/nologin into user-util.c This adds a new call get_user_creds_clean(), which is just like get_user_creds() but returns NULL in the home/shell parameters if they contain no useful information. This code previously lived in execute.c, but by generalizing this we can reuse it in run.c. --- src/basic/user-util.c | 32 +++++++++++++++++++++++++++++++- src/basic/user-util.h | 1 + src/core/execute.c | 14 +++----------- src/run/run.c | 18 +++++++++++------- 4 files changed, 46 insertions(+), 19 deletions(-) (limited to 'src/basic/user-util.c') diff --git a/src/basic/user-util.c b/src/basic/user-util.c index 122d9a0c7c..0522bce1d1 100644 --- a/src/basic/user-util.c +++ b/src/basic/user-util.c @@ -31,14 +31,15 @@ #include #include -#include "missing.h" #include "alloc-util.h" #include "fd-util.h" #include "formats-util.h" #include "macro.h" +#include "missing.h" #include "parse-util.h" #include "path-util.h" #include "string-util.h" +#include "strv.h" #include "user-util.h" #include "utf8.h" @@ -175,6 +176,35 @@ int get_user_creds( return 0; } +int get_user_creds_clean( + const char **username, + uid_t *uid, gid_t *gid, + const char **home, + const char **shell) { + + int r; + + /* Like get_user_creds(), but resets home/shell to NULL if they don't contain anything relevant. */ + + r = get_user_creds(username, uid, gid, home, shell); + if (r < 0) + return r; + + if (shell && + (isempty(*shell) || PATH_IN_SET(*shell, + "/bin/nologin", + "/sbin/nologin", + "/usr/bin/nologin", + "/usr/sbin/nologin"))) + *shell = NULL; + + if (home && + (isempty(*home) || path_equal(*home, "/"))) + *home = NULL; + + return 0; +} + int get_group_creds(const char **groupname, gid_t *gid) { struct group *g; gid_t id; diff --git a/src/basic/user-util.h b/src/basic/user-util.h index f569363811..6c61f63cae 100644 --- a/src/basic/user-util.h +++ b/src/basic/user-util.h @@ -40,6 +40,7 @@ char* getlogname_malloc(void); char* getusername_malloc(void); int get_user_creds(const char **username, uid_t *uid, gid_t *gid, const char **home, const char **shell); +int get_user_creds_clean(const char **username, uid_t *uid, gid_t *gid, const char **home, const char **shell); int get_group_creds(const char **groupname, gid_t *gid); char* uid_to_name(uid_t uid); diff --git a/src/core/execute.c b/src/core/execute.c index 3877293b4f..c7a3ea39e7 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2051,22 +2051,14 @@ static int exec_child( } else { if (context->user) { username = context->user; - r = get_user_creds(&username, &uid, &gid, &home, &shell); + r = get_user_creds_clean(&username, &uid, &gid, &home, &shell); if (r < 0) { *exit_status = EXIT_USER; return r; } - /* Don't set $HOME or $SHELL if they are are not particularly enlightening anyway. */ - if (isempty(home) || path_equal(home, "/")) - home = NULL; - - if (isempty(shell) || PATH_IN_SET(shell, - "/bin/nologin", - "/sbin/nologin", - "/usr/bin/nologin", - "/usr/sbin/nologin")) - shell = NULL; + /* Note that we don't set $HOME or $SHELL if they are are not particularly enlightening anyway + * (i.e. are "/" or "/bin/nologin"). */ } if (context->group) { diff --git a/src/run/run.c b/src/run/run.c index 2dd229868c..81b53fdfab 100644 --- a/src/run/run.c +++ b/src/run/run.c @@ -1168,17 +1168,21 @@ static int start_transient_scope( uid_t uid; gid_t gid; - r = get_user_creds(&arg_exec_user, &uid, &gid, &home, &shell); + r = get_user_creds_clean(&arg_exec_user, &uid, &gid, &home, &shell); if (r < 0) return log_error_errno(r, "Failed to resolve user %s: %m", arg_exec_user); - r = strv_extendf(&user_env, "HOME=%s", home); - if (r < 0) - return log_oom(); + if (home) { + r = strv_extendf(&user_env, "HOME=%s", home); + if (r < 0) + return log_oom(); + } - r = strv_extendf(&user_env, "SHELL=%s", shell); - if (r < 0) - return log_oom(); + if (shell) { + r = strv_extendf(&user_env, "SHELL=%s", shell); + if (r < 0) + return log_oom(); + } r = strv_extendf(&user_env, "USER=%s", arg_exec_user); if (r < 0) -- cgit v1.2.3-54-g00ecf From 36d854780c01d589e5da1fc6e94f46aa41f7016f Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Wed, 28 Sep 2016 18:37:39 +0200 Subject: core: do not fail in a container if we can't use setgroups It might be blocked through /proc/PID/setgroups --- src/basic/capability-util.c | 3 ++- src/basic/user-util.c | 27 ++++++++++++++++++++++++++- src/basic/user-util.h | 2 ++ src/core/execute.c | 2 +- 4 files changed, 31 insertions(+), 3 deletions(-) (limited to 'src/basic/user-util.c') diff --git a/src/basic/capability-util.c b/src/basic/capability-util.c index d4c5bd6937..f8db6e0212 100644 --- a/src/basic/capability-util.c +++ b/src/basic/capability-util.c @@ -31,6 +31,7 @@ #include "log.h" #include "macro.h" #include "parse-util.h" +#include "user-util.h" #include "util.h" int have_effective_cap(int value) { @@ -295,7 +296,7 @@ int drop_privileges(uid_t uid, gid_t gid, uint64_t keep_capabilities) { if (setresgid(gid, gid, gid) < 0) return log_error_errno(errno, "Failed to change group ID: %m"); - if (setgroups(0, NULL) < 0) + if (maybe_setgroups(0, NULL) < 0) return log_error_errno(errno, "Failed to drop auxiliary groups list: %m"); /* Ensure we keep the permitted caps across the setresuid() */ diff --git a/src/basic/user-util.c b/src/basic/user-util.c index 0522bce1d1..16496fccfa 100644 --- a/src/basic/user-util.c +++ b/src/basic/user-util.c @@ -33,6 +33,7 @@ #include "alloc-util.h" #include "fd-util.h" +#include "fileio.h" #include "formats-util.h" #include "macro.h" #include "missing.h" @@ -460,7 +461,7 @@ int get_shell(char **_s) { int reset_uid_gid(void) { - if (setgroups(0, NULL) < 0) + if (maybe_setgroups(0, NULL) < 0) return -errno; if (setresgid(0, 0, 0) < 0) @@ -602,3 +603,27 @@ bool valid_home(const char *p) { return true; } + +int maybe_setgroups(size_t size, const gid_t *list) { + static int cached_can_setgroups = -1; + /* check if setgroups is allowed before we try to drop all the auxiliary groups */ + if (size == 0) { + if (cached_can_setgroups < 0) { + _cleanup_free_ char *setgroups_content = NULL; + int r = read_one_line_file("/proc/self/setgroups", &setgroups_content); + if (r < 0 && errno != ENOENT) + return r; + if (r < 0) { + /* old kernels don't have /proc/self/setgroups, so assume we can use setgroups */ + cached_can_setgroups = true; + } else { + cached_can_setgroups = streq(setgroups_content, "allow"); + if (!cached_can_setgroups) + log_debug("skip setgroups, /proc/self/setgroups is set to 'deny'"); + } + } + if (!cached_can_setgroups) + return 0; + } + return setgroups(size, list); +} diff --git a/src/basic/user-util.h b/src/basic/user-util.h index 6c61f63cae..dfea561bde 100644 --- a/src/basic/user-util.h +++ b/src/basic/user-util.h @@ -86,3 +86,5 @@ bool valid_user_group_name(const char *u); bool valid_user_group_name_or_id(const char *u); bool valid_gecos(const char *d); bool valid_home(const char *p); + +int maybe_setgroups(size_t size, const gid_t *list); diff --git a/src/core/execute.c b/src/core/execute.c index 82d8c978c1..019ff8490b 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -781,7 +781,7 @@ static int enforce_groups(const ExecContext *context, const char *username, gid_ k++; } - if (setgroups(k, gids) < 0) { + if (maybe_setgroups(k, gids) < 0) { free(gids); return -errno; } -- cgit v1.2.3-54-g00ecf From 97f0e76f18d322d29bcfbc4ab6bb9cd67a1cdd54 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 6 Oct 2016 17:54:12 +0200 Subject: user-util: rework maybe_setgroups() a bit Let's drop the caching of the setgroups /proc field for now. While there's a strict regime in place when it changes states, let's better not cache it since we cannot really be sure we follow that regime correctly. More importantly however, this is not in performance sensitive code, and there's no indication the cache is really beneficial, hence let's drop the caching and make things a bit simpler. Also, while we are at it, rework the error handling a bit, and always return negative errno-style error codes, following our usual coding style. This has the benefit that we can sensible hanld read_one_line_file() errors, without having to updat errno explicitly. --- src/basic/capability-util.c | 5 +++-- src/basic/user-util.c | 49 ++++++++++++++++++++++++++------------------- src/core/execute.c | 10 +++++---- 3 files changed, 37 insertions(+), 27 deletions(-) (limited to 'src/basic/user-util.c') diff --git a/src/basic/capability-util.c b/src/basic/capability-util.c index f8db6e0212..c3de20a0e8 100644 --- a/src/basic/capability-util.c +++ b/src/basic/capability-util.c @@ -296,8 +296,9 @@ int drop_privileges(uid_t uid, gid_t gid, uint64_t keep_capabilities) { if (setresgid(gid, gid, gid) < 0) return log_error_errno(errno, "Failed to change group ID: %m"); - if (maybe_setgroups(0, NULL) < 0) - return log_error_errno(errno, "Failed to drop auxiliary groups list: %m"); + r = maybe_setgroups(0, NULL); + if (r < 0) + return log_error_errno(r, "Failed to drop auxiliary groups list: %m"); /* Ensure we keep the permitted caps across the setresuid() */ if (prctl(PR_SET_KEEPCAPS, 1) < 0) diff --git a/src/basic/user-util.c b/src/basic/user-util.c index 16496fccfa..de6c93056e 100644 --- a/src/basic/user-util.c +++ b/src/basic/user-util.c @@ -460,9 +460,11 @@ int get_shell(char **_s) { } int reset_uid_gid(void) { + int r; - if (maybe_setgroups(0, NULL) < 0) - return -errno; + r = maybe_setgroups(0, NULL); + if (r < 0) + return r; if (setresgid(0, 0, 0) < 0) return -errno; @@ -605,25 +607,30 @@ bool valid_home(const char *p) { } int maybe_setgroups(size_t size, const gid_t *list) { - static int cached_can_setgroups = -1; - /* check if setgroups is allowed before we try to drop all the auxiliary groups */ - if (size == 0) { - if (cached_can_setgroups < 0) { - _cleanup_free_ char *setgroups_content = NULL; - int r = read_one_line_file("/proc/self/setgroups", &setgroups_content); - if (r < 0 && errno != ENOENT) - return r; - if (r < 0) { - /* old kernels don't have /proc/self/setgroups, so assume we can use setgroups */ - cached_can_setgroups = true; - } else { - cached_can_setgroups = streq(setgroups_content, "allow"); - if (!cached_can_setgroups) - log_debug("skip setgroups, /proc/self/setgroups is set to 'deny'"); - } - } - if (!cached_can_setgroups) + int r; + + /* Check if setgroups is allowed before we try to drop all the auxiliary groups */ + if (size == 0) { /* Dropping all aux groups? */ + _cleanup_free_ char *setgroups_content = NULL; + bool can_setgroups; + + r = read_one_line_file("/proc/self/setgroups", &setgroups_content); + if (r == -ENOENT) + /* Old kernels don't have /proc/self/setgroups, so assume we can use setgroups */ + can_setgroups = true; + else if (r < 0) + return r; + else + can_setgroups = streq(setgroups_content, "allow"); + + if (!can_setgroups) { + log_debug("Skipping setgroups(), /proc/self/setgroups is set to 'deny'"); return 0; + } } - return setgroups(size, list); + + if (setgroups(size, list) < 0) + return -errno; + + return 0; } diff --git a/src/core/execute.c b/src/core/execute.c index e4a23ac169..d5c4e60796 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -781,9 +781,10 @@ static int enforce_groups(const ExecContext *context, const char *username, gid_ k++; } - if (maybe_setgroups(k, gids) < 0) { + r = maybe_setgroups(k, gids); + if (r < 0) { free(gids); - return -errno; + return r; } free(gids); @@ -950,8 +951,9 @@ static int setup_pam( * If this fails, ignore the error - but expect sd-pam threads * to fail to exit normally */ - if (maybe_setgroups(0, NULL) < 0) - log_warning_errno(errno, "Failed to setgroups() in sd-pam: %m"); + r = maybe_setgroups(0, NULL); + if (r < 0) + log_warning_errno(r, "Failed to setgroups() in sd-pam: %m"); if (setresgid(gid, gid, gid) < 0) log_warning_errno(errno, "Failed to setresgid() in sd-pam: %m"); if (setresuid(uid, uid, uid) < 0) -- cgit v1.2.3-54-g00ecf