Merge pull request #12431 from poettering/tmpfiles-chmod-chown-order

tmpfiles: run chown() before chmod()

diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c
index 73e0e33d0a8f3709a6f5c2dfd948ffd45cf8c730..eabc51101d992447a0cf1658a5606cab8259a3fd 100644 (file)
--- a/src/tmpfiles/tmpfiles.c
+++ b/src/tmpfiles/tmpfiles.c
@@ -776,8 +776,24 @@ static bool hardlink_vulnerable(const struct stat *st) {
         return !S_ISDIR(st->st_mode) && st->st_nlink > 1 && dangerous_hardlinks();
 }
 
+static mode_t process_mask_perms(mode_t mode, mode_t current) {
+
+        if ((current & 0111) == 0)
+                mode &= ~0111;
+        if ((current & 0222) == 0)
+                mode &= ~0222;
+        if ((current & 0444) == 0)
+                mode &= ~0444;
+        if (!S_ISDIR(current))
+                mode &= ~07000; /* remove sticky/sgid/suid bit, unless directory */
+
+        return mode;
+}
+
 static int fd_set_perms(Item *i, int fd, const char *path, const struct stat *st) {
         struct stat stbuf;
+        mode_t new_mode;
+        bool do_chown;
 
         assert(i);
         assert(fd);
@@ -797,35 +813,39 @@ static int fd_set_perms(Item *i, int fd, const char *path, const struct stat *st
                                        "Refusing to set permissions on hardlinked file %s while the fs.protected_hardlinks sysctl is turned off.",
                                        path);
 
-        if (i->mode_set) {
+        /* Do we need a chown()? */
+        do_chown =
+                (i->uid_set && i->uid != st->st_uid) ||
+                (i->gid_set && i->gid != st->st_gid);
+
+        /* Calculate the mode to apply */
+        new_mode = i->mode_set ? (i->mask_perms ?
+                                  process_mask_perms(i->mode, st->st_mode) :
+                                  i->mode) :
+                                 (st->st_mode & 07777);
+
+        if (i->mode_set && do_chown) {
+                /* Before we issue the chmod() let's reduce the access mode to the common bits of the old and
+                 * the new mode. That way there's no time window where the file exists under the old owner
+                 * with more than the old access modes — and not under the new owner with more than the new
+                 * access modes either. */
+
                 if (S_ISLNK(st->st_mode))
-                        log_debug("Skipping mode fix for symlink %s.", path);
+                        log_debug("Skipping temporary mode fix for symlink %s.", path);
                 else {
-                        mode_t m = i->mode;
-
-                        if (i->mask_perms) {
-                                if (!(st->st_mode & 0111))
-                                        m &= ~0111;
-                                if (!(st->st_mode & 0222))
-                                        m &= ~0222;
-                                if (!(st->st_mode & 0444))
-                                        m &= ~0444;
-                                if (!S_ISDIR(st->st_mode))
-                                        m &= ~07000; /* remove sticky/sgid/suid bit, unless directory */
-                        }
+                        mode_t m = new_mode & st->st_mode; /* Mask new mode by old mode */
 
-                        if (m == (st->st_mode & 07777))
-                                log_debug("\"%s\" has correct mode %o already.", path, st->st_mode);
+                        if (((m ^ st->st_mode) & 07777) == 0)
+                                log_debug("\"%s\" matches temporary mode %o already.", path, m);
                         else {
-                                log_debug("Changing \"%s\" to mode %o.", path, m);
+                                log_debug("Temporarily changing \"%s\" to mode %o.", path, m);
                                 if (fchmod_opath(fd, m) < 0)
                                         return log_error_errno(errno, "fchmod() of %s failed: %m", path);
                         }
                 }
         }
 
-        if ((i->uid_set && i->uid != st->st_uid) ||
-            (i->gid_set && i->gid != st->st_gid)) {
+        if (do_chown) {
                 log_debug("Changing \"%s\" to owner "UID_FMT":"GID_FMT,
                           path,
                           i->uid_set ? i->uid : UID_INVALID,
@@ -839,6 +859,24 @@ static int fd_set_perms(Item *i, int fd, const char *path, const struct stat *st
                         return log_error_errno(errno, "fchownat() of %s failed: %m", path);
         }
 
+        /* Now, apply the final mode. We do this in two cases: when the user set a mode explicitly, or after a
+         * chown(), since chown()'s mangle the access mode in regards to sgid/suid in some conditions. */
+        if (i->mode_set || do_chown) {
+                if (S_ISLNK(st->st_mode))
+                        log_debug("Skipping mode fix for symlink %s.", path);
+                else {
+                       /* Check if the chmod() is unnecessary. Note that if we did a chown() before we always
+                        * chmod() here again, since it might have mangled the bits. */
+                        if (!do_chown && ((new_mode ^ st->st_mode) & 07777) == 0)
+                                log_debug("\"%s\" matches mode %o already.", path, new_mode);
+                        else {
+                                log_debug("Changing \"%s\" to mode %o.", path, new_mode);
+                                if (fchmod_opath(fd, new_mode) < 0)
+                                        return log_error_errno(errno, "fchmod() of %s failed: %m", path);
+                        }
+                }
+        }
+
 shortcut:
         return label_fix(path, 0);
 }

diff --git a/test/TEST-22-TMPFILES/test-09.sh b/test/TEST-22-TMPFILES/test-09.sh
new file mode 100755 (executable)
index 0000000..b69abda
--- /dev/null
+++ b/test/TEST-22-TMPFILES/test-09.sh
@@ -0,0 +1,59 @@
+#!/bin/bash
+
+set -e
+set -x
+
+# Make sure that the "stat" output is not locale dependent.
+export LANG=C LC_ALL=C
+
+# first, create file without suid/sgid
+systemd-tmpfiles --create - <<EOF
+f     /tmp/xxx    0755 1 1 - -
+f     /tmp/yyy    0755 1 1 - -
+EOF
+
+test "$(stat -c %F:%u:%g:%a /tmp/xxx)" = "regular empty file:1:1:755"
+test "$(stat -c %F:%u:%g:%a /tmp/yyy)" = "regular empty file:1:1:755"
+
+# then, add suid/sgid
+systemd-tmpfiles --create - <<EOF
+f     /tmp/xxx    04755
+f     /tmp/yyy    02755
+EOF
+
+test "$(stat -c %F:%u:%g:%a /tmp/xxx)" = "regular empty file:1:1:4755"
+test "$(stat -c %F:%u:%g:%a /tmp/yyy)" = "regular empty file:1:1:2755"
+
+# then, chown the files to somebody else
+systemd-tmpfiles --create - <<EOF
+f     /tmp/xxx    - 2 2
+f     /tmp/yyy    - 2 2
+EOF
+
+test "$(stat -c %F:%u:%g:%a /tmp/xxx)" = "regular empty file:2:2:4755"
+test "$(stat -c %F:%u:%g:%a /tmp/yyy)" = "regular empty file:2:2:2755"
+
+# then, chown the files to a third user/group but also drop to a mask that has
+# both more and fewer bits set
+systemd-tmpfiles --create - <<EOF
+f     /tmp/xxx    0770 3 3
+f     /tmp/yyy    0770 3 3
+EOF
+
+test "$(stat -c %F:%u:%g:%a /tmp/xxx)" = "regular empty file:3:3:770"
+test "$(stat -c %F:%u:%g:%a /tmp/yyy)" = "regular empty file:3:3:770"
+
+# return to the beginning
+systemd-tmpfiles --create - <<EOF
+f     /tmp/xxx    0755 1 1 - -
+f     /tmp/yyy    0755 1 1 - -
+EOF
+
+test "$(stat -c %F:%u:%g:%a /tmp/xxx)" = "regular empty file:1:1:755"
+test "$(stat -c %F:%u:%g:%a /tmp/yyy)" = "regular empty file:1:1:755"
+
+# remove everything
+systemd-tmpfiles --remove - <<EOF
+r /tmp/xxx
+r /tmp/yyy
+EOF

diff --git a/test/test-functions b/test/test-functions
index 0394076f0aafc4350d950efa15687324e36e610d..47c5b2cb170477715884ac9ac439204d8211fc5a 100644 (file)
--- a/test/test-functions
+++ b/test/test-functions
@@ -706,17 +706,20 @@ install_libnss() {
 install_dbus() {
     inst $ROOTLIBDIR/system/dbus.socket
 
-    # Fedora rawhide replaced dbus.service with dbus-daemon.service
-    if [ -f $ROOTLIBDIR/system/dbus-daemon.service ]; then
+    # Newer Fedora versions use dbus-broker by default. Let's install it is available.
+    if [ -f $ROOTLIBDIR/system/dbus-broker.service ]; then
+        inst $ROOTLIBDIR/system/dbus-broker.service
+        inst_symlink /etc/systemd/system/dbus.service
+        inst /usr/bin/dbus-broker
+        inst /usr/bin/dbus-broker-launch
+    elif [ -f $ROOTLIBDIR/system/dbus-daemon.service ]; then
+        # Fedora rawhide replaced dbus.service with dbus-daemon.service
         inst $ROOTLIBDIR/system/dbus-daemon.service
         # Alias symlink
         inst_symlink /etc/systemd/system/dbus.service
     else
         inst $ROOTLIBDIR/system/dbus.service
     fi
-    # Newer Fedora versions use dbus-broker by default. Let's install it is available.
-    [ -f /usr/bin/dbus-broker ] && inst /usr/bin/dbus-broker
-    [ -f /usr/bin/dbus-broker-launch ] && inst /usr/bin/dbus-broker-launch
 
     find \
         /etc/dbus-1 /usr/share/dbus-1 -xtype f \