From d1d0b895dc9389875983af0dde632d74db5b7773 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 16 Mar 2022 18:42:51 +0100 Subject: [PATCH 1/6] nspawn: rebreak all comments in outer_child() --- src/nspawn/nspawn.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index 5102c16438..8e0bc1c077 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -3616,10 +3616,11 @@ static int outer_child( ssize_t l; int r; - /* This is the "outer" child process, i.e the one forked off by the container manager itself. It already has - * its own CLONE_NEWNS namespace (which was created by the clone()). It still lives in the host's CLONE_NEWPID, - * CLONE_NEWUTS, CLONE_NEWIPC, CLONE_NEWUSER and CLONE_NEWNET namespaces. After it completed a number of - * initializations a second child (the "inner" one) is forked off it, and it exits. */ + /* This is the "outer" child process, i.e the one forked off by the container manager itself. It + * already has its own CLONE_NEWNS namespace (which was created by the clone()). It still lives in + * the host's CLONE_NEWPID, CLONE_NEWUTS, CLONE_NEWIPC, CLONE_NEWUSER and CLONE_NEWNET + * namespaces. After it completed a number of initializations a second child (the "inner" one) is + * forked off it, and it exits. */ assert(barrier); assert(directory); @@ -3649,10 +3650,10 @@ static int outer_child( return r; if (dissected_image) { - /* If we are operating on a disk image, then mount its root directory now, but leave out the rest. We - * can read the UID shift from it if we need to. Further down we'll mount the rest, but then with the - * uid shift known. That way we can mount VFAT file systems shifted to the right place right away. This - * makes sure ESP partitions and userns are compatible. */ + /* If we are operating on a disk image, then mount its root directory now, but leave out the + * rest. We can read the UID shift from it if we need to. Further down we'll mount the rest, + * but then with the uid shift known. That way we can mount VFAT file systems shifted to the + * right place right away. This makes sure ESP partitions and userns are compatible. */ r = dissected_image_mount_and_warn( dissected_image, @@ -3682,9 +3683,9 @@ static int outer_child( "Short write while sending UID shift."); if (arg_userns_mode == USER_NAMESPACE_PICK) { - /* When we are supposed to pick the UID shift, the parent will check now whether the UID shift - * we just read from the image is available. If yes, it will send the UID shift back to us, if - * not it will pick a different one, and send it back to us. */ + /* When we are supposed to pick the UID shift, the parent will check now whether the + * UID shift we just read from the image is available. If yes, it will send the UID + * shift back to us, if not it will pick a different one, and send it back to us. */ l = recv(uid_shift_socket, &arg_uid_shift, sizeof(arg_uid_shift), 0); if (l < 0) @@ -3740,7 +3741,8 @@ static int outer_child( return r; if (arg_userns_mode != USER_NAMESPACE_NO && bind_user_context) { - /* Send the user maps we determined to the parent, so that it installs it in our user namespace UID map table */ + /* Send the user maps we determined to the parent, so that it installs it in our user + * namespace UID map table */ for (size_t i = 0; i < bind_user_context->n_data; i++) { uid_t map[] = { @@ -3833,15 +3835,13 @@ static int outer_child( unified_cgroup_hierarchy_socket = safe_close(unified_cgroup_hierarchy_socket); } - /* Mark everything as shared so our mounts get propagated down. This is - * required to make new bind mounts available in systemd services - * inside the container that create a new mount namespace. - * See https://github.com/systemd/systemd/issues/3860 - * Further submounts (such as /dev) done after this will inherit the - * shared propagation mode. + /* Mark everything as shared so our mounts get propagated down. This is required to make new bind + * mounts available in systemd services inside the container that create a new mount namespace. See + * https://github.com/systemd/systemd/issues/3860 Further submounts (such as /dev) done after this + * will inherit the shared propagation mode. * - * IMPORTANT: Do not overmount the root directory anymore from now on to - * enable moving the root directory mount to root later on. + * IMPORTANT: Do not overmount the root directory anymore from now on to enable moving the root + * directory mount to root later on. * https://github.com/systemd/systemd/issues/3847#issuecomment-562735251 */ r = mount_nofollow_verbose(LOG_ERR, NULL, directory, NULL, MS_SHARED|MS_REC, NULL); From 1eb874b97819983f2e9a3355efbb90e929d39398 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Mar 2022 10:02:41 +0100 Subject: [PATCH 2/6] nspawn: make more stuff const And if we make it const, we can also make it static. --- src/nspawn/nspawn.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index 8e0bc1c077..1a654d8817 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -3563,7 +3563,7 @@ static int inner_child( static int setup_notify_child(void) { _cleanup_close_ int fd = -1; - union sockaddr_union sa = { + static const union sockaddr_union sa = { .un.sun_family = AF_UNIX, .un.sun_path = NSPAWN_NOTIFY_SOCKET_PATH, }; From aff7ae0d67e0cd4b3c60e043ea9112d3c2972fce Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Mar 2022 10:31:55 +0100 Subject: [PATCH 3/6] nspawn: if we refuse to operate on some directory, explain why (Also, some refactoring to use safer path_join()) --- src/nspawn/nspawn.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index 1a654d8817..a185585b50 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -5612,31 +5612,36 @@ static int run(int argc, char *argv[]) { } if (arg_start_mode == START_BOOT) { + _cleanup_free_ char *b = NULL; const char *p; - if (arg_pivot_root_new) - p = prefix_roota(arg_directory, arg_pivot_root_new); - else + if (arg_pivot_root_new) { + b = path_join(arg_directory, arg_pivot_root_new); + if (!b) + return log_oom(); + + p = b; + } else p = arg_directory; if (path_is_os_tree(p) <= 0) { - log_error("Directory %s doesn't look like an OS root directory (os-release file is missing). Refusing.", p); - r = -EINVAL; + r = log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Directory %s doesn't look like an OS root directory (os-release file is missing). Refusing.", p); goto finish; } } else { - const char *p, *q; + _cleanup_free_ char *p = NULL; if (arg_pivot_root_new) - p = prefix_roota(arg_directory, arg_pivot_root_new); + p = path_join(arg_directory, arg_pivot_root_new, "/usr/"); else - p = arg_directory; + p = path_join(arg_directory, "/usr/"); + if (!p) + return log_oom(); - q = strjoina(p, "/usr/"); - - if (laccess(q, F_OK) < 0) { - log_error("Directory %s doesn't look like it has an OS tree. Refusing.", p); - r = -EINVAL; + if (laccess(p, F_OK) < 0) { + r = log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Directory %s doesn't look like it has an OS tree (/usr/ directory is missing). Refusing.", arg_directory); goto finish; } } From 264caae299aa8f42f20460ad3280add657a3747f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Mar 2022 12:03:03 +0100 Subject: [PATCH 4/6] base-filesystem: use uid_is_valid() at one more place --- src/shared/base-filesystem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/base-filesystem.c b/src/shared/base-filesystem.c index 5f5328c8cf..3a77f37a69 100644 --- a/src/shared/base-filesystem.c +++ b/src/shared/base-filesystem.c @@ -176,7 +176,7 @@ int base_filesystem_create(const char *root, uid_t uid, gid_t gid) { return -errno; } - if (uid != UID_INVALID || gid != UID_INVALID) + if (uid_is_valid(uid) || gid_is_valid(gid)) if (fchownat(fd, table[i].dir, uid, gid, AT_SYMLINK_NOFOLLOW) < 0) return log_error_errno(errno, "Failed to chown directory at %s/%s: %m", root, table[i].dir); } From 50ae2966d20b0b4a19def060de3b966b7a70b54a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Mar 2022 13:46:12 +0100 Subject: [PATCH 5/6] nspawn: make sure host root can write to the uidmapped mounts we prepare for the container payload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When using user namespaces in conjunction with uidmapped mounts, nspawn so far set up two uidmappings: 1. One that is used for the uidmapped mount and that maps the UID range 0…65535 on the backing fs to some high UID range X…X+65535 on the uidmapped fs. (Let's call this mapping the "mount mapping") 2. One that is used for the userns namespace the container payload processes run in, that maps X…X+65535 back to 0…65535. (Let's call this one the "process mapping"). These mappings hence are pretty much identical, one just moves things up and one back down. (Reminder: we do all this so that the processes can run under high UIDs while running off file systems that require no recursive chown()ing, i.e. we want processes with high UID range but files with low UID range.) This creates one problem, i.e. issue #20989: if nspawn (which runs as host root, i.e. host UID 0) wants to add inodes to the uidmapped mount it can't do that, since host UID 0 is not defined in the mount mapping (only the X…X+65536 range is, after all, and X > 0), and processes whose UID is not mapped in a uidmapped fs cannot create inodes in it since those would be owned by an unmapped UID, which then triggers the famous EOVERFLOW error. Let's fix this, by explicitly including an entry for the host UID 0 in the mount mapping. Specifically, we'll extend the mount mapping to map UID 2147483646 (which is INT32_MAX-1, see code for an explanation why I picked this one) of the backing fs to UID 0 on the uidmapped fs. This way nspawn can creates inode on the uidmapped as it likes (which will then actually be owned by UID 2147483646 on the backing fs), and as it always did. Note that we do *not* create a similar entry in the process mapping. Thus any files created by nspawn that way (and not chown()ed to something better) will appear as unmapped (i.e. as overflowuid/"nobody") in the container payload. And that's good. Of course, the latter is mostly theoretic, as nspawn should generally chown() the inodes it creates to UID ranges that actually make sense for the container (and we generally already do this correctly), but it#s good to know that we are safe here, given we might accidentally forget to chown() some inodes we create. Net effect: the two mappings will not be identical anymore. The mount mapping has one entry more, and the only reason it exists is so that nspawn can access the uidmapped fs reasonably independently from any process mapping. Fixes: #20989 --- src/basic/user-util.h | 13 +++++++++++++ src/nspawn/nspawn-mount.c | 2 +- src/nspawn/nspawn.c | 2 +- src/shared/dissect-image.c | 2 +- src/shared/mount-util.c | 28 +++++++++++++++++++++++----- src/shared/mount-util.h | 13 ++++++++++++- 6 files changed, 51 insertions(+), 9 deletions(-) diff --git a/src/basic/user-util.h b/src/basic/user-util.h index 40979d1080..e1692c4f66 100644 --- a/src/basic/user-util.h +++ b/src/basic/user-util.h @@ -67,6 +67,19 @@ int take_etc_passwd_lock(const char *root); #define UID_NOBODY ((uid_t) 65534U) #define GID_NOBODY ((gid_t) 65534U) +/* If REMOUNT_IDMAP_HOST_ROOT is set for remount_idmap() we'll include a mapping here that maps the host root + * user accessing the idmapped mount to the this user ID on the backing fs. This is the last valid UID in the + * *signed* 32bit range. You might wonder why precisely use this specific UID for this purpose? Well, we + * definitely cannot use the first 0…65536 UIDs for that, since in most cases that's precisely the file range + * we intend to map to some high UID range, and since UID mappings have to be bijective we thus cannot use + * them at all. Furthermore the UID range beyond INT32_MAX (i.e. the range above the signed 32bit range) is + * icky, since many APIs cannot use it (example: setfsuid() returns the old UID as signed integer). Following + * our usual logic of assigning a 16bit UID range to each container, so that the upper 16bit of a 32bit UID + * value indicate kind of a "container ID" and the lower 16bit map directly to the intended user you can read + * this specific UID as the "nobody" user of the container with ID 0x7FFF, which is kinda nice. */ +#define UID_MAPPED_ROOT ((uid_t) (INT32_MAX-1)) +#define GID_MAPPED_ROOT ((gid_t) (INT32_MAX-1)) + #define ETC_PASSWD_LOCK_PATH "/etc/.pwd.lock" /* The following macros add 1 when converting things, since UID 0 is a valid UID, while the pointer diff --git a/src/nspawn/nspawn-mount.c b/src/nspawn/nspawn-mount.c index 40773d90c1..f2fad0f462 100644 --- a/src/nspawn/nspawn-mount.c +++ b/src/nspawn/nspawn-mount.c @@ -780,7 +780,7 @@ static int mount_bind(const char *dest, CustomMount *m, uid_t uid_shift, uid_t u } if (idmapped) { - r = remount_idmap(where, uid_shift, uid_range); + r = remount_idmap(where, uid_shift, uid_range, REMOUNT_IDMAP_HOST_ROOT); if (r < 0) return log_error_errno(r, "Failed to map ids for bind mount %s: %m", where); } diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index a185585b50..7fe061899c 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -3781,7 +3781,7 @@ static int outer_child( IN_SET(arg_userns_ownership, USER_NAMESPACE_OWNERSHIP_MAP, USER_NAMESPACE_OWNERSHIP_AUTO) && arg_uid_shift != 0) { - r = remount_idmap(directory, arg_uid_shift, arg_uid_range); + r = remount_idmap(directory, arg_uid_shift, arg_uid_range, REMOUNT_IDMAP_HOST_ROOT); if (r == -EINVAL || ERRNO_IS_NOT_SUPPORTED(r)) { /* This might fail because the kernel or file system doesn't support idmapping. We * can't really distinguish this nicely, nor do we have any guarantees about the diff --git a/src/shared/dissect-image.c b/src/shared/dissect-image.c index 14519ead70..14361a0e5a 100644 --- a/src/shared/dissect-image.c +++ b/src/shared/dissect-image.c @@ -1807,7 +1807,7 @@ static int mount_partition( (void) fs_grow(node, p); if (remap_uid_gid) { - r = remount_idmap(p, uid_shift, uid_range); + r = remount_idmap(p, uid_shift, uid_range, REMOUNT_IDMAP_HOST_ROOT); if (r < 0) return r; } diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index 7f88a982bd..ae1f2f0b80 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -1049,14 +1049,31 @@ int make_mount_point(const char *path) { return 1; } -static int make_userns(uid_t uid_shift, uid_t uid_range) { - char line[DECIMAL_STR_MAX(uid_t)*3+3+1]; +static int make_userns(uid_t uid_shift, uid_t uid_range, RemountIdmapFlags flags) { _cleanup_close_ int userns_fd = -1; + _cleanup_free_ char *line = NULL; /* Allocates a userns file descriptor with the mapping we need. For this we'll fork off a child * process whose only purpose is to give us a new user namespace. It's killed when we got it. */ - xsprintf(line, UID_FMT " " UID_FMT " " UID_FMT "\n", 0, uid_shift, uid_range); + if (asprintf(&line, UID_FMT " " UID_FMT " " UID_FMT "\n", 0, uid_shift, uid_range) < 0) + return log_oom_debug(); + + /* If requested we'll include an entry in the mapping so that the host root user can make changes to + * the uidmapped mount like it normally would. Specifically, we'll map the user with UID_HOST_ROOT on + * the backing fs to UID 0. This is useful, since nspawn code wants to create various missing inodes + * in the OS tree before booting into it, and this becomes very easy and straightforward to do if it + * can just do it under its own regular UID. Note that in that case the container's runtime uidmap + * (i.e. the one the container payload processes run in) will leave this UID unmapped, i.e. if we + * accidentally leave files owned by host root in the already uidmapped tree around they'll show up + * as owned by 'nobody', which is safe. (Of course, we shouldn't leave such inodes around, but always + * chown() them to the container's own UID range, but it's good to have a safety net, in case we + * forget it.) */ + if (flags & REMOUNT_IDMAP_HOST_ROOT) + if (strextendf(&line, + UID_FMT " " UID_FMT " " UID_FMT "\n", + UID_MAPPED_ROOT, 0, 1) < 0) + return log_oom_debug(); /* We always assign the same UID and GID ranges */ userns_fd = userns_acquire(line, line); @@ -1069,7 +1086,8 @@ static int make_userns(uid_t uid_shift, uid_t uid_range) { int remount_idmap( const char *p, uid_t uid_shift, - uid_t uid_range) { + uid_t uid_range, + RemountIdmapFlags flags) { _cleanup_close_ int mount_fd = -1, userns_fd = -1; int r; @@ -1085,7 +1103,7 @@ int remount_idmap( return log_debug_errno(errno, "Failed to open tree of mounted filesystem '%s': %m", p); /* Create a user namespace mapping */ - userns_fd = make_userns(uid_shift, uid_range); + userns_fd = make_userns(uid_shift, uid_range, flags); if (userns_fd < 0) return userns_fd; diff --git a/src/shared/mount-util.h b/src/shared/mount-util.h index 9caf92308a..960d586ba9 100644 --- a/src/shared/mount-util.h +++ b/src/shared/mount-util.h @@ -112,7 +112,18 @@ int mount_image_in_namespace(pid_t target, const char *propagate_path, const cha int make_mount_point(const char *path); -int remount_idmap(const char *p, uid_t uid_shift, uid_t uid_range); +typedef enum RemountIdmapFlags { + /* Include a mapping from UID_MAPPED_ROOT (i.e. UID 2^31-2) on the backing fs to UID 0 on the + * uidmapped fs. This is useful to ensure that the host root user can safely add inodes to the + * uidmapped fs (which otherwise wouldn't work as the host root user is not defined on the uidmapped + * mount and any attempts to create inodes will then be refused with EOVERFLOW). The idea is that + * these inodes are quickly re-chown()ed to more suitable UIDs/GIDs. Any code that intends to be able + * to add inodes to file systems mapped this way should set this flag, but given it comes with + * certain security implications defaults to off, and requires explicit opt-in. */ + REMOUNT_IDMAP_HOST_ROOT = 1 << 0, +} RemountIdmapFlags; + +int remount_idmap(const char *p, uid_t uid_shift, uid_t uid_range, RemountIdmapFlags flags); /* Creates a mount point (not parents) based on the source path or stat - ie, a file or a directory */ int make_mount_point_inode_from_stat(const struct stat *st, const char *dest, mode_t mode); From 0456118807f3afb99bef0249e952f9cdff9fcce2 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Mar 2022 15:01:11 +0100 Subject: [PATCH 6/6] homed: permit inodes owned by UID_MAPPED_ROOT to be created in $HOME If people use nspawn in their $HOME we should allow this inodes owned by this special UID to be created temporarily, so that UID mapped nspawn containers just work. --- src/home/homework-mount.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/home/homework-mount.c b/src/home/homework-mount.c index 35645e292e..3095a10ff9 100644 --- a/src/home/homework-mount.c +++ b/src/home/homework-mount.c @@ -216,6 +216,12 @@ static int make_userns(uid_t stored_uid, uid_t exposed_uid) { if (r < 0) return log_oom(); + /* Map nspawn's mapped root UID as identity mapping so that people can run nspawn uidmap mounted + * containers off $HOME, if they want. */ + r = strextendf(&text, UID_FMT " " UID_FMT " " UID_FMT "\n", UID_MAPPED_ROOT, UID_MAPPED_ROOT, 1); + if (r < 0) + return log_oom(); + /* Leave everything else unmapped, starting from UID_NOBODY itself. Specifically, this means the * whole space outside of 16bit remains unmapped */