From cd13d971dc1ed56a6b23e32558e1ac9bf37b1226 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 7 Mar 2019 10:31:26 +0100 Subject: [PATCH 01/13] logind: normalize home path when creating user object Triggered by: #11910 --- src/login/logind-user.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/login/logind-user.c b/src/login/logind-user.c index fe47c78bdb..2b327cbe19 100644 --- a/src/login/logind-user.c +++ b/src/login/logind-user.c @@ -68,6 +68,8 @@ int user_new(User **ret, if (!u->home) return -ENOMEM; + path_simplify(u->home, true); + if (asprintf(&u->state_file, "/run/systemd/users/"UID_FMT, uid) < 0) return -ENOMEM; From 71ae7b576cf9d0e4e7280790bb7fb4c5fc3dd34e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 7 Mar 2019 10:32:48 +0100 Subject: [PATCH 02/13] user-util: filter out invalid user record data a bit more thorougly --- src/basic/user-util.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/basic/user-util.c b/src/basic/user-util.c index 260f3d2057..b8d1066929 100644 --- a/src/basic/user-util.c +++ b/src/basic/user-util.c @@ -238,14 +238,21 @@ int get_user_creds( } if (home) { - if (FLAGS_SET(flags, USER_CREDS_CLEAN) && empty_or_root(p->pw_dir)) - *home = NULL; + if (FLAGS_SET(flags, USER_CREDS_CLEAN) && + (empty_or_root(p->pw_dir) || + !path_is_valid(p->pw_dir) || + !path_is_absolute(p->pw_dir))) + *home = NULL; /* Note: we don't insist on normalized paths, since there are setups that have /./ in the path */ else *home = p->pw_dir; } if (shell) { - if (FLAGS_SET(flags, USER_CREDS_CLEAN) && (isempty(p->pw_shell) || is_nologin_shell(p->pw_shell))) + if (FLAGS_SET(flags, USER_CREDS_CLEAN) && + (isempty(p->pw_shell) || + !path_is_valid(p->pw_dir) || + !path_is_absolute(p->pw_shell) || + is_nologin_shell(p->pw_shell))) *shell = NULL; else *shell = p->pw_shell; From 458e60b3a91cfeb72163a312187ce672a9c4dfe0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 7 Mar 2019 10:37:04 +0100 Subject: [PATCH 03/13] path-util: check validity before normalization in path_simplify_and_warn() As the normalization check includes a validation check the order matters. --- src/basic/path-util.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/basic/path-util.c b/src/basic/path-util.c index 221517303e..084e0a9f29 100644 --- a/src/basic/path-util.c +++ b/src/basic/path-util.c @@ -1132,13 +1132,6 @@ int path_simplify_and_warn( path_simplify(path, true); - if (!path_is_normalized(path)) { - log_syntax(unit, LOG_ERR, filename, line, 0, - "%s= path is not normalized%s: %s", - lvalue, fatal ? "" : ", ignoring", path); - return -EINVAL; - } - if (!path_is_valid(path)) { log_syntax(unit, LOG_ERR, filename, line, 0, "%s= path has invalid length (%zu bytes)%s.", @@ -1146,5 +1139,12 @@ int path_simplify_and_warn( return -EINVAL; } + if (!path_is_normalized(path)) { + log_syntax(unit, LOG_ERR, filename, line, 0, + "%s= path is not normalized%s: %s", + lvalue, fatal ? "" : ", ignoring", path); + return -EINVAL; + } + return 0; } From 0b78b1370c1b5176cb5c1fa47d6bb3c07939b2f3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 7 Mar 2019 10:44:59 +0100 Subject: [PATCH 04/13] path-util: minimize variable scope --- src/basic/path-util.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/basic/path-util.c b/src/basic/path-util.c index 084e0a9f29..6d193b0a96 100644 --- a/src/basic/path-util.c +++ b/src/basic/path-util.c @@ -1103,7 +1103,7 @@ int path_simplify_and_warn( unsigned line, const char *lvalue) { - bool absolute, fatal = flag & PATH_CHECK_FATAL; + bool fatal = flag & PATH_CHECK_FATAL; assert(!FLAGS_SET(flag, PATH_CHECK_ABSOLUTE | PATH_CHECK_RELATIVE)); @@ -1113,6 +1113,8 @@ int path_simplify_and_warn( } if (flag & (PATH_CHECK_ABSOLUTE | PATH_CHECK_RELATIVE)) { + bool absolute; + absolute = path_is_absolute(path); if (!absolute && (flag & PATH_CHECK_ABSOLUTE)) { From 11de56b9fab85aadcc394e21e6092c3a5d781453 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 7 Mar 2019 10:45:14 +0100 Subject: [PATCH 05/13] user-util: use SYNTHETIC_ERRNO() where we can --- src/basic/path-util.c | 46 +++++++++++++++++-------------------------- 1 file changed, 18 insertions(+), 28 deletions(-) diff --git a/src/basic/path-util.c b/src/basic/path-util.c index 6d193b0a96..1fa813b747 100644 --- a/src/basic/path-util.c +++ b/src/basic/path-util.c @@ -1107,46 +1107,36 @@ int path_simplify_and_warn( assert(!FLAGS_SET(flag, PATH_CHECK_ABSOLUTE | PATH_CHECK_RELATIVE)); - if (!utf8_is_valid(path)) { - log_syntax_invalid_utf8(unit, LOG_ERR, filename, line, path); - return -EINVAL; - } + if (!utf8_is_valid(path)) + return log_syntax_invalid_utf8(unit, LOG_ERR, filename, line, path); if (flag & (PATH_CHECK_ABSOLUTE | PATH_CHECK_RELATIVE)) { bool absolute; absolute = path_is_absolute(path); - if (!absolute && (flag & PATH_CHECK_ABSOLUTE)) { - log_syntax(unit, LOG_ERR, filename, line, 0, - "%s= path is not absolute%s: %s", - lvalue, fatal ? "" : ", ignoring", path); - return -EINVAL; - } + if (!absolute && (flag & PATH_CHECK_ABSOLUTE)) + return log_syntax(unit, LOG_ERR, filename, line, SYNTHETIC_ERRNO(EINVAL), + "%s= path is not absolute%s: %s", + lvalue, fatal ? "" : ", ignoring", path); - if (absolute && (flag & PATH_CHECK_RELATIVE)) { - log_syntax(unit, LOG_ERR, filename, line, 0, - "%s= path is absolute%s: %s", - lvalue, fatal ? "" : ", ignoring", path); - return -EINVAL; - } + if (absolute && (flag & PATH_CHECK_RELATIVE)) + return log_syntax(unit, LOG_ERR, filename, line, SYNTHETIC_ERRNO(EINVAL), + "%s= path is absolute%s: %s", + lvalue, fatal ? "" : ", ignoring", path); } path_simplify(path, true); - if (!path_is_valid(path)) { - log_syntax(unit, LOG_ERR, filename, line, 0, - "%s= path has invalid length (%zu bytes)%s.", - lvalue, strlen(path), fatal ? "" : ", ignoring"); - return -EINVAL; - } + if (!path_is_valid(path)) + return log_syntax(unit, LOG_ERR, filename, line, SYNTHETIC_ERRNO(EINVAL), + "%s= path has invalid length (%zu bytes)%s.", + lvalue, strlen(path), fatal ? "" : ", ignoring"); - if (!path_is_normalized(path)) { - log_syntax(unit, LOG_ERR, filename, line, 0, - "%s= path is not normalized%s: %s", - lvalue, fatal ? "" : ", ignoring", path); - return -EINVAL; - } + if (!path_is_normalized(path)) + return log_syntax(unit, LOG_ERR, filename, line, SYNTHETIC_ERRNO(EINVAL), + "%s= path is not normalized%s: %s", + lvalue, fatal ? "" : ", ignoring", path); return 0; } From 47436d30bb117d85149b1764733bd0ed8a53f0ce Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 7 Mar 2019 10:52:35 +0100 Subject: [PATCH 06/13] =?UTF-8?q?user-util:=20paranoia=20=E2=80=94=20add?= =?UTF-8?q?=20overflow=20check=20on=20ERANGE=20loop?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/basic/user-util.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/basic/user-util.c b/src/basic/user-util.c index b8d1066929..4cf4c5a341 100644 --- a/src/basic/user-util.c +++ b/src/basic/user-util.c @@ -350,6 +350,9 @@ char* uid_to_name(uid_t uid) { if (r != ERANGE) break; + if (bufsize > LONG_MAX/2) /* overflow check */ + return NULL; + bufsize *= 2; } } @@ -391,6 +394,9 @@ char* gid_to_name(gid_t gid) { if (r != ERANGE) break; + if (bufsize > LONG_MAX/2) /* overflow check */ + return NULL; + bufsize *= 2; } } From d575f88bbe5bedfdb90cdef2d0aef7df63595ae1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 7 Mar 2019 10:53:23 +0100 Subject: [PATCH 07/13] user-util: be more strict when reading $HOME and $SHELL --- src/basic/user-util.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/basic/user-util.c b/src/basic/user-util.c index 4cf4c5a341..47d6763f1c 100644 --- a/src/basic/user-util.c +++ b/src/basic/user-util.c @@ -458,7 +458,7 @@ int get_home_dir(char **_h) { /* Take the user specified one */ e = secure_getenv("HOME"); - if (e && path_is_absolute(e)) { + if (e && path_is_valid(e) && path_is_absolute(e)) { h = strdup(e); if (!h) return -ENOMEM; @@ -493,7 +493,8 @@ int get_home_dir(char **_h) { if (!p) return errno > 0 ? -errno : -ESRCH; - if (!path_is_absolute(p->pw_dir)) + if (!path_is_valid(p->pw_dir) || + !path_is_absolute(p->pw_dir)) return -EINVAL; h = strdup(p->pw_dir); @@ -514,7 +515,7 @@ int get_shell(char **_s) { /* Take the user specified one */ e = getenv("SHELL"); - if (e) { + if (e && path_is_valid(e) && path_is_absolute(e)) { s = strdup(e); if (!s) return -ENOMEM; @@ -549,7 +550,8 @@ int get_shell(char **_s) { if (!p) return errno > 0 ? -errno : -ESRCH; - if (!path_is_absolute(p->pw_shell)) + if (!path_is_valid(p->pw_shell) || + !path_is_absolute(p->pw_shell)) return -EINVAL; s = strdup(p->pw_shell); From db246781a0ea28eb63137bb58486b1419e50d6f8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 7 Mar 2019 10:54:11 +0100 Subject: [PATCH 08/13] user-util: simplify paths retrieved from $HOME and $SHELL Let's add some extra paranoia, after #11910 --- src/basic/user-util.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/basic/user-util.c b/src/basic/user-util.c index 47d6763f1c..5f1bd5f5a2 100644 --- a/src/basic/user-util.c +++ b/src/basic/user-util.c @@ -463,7 +463,7 @@ int get_home_dir(char **_h) { if (!h) return -ENOMEM; - *_h = h; + *_h = path_simplify(h, true); return 0; } @@ -501,7 +501,7 @@ int get_home_dir(char **_h) { if (!h) return -ENOMEM; - *_h = h; + *_h = path_simplify(h, true); return 0; } @@ -520,7 +520,7 @@ int get_shell(char **_s) { if (!s) return -ENOMEM; - *_s = s; + *_s = path_simplify(s, true); return 0; } @@ -558,7 +558,7 @@ int get_shell(char **_s) { if (!s) return -ENOMEM; - *_s = s; + *_s = path_simplify(s, true); return 0; } From 7bbead1d0b8339d9878f8c9debf4d0966f442b9e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 7 Mar 2019 11:00:13 +0100 Subject: [PATCH 09/13] execute: simplify paths we set as HOME/SHELL for invoked programs --- src/core/execute.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/core/execute.c b/src/core/execute.c index c6fd82bbf3..d74affebb3 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -1697,6 +1697,8 @@ static int build_environment( x = strappend("HOME=", home); if (!x) return -ENOMEM; + + path_simplify(x + 5, true); our_env[n_env++] = x; } @@ -1716,6 +1718,8 @@ static int build_environment( x = strappend("SHELL=", shell); if (!x) return -ENOMEM; + + path_simplify(x + 6, true); our_env[n_env++] = x; } From 9e73208afc7cf8dbeb4733e08b93393cc8f2ce8a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 7 Mar 2019 11:00:40 +0100 Subject: [PATCH 10/13] execute: no need to synthesize $HOME for uid==0 again, get_home_dir() already does that --- src/core/execute.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index d74affebb3..fb7564b9fe 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2742,12 +2742,6 @@ static int acquire_home(const ExecContext *c, uid_t uid, const char** home, char if (!c->working_directory_home) return 0; - if (uid == 0) { - /* Hardcode /root as home directory for UID 0 */ - *home = "/root"; - return 1; - } - r = get_home_dir(buf); if (r < 0) return r; From b2a3953f817d8db15393e30d1f46e4fa85fcf23a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 12 Mar 2019 15:52:06 +0100 Subject: [PATCH 11/13] user-util: extra paranoia, make sure $SHELL can't be fucked with in suid programs It's better to be safe than sorry, let's not allow overriding of the user shell in suid binaries. Similar for $USER. --- src/basic/user-util.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/basic/user-util.c b/src/basic/user-util.c index 5f1bd5f5a2..a479590e47 100644 --- a/src/basic/user-util.c +++ b/src/basic/user-util.c @@ -80,7 +80,7 @@ char* getlogname_malloc(void) { char *getusername_malloc(void) { const char *e; - e = getenv("USER"); + e = secure_getenv("USER"); if (e) return strdup(e); @@ -514,7 +514,7 @@ int get_shell(char **_s) { assert(_s); /* Take the user specified one */ - e = getenv("SHELL"); + e = secure_getenv("SHELL"); if (e && path_is_valid(e) && path_is_absolute(e)) { s = strdup(e); if (!s) From f3b5c814abda9707e00510fb50412427362cca9d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 11 Mar 2019 18:28:06 +0100 Subject: [PATCH 12/13] login: drop redundant newline --- src/login/logind-dbus.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 13e7d3bcda..a3b8f43346 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -2711,7 +2711,6 @@ static int method_can_reboot_to_boot_loader_menu( Manager *m = userdata; int r; - assert(message); assert(m); From f3ae265f5c247e172650900bd574679239be925d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 12 Mar 2019 15:53:05 +0100 Subject: [PATCH 13/13] update TODO --- TODO | 3 +++ 1 file changed, 3 insertions(+) diff --git a/TODO b/TODO index 9e4e07da5e..d8e13e5e61 100644 --- a/TODO +++ b/TODO @@ -23,6 +23,9 @@ Janitorial Clean-ups: Features: +* maybe add a seccomp-based high-level filter that blocks creation of suid/sgid + files. + * make MAINPID= message reception checks even stricter: if service uses User=, then check sending UID and ignore message if it doesn't match the user or root.