From 30ff18d8a2cd74328e5915a14c059cd122f32710 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 14 Mar 2019 16:47:03 +0100 Subject: [PATCH 1/6] fs-util: change chmod_and_chown() to not complain if stat data already matches Let's reduce the chance of failure: if we can't apply the chmod/chown requested, check if it's applied anyway, and if so, supress the error. This is even race-free since we operate on an O_PATH fd anyway. --- src/basic/fs-util.c | 79 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 62 insertions(+), 17 deletions(-) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index 431c8501ea..ce1f5454c8 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -215,64 +215,109 @@ int readlink_and_make_absolute(const char *p, char **r) { int chmod_and_chown(const char *path, mode_t mode, uid_t uid, gid_t gid) { char fd_path[STRLEN("/proc/self/fd/") + DECIMAL_STR_MAX(int) + 1]; _cleanup_close_ int fd = -1; + bool st_valid = false; + struct stat st; + int r; + assert(path); - /* Under the assumption that we are running privileged we first change the access mode and only then hand out - * ownership to avoid a window where access is too open. */ + /* Under the assumption that we are running privileged we first change the access mode and only then + * hand out ownership to avoid a window where access is too open. */ - fd = open(path, O_PATH|O_CLOEXEC|O_NOFOLLOW); /* Let's acquire an O_PATH fd, as precaution to change mode/owner - * on the same file */ + fd = open(path, O_PATH|O_CLOEXEC|O_NOFOLLOW); /* Let's acquire an O_PATH fd, as precaution to change + * mode/owner on the same file */ if (fd < 0) return -errno; xsprintf(fd_path, "/proc/self/fd/%i", fd); if (mode != MODE_INVALID) { - if ((mode & S_IFMT) != 0) { - struct stat st; if (stat(fd_path, &st) < 0) return -errno; if ((mode & S_IFMT) != (st.st_mode & S_IFMT)) return -EINVAL; + + st_valid = true; } - if (chmod(fd_path, mode & 07777) < 0) - return -errno; + if (chmod(fd_path, mode & 07777) < 0) { + r = -errno; + + if (!st_valid && stat(fd_path, &st) < 0) + return -errno; + + if ((mode & 07777) != (st.st_mode & 07777)) + return r; + + st_valid = true; + } } - if (uid != UID_INVALID || gid != GID_INVALID) - if (chown(fd_path, uid, gid) < 0) - return -errno; + if (uid != UID_INVALID || gid != GID_INVALID) { + if (chown(fd_path, uid, gid) < 0) { + r = -errno; + + if (!st_valid && stat(fd_path, &st) < 0) + return -errno; + + if (uid != UID_INVALID && st.st_uid != uid) + return r; + if (gid != GID_INVALID && st.st_gid != gid) + return r; + } + } return 0; } int fchmod_and_chown(int fd, mode_t mode, uid_t uid, gid_t gid) { + bool st_valid = false; + struct stat st; + int r; + /* Under the assumption that we are running privileged we first change the access mode and only then hand out * ownership to avoid a window where access is too open. */ if (mode != MODE_INVALID) { - if ((mode & S_IFMT) != 0) { - struct stat st; if (fstat(fd, &st) < 0) return -errno; if ((mode & S_IFMT) != (st.st_mode & S_IFMT)) return -EINVAL; + + st_valid = true; } - if (fchmod(fd, mode & 0777) < 0) - return -errno; + if (fchmod(fd, mode & 07777) < 0) { + r = -errno; + + if (!st_valid && fstat(fd, &st) < 0) + return -errno; + + if ((mode & 07777) != (st.st_mode & 07777)) + return r; + + st_valid = true; + } } if (uid != UID_INVALID || gid != GID_INVALID) - if (fchown(fd, uid, gid) < 0) - return -errno; + if (fchown(fd, uid, gid) < 0) { + r = -errno; + + if (!st_valid && fstat(fd, &st) < 0) + return -errno; + + if (uid != UID_INVALID && st.st_uid != uid) + return r; + if (gid != GID_INVALID && st.st_gid != gid) + return r; + } return 0; } From edbfeb1204b41fc5e08432acce89d1bf9a556fd9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 14 Mar 2019 16:56:45 +0100 Subject: [PATCH 2/6] execute: use path_join() where appropriate --- src/core/execute.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index cf137fcd3a..e7be6c4a2d 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2074,7 +2074,7 @@ static int setup_exec_directory( STRV_FOREACH(rt, context->directories[type].paths) { _cleanup_free_ char *p = NULL, *pp = NULL; - p = strjoin(params->prefix[type], "/", *rt); + p = path_join(params->prefix[type], *rt); if (!p) { r = -ENOMEM; goto fail; @@ -2110,7 +2110,7 @@ static int setup_exec_directory( * Also, note that we don't do this for EXEC_DIRECTORY_RUNTIME as that's often used for sharing * files or sockets with other services. */ - private_root = strjoin(params->prefix[type], "/private"); + private_root = path_join(params->prefix[type], "private"); if (!private_root) { r = -ENOMEM; goto fail; @@ -2121,7 +2121,7 @@ static int setup_exec_directory( if (r < 0) goto fail; - pp = strjoin(private_root, "/", *rt); + pp = path_join(private_root, *rt); if (!pp) { r = -ENOMEM; goto fail; From 40cd2ecc26b776ef085fd0fd29e8e96f6422a0d3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 14 Mar 2019 17:00:35 +0100 Subject: [PATCH 3/6] execute: also do the private/ symlink dance when runtime dir preservation is requested In that case it's not safe to leave a regular dir around, hence, move it to private/ too. --- src/core/execute.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/core/execute.c b/src/core/execute.c index e7be6c4a2d..0fe084f211 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2085,7 +2085,8 @@ static int setup_exec_directory( goto fail; if (context->dynamic_user && - !IN_SET(type, EXEC_DIRECTORY_RUNTIME, EXEC_DIRECTORY_CONFIGURATION)) { + (!IN_SET(type, EXEC_DIRECTORY_RUNTIME, EXEC_DIRECTORY_CONFIGURATION) || + (type == EXEC_DIRECTORY_RUNTIME && context->runtime_directory_preserve_mode != EXEC_PRESERVE_NO))) { _cleanup_free_ char *private_root = NULL; /* So, here's one extra complication when dealing with DynamicUser=1 units. In that case we From d484580ca6f0e79abe6f3f5c677323a22d9e22d7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 14 Mar 2019 17:01:46 +0100 Subject: [PATCH 4/6] execute: remove one redundant comparison check --- src/core/execute.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 0fe084f211..2012cd596a 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2164,11 +2164,12 @@ static int setup_exec_directory( } } else { r = mkdir_label(p, context->directories[type].mode); - if (r < 0 && r != -EEXIST) - goto fail; - if (r == -EEXIST) { + if (r < 0) { struct stat st; + if (r != -EEXIST) + goto fail; + if (stat(p, &st) < 0) { r = -errno; goto fail; From 206e9864de460dd79d9edd7bedb47dee168765e1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 14 Mar 2019 17:19:30 +0100 Subject: [PATCH 5/6] core: change ownership/mode of the execution directories also for static users It's probably unexpected if we do a recursive chown() when dynamic users are used but not on static users. hence, let's tweak the logic slightly, and recursively chown in both cases, except when operating on the configuration directory. Fixes: #11842 --- src/core/execute.c | 47 +++++++++++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 2012cd596a..6df90166ee 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2157,37 +2157,42 @@ static int setup_exec_directory( if (r < 0) goto fail; - /* Lock down the access mode */ - if (chmod(pp, context->directories[type].mode) < 0) { - r = -errno; - goto fail; - } } else { r = mkdir_label(p, context->directories[type].mode); if (r < 0) { - struct stat st; - if (r != -EEXIST) goto fail; - if (stat(p, &st) < 0) { - r = -errno; - goto fail; - } - if (((st.st_mode ^ context->directories[type].mode) & 07777) != 0) - log_warning("%s \'%s\' already exists but the mode is different. " - "(filesystem: %o %sMode: %o)", - exec_directory_type_to_string(type), *rt, - st.st_mode & 07777, exec_directory_type_to_string(type), context->directories[type].mode & 07777); - if (!context->dynamic_user) + if (type == EXEC_DIRECTORY_CONFIGURATION) { + struct stat st; + + /* Don't change the owner/access mode of the configuration directory, + * as in the common case it is not written to by a service, and shall + * not be writable. */ + + if (stat(p, &st) < 0) { + r = -errno; + goto fail; + } + + /* Still complain if the access mode doesn't match */ + if (((st.st_mode ^ context->directories[type].mode) & 07777) != 0) + log_warning("%s \'%s\' already exists but the mode is different. " + "(File system: %o %sMode: %o)", + exec_directory_type_to_string(type), *rt, + st.st_mode & 07777, exec_directory_type_to_string(type), context->directories[type].mode & 07777); + continue; + } } } - /* Don't change the owner of the configuration directory, as in the common case it is not written to by - * a service, and shall not be writable. */ - if (type == EXEC_DIRECTORY_CONFIGURATION) - continue; + /* Lock down the access mode (we use chmod_and_chown() to make this idempotent. We don't + * specifiy UID/GID here, so that path_chown_recursive() can optimize things depending on the + * current UID/GID ownership.) */ + r = chmod_and_chown(pp ?: p, context->directories[type].mode, UID_INVALID, GID_INVALID); + if (r < 0) + goto fail; /* Then, change the ownership of the whole tree, if necessary */ r = path_chown_recursive(pp ?: p, uid, gid); From 08f6769675a30cf2c322d31b9fdb21bb6e1fb7e7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 14 Mar 2019 17:24:47 +0100 Subject: [PATCH 6/6] execute: generalize uid/gid handling in two cases for any kind of uid/gid --- src/core/execute.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 6df90166ee..d46476630c 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -3161,7 +3161,7 @@ static int exec_child( USER_PROCESS, username); - if (context->user) { + if (uid_is_valid(uid)) { r = chown_terminal(STDIN_FILENO, uid); if (r < 0) { *exit_status = EXIT_STDIN; @@ -3466,7 +3466,7 @@ static int exec_child( } if (needs_setuid) { - if (context->user) { + if (uid_is_valid(uid)) { r = enforce_user(context, uid); if (r < 0) { *exit_status = EXIT_USER;