From 52f05ef21d7790f37bc3cd6e54fb9a4bcb16efa5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 12 Nov 2021 11:06:46 +0100 Subject: [PATCH 1/5] umask-util: add helper that resets umask until end of current code block --- src/basic/umask-util.h | 3 +++ src/nspawn/nspawn.c | 9 +++------ src/shared/dev-setup.c | 3 +-- src/test/test-fs-util.c | 3 ++- 4 files changed, 9 insertions(+), 9 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/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-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"); From d73020f2420aa3f220481016829aaa2602abf081 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 12 Nov 2021 11:09:40 +0100 Subject: [PATCH 2/5] namespace: rebreak a few comments --- src/core/namespace.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/src/core/namespace.c b/src/core/namespace.c index 68704dff06..5d18b26a74 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) @@ -930,10 +929,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 +972,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 +1082,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); @@ -1972,11 +1970,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 +2238,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 From cdf42f9bd40ff21a67d58b948efea055d56ad398 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 12 Nov 2021 11:11:27 +0100 Subject: [PATCH 3/5] namespace: make whole namespace_setup() work regardless of configured umask Let's reset the umask during the whole namespace_setup() logic, so that all our mkdir() + mknod() are not subjected to whatever umask might currently be set. This mostly moves the umask save/restore logic out of mount_private_dev() and into the stack frame of namespace_setup() that is further out. Fixes #19899 --- src/core/namespace.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/core/namespace.c b/src/core/namespace.c index 5d18b26a74..2239bbfb8a 100644 --- a/src/core/namespace.c +++ b/src/core/namespace.c @@ -852,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); @@ -1898,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; From 30443439274cc223583c6c57f7d9041e440e346f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 12 Nov 2021 11:16:02 +0100 Subject: [PATCH 4/5] namespace: make tmp dir handling code independent of umask too Let's make all code in namespace.c robust towards weird umask. This doesn't matter too much given that the parent dirs we deal here almost certainly exist anyway, but let's clean this up anyway and make it fully clean. --- src/core/namespace.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/core/namespace.c b/src/core/namespace.c index 2239bbfb8a..c01975b9de 100644 --- a/src/core/namespace.c +++ b/src/core/namespace.c @@ -2508,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; @@ -2516,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) { @@ -2574,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) @@ -2589,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; From 875afa02fabe1dad5aa3d1e9bff89d493a369fd0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 12 Nov 2021 13:41:59 +0100 Subject: [PATCH 5/5] tests: add test case for UMask=+BindPaths= combination Inspired by the test case described in #19899 --- src/test/test-execute.c | 5 +++++ test/test-execute/exec-umask-namespace.service | 12 ++++++++++++ 2 files changed, 17 insertions(+) create mode 100644 test/test-execute/exec-umask-namespace.service 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/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