Re: [PATCH v2 3/5] userns: check the opener and the writer before mapping uid 0

[Bradley Morgan]

On 14 October 2026 21:47:48 BST, Josef Bacik <josef@xxxxxxxxxxxxxx> wrote:
>verify_root_map() looks at the cred that opened the map file: it asks
>whether it has CAP_SETFCAP over the parent, or, for an opener inside the
>namespace, whether the creator had.  Two things are missing.
>
>The writer isn't checked.  For every other privileged mapping
>new_idmap_permitted() wants the capability from the opener and from the
>task that calls write(), but a map that consists of the single line
>"0 0 1" takes the unprivileged branch, where only the opener's euid
>counts.  So an open uid_map file is a token for mapping uid 0:
>
>  task A: uid 0, full caps          task B: uid 0, no CAP_SETFCAP
>  fd = open("/proc/<pid>/uid_map")
>  B gets fd by inheritance
>  or SCM_RIGHTS
>                                    write(fd, "0 0 1")
>                                      opener had CAP_SETFCAP -> allowed
>
>And CAP_SETFCAP over the parent isn't always enough.  The parent may map
>its own parent's uid 0, so its root user is the root user of ancestors
>further up, and the capability has to count there too.  A task without
>CAP_SETFCAP that unshared a namespace, which a privileged task then
>mapped "0 0 1" for, has CAP_SETFCAP in it.  It could unshare again and
>pass on uid 0 of the initial namespace one level further down.
>
>Find the topmost namespace that shares the parent's root user with
>cap_root_level(), and require that the opener and the writer both reach
>that far: by ns->setfcap_level for a task in the namespace that is being
>mapped, by cap_setfcap_level() for one outside it.
>
>This only affects maps that contain uid 0 of the parent namespace.  A
>write by a task without CAP_SETFCAP to a file opened by one with it, and
>a map by a task whose CAP_SETFCAP stops short of the root user it passes
>on, now fail with -EPERM.

Ok. Look. I feel there will be nits on this exact review. But I review
more based on "is this code correct? ", because the users are my priority 
right now until I learn maintainers opinions or liking.

Reviewed-by: Bradley Morgan <brads@xxxxxxxxxxxxxx>


>
>Fixes: db2e718a4798 ("capabilities: require CAP_SETFCAP to map uid 0")
>Assisted-by: LLM
>Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
>---
> include/linux/capability.h |  1 +
> kernel/user_namespace.c    | 29 +++++++++++++++++++++--------
> security/commoncap.c       | 23 +++++++++++++++++++++++
> 3 files changed, 45 insertions(+), 8 deletions(-)
>
>diff --git a/include/linux/capability.h b/include/linux/capability.h
>index 026974a5111b..90f9f976d216 100644
>--- a/include/linux/capability.h
>+++ b/include/linux/capability.h
>@@ -224,5 +224,6 @@ int cap_convert_nscap(const struct mnt_idmap *idmap, struct dentry *dentry,
> 		      const void **ivalue, size_t size);
> 
> int cap_setfcap_level(const struct cred *cred, struct user_namespace
> *ns);
>+int cap_root_level(kuid_t kuid, struct user_namespace *ns);
> 
> #endif /* !_LINUX_CAPABILITY_H */
>diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
>index 330085a7d7d4..7dba6594516d 100644
>--- a/kernel/user_namespace.c
>+++ b/kernel/user_namespace.c
>@@ -888,8 +888,9 @@ EXPORT_SYMBOL_IF_KUNIT(uid_gid_map_sort);
>  * @new_map: requested idmap
>  *
>  * If a process requests mapping parent uid 0 into the new ns, verify that the
>- * process writing the map had the CAP_SETFCAP capability as the target process
>- * will be able to write fscaps that are valid in ancestor user namespaces.
>+ * process that opened the map file and the process writing the map had the
>+ * CAP_SETFCAP capability as the target process will be able to write fscaps
>+ * that are valid in ancestor user namespaces.
>  *
>  * Return: true if the mapping is allowed, false if not.
>  */
>@@ -897,7 +898,7 @@ static bool verify_root_map(const struct file *file,
> 			    struct user_namespace *map_ns,
> 			    struct uid_gid_map *new_map)
> {
>-	int idx;
>+	int idx, level;
> 	const struct user_namespace *file_ns = file->f_cred->user_ns;
> 	struct uid_gid_extent *extent0 = NULL;
> 
>@@ -915,24 +916,36 @@ static bool verify_root_map(const struct file *file,
> 	if (!extent0)
> 		return true;
> 
>+	/* The parent may in turn share its root user with its ancestors. */
>+	level = cap_root_level(make_kuid(map_ns->parent, 0), map_ns->parent);
>+
> 	if (map_ns == file_ns) {
> 		/* The process unshared its ns and is writing to its own
> 		 * /proc/self/uid_map.  User already has full capabilites in
> 		 * the new namespace.  Verify that the parent had CAP_SETFCAP
>-		 * when it unshared.
>+		 * when it unshared, as far up as that root user goes.
> 		 * */
>-		if (file_ns->setfcap_level >= file_ns->level)
>+		if (file_ns->setfcap_level > level)
> 			return false;
> 	} else {
> 		/* Process p1 is writing to uid_map of p2, who is in a child
> 		 * user namespace to p1's.  Verify that the opener of the map
> 		 * file has CAP_SETFCAP against the parent of the new map
>-		 * namespace */
>-		if (!file_ns_capable(file, map_ns->parent, CAP_SETFCAP))
>+		 * namespace, and as far up as that root user goes.
>+		 */
>+		if (!file_ns_capable(file, map_ns->parent, CAP_SETFCAP) ||
>+		    cap_setfcap_level(file->f_cred, map_ns->parent) > level)
> 			return false;
> 	}
> 
>-	return true;
>+	/* The file may have been handed to someone else since it was opened,
>+	 * so the same goes for the process that is doing the write.
>+	 */
>+	if (map_ns == current_user_ns())
>+		return map_ns->setfcap_level <= level;
>+
>+	return ns_capable(map_ns->parent, CAP_SETFCAP) &&
>+	       cap_setfcap_level(current_cred(), map_ns->parent) <= level;
> }
> 
> /*
>diff --git a/security/commoncap.c b/security/commoncap.c
>index a51b9d51d96d..f52f1c7a87be 100644
>--- a/security/commoncap.c
>+++ b/security/commoncap.c
>@@ -157,6 +157,29 @@ int cap_setfcap_level(const struct cred *cred, struct user_namespace *ns)
> 	return cred->user_ns->level + 1;
> }
> 
>+/**
>+ * cap_root_level - Find the topmost namespace in which a kuid is the root user
>+ * @kuid: The kuid to look for
>+ * @ns: The user namespace to start from
>+ *
>+ * Return: the lowest ->level among @ns and its ancestors in which @kuid is
>+ * uid 0, which is how far up file capabilities with that root user are
>+ * honoured; INT_MAX if there is no such namespace.
>+ */
>+int cap_root_level(kuid_t kuid, struct user_namespace *ns)
>+{
>+	int level = INT_MAX;
>+
>+	for (;; ns = ns->parent) {
>+		if (from_kuid(ns, kuid) == 0)
>+			level = ns->level;
>+		if (ns == &init_user_ns)
>+			break;
>+	}
>+
>+	return level;
>+}
>+
> /**
>  * cap_settime - Determine whether the current process may set the system clock
>  * @ts: The time to set
>
>

--- Thanks!
"I'm not a very positive person" - Linus torvalds