From 5de6cce58b3e8b79239b6e83653459d91af6e57c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 19 Oct 2018 11:26:59 +0200 Subject: [PATCH 1/4] chown-recursive: let's rework the recursive logic to use O_PATH That way we can pin a specific inode and analyze it and manipulate it without it being swapped out beneath our hands. Fixes a vulnerability originally found by Jann Horn from Google. CVE-2018-15687 LP: #1796692 https://bugzilla.redhat.com/show_bug.cgi?id=1639076 --- src/core/chown-recursive.c | 144 ++++++++++++++++++------------------- 1 file changed, 69 insertions(+), 75 deletions(-) diff --git a/src/core/chown-recursive.c b/src/core/chown-recursive.c index c4794501c2c..27c64489b55 100644 --- a/src/core/chown-recursive.c +++ b/src/core/chown-recursive.c @@ -1,17 +1,19 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ -#include -#include #include +#include +#include -#include "user-util.h" -#include "macro.h" -#include "fd-util.h" -#include "dirent-util.h" #include "chown-recursive.h" +#include "dirent-util.h" +#include "fd-util.h" +#include "macro.h" +#include "stdio-util.h" +#include "strv.h" +#include "user-util.h" -static int chown_one(int fd, const char *name, const struct stat *st, uid_t uid, gid_t gid) { - int r; +static int chown_one(int fd, const struct stat *st, uid_t uid, gid_t gid) { + char procfs_path[STRLEN("/proc/self/fd/") + DECIMAL_STR_MAX(int) + 1]; assert(fd >= 0); assert(st); @@ -20,90 +22,82 @@ static int chown_one(int fd, const char *name, const struct stat *st, uid_t uid, (!gid_is_valid(gid) || st->st_gid == gid)) return 0; - if (name) - r = fchownat(fd, name, uid, gid, AT_SYMLINK_NOFOLLOW); - else - r = fchown(fd, uid, gid); - if (r < 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. */ + xsprintf(procfs_path, "/proc/self/fd/%i", fd); + + if (chown(procfs_path, uid, gid) < 0) return -errno; - /* The linux kernel alters the mode in some cases of chown(). Let's undo this. */ - if (name) { - if (!S_ISLNK(st->st_mode)) - r = fchmodat(fd, name, st->st_mode, 0); - else /* There's currently no AT_SYMLINK_NOFOLLOW for fchmodat() */ - r = 0; - } else - r = fchmod(fd, st->st_mode); - if (r < 0) - return -errno; + /* The linux kernel alters the mode in some cases of chown(). 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) < 0) + return -errno; return 1; } static int chown_recursive_internal(int fd, const struct stat *st, uid_t uid, gid_t gid) { + _cleanup_closedir_ DIR *d = NULL; bool changed = false; + struct dirent *de; int r; assert(fd >= 0); assert(st); - if (S_ISDIR(st->st_mode)) { - _cleanup_closedir_ DIR *d = NULL; - struct dirent *de; + d = fdopendir(fd); + if (!d) { + safe_close(fd); + return -errno; + } - d = fdopendir(fd); - if (!d) { - r = -errno; - goto finish; + FOREACH_DIRENT_ALL(de, d, return -errno) { + _cleanup_close_ int path_fd = -1; + struct stat fst; + + if (dot_or_dot_dot(de->d_name)) + continue; + + /* Let's pin the child inode we want to fix now with an O_PATH fd, so that it cannot be swapped out + * while we manipulate it. */ + path_fd = openat(dirfd(d), de->d_name, O_PATH|O_CLOEXEC|O_NOFOLLOW); + if (path_fd < 0) + return -errno; + + if (fstat(path_fd, &fst) < 0) + return -errno; + + if (S_ISDIR(fst.st_mode)) { + int subdir_fd; + + /* Convert it to a "real" (i.e. non-O_PATH) fd now */ + subdir_fd = fd_reopen(path_fd, O_RDONLY|O_CLOEXEC|O_NOATIME); + if (subdir_fd < 0) + return subdir_fd; + + r = chown_recursive_internal(subdir_fd, &fst, uid, gid); /* takes possession of subdir_fd even on failure */ + if (r < 0) + return r; + if (r > 0) + changed = true; + } else { + r = chown_one(path_fd, &fst, uid, gid); + if (r < 0) + return r; + if (r > 0) + changed = true; } - fd = -1; + } - FOREACH_DIRENT_ALL(de, d, r = -errno; goto finish) { - struct stat fst; - - if (dot_or_dot_dot(de->d_name)) - continue; - - if (fstatat(dirfd(d), de->d_name, &fst, AT_SYMLINK_NOFOLLOW) < 0) { - r = -errno; - goto finish; - } - - if (S_ISDIR(fst.st_mode)) { - int subdir_fd; - - subdir_fd = openat(dirfd(d), de->d_name, O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC|O_NOFOLLOW|O_NOATIME); - if (subdir_fd < 0) { - r = -errno; - goto finish; - } - - r = chown_recursive_internal(subdir_fd, &fst, uid, gid); - if (r < 0) - goto finish; - if (r > 0) - changed = true; - } else { - r = chown_one(dirfd(d), de->d_name, &fst, uid, gid); - if (r < 0) - goto finish; - if (r > 0) - changed = true; - } - } - - r = chown_one(dirfd(d), NULL, st, uid, gid); - } else - r = chown_one(fd, NULL, st, uid, gid); + r = chown_one(dirfd(d), st, uid, gid); if (r < 0) - goto finish; + return r; - r = r > 0 || changed; - -finish: - safe_close(fd); - return r; + return r > 0 || changed; } int path_chown_recursive(const char *path, uid_t uid, gid_t gid) { @@ -111,7 +105,7 @@ int path_chown_recursive(const char *path, uid_t uid, gid_t gid) { struct stat st; int r; - fd = open(path, O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC|O_NOFOLLOW|O_NOATIME); + fd = open(path, O_RDONLY|O_DIRECTORY|O_CLOEXEC|O_NOFOLLOW|O_NOATIME); if (fd < 0) return -errno; From f89bc84f3242449cbc308892c87573b131f121df Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 19 Oct 2018 11:28:40 +0200 Subject: [PATCH 2/4] chown-recursive: also drop ACLs when recursively chown()ing Let's better be safe than sorry and also drop ACLs. --- src/core/chown-recursive.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/core/chown-recursive.c b/src/core/chown-recursive.c index 27c64489b55..447b7712676 100644 --- a/src/core/chown-recursive.c +++ b/src/core/chown-recursive.c @@ -3,6 +3,7 @@ #include #include #include +#include #include "chown-recursive.h" #include "dirent-util.h" @@ -14,6 +15,7 @@ static int chown_one(int fd, const struct stat *st, uid_t uid, gid_t gid) { char procfs_path[STRLEN("/proc/self/fd/") + DECIMAL_STR_MAX(int) + 1]; + const char *n; assert(fd >= 0); assert(st); @@ -26,13 +28,19 @@ static int chown_one(int fd, const struct stat *st, uid_t uid, gid_t gid) { * O_PATH. (Note: fchown() and fchmod() do not work with O_PATH, the kernel refuses that. */ xsprintf(procfs_path, "/proc/self/fd/%i", fd); + /* Drop any ACL if there is one */ + FOREACH_STRING(n, "system.posix_acl_access", "system.posix_acl_default") + if (removexattr(procfs_path, n) < 0) + 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(). 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. */ + /* 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) < 0) return -errno; From cd6b7d50c337b3676a3d5fc2188ff298dcbdb939 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 19 Oct 2018 11:42:11 +0200 Subject: [PATCH 3/4] chown-recursive: TAKE_FD() is your friend --- src/core/chown-recursive.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/core/chown-recursive.c b/src/core/chown-recursive.c index 447b7712676..7767301f7d9 100644 --- a/src/core/chown-recursive.c +++ b/src/core/chown-recursive.c @@ -111,7 +111,6 @@ static int chown_recursive_internal(int fd, const struct stat *st, uid_t uid, gi int path_chown_recursive(const char *path, uid_t uid, gid_t gid) { _cleanup_close_ int fd = -1; struct stat st; - int r; fd = open(path, O_RDONLY|O_DIRECTORY|O_CLOEXEC|O_NOFOLLOW|O_NOATIME); if (fd < 0) @@ -130,8 +129,5 @@ int path_chown_recursive(const char *path, uid_t uid, gid_t gid) { (!gid_is_valid(gid) || st.st_gid == gid)) return 0; - r = chown_recursive_internal(fd, &st, uid, gid); - fd = -1; /* we donated the fd to the call, regardless if it succeeded or failed */ - - return r; + return chown_recursive_internal(TAKE_FD(fd), &st, uid, gid); /* we donate the fd to the call, regardless if it succeeded or failed */ } From cb9e44db36caefcbb8ee7a12e14217305ed69ff2 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 19 Oct 2018 11:31:37 +0200 Subject: [PATCH 4/4] test: add test case for recursive chown()ing --- src/test/meson.build | 5 ++ src/test/test-chown-rec.c | 160 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 165 insertions(+) create mode 100644 src/test/test-chown-rec.c diff --git a/src/test/meson.build b/src/test/meson.build index cf060c3bd1b..18a67a10cb2 100644 --- a/src/test/meson.build +++ b/src/test/meson.build @@ -68,6 +68,11 @@ tests += [ libshared], []], + [['src/test/test-chown-rec.c'], + [libcore, + libshared], + []], + [['src/test/test-job-type.c'], [libcore, libshared], diff --git a/src/test/test-chown-rec.c b/src/test/test-chown-rec.c new file mode 100644 index 00000000000..786b0521b2c --- /dev/null +++ b/src/test/test-chown-rec.c @@ -0,0 +1,160 @@ +/* SPDX-License-Identifier: LGPL-2.1+ */ + +#include + +#include "alloc-util.h" +#include "chown-recursive.h" +#include "fileio.h" +#include "log.h" +#include "rm-rf.h" +#include "string-util.h" +#include "tests.h" + +static const uint8_t acl[] = { + 0x02, 0x00, 0x00, 0x00, 0x01, 0x00, 0x07, 0x00, + 0xff, 0xff, 0xff, 0xff, 0x02, 0x00, 0x07, 0x00, + 0x02, 0x00, 0x00, 0x00, 0x04, 0x00, 0x07, 0x00, + 0xff, 0xff, 0xff, 0xff, 0x10, 0x00, 0x07, 0x00, + 0xff, 0xff, 0xff, 0xff, 0x20, 0x00, 0x05, 0x00, + 0xff, 0xff, 0xff, 0xff, +}; + +static const uint8_t default_acl[] = { + 0x02, 0x00, 0x00, 0x00, 0x01, 0x00, 0x07, 0x00, + 0xff, 0xff, 0xff, 0xff, 0x04, 0x00, 0x07, 0x00, + 0xff, 0xff, 0xff, 0xff, 0x08, 0x00, 0x07, 0x00, + 0x04, 0x00, 0x00, 0x00, 0x10, 0x00, 0x07, 0x00, + 0xff, 0xff, 0xff, 0xff, 0x20, 0x00, 0x05, 0x00, + 0xff, 0xff, 0xff, 0xff, +}; + +static bool has_xattr(const char *p) { + char buffer[sizeof(acl) * 4]; + + if (lgetxattr(p, "system.posix_acl_access", buffer, sizeof(buffer)) < 0) { + if (IN_SET(errno, EOPNOTSUPP, ENOTTY, ENODATA, ENOSYS)) + return false; + } + + return true; +} + +static void test_chown_recursive(void) { + _cleanup_(rm_rf_physical_and_freep) char *t = NULL; + struct stat st; + const char *p; + + umask(022); + assert_se(mkdtemp_malloc(NULL, &t) >= 0); + + p = strjoina(t, "/dir"); + assert_se(mkdir(p, 0777) >= 0); + assert_se(lstat(p, &st) >= 0); + assert_se(S_ISDIR(st.st_mode)); + assert_se((st.st_mode & 07777) == 0755); + assert_se(st.st_uid == 0); + assert_se(st.st_gid == 0); + assert_se(!has_xattr(p)); + + p = strjoina(t, "/dir/symlink"); + assert_se(symlink("../../", p) >= 0); + assert_se(lstat(p, &st) >= 0); + assert_se(S_ISLNK(st.st_mode)); + assert_se((st.st_mode & 07777) == 0777); + assert_se(st.st_uid == 0); + assert_se(st.st_gid == 0); + assert_se(!has_xattr(p)); + + p = strjoina(t, "/dir/reg"); + assert_se(mknod(p, S_IFREG|0777, 0) >= 0); + assert_se(lstat(p, &st) >= 0); + assert_se(S_ISREG(st.st_mode)); + assert_se((st.st_mode & 07777) == 0755); + assert_se(st.st_uid == 0); + assert_se(st.st_gid == 0); + assert_se(!has_xattr(p)); + + p = strjoina(t, "/dir/sock"); + assert_se(mknod(p, S_IFSOCK|0777, 0) >= 0); + assert_se(lstat(p, &st) >= 0); + assert_se(S_ISSOCK(st.st_mode)); + assert_se((st.st_mode & 07777) == 0755); + assert_se(st.st_uid == 0); + assert_se(st.st_gid == 0); + assert_se(!has_xattr(p)); + + p = strjoina(t, "/dir/fifo"); + assert_se(mknod(p, S_IFIFO|0777, 0) >= 0); + assert_se(lstat(p, &st) >= 0); + assert_se(S_ISFIFO(st.st_mode)); + assert_se((st.st_mode & 07777) == 0755); + assert_se(st.st_uid == 0); + assert_se(st.st_gid == 0); + assert_se(!has_xattr(p)); + + /* We now apply an xattr to the dir, and check it again */ + p = strjoina(t, "/dir"); + assert_se(setxattr(p, "system.posix_acl_access", acl, sizeof(acl), 0) >= 0); + assert_se(setxattr(p, "system.posix_acl_default", default_acl, sizeof(default_acl), 0) >= 0); + assert_se(lstat(p, &st) >= 0); + assert_se(S_ISDIR(st.st_mode)); + assert_se((st.st_mode & 07777) == 0775); /* acl change changed the mode too */ + assert_se(st.st_uid == 0); + assert_se(st.st_gid == 0); + assert_se(has_xattr(p)); + + assert_se(path_chown_recursive(t, 1, 2) >= 0); + + p = strjoina(t, "/dir"); + assert_se(lstat(p, &st) >= 0); + assert_se(S_ISDIR(st.st_mode)); + assert_se((st.st_mode & 07777) == 0775); + assert_se(st.st_uid == 1); + assert_se(st.st_gid == 2); + assert_se(!has_xattr(p)); + + p = strjoina(t, "/dir/symlink"); + assert_se(lstat(p, &st) >= 0); + assert_se(S_ISLNK(st.st_mode)); + assert_se((st.st_mode & 07777) == 0777); + assert_se(st.st_uid == 1); + assert_se(st.st_gid == 2); + assert_se(!has_xattr(p)); + + p = strjoina(t, "/dir/reg"); + assert_se(lstat(p, &st) >= 0); + assert_se(S_ISREG(st.st_mode)); + assert_se((st.st_mode & 07777) == 0755); + assert_se(st.st_uid == 1); + assert_se(st.st_gid == 2); + assert_se(!has_xattr(p)); + + p = strjoina(t, "/dir/sock"); + assert_se(lstat(p, &st) >= 0); + assert_se(S_ISSOCK(st.st_mode)); + assert_se((st.st_mode & 07777) == 0755); + assert_se(st.st_uid == 1); + assert_se(st.st_gid == 2); + assert_se(!has_xattr(p)); + + p = strjoina(t, "/dir/fifo"); + assert_se(lstat(p, &st) >= 0); + assert_se(S_ISFIFO(st.st_mode)); + assert_se((st.st_mode & 07777) == 0755); + assert_se(st.st_uid == 1); + assert_se(st.st_gid == 2); + assert_se(!has_xattr(p)); +} + +int main(int argc, char *argv[]) { + log_set_max_level(LOG_DEBUG); + log_parse_environment(); + log_open(); + + if (geteuid() != 0) + return log_tests_skipped("not running as root"); + + test_chown_recursive(); + + return EXIT_SUCCESS; +}