From 908665f4e93126bdd8f9ba08935394de26e7966f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 29 Apr 2019 16:09:36 +0200 Subject: [PATCH 1/4] test-functions: follow fedora dbus-daemon/dbus-broker rearrangements Apparently dbus.service is now a symlink to either dbus-broker.service or dbus-daemon.service. Copy in the right target service in each case. --- test/test-functions | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/test/test-functions b/test/test-functions index 50c6e96902..206b0270ea 100644 --- a/test/test-functions +++ b/test/test-functions @@ -684,17 +684,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 \ From 8b364a38238bf828ae6df93fdbca7897bd8f042c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 29 Apr 2019 13:21:40 +0200 Subject: [PATCH 2/4] tmpfiles: split out ~ mode handling into a helper function No change of behaviour, just some minor refactoring. --- src/tmpfiles/tmpfiles.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index d9d1cc1c1a..e157714662 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -776,6 +776,20 @@ 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; @@ -797,22 +811,16 @@ 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) { if (S_ISLNK(st->st_mode)) log_debug("Skipping 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 */ - } + if (i->mask_perms) + m = process_mask_perms(m, st->st_mode); if (m == (st->st_mode & 07777)) log_debug("\"%s\" has correct mode %o already.", path, st->st_mode); From a9f55d0e7ed8b718641605f91553e4f150390dbd Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 29 Apr 2019 16:11:52 +0200 Subject: [PATCH 3/4] tmpfiles: be more careful when adjusting chmod() + chown() chown() might drop the suid/sgid bit from files. hence let's chmod() after chown(). But also, let's tighten the transition a bit: before issuing chown() let's set the file mask to the minimum of the old and new access bitmask, so that at no point in time additional privs are available on the file with a non-matching ownership. Fixes: #12354 --- src/tmpfiles/tmpfiles.c | 52 ++++++++++++++++++++++++++++++++--------- 1 file changed, 41 insertions(+), 11 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index e157714662..a5e08d740f 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -792,6 +792,8 @@ static mode_t process_mask_perms(mode_t mode, mode_t current) { 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); @@ -811,29 +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); + /* 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 (i->mode_set) { 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; + mode_t m = new_mode & st->st_mode; /* Mask new mode by old mode */ - if (i->mask_perms) - m = process_mask_perms(m, st->st_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, @@ -847,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); } From 34015aea1703c7240e5d0188a98aed4e8aab5b3b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 29 Apr 2019 15:44:45 +0200 Subject: [PATCH 4/4] tests: add test for suid/sgid handling in tmpfiles --- test/TEST-22-TMPFILES/test-09.sh | 59 ++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100755 test/TEST-22-TMPFILES/test-09.sh diff --git a/test/TEST-22-TMPFILES/test-09.sh b/test/TEST-22-TMPFILES/test-09.sh new file mode 100755 index 0000000000..b69abda644 --- /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 - <