From 2dbb7e94af4f9366db2f1d5c4ca37d84f4df8d96 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 29 Apr 2019 20:15:06 +0200 Subject: [PATCH 1/6] fs-util: rewrite chmod_and_chown() Inspired by #12431 let's also rework chmod_and_chown() and make sure we never add more rights to a file not owned by the right user. Also, let's make chmod_and_chown() just a wrapper arond fchmod_and_chown(). let's also change strategy: instead of chown()ing first and stating after on failure and supressing errors, let's avoid the chown in the firts place, in the interest on keeping things minimal. --- src/basic/fs-util.c | 127 ++++++++++++++------------------------------ 1 file changed, 41 insertions(+), 86 deletions(-) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index d1c06cf12a3..5a3f8b827f3 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -213,113 +213,68 @@ 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. */ - 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) { - - 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) { - 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) { - 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; + return fchmod_and_chown(fd, mode, uid, gid); } int fchmod_and_chown(int fd, mode_t mode, uid_t uid, gid_t gid) { - bool st_valid = false; + char fd_path[STRLEN("/proc/self/fd/") + DECIMAL_STR_MAX(int) + 1]; + bool do_chown, do_chmod; 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. */ + /* Change ownership and access mode of the specified fd. Tries to do so safely, ensuring that at no + * point in time the access mode is above the old access mode under the old ownership or the new + * access mode under the new ownership. Note: this call tries hard to leave the access mode + * unaffected if the uid/gid is changed, i.e. it undoes implicit suid/sgid dropping the kernel does + * on chown(). + * + * This call is happy with O_PATH fds, since we always go via /proc/self/fd/ to change + * ownership/access mode. */ - if (mode != MODE_INVALID) { - if ((mode & S_IFMT) != 0) { + xsprintf(fd_path, "/proc/self/fd/%i", fd); + if (stat(fd_path, &st) < 0) + return -errno; - if (fstat(fd, &st) < 0) + do_chown = + (uid != UID_INVALID && st.st_uid != uid) || + (gid != GID_INVALID && st.st_gid != gid); + + do_chmod = + !S_ISLNK(st.st_mode) && /* chmod is not defined on symlinks */ + ((mode != MODE_INVALID && ((st.st_mode ^ mode) & 07777) != 0) || + do_chown); /* If we change ownership, make sure we reset the mode afterwards, since chown() + * modifies the access mode too */ + + if (mode == MODE_INVALID) + mode = st.st_mode; /* If we only shall do a chown(), save original mode, since chown() might break it. */ + else if ((mode & S_IFMT) != 0 && ((mode ^ st.st_mode) & S_IFMT) != 0) + return -EINVAL; /* insist on the right file type if it was specified */ + + if (do_chown && do_chmod) { + mode_t minimal = st.st_mode & mode; /* the subset of the old and the new mask */ + + if (((minimal ^ st.st_mode) & 07777) != 0) + if (chmod(fd_path, minimal & 07777) < 0) return -errno; - - if ((mode & S_IFMT) != (st.st_mode & S_IFMT)) - return -EINVAL; - - st_valid = true; - } - - 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) { - r = -errno; + if (do_chown) + if (chown(fd_path, uid, gid) < 0) + return -errno; - if (!st_valid && fstat(fd, &st) < 0) - return -errno; + if (do_chmod) + if (chmod(fd_path, mode & 07777) < 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; + return do_chown || do_chmod; } int fchmod_umask(int fd, mode_t m) { From c1447be4d4fe7b4228586c675d1222d65419969d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 29 Apr 2019 20:18:21 +0200 Subject: [PATCH 2/6] tests: add simple test case for chmod_and_chown() --- src/test/test-fs-util.c | 46 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/src/test/test-fs-util.c b/src/test/test-fs-util.c index f0f015056e2..0610416d370 100644 --- a/src/test/test-fs-util.c +++ b/src/test/test-fs-util.c @@ -15,6 +15,7 @@ #include "strv.h" #include "tests.h" #include "tmpfile-util.h" +#include "umask-util.h" #include "user-util.h" #include "util.h" #include "virt.h" @@ -746,6 +747,50 @@ static void test_rename_noreplace(void) { } } +static void test_chmod_and_chown(void) { + _cleanup_(rm_rf_physical_and_freep) char *d = NULL; + _unused_ _cleanup_umask_ mode_t u = umask(0000); + struct stat st; + const char *p; + + if (geteuid() != 0) + return; + + log_info("/* %s */", __func__); + + assert_se(mkdtemp_malloc(NULL, &d) >= 0); + + p = strjoina(d, "/reg"); + assert_se(mknod(p, S_IFREG | 0123, 0) >= 0); + + assert_se(chmod_and_chown(p, S_IFREG | 0321, 1, 2) >= 0); + assert_se(chmod_and_chown(p, S_IFDIR | 0555, 3, 4) == -EINVAL); + + assert_se(lstat(p, &st) >= 0); + assert_se(S_ISREG(st.st_mode)); + assert_se((st.st_mode & 07777) == 0321); + + p = strjoina(d, "/dir"); + assert_se(mkdir(p, 0123) >= 0); + + assert_se(chmod_and_chown(p, S_IFDIR | 0321, 1, 2) >= 0); + assert_se(chmod_and_chown(p, S_IFREG | 0555, 3, 4) == -EINVAL); + + assert_se(lstat(p, &st) >= 0); + assert_se(S_ISDIR(st.st_mode)); + assert_se((st.st_mode & 07777) == 0321); + + p = strjoina(d, "/lnk"); + assert_se(symlink("idontexist", p) >= 0); + + assert_se(chmod_and_chown(p, S_IFLNK | 0321, 1, 2) >= 0); + assert_se(chmod_and_chown(p, S_IFREG | 0555, 3, 4) == -EINVAL); + assert_se(chmod_and_chown(p, S_IFDIR | 0555, 3, 4) == -EINVAL); + + assert_se(lstat(p, &st) >= 0); + assert_se(S_ISLNK(st.st_mode)); +} + int main(int argc, char *argv[]) { test_setup_logging(LOG_INFO); @@ -762,6 +807,7 @@ int main(int argc, char *argv[]) { test_unlinkat_deallocate(); test_fsync_directory_of_file(); test_rename_noreplace(); + test_chmod_and_chown(); return 0; } From f8a8579e84bf5cb33d2c331efd74ffeaf8072539 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 30 Apr 2019 09:28:10 +0200 Subject: [PATCH 3/6] tmpfile-util: simplify umask reset declaration --- src/basic/tmpfile-util.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/basic/tmpfile-util.c b/src/basic/tmpfile-util.c index e77af7659f0..5100d6a5ff9 100644 --- a/src/basic/tmpfile-util.c +++ b/src/basic/tmpfile-util.c @@ -58,13 +58,11 @@ int fopen_temporary(const char *path, FILE **_f, char **_temp_path) { /* This is much like mkostemp() but is subject to umask(). */ int mkostemp_safe(char *pattern) { - _cleanup_umask_ mode_t u = 0; + _unused_ _cleanup_umask_ mode_t u = umask(0077); int fd; assert(pattern); - u = umask(077); - fd = mkostemp(pattern, O_CLOEXEC); if (fd < 0) return -errno; From 4b3b5bc71b791a67d991389d8f59ab891b051b86 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 30 Apr 2019 19:25:29 +0200 Subject: [PATCH 4/6] tree-wide: port various places over to use chmod_and_chown() Doing this properly is hard, hence let's unify the code. --- src/basic/fs-util.c | 8 +------- src/core/chown-recursive.c | 23 +++++++---------------- src/core/execute.c | 23 +++++++++++------------ src/random-seed/random-seed.c | 4 ++-- src/udev/udev-node.c | 8 ++++---- src/udev/udev-rules.c | 24 +++++++----------------- 6 files changed, 32 insertions(+), 58 deletions(-) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index 5a3f8b827f3..7dfc1b309d9 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -359,13 +359,7 @@ int touch_file(const char *path, bool parents, usec_t stamp, uid_t uid, gid_t gi * something fchown(), fchmod(), futimensat() don't allow. */ xsprintf(fdpath, "/proc/self/fd/%i", fd); - if (mode != MODE_INVALID) - if (chmod(fdpath, mode) < 0) - ret = -errno; - - if (uid_is_valid(uid) || gid_is_valid(gid)) - if (chown(fdpath, uid, gid) < 0 && ret >= 0) - ret = -errno; + ret = fchmod_and_chown(fd, mode, uid, gid); if (stamp != USEC_INFINITY) { struct timespec ts[2]; diff --git a/src/core/chown-recursive.c b/src/core/chown-recursive.c index fb428658756..24cdf25b838 100644 --- a/src/core/chown-recursive.c +++ b/src/core/chown-recursive.c @@ -8,6 +8,7 @@ #include "chown-recursive.h" #include "dirent-util.h" #include "fd-util.h" +#include "fs-util.h" #include "macro.h" #include "stdio-util.h" #include "strv.h" @@ -22,16 +23,13 @@ static int chown_one( char procfs_path[STRLEN("/proc/self/fd/") + DECIMAL_STR_MAX(int) + 1]; const char *n; + int r; assert(fd >= 0); assert(st); - if ((!uid_is_valid(uid) || st->st_uid == uid) && - (!gid_is_valid(gid) || st->st_gid == gid)) - return 0; - - /* We change ownership through the /proc/self/fd/%i path, so that we have a stable reference that works with - * O_PATH. (Note: fchown() and fchmod() do not work with O_PATH, the kernel refuses that. */ + /* We change ACLs through the /proc/self/fd/%i path, so that we have a stable reference that works + * with O_PATH. */ xsprintf(procfs_path, "/proc/self/fd/%i", fd); /* Drop any ACL if there is one */ @@ -40,16 +38,9 @@ static int chown_one( if (!IN_SET(errno, ENODATA, EOPNOTSUPP, ENOSYS, ENOTTY)) return -errno; - if (chown(procfs_path, uid, gid) < 0) - return -errno; - - /* The linux kernel alters the mode in some cases of chown(), as well when we change ACLs. Let's undo this. We - * do this only for non-symlinks however. That's because for symlinks the access mode is ignored anyway and - * because on some kernels/file systems trying to change the access mode will succeed but has no effect while - * on others it actively fails. */ - if (!S_ISLNK(st->st_mode)) - if (chmod(procfs_path, st->st_mode & 07777 & mask) < 0) - return -errno; + r = fchmod_and_chown(fd, st->st_mode & mask, uid, gid); + if (r < 0) + return r; return 1; } diff --git a/src/core/execute.c b/src/core/execute.c index e90c3ac4f38..ab2a4de37a5 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -728,25 +728,24 @@ static int setup_output( } static int chown_terminal(int fd, uid_t uid) { - struct stat st; + int r; assert(fd >= 0); /* Before we chown/chmod the TTY, let's ensure this is actually a tty */ - if (isatty(fd) < 1) - return 0; + if (isatty(fd) < 1) { + if (IN_SET(errno, EINVAL, ENOTTY)) + return 0; /* not a tty */ + + return -errno; + } /* This might fail. What matters are the results. */ - (void) fchown(fd, uid, -1); - (void) fchmod(fd, TTY_MODE); + r = fchmod_and_chown(fd, TTY_MODE, uid, -1); + if (r < 0) + return r; - if (fstat(fd, &st) < 0) - return -errno; - - if (st.st_uid != uid || (st.st_mode & 0777) != TTY_MODE) - return -EPERM; - - return 0; + return 1; } static int setup_confirm_stdio(const char *vc, int *_saved_stdin, int *_saved_stdout) { diff --git a/src/random-seed/random-seed.c b/src/random-seed/random-seed.c index 71c2dba4318..510a2715f2a 100644 --- a/src/random-seed/random-seed.c +++ b/src/random-seed/random-seed.c @@ -10,6 +10,7 @@ #include "alloc-util.h" #include "fd-util.h" +#include "fs-util.h" #include "io-util.h" #include "log.h" #include "main-func.h" @@ -156,8 +157,7 @@ static int run(int argc, char *argv[]) { /* This is just a safety measure. Given that we are root and * most likely created the file ourselves the mode and owner * should be correct anyway. */ - (void) fchmod(seed_fd, 0600); - (void) fchown(seed_fd, 0, 0); + (void) fchmod_and_chown(seed_fd, 0600, 0, 0); k = loop_read(random_fd, buf, buf_size, false); if (k < 0) diff --git a/src/udev/udev-node.c b/src/udev/udev-node.c index cfbbd7b2832..67d573cf2b6 100644 --- a/src/udev/udev-node.c +++ b/src/udev/udev-node.c @@ -310,10 +310,10 @@ static int node_permissions_apply(sd_device *dev, bool apply, if ((stats.st_mode & 0777) != (mode & 0777) || stats.st_uid != uid || stats.st_gid != gid) { log_device_debug(dev, "Setting permissions %s, %#o, uid=%u, gid=%u", devnode, mode, uid, gid); - if (chmod(devnode, mode) < 0) - r = log_device_warning_errno(dev, errno, "Failed to set mode of %s to %#o: %m", devnode, mode); - if (chown(devnode, uid, gid) < 0) - r = log_device_warning_errno(dev, errno, "Failed to set owner of %s to uid=%u, gid=%u: %m", devnode, uid, gid); + + r = chmod_and_chown(devnode, mode, uid, gid); + if (r < 0) + log_device_warning_errno(dev, r, "Failed to set owner/mode of %s to uid=" UID_FMT ", gid=" GID_FMT ", mode=%#o: %m", devnode, uid, gid, mode); } else log_device_debug(dev, "Preserve permissions of %s, %#o, uid=%u, gid=%u", devnode, mode, uid, gid); diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index 96840b272ca..696d98a40ad 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -22,6 +22,7 @@ #include "escape.h" #include "fd-util.h" #include "fileio.h" +#include "format-util.h" #include "fs-util.h" #include "glob-util.h" #include "libudev-util.h" @@ -2591,25 +2592,14 @@ int udev_rules_apply_static_dev_perms(UdevRules *rules) { else mode = 0600; } - if (mode != (stats.st_mode & 01777)) { - r = chmod(device_node, mode); - if (r < 0) - return log_error_errno(errno, "Failed to chmod '%s' %#o: %m", - device_node, mode); - else - log_debug("chmod '%s' %#o", device_node, mode); - } - if ((uid != 0 && uid != stats.st_uid) || (gid != 0 && gid != stats.st_gid)) { - r = chown(device_node, uid, gid); - if (r < 0) - return log_error_errno(errno, "Failed to chown '%s' %u %u: %m", - device_node, uid, gid); - else - log_debug("chown '%s' %u %u", device_node, uid, gid); - } + r = chmod_and_chown(device_node, mode, uid, gid); + if (r < 0) + return log_error_errno(r, "Failed to chown/chmod '%s' uid=" UID_FMT ", gid=" GID_FMT ", mode=%#o: %m", device_node, uid, gid, mode); + if (r > 0) + log_debug("chown/chmod '%s' uid=" UID_FMT ", gid=" GID_FMT ", mode=%#o", device_node, uid, gid, mode); - utimensat(AT_FDCWD, device_node, NULL, 0); + (void) utimensat(AT_FDCWD, device_node, NULL, 0); break; } case TK_END: From 1802d5f2cf24e43a81502fbeaa839d9b9eb7e361 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 30 Apr 2019 19:26:14 +0200 Subject: [PATCH 5/6] terminal-util: reset access mode in vt_restore(), too Only changing ownership back to root is not enough we also need to change the access mode, otherwise the user might have set 666 first, and thus allow everyone access before and after the chown(). --- src/basic/terminal-util.c | 12 +++++------- src/basic/terminal-util.h | 3 +++ src/core/execute.c | 3 --- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/basic/terminal-util.c b/src/basic/terminal-util.c index 71238ac9c43..3a0d16a74f5 100644 --- a/src/basic/terminal-util.c +++ b/src/basic/terminal-util.c @@ -1291,8 +1291,7 @@ int vt_restore(int fd) { }; int r, q = 0; - r = ioctl(fd, KDSETMODE, KD_TEXT); - if (r < 0) + if (ioctl(fd, KDSETMODE, KD_TEXT) < 0) q = log_debug_errno(errno, "Failed to set VT in text mode, ignoring: %m"); r = vt_reset_keyboard(fd); @@ -1302,18 +1301,17 @@ int vt_restore(int fd) { q = r; } - r = ioctl(fd, VT_SETMODE, &mode); - if (r < 0) { + if (ioctl(fd, VT_SETMODE, &mode) < 0) { log_debug_errno(errno, "Failed to set VT_AUTO mode, ignoring: %m"); if (q >= 0) q = -errno; } - r = fchown(fd, 0, (gid_t) -1); + r = fchmod_and_chown(fd, TTY_MODE, 0, (gid_t) -1); if (r < 0) { - log_debug_errno(errno, "Failed to chown VT, ignoring: %m"); + log_debug_errno(r, "Failed to chmod()/chown() VT, ignoring: %m"); if (q >= 0) - q = -errno; + q = r; } return q; diff --git a/src/basic/terminal-util.h b/src/basic/terminal-util.h index 90adc14d176..87d09f87598 100644 --- a/src/basic/terminal-util.h +++ b/src/basic/terminal-util.h @@ -163,3 +163,6 @@ int vt_restore(int fd); int vt_release(int fd, bool restore_vt); void get_log_colors(int priority, const char **on, const char **off, const char **highlight); + +/* This assumes there is a 'tty' group */ +#define TTY_MODE 0620 diff --git a/src/core/execute.c b/src/core/execute.c index ab2a4de37a5..640efac295d 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -97,9 +97,6 @@ #define IDLE_TIMEOUT_USEC (5*USEC_PER_SEC) #define IDLE_TIMEOUT2_USEC (1*USEC_PER_SEC) -/* This assumes there is a 'tty' group */ -#define TTY_MODE 0620 - #define SNDBUF_SIZE (8*1024*1024) static int shift_fds(int fds[], size_t n_fds) { From f63383a31fa42ca5171692188a10c5be3f128b95 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 30 Apr 2019 19:32:31 +0200 Subject: [PATCH 6/6] test-fs-util: don't validate mode of symlinks Some file systems refuse changing the mode, and the mode has zero effect anyway, hence let's no test for this. --- src/test/test-fs-util.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/test-fs-util.c b/src/test/test-fs-util.c index 0610416d370..85d679826fc 100644 --- a/src/test/test-fs-util.c +++ b/src/test/test-fs-util.c @@ -636,7 +636,6 @@ static void test_touch_file(void) { assert_se(st.st_uid == test_uid); assert_se(st.st_gid == test_gid); assert_se(S_ISLNK(st.st_mode)); - assert_se((st.st_mode & 0777) == 0640); assert_se(timespec_load(&st.st_mtim) == test_mtime); }