Merge pull request #21320 from poettering/namespace-mkdir-umask

make pid1 namespace code independent of umask

7 files changed, 51 insertions, 33 deletions

diff --git a/src/basic/umask-util.h b/src/basic/umask-util.h
index bd7c2bdb8c..90d18f70ba 100644
--- a/src/basic/umask-util.h
+++ b/src/basic/umask-util.h
@@ -24,3 +24,6 @@ assert_cc((S_IFMT & 0777) == 0);
         for (_cleanup_umask_ mode_t _saved_umask_ = umask(mask) | S_IFMT; \
              FLAGS_SET(_saved_umask_, S_IFMT);                          \
              _saved_umask_ &= 0777)
+
+#define BLOCK_WITH_UMASK(mask) \
+        _unused_ _cleanup_umask_ mode_t _saved_umask_ = umask(mask);
diff --git a/src/core/namespace.c b/src/core/namespace.c
index 68704dff06..c01975b9de 100644
--- a/src/core/namespace.c
+++ b/src/core/namespace.c
@@ -806,8 +806,7 @@ static int clone_device_node(
                 *make_devnode = false;
         }
 
-        /* We're about to fall back to bind-mounting the device
-         * node. So create a dummy bind-mount target.
+        /* We're about to fall back to bind-mounting the device node. So create a dummy bind-mount target.
          * Do not prepare device-node SELinux label (see issue 13762) */
         r = mknod(dn, S_IFREG, 0);
         if (r < 0 && errno != EEXIST)
@@ -853,13 +852,10 @@ static int mount_private_dev(MountEntry *m) {
         char temporary_mount[] = "/tmp/namespace-dev-XXXXXX";
         const char *d, *dev = NULL, *devpts = NULL, *devshm = NULL, *devhugepages = NULL, *devmqueue = NULL, *devlog = NULL, *devptmx = NULL;
         bool can_mknod = true;
-        _unused_ _cleanup_umask_ mode_t u;
         int r;
 
         assert(m);
 
-        u = umask(0000);
-
         if (!mkdtemp(temporary_mount))
                 return log_debug_errno(errno, "Failed to create temporary directory '%s': %m", temporary_mount);
 
@@ -930,10 +926,8 @@ static int mount_private_dev(MountEntry *m) {
         if (r < 0)
                 log_debug_errno(r, "Failed to set up basic device tree at '%s', ignoring: %m", temporary_mount);
 
-        /* Create the /dev directory if missing. It is more likely to be
-         * missing when the service is started with RootDirectory. This is
-         * consistent with mount units creating the mount points when missing.
-         */
+        /* Create the /dev directory if missing. It is more likely to be missing when the service is started
+         * with RootDirectory. This is consistent with mount units creating the mount points when missing. */
         (void) mkdir_p_label(mount_entry_path(m), 0755);
 
         /* Unmount everything in old /dev */
@@ -975,8 +969,8 @@ static int mount_bind_dev(const MountEntry *m) {
 
         assert(m);
 
-        /* Implements the little brother of mount_private_dev(): simply bind mounts the host's /dev into the service's
-         * /dev. This is only used when RootDirectory= is set. */
+        /* Implements the little brother of mount_private_dev(): simply bind mounts the host's /dev into the
+         * service's /dev. This is only used when RootDirectory= is set. */
 
         (void) mkdir_p_label(mount_entry_path(m), 0755);
 
@@ -1085,7 +1079,8 @@ static int mount_tmpfs(const MountEntry *m) {
         entry_path = mount_entry_path(m);
         inner_path = mount_entry_unprefixed_path(m);
 
-        /* First, get rid of everything that is below if there is anything. Then, overmount with our new tmpfs */
+        /* First, get rid of everything that is below if there is anything. Then, overmount with our new
+         * tmpfs */
 
         (void) mkdir_p_label(entry_path, 0755);
         (void) umount_recursive(entry_path, 0);
@@ -1900,6 +1895,10 @@ int setup_namespace(
 
         assert(ns_info);
 
+        /* Make sure that all mknod(), mkdir() calls we do are unaffected by the umask, and the access modes
+         * we configure take effect */
+        BLOCK_WITH_UMASK(0000);
+
         if (!isempty(propagate_dir) && !isempty(incoming_dir))
                 setup_propagate = true;
 
@@ -1972,11 +1971,11 @@ int setup_namespace(
                  * we create it if it doesn't already exist. */
                 (void) mkdir_p_label("/run/systemd", 0755);
 
-                /* Always create the mount namespace in a temporary directory, instead of operating
-                 * directly in the root. The temporary directory prevents any mounts from being
-                 * potentially obscured my other mounts we already applied.
-                 * We use the same mount point for all images, which is safe, since they all live
-                 * in their own namespaces after all, and hence won't see each other. */
+                /* Always create the mount namespace in a temporary directory, instead of operating directly
+                 * in the root. The temporary directory prevents any mounts from being potentially obscured
+                 * my other mounts we already applied.  We use the same mount point for all images, which is
+                 * safe, since they all live in their own namespaces after all, and hence won't see each
+                 * other. */
 
                 root = "/run/systemd/unit-root";
                 (void) mkdir_label(root, 0700);
@@ -2240,8 +2239,8 @@ int setup_namespace(
                 (void) mkdir_p(propagate_dir, 0600);
 
         if (n_extension_images > 0)
-                /* ExtensionImages mountpoint directories will be created
-                 * while parsing the mounts to create, so have the parent ready */
+                /* ExtensionImages mountpoint directories will be created while parsing the mounts to create,
+                 * so have the parent ready */
                 (void) mkdir_p(extension_dir, 0600);
 
         /* Remount / as SLAVE so that nothing now mounted in the namespace
@@ -2509,7 +2508,8 @@ static int make_tmp_prefix(const char *prefix) {
         if (errno != ENOENT)
                 return -errno;
 
-        r = mkdir_parents(prefix, 0755);
+        RUN_WITH_UMASK(000)
+                r = mkdir_parents(prefix, 0755);
         if (r < 0)
                 return r;
 
@@ -2517,7 +2517,8 @@ static int make_tmp_prefix(const char *prefix) {
         if (r < 0)
                 return r;
 
-        if (mkdir(t, 0777) < 0)
+        if (mkdir(t, 0777) < 0) /* umask will corrupt this access mode, but that doesn't matter, we need to
+                                 * call chmod() anyway for the suid bit, below. */
                 return -errno;
 
         if (chmod(t, 01777) < 0) {
@@ -2575,10 +2576,9 @@ static int setup_one_tmp_dir(const char *id, const char *prefix, char **path, ch
                 if (!y)
                         return -ENOMEM;
 
-                RUN_WITH_UMASK(0000) {
+                RUN_WITH_UMASK(0000)
                         if (mkdir(y, 0777 | S_ISVTX) < 0)
                                     return -errno;
-                }
 
                 r = label_fix_container(y, prefix, 0);
                 if (r < 0)
@@ -2590,7 +2590,8 @@ static int setup_one_tmp_dir(const char *id, const char *prefix, char **path, ch
                 /* Trouble: we failed to create the directory. Instead of failing, let's simulate /tmp being
                  * read-only. This way the service will get the EROFS result as if it was writing to the real
                  * file system. */
-                r = mkdir_p(RUN_SYSTEMD_EMPTY, 0500);
+                RUN_WITH_UMASK(0000)
+                        r = mkdir_p(RUN_SYSTEMD_EMPTY, 0500);
                 if (r < 0)
                         return r;
 
diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c
index 66daeb4f64..b85b8c3d43 100644
--- a/src/nspawn/nspawn.c
+++ b/src/nspawn/nspawn.c
@@ -2216,13 +2216,12 @@ static int copy_devnodes(const char *dest) {
                 "tty\0"
                 "net/tun\0";
 
-        _unused_ _cleanup_umask_ mode_t u;
         const char *d;
         int r = 0;
 
         assert(dest);
 
-        u = umask(0000);
+        BLOCK_WITH_UMASK(0000);
 
         /* Create /dev/net, so that we can create /dev/net/tun in it */
         if (userns_mkdir(dest, "/dev/net", 0755, 0, 0) < 0)
@@ -2299,11 +2298,10 @@ static int copy_devnodes(const char *dest) {
 }
 
 static int make_extra_nodes(const char *dest) {
-        _unused_ _cleanup_umask_ mode_t u;
         size_t i;
         int r;
 
-        u = umask(0000);
+        BLOCK_WITH_UMASK(0000);
 
         for (i = 0; i < arg_n_extra_nodes; i++) {
                 _cleanup_free_ char *path = NULL;
@@ -2500,12 +2498,11 @@ static int setup_kmsg(int kmsg_socket) {
         _cleanup_(unlink_and_freep) char *from = NULL;
         _cleanup_free_ char *fifo = NULL;
         _cleanup_close_ int fd = -1;
-        _unused_ _cleanup_umask_ mode_t u;
         int r;
 
         assert(kmsg_socket >= 0);
 
-        u = umask(0000);
+        BLOCK_WITH_UMASK(0000);
 
         /* We create the kmsg FIFO as as temporary file in /run, but immediately delete it after bind mounting it to
          * /proc/kmsg. While FIFOs on the reading side behave very similar to /proc/kmsg, their writing side behaves
diff --git a/src/shared/dev-setup.c b/src/shared/dev-setup.c
index c3e717ae11..0390abbfdc 100644
--- a/src/shared/dev-setup.c
+++ b/src/shared/dev-setup.c
@@ -81,13 +81,12 @@ int make_inaccessible_nodes(
                 { "inaccessible/blk",  S_IFBLK  | 0000 },
         };
 
-        _unused_ _cleanup_umask_ mode_t u;
         int r;
 
         if (!parent_dir)
                 parent_dir = "/run/systemd";
 
-        u = umask(0000);
+        BLOCK_WITH_UMASK(0000);
 
         /* Set up inaccessible (and empty) file nodes of all types. This are used to as mount sources for over-mounting
          * ("masking") file nodes that shall become inaccessible and empty for specific containers or services. We try
diff --git a/src/test/test-execute.c b/src/test/test-execute.c
index c81842a0d0..5818adbb55 100644
--- a/src/test/test-execute.c
+++ b/src/test/test-execute.c
@@ -1109,6 +1109,10 @@ static void test_exec_condition(Manager *m) {
         test_service(m, "exec-condition-skip.service", SERVICE_SKIP_CONDITION);
 }
 
+static void test_exec_umask_namespace(Manager *m) {
+        test(m, "exec-umask-namespace.service", can_unshare ? 0 : EXIT_NAMESPACE, CLD_EXITED);
+}
+
 typedef struct test_entry {
         test_function_t f;
         const char *name;
@@ -1191,6 +1195,7 @@ int main(int argc, char *argv[]) {
                 entry(test_exec_specifier),
                 entry(test_exec_execsearchpath_specifier),
                 entry(test_exec_systemcallfilter_system),
+                entry(test_exec_umask_namespace),
                 {},
         };
         int r;
diff --git a/src/test/test-fs-util.c b/src/test/test-fs-util.c
index 41ddec4783..d8273bc846 100644
--- a/src/test/test-fs-util.c
+++ b/src/test/test-fs-util.c
@@ -765,7 +765,6 @@ static void test_rename_noreplace(void) {
 
 static void test_chmod_and_chown(void) {
         _cleanup_(rm_rf_physical_and_freep) char *d = NULL;
-        _unused_ _cleanup_umask_ mode_t u = umask(0000);
         struct stat st;
         const char *p;
 
@@ -774,6 +773,8 @@ static void test_chmod_and_chown(void) {
 
         log_info("/* %s */", __func__);
 
+        BLOCK_WITH_UMASK(0000);
+
         assert_se(mkdtemp_malloc(NULL, &d) >= 0);
 
         p = strjoina(d, "/reg");
diff --git a/test/test-execute/exec-umask-namespace.service b/test/test-execute/exec-umask-namespace.service
new file mode 100644
index 0000000000..8419c86c9a
--- /dev/null
+++ b/test/test-execute/exec-umask-namespace.service
@@ -0,0 +1,12 @@
+# SPDX-License-Identifier: LGPL-2.1-or-later
+[Unit]
+Description=Test for UMask= + namespacing
+
+[Service]
+ExecStart=/bin/ls -lahd /tmp/subdir
+Type=oneshot
+User=65534
+Group=65534
+TemporaryFileSystem=/tmp:ro
+BindPaths=/etc:/tmp/subdir/subsub
+UMask=0007