From 607b358ef2df56cb3451e68b1489c5788882dfd3 Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Mon, 25 Mar 2019 16:21:11 +0100
Subject: [PATCH] core: drop suid/sgid bit of files/dirs when doing recursive
 chown

This adds some extra paranoia: when we recursively chown a directory for
use with DynamicUser=1 services we'll now drop suid/sgid from all files
we chown().

Of course, such files should not exist in the first place, and noone
should get access to those dirs who isn't root anyway, but let's better
be safe than sorry, and drop everything we come across.
---
 src/core/chown-recursive.c | 33 +++++++++++++++++++++++++--------
 src/core/chown-recursive.h |  2 +-
 src/core/execute.c         |  6 ++++--
 src/test/test-chown-rec.c  |  2 +-
 4 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/src/core/chown-recursive.c b/src/core/chown-recursive.c
index 7767301f7d9..49d96b73760 100644
--- a/src/core/chown-recursive.c
+++ b/src/core/chown-recursive.c
@@ -13,7 +13,13 @@
 #include "strv.h"
 #include "user-util.h"
 
-static int chown_one(int fd, const struct stat *st, uid_t uid, gid_t gid) {
+static int chown_one(
+                int fd,
+                const struct stat *st,
+                uid_t uid,
+                gid_t gid,
+                mode_t mask) {
+
         char procfs_path[STRLEN("/proc/self/fd/") + DECIMAL_STR_MAX(int) + 1];
         const char *n;
 
@@ -42,13 +48,19 @@ static int chown_one(int fd, const struct stat *st, uid_t uid, gid_t gid) {
          * 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)
+                if (chmod(procfs_path, st->st_mode & 07777 & mask) < 0)
                         return -errno;
 
         return 1;
 }
 
-static int chown_recursive_internal(int fd, const struct stat *st, uid_t uid, gid_t gid) {
+static int chown_recursive_internal(
+                int fd,
+                const struct stat *st,
+                uid_t uid,
+                gid_t gid,
+                mode_t mask) {
+
         _cleanup_closedir_ DIR *d = NULL;
         bool changed = false;
         struct dirent *de;
@@ -87,13 +99,13 @@ static int chown_recursive_internal(int fd, const struct stat *st, uid_t uid, gi
                         if (subdir_fd < 0)
                                 return subdir_fd;
 
-                        r = chown_recursive_internal(subdir_fd, &fst, uid, gid); /* takes possession of subdir_fd even on failure */
+                        r = chown_recursive_internal(subdir_fd, &fst, uid, gid, mask); /* 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);
+                        r = chown_one(path_fd, &fst, uid, gid, mask);
                         if (r < 0)
                                 return r;
                         if (r > 0)
@@ -101,14 +113,19 @@ static int chown_recursive_internal(int fd, const struct stat *st, uid_t uid, gi
                 }
         }
 
-        r = chown_one(dirfd(d), st, uid, gid);
+        r = chown_one(dirfd(d), st, uid, gid, mask);
         if (r < 0)
                 return r;
 
         return r > 0 || changed;
 }
 
-int path_chown_recursive(const char *path, uid_t uid, gid_t gid) {
+int path_chown_recursive(
+                const char *path,
+                uid_t uid,
+                gid_t gid,
+                mode_t mask) {
+
         _cleanup_close_ int fd = -1;
         struct stat st;
 
@@ -129,5 +146,5 @@ int path_chown_recursive(const char *path, uid_t uid, gid_t gid) {
             (!gid_is_valid(gid) || st.st_gid == gid))
                 return 0;
 
-        return chown_recursive_internal(TAKE_FD(fd), &st, uid, gid); /* we donate the fd to the call, regardless if it succeeded or failed */
+        return chown_recursive_internal(TAKE_FD(fd), &st, uid, gid, mask); /* we donate the fd to the call, regardless if it succeeded or failed */
 }
diff --git a/src/core/chown-recursive.h b/src/core/chown-recursive.h
index f3fa40a6563..bfee05f3be5 100644
--- a/src/core/chown-recursive.h
+++ b/src/core/chown-recursive.h
@@ -3,4 +3,4 @@
 
 #include <sys/types.h>
 
-int path_chown_recursive(const char *path, uid_t uid, gid_t gid);
+int path_chown_recursive(const char *path, uid_t uid, gid_t gid, mode_t mask);
diff --git a/src/core/execute.c b/src/core/execute.c
index dabb6d824fb..6698f59a468 100644
--- a/src/core/execute.c
+++ b/src/core/execute.c
@@ -2194,8 +2194,10 @@ static int setup_exec_directory(
                 if (r < 0)
                         goto fail;
 
-                /* Then, change the ownership of the whole tree, if necessary */
-                r = path_chown_recursive(pp ?: p, uid, gid);
+                /* Then, change the ownership of the whole tree, if necessary. When dynamic users are used we
+                 * drop the suid/sgid bits, since we really don't want SUID/SGID files for dynamic UID/GID
+                 * assignments to exist.*/
+                r = path_chown_recursive(pp ?: p, uid, gid, context->dynamic_user ? 01777 : 07777);
                 if (r < 0)
                         goto fail;
         }
diff --git a/src/test/test-chown-rec.c b/src/test/test-chown-rec.c
index 7e93e487d2a..aa11bd270ff 100644
--- a/src/test/test-chown-rec.c
+++ b/src/test/test-chown-rec.c
@@ -106,7 +106,7 @@ static void test_chown_recursive(void) {
         assert_se(st.st_gid == gid);
         assert_se(has_xattr(p));
 
-        assert_se(path_chown_recursive(t, 1, 2) >= 0);
+        assert_se(path_chown_recursive(t, 1, 2, 07777) >= 0);
 
         p = strjoina(t, "/dir");
         assert_se(lstat(p, &st) >= 0);