diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index d1c06cf12a3..7dfc1b309d9 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) { @@ -404,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/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/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; 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..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) { @@ -728,25 +725,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/test/test-fs-util.c b/src/test/test-fs-util.c index f0f015056e2..85d679826fc 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" @@ -635,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); } @@ -746,6 +746,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 +806,7 @@ int main(int argc, char *argv[]) { test_unlinkat_deallocate(); test_fsync_directory_of_file(); test_rename_noreplace(); + test_chmod_and_chown(); return 0; } diff --git a/src/udev/udev-node.c b/src/udev/udev-node.c index 9549a1ba442..7d254785776 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: