systemd: chown_one() can dereference symlinks

@@ -462,44 +462,44 @@ CHANGES WITH 240:
         Ellsel, Colin Guthrie, dana, Daniel, Daniele Medri, Daniel Kahn
         Gillmor, Daniel Rusek, Daniel van Vugt, Dariusz Gadomski, Dave Reisner,
         David Anderson, Davide Cavalca, David Leeds, David Malcolm, David
-        Strauss, David Tardon, Dimitri John Ledkov, dj-kaktus, Dongsu Park,
-        Elias Probst, Emil Soleyman, Erik Kooistra, Ervin Peters, Evgeni Golov,
-        Evgeny Vereshchagin, Fabrice Fontaine, Faheel Ahmad, Faizal Luthfi,
-        Felix Yan, Filipe Brandenburger, Franck Bui, Frank Schaefer, Frantisek
-        Sumsal, Gautier Husson, Gianluca Boiano, Giuseppe Scrivano, glitsj16,
-        Hans de Goede, Harald Hoyer, Harry Mallon, Harshit Jain, Helmut Grohne,
-        Henry Tung, Hui Yiqun, imayoda, Insun Pyo, Iwan Timmer, Jan Janssen,
-        Jan Pokorný, Jan Synacek, Jason A. Donenfeld, javitoom, Jérémy Nouhaud,
-        Jeremy Su, Jiuyang Liu, João Paulo Rechi Vita, Joe Hershberger, Joe
-        Rayhawk, Joerg Behrmann, Joerg Steffens, Jonas Dorel, Jon Ringle, Josh
-        Soref, Julian Andres Klode, Jun Bo Bi, Jürg Billeter, Keith Busch, Khem
-        Raj, Kirill Marinushkin, Larry Bernstone, Lennart Poettering, Lion
-        Yang, Li Song, Lorenz Hübschle-Schneider, Lubomir Rintel, Lucas
-        Werkmeister, Ludwin Janvier, Lukáš Nykrýn, Luke Shumaker, mal,
-        Marc-Antoine Perennou, Marcin Skarbek, Marco Trevisan (Treviño), Marian
-        Cepok, Mario Hros, Marko Myllynen, Markus Grimm, Martin Pitt, Martin
-        Sobotka, Martin Wilck, Mathieu Trudel-Lapierre, Matthew Leeds, Michael
-        Biebl, Michael Olbrich, Michael 'pbone' Pobega, Michael Scherer, Michal
-        Koutný, Michal Sekletar, Michal Soltys, Mike Gilbert, Mike Palmer,
-        Muhammet Kara, Neal Gompa, Neil Brown, Network Silence, Niklas
-        Tibbling, Nikolas Nyby, Nogisaka Sadata, Oliver Smith, Patrik Flykt,
-        Pavel Hrdina, Paweł Szewczyk, Peter Hutterer, Piotr Drąg, Ray Strode,
-        Reinhold Mueller, Renaud Métrich, Roman Gushchin, Ronny Chevalier,
-        Rubén Suárez Alvarez, Ruixin Bao, RussianNeuroMancer, Ryutaroh
-        Matsumoto, Saleem Rashid, Sam Morris, Samuel Morris, Sandy Carter,
-        scootergrisen, Sébastien Bacher, Sergey Ptashnick, Shawn Landden,
-        Shengyao Xue, Shih-Yuan Lee (FourDollars), Silvio Knizek, Sjoerd
-        Simons, Stasiek Michalski, Stephen Gallagher, Steven Allen, Steve
-        Ramage, Susant Sahani, Sven Joachim, Sylvain Plantefève, Tanu Kaskinen,
-        Tejun Heo, Thiago Macieira, Thomas Blume, Thomas Haller, Thomas
-        H. P. Andersen, Tim Ruffing, TJ, Tobias Jungel, Todd Walton, Tommi
-        Rantala, Tomsod M, Tony Novak, Tore Anderson, Trevonn, Victor
-        Laskurain, Victor Tapia, Violet Halo, Vojtech Trefny, welaq, William
-        A. Kennington III, William Douglas, Wyatt Ward, Xiang Fan, Xi Ruoyao,
-        Xuanwo, Yann E. Morin, YmrDtnJu, Yu Watanabe, Zbigniew
-        Jędrzejewski-Szmek, Zhang Xianwei, Zsolt Dollenstein
-
-        — Hamburg, 2018-12-21
+        Strauss, David Tardon, Dimitri John Ledkov, Dmitry Torokhov, dj-kaktus,
+        Dongsu Park, Elias Probst, Emil Soleyman, Erik Kooistra, Ervin Peters,
+        Evgeni Golov, Evgeny Vereshchagin, Fabrice Fontaine, Faheel Ahmad,
+        Faizal Luthfi, Felix Yan, Filipe Brandenburger, Franck Bui, Frank
+        Schaefer, Frantisek Sumsal, Gautier Husson, Gianluca Boiano, Giuseppe
+        Scrivano, glitsj16, Hans de Goede, Harald Hoyer, Harry Mallon, Harshit
+        Jain, Helmut Grohne, Henry Tung, Hui Yiqun, imayoda, Insun Pyo, Iwan
+        Timmer, Jan Janssen, Jan Pokorný, Jan Synacek, Jason A. Donenfeld,
+        javitoom, Jérémy Nouhaud, Jeremy Su, Jiuyang Liu, João Paulo Rechi
+        Vita, Joe Hershberger, Joe Rayhawk, Joerg Behrmann, Joerg Steffens,
+        Jonas Dorel, Jon Ringle, Josh Soref, Julian Andres Klode, Jun Bo Bi,
+        Jürg Billeter, Keith Busch, Khem Raj, Kirill Marinushkin, Larry
+        Bernstone, Lennart Poettering, Lion Yang, Li Song, Lorenz
+        Hübschle-Schneider, Lubomir Rintel, Lucas Werkmeister, Ludwin Janvier,
+        Lukáš Nykrýn, Luke Shumaker, mal, Marc-Antoine Perennou, Marcin
+        Skarbek, Marco Trevisan (Treviño), Marian Cepok, Mario Hros, Marko
+        Myllynen, Markus Grimm, Martin Pitt, Martin Sobotka, Martin Wilck,
+        Mathieu Trudel-Lapierre, Matthew Leeds, Michael Biebl, Michael Olbrich,
+        Michael 'pbone' Pobega, Michael Scherer, Michal Koutný, Michal
+        Sekletar, Michal Soltys, Mike Gilbert, Mike Palmer, Muhammet Kara, Neal
+        Gompa, Neil Brown, Network Silence, Niklas Tibbling, Nikolas Nyby,
+        Nogisaka Sadata, Oliver Smith, Patrik Flykt, Pavel Hrdina, Paweł
+        Szewczyk, Peter Hutterer, Piotr Drąg, Ray Strode, Reinhold Mueller,
+        Renaud Métrich, Roman Gushchin, Ronny Chevalier, Rubén Suárez Alvarez,
+        Ruixin Bao, RussianNeuroMancer, Ryutaroh Matsumoto, Saleem Rashid, Sam
+        Morris, Samuel Morris, Sandy Carter, scootergrisen, Sébastien Bacher,
+        Sergey Ptashnick, Shawn Landden, Shengyao Xue, Shih-Yuan Lee
+        (FourDollars), Silvio Knizek, Sjoerd Simons, Stasiek Michalski, Stephen
+        Gallagher, Steven Allen, Steve Ramage, Susant Sahani, Sven Joachim,
+        Sylvain Plantefève, Tanu Kaskinen, Tejun Heo, Thiago Macieira, Thomas
+        Blume, Thomas Haller, Thomas H. P. Andersen, Tim Ruffing, TJ, Tobias
+        Jungel, Todd Walton, Tommi Rantala, Tomsod M, Tony Novak, Tore
+        Anderson, Trevonn, Victor Laskurain, Victor Tapia, Violet Halo, Vojtech
+        Trefny, welaq, William A. Kennington III, William Douglas, Wyatt Ward,
+        Xiang Fan, Xi Ruoyao, Xuanwo, Yann E. Morin, YmrDtnJu, Yu Watanabe,
+        Zbigniew Jędrzejewski-Szmek, Zhang Xianwei, Zsolt Dollenstein
+
+        — Warsaw, 2018-12-21
 
 CHANGES WITH 239:
 

@@ -1,17 +1,21 @@
 /* SPDX-License-Identifier: LGPL-2.1+ */
 
-#include <sys/types.h>
-#include <sys/stat.h>
 #include <fcntl.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <sys/xattr.h>
 
-#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];
+        const char *n;
 
         assert(fd >= 0);
         assert(st);
@@ -20,98 +24,95 @@ 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)
-                return -errno;
+        /* 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);
 
-        /* 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)
+        /* 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(), 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;
+
         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) {
-                        r = -errno;
-                        goto finish;
-                }
-                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;
-                        }
+        d = fdopendir(fd);
+        if (!d) {
+                safe_close(fd);
+                return -errno;
+        }
+
+        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;
                 }
+        }
 
-                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) {
         _cleanup_close_ int fd = -1;
         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;
 
@@ -128,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 */
 }

@@ -68,6 +68,11 @@ tests += [
           libshared],
          []],
 
+        [['src/test/test-chown-rec.c'],
+         [libcore,
+          libshared],
+         []],
+
         [['src/test/test-job-type.c'],
          [libcore,
           libshared],

@@ -0,0 +1,160 @@
+/* SPDX-License-Identifier: LGPL-2.1+ */
+
+#include <sys/xattr.h>
+
+#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;
+}