Re: [PATCH v5 4/5] capabilities: limit fscaps to where CAP_SETFCAP reaches

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] capabilities: unconverted v2 fscap bypass in cap_convert_nscap()
--

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

capabilities: limit fscaps to where CAP_SETFCAP reaches

This commit restricts writing file capabilities in cap_convert_nscap() so
that the root user named by a v2 or v3 security.capability xattr (both as
seen through an idmapped mount and on the underlying filesystem) cannot be
uid 0 in an ancestor user namespace beyond task_ns->setfcap_level.

> diff --git a/security/commoncap.c b/security/commoncap.c
> index 8a9d998b18972..3551ce9b046ad 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -216,6 +216,29 @@ bool cred_may_setfcap_userns(const struct cred *cred, struct user_namespace *ns)
>  	return !top || cred_setfcap_reaches(cred, ns->parent, top->level);
>  }
>  
> +/**
> + * 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.
> + */
> +static 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
> @@ -671,19 +694,41 @@ int cap_convert_nscap(const struct mnt_idmap *idmap, struct dentry *dentry,
>  		return -EINVAL;
>  	if (!capable_wrt_inode_uidgid(idmap, inode, CAP_SETFCAP))
>  		return -EPERM;
> -	if (size == XATTR_CAPS_SZ_2 && (idmap == &nop_mnt_idmap))
> -		if (ns_capable(inode->i_sb->s_user_ns, CAP_SETFCAP))
> -			/* user is privileged, just write the v2 */
> -			return size;
> +	/*
> +	 * A v2 xattr belongs to the root user of the filesystem's namespace,
> +	 * which may be the root user of ancestors of our namespace as well.
> +	 * Only write it unconverted if that root user is within reach, the
> +	 * checks below handle everything else.
> +	 */
> +	if (size == XATTR_CAPS_SZ_2 && idmap == &nop_mnt_idmap &&
> +	    ns_capable(fs_ns, CAP_SETFCAP) &&
> +	    cap_root_level(make_kuid(fs_ns, 0), task_ns) >=
> +	    task_ns->setfcap_level)
> +		/* user is privileged, just write the v2 */
> +		return size;

[Severity: High]
What happens in cap_convert_nscap() if uid 0 is not mapped in fs_ns when
writing a v2 capability xattr (size == XATTR_CAPS_SZ_2) on a non-idmapped
mount (idmap == &nop_mnt_idmap)?

Unlike the v3 conversion path further down in cap_convert_nscap(), which
checks vfsuid_valid(vfsrootid) and uid_valid(rootid) before calling
cap_root_level(), this v2 fast-path passes make_kuid(fs_ns, 0) directly to
cap_root_level() without checking uid_valid().

When uid 0 is not mapped in fs_ns, make_kuid(fs_ns, 0) returns INVALID_UID.
In cap_root_level(INVALID_UID, task_ns), from_kuid(ns, INVALID_UID) returns
(uid_t)-1 for every namespace up to init_user_ns, so cap_root_level() returns
INT_MAX. Because INT_MAX >= task_ns->setfcap_level is always true,
cap_convert_nscap() returns size and writes the unconverted v2 xattr.

For example, if a task without CAP_SETFCAP unshares a user namespace
(setting setfcap_level to 1) and maps only a non-zero UID (or creates an
unmapped child user namespace and mounts a tmpfs in it), make_kuid(fs_ns, 0)
is INVALID_UID. Wouldn't that allow the task to write an unconverted v2
security.capability xattr on the tmpfs inode, which could then be honoured
through an overlay mounted in init_user_ns, or honoured if fs_ns->uid_map is
later written to map uid 0? And when task_ns == fs_ns has no uid 0 mapping,
writing a v2 xattr writes an unconverted v2 xattr instead of falling through
to rootid_from_xattr() and failing with -EINVAL.

Should the v2 check verify uid_valid(make_kuid(fs_ns, 0)) (or should
cap_root_level() reject !uid_valid(kuid)) before allowing the unconverted v2
xattr to be written?

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