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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] userns: verify_root_map() misses ancestor root mapped to non-zero parent UID
--

commit 768466ca2ab242701f21cc059dcec4536bf3c7d0
Author: Josef Bacik <josef@toxicpanda.com>

userns: check the opener and the writer before mapping uid 0

This commit updates verify_root_map() to check both the opener of the uid_map
file and the task performing the write when mapping parent uid 0. It also adds
cap_root_level() to verify that both tasks have CAP_SETFCAP reaching the
topmost ancestor namespace that shares the parent's root user.

> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index 7bd3692c2f3eb..f948cf31ecdfd 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c

[ ... ]

> @@ -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);

[Severity: High]
Can an ancestor namespace's root user be mapped into map_ns without passing
this check if it is mapped to a non-zero uid in map_ns->parent?

In proc_uid_map_write() -> map_write() -> new_idmap_permitted() ->
verify_root_map(), the loop in kernel/user_namespace.c:verify_root_map()
only searches new_map for an extent with lower_first == 0 and then checks
make_kuid(map_ns->parent, 0):

	for (idx = 0; idx < new_map->nr_extents; idx++) {
		if (new_map->nr_extents <= UID_GID_MAP_MAX_BASE_EXTENTS)
			extent0 = &new_map->extent[idx];
		else
			extent0 = &new_map->forward[idx];
		if (extent0->lower_first == 0)
			break;

		extent0 = NULL;
	}

	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);

Unlike setfcap_top() in security/commoncap.c, which walks all ancestors
p = ns->parent and checks make_kuid(p, 0), verify_root_map() assumes an
ancestor's root user can only appear at uid 0 in map_ns->parent.

However, an ancestor p's root user make_kuid(p, 0) (for example,
KUIDT_INIT(0) from init_user_ns) can be mapped to a non-zero uid such as
1000 in ns1 = map_ns->parent (via "1000 0 1" or "0 2000 1\n1000 0 1" in
ns1->uid_map).

When a child namespace ns2 (map_ns) of ns1 is then mapped with "0 1000 1",
extent->lower_first is 1000 rather than 0, so verify_root_map() sees
!extent0 and returns true without checking CAP_SETFCAP or setfcap_level for
either the opener or the writer. Similarly, if "1000 0 1" is also included
when uid 0 in ns1 is KUIDT_INIT(2000), cap_root_level(make_kuid(ns1, 0), ns1)
only checks down to ns1->level rather than init_user_ns at level 0.

In new_idmap_permitted(), the unprivileged single-extent self-mapping path
then allows writing "0 1000 1" without CAP_SETUID or CAP_SETFCAP:

	if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1) &&
	    uid_eq(ns->owner, cred->euid)) {
		u32 id = new_map->extent[0].lower_first;
		if (cap_setid == CAP_SETUID) {
			kuid_t uid = make_kuid(ns->parent, id);
			if (uid_eq(uid, cred->euid))
				return true;
		}

Even with the subsequent commit in the series ("capabilities: limit fscaps
to where CAP_SETFCAP reaches"), if ns2 was created by a task in ns1 with
CAP_SETFCAP (so ns2->setfcap_level == 0 and ns2->owner == KUIDT_INIT(0)), a
task in ns1 with euid == KUIDT_INIT(0) that lacks CAP_SETFCAP can open
/proc/<pid>/uid_map (or receive the open fd) and write "0 1000 1". This
maps KUIDT_INIT(0) as uid 0 in ns2 while ns2->setfcap_level == 0, allowing
tasks in ns2 to pass cap_convert_nscap() and write security.capability
xattrs valid in init_user_ns.

Should verify_root_map() check whether any extent in new_map maps a kuid
that is uid 0 in any ancestor namespace, and compute the minimum
cap_root_level() across all mapped ancestor root users?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-b4-setfcap-userns-v4-0-88aa0ffaa08f@toxicpanda.com?part=3