Re: [PATCH v3 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: v2 xattr bypasses setfcap_level check in cap_convert_nscap()
--

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

capabilities: limit fscaps to where CAP_SETFCAP reaches

This commit updates cap_convert_nscap() in security/commoncap.c to verify
that both the mount-mapped root uid (vfsrootid) and the filesystem root uid
(rootid) do not map to uid 0 in a user namespace ancestor above
task_ns->setfcap_level. This prevents a task that entered a user namespace
without CAP_SETFCAP from setting file capabilities for an ancestor's root
user when a privileged task has mapped that uid into the namespace.

> diff --git a/security/commoncap.c b/security/commoncap.c
> index b069142230395..1a419a4a872c8 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -703,10 +703,24 @@ int cap_convert_nscap(const struct mnt_idmap *idmap, struct dentry *dentry,
>  	if (!vfsuid_valid(vfsrootid))
>  		return -EINVAL;
>  
> +	/*
> +	 * The root user may be the root user of ancestors of our namespace as
> +	 * well.  CAP_SETFCAP that we got for entering it doesn't cover those.
> +	 * On an idmapped mount the root user is vfsrootid as seen through
> +	 * this mount and rootid as seen through every other mount of the
> +	 * filesystem, so both have to stay within reach.
> +	 */
> +	if (cap_root_level(vfsuid_into_kuid(vfsrootid), task_ns) <
> +	    task_ns->setfcap_level)
> +		return -EPERM;
> +
>  	rootid = from_vfsuid(idmap, fs_ns, vfsrootid);
>  	if (!uid_valid(rootid))
>  		return -EINVAL;
>  
> +	if (cap_root_level(rootid, task_ns) < task_ns->setfcap_level)
> +		return -EPERM;

[Severity: High]
Can a v2 xattr bypass these new setfcap_level checks in
security/commoncap.c:cap_convert_nscap()?

Earlier in cap_convert_nscap(), v2 capabilities take an early return before
reaching rootid_from_xattr() and the new cap_root_level() checks:

	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;

Because a task in task_ns has CAP_SETFCAP in task_ns,
ns_capable(inode->i_sb->s_user_ns, CAP_SETFCAP) succeeds whenever fs_ns
(inode->i_sb->s_user_ns) is task_ns or a descendant of task_ns, regardless
of task_ns->setfcap_level.

In the scenario from the commit message (task B has uid 0 and CAP_SYS_ADMIN
without CAP_SETFCAP in init_user_ns, unshares ns1 with ns1->setfcap_level = 1,
and task A writes "0 0 1" to ns1's uid_map), task B can mount a filesystem
in ns1 (s_user_ns == ns1) and write a v2 security.capability xattr
(size == XATTR_CAPS_SZ_2). While a v3 xattr with rootid 0 is rejected with
-EPERM, the v2 write returns early and stores a raw v2 xattr whose root user
is make_kuid(ns1, 0), which is kuid 0.

If task B in init_user_ns then mounts overlayfs in init_user_ns
(ovl_sb->s_user_ns == &init_user_ns) with that ns1 directory as lowerdir and
executes the file in init_user_ns, get_file_caps() checks
current_in_userns(file->f_path.mnt->mnt_sb->s_user_ns) against
ovl_sb->s_user_ns (&init_user_ns) and reads the capability through:

  get_vfs_caps_from_disk()
    __vfs_getxattr()
      ovl_xattr_get()               [fs/overlayfs/xattrs.c]
        vfs_getxattr()              [under with_ovl_creds(dentry->d_sb)]
          cap_inode_getsecurity()   [security/commoncap.c]

In fs/overlayfs/xattrs.c:ovl_xattr_get() (and similarly during copy-up in
fs/overlayfs/copy_up.c:ovl_copy_xattr()):

	with_ovl_creds(dentry->d_sb)
		return vfs_getxattr(mnt_idmap(realpath.mnt), realpath.dentry, name, value, size);

And in security/commoncap.c:cap_inode_getsecurity():

	if (is_v2header(size, cap)) {
		root = 0;
	...
	kroot = make_kuid(fs_ns, root);

cap_inode_getsecurity() resolves kroot to make_kuid(ns1, 0) == kuid 0 and
returns a valid v2 capability to get_vfs_caps_from_disk(), granting the file
capabilities in init_user_ns.

Additionally, if task A maps kuid 0 as a non-zero uid in ns1 (for example,
"0 1000 1" and "1000 0 1" with ns1->setfcap_level = 1), task B in ns1 can
unshare a child namespace ns2 and map "0 1000 1" in ns2 (permitted by
verify_root_map() because lower_first is 1000 != 0 in ns1). On a filesystem
mounted in ns2 (fs_ns == ns2), a caller in ns1 or ns2 writing a v2 xattr
satisfies ns_capable(ns2, CAP_SETFCAP) and writes an unconverted v2 xattr
whose root user is make_kuid(ns2, 0) == kuid 0 rather than make_kuid(ns1, 0).

Should the v2 fast path in cap_convert_nscap() be moved after the vfsrootid
and rootid checks (or verify that vfsrootid and rootid match and satisfy
cap_root_level() >= task_ns->setfcap_level before returning size)?

> +
>  	nsrootid = from_kuid(fs_ns, rootid);
>  	if (nsrootid == -1)
>  		return -EINVAL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-b4-setfcap-userns-v3-0-97c7ed4e43e1@toxicpanda.com?part=4