Re: [PATCH v5 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: missing CAP_SETFCAP check in verify_root_map() for tasks in map_ns
--

commit 715d4f4455aa94fb0eaba838f8aa378efb3afe54
Author: Josef Bacik <josef@toxicpanda.com>

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

This commit updates verify_root_map() to find the topmost ancestor whose root
user is included in a new uid_map and checks that both the file opener and the
writer hold CAP_SETFCAP reaching that ancestor's level.

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

[ ... ]

> @@ -897,42 +937,40 @@ static bool verify_root_map(const struct file *file,
>  			    struct user_namespace *map_ns,
>  			    struct uid_gid_map *new_map)
>  {
> -	int idx;
>  	const struct user_namespace *file_ns = file->f_cred->user_ns;
> -	struct uid_gid_extent *extent0 = NULL;
> -
> -	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;
> -	}
> +	int level;
>  
> -	if (!extent0)
> +	level = map_root_level(map_ns->parent, new_map);
> +	if (level == INT_MAX)
>  		return true;
>  
>  	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 those root users go.
>  		 * */
> -		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 those root users go.
> +		 */
> +		if (!file_ns_capable(file, map_ns->parent, CAP_SETFCAP) ||
> +		    !cred_setfcap_reaches(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) &&
> +	       cred_setfcap_reaches(current_cred(), map_ns->parent, level);
>  }

[Severity: High]
When a task is in map_ns (so map_ns == file_ns for the opener or
map_ns == current_user_ns() for the writer), does checking only
map_ns->setfcap_level <= level allow a task that has dropped CAP_SETFCAP in
map_ns to still map the ancestor root user?

For example, if a process running as uid 0 with CAP_SETFCAP in init_user_ns
calls unshare(CLONE_NEWUSER), create_user_ns() sets map_ns->setfcap_level to 0.
Inside map_ns (before uid_map is written), two cases can occur:

1. Task A opens /proc/self/uid_map and passes the descriptor (by fork or
   SCM_RIGHTS) to Task B in map_ns, which has dropped CAP_SETFCAP and all
   other capabilities via capset().
2. A task in map_ns drops CAP_SETFCAP via capset() while keeping
   CAP_SYS_ADMIN, and then opens /proc/self/uid_map itself.

When the task without CAP_SETFCAP writes "0 0 1" through:

  proc_uid_map_write()
    -> map_write()
      -> new_idmap_permitted()
        -> verify_root_map()

map_write() only checks file_ns_capable(file, map_ns, CAP_SYS_ADMIN) on the
opener, and new_idmap_permitted() takes the single-extent unprivileged branch
which only checks uid_eq(ns->owner, cred->euid):

kernel/user_namespace.c:new_idmap_permitted() {
	if (cap_setid == CAP_SETUID && !verify_root_map(file, ns, new_map))
		return false;

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

Because verify_root_map() only checks map_ns->setfcap_level <= level when
map_ns == file_ns and when map_ns == current_user_ns(), the write succeeds
even though the task dropped CAP_SETFCAP. Once "0 0 1" is installed in
map_ns->uid_map, make_kuid(map_ns, 0) becomes KUIDT_INIT(0) instead of
INVALID_UID, allowing the task in map_ns to call execve() and regain full
capabilities (including CAP_SETFCAP) via handle_privileged_root() in
security/commoncap.c.

Note that cap_setfcap_level() in security/commoncap.c already handles a cred
whose user_ns is the target namespace by checking whether CAP_SETFCAP is still
raised in cred->cap_effective before returning cred->user_ns->setfcap_level:

security/commoncap.c:cap_setfcap_level() {
	if (cap_capable(cred, ns, CAP_SETFCAP, CAP_OPT_NOAUDIT) != 0)
		return ns->level + 1;

	if (cap_raised(cred->cap_effective, CAP_SETFCAP))
		return cred->user_ns->setfcap_level;

	/* All we have is that we own a child of our namespace. */
	return cred->user_ns->level + 1;
}

When map_ns == file_ns (or map_ns == current_user_ns()), should
verify_root_map() check file_ns_capable(file, map_ns, CAP_SETFCAP) and
cred_setfcap_reaches(file->f_cred, map_ns, level) for the opener, and
ns_capable(map_ns, CAP_SETFCAP) and
cred_setfcap_reaches(current_cred(), map_ns, level) for the writer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-b4-setfcap-userns-v5-0-0620223be951@toxicpanda.com?part=3