Re: [PATCH v2 -next 09/18] landlock: Add READ_METADATA and WRITE_METADATA access rights

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] landlock: MAC bypass for SUID stripping via chown()
--

commit 229580d85eb842f4fd8d395335278d79b5773959
Author: Cai Xinchen <caixinchen1@huawei.com>

landlock: Add READ_METADATA and WRITE_METADATA access rights
    
Add two new filesystem access rights to control file metadata access:
- LANDLOCK_ACCESS_FS_READ_METADATA
- LANDLOCK_ACCESS_FS_WRITE_METADATA

Link: https://lore.kernel.org/all/abc960a1-e66e-792e-6869-cfd201c29dbe@digikod.net/

>     hook_inode_setattr only restricts explicit metadata changes, i.e. it
>     checks WRITE_METADATA only when the ia_valid mask contains
>     ATTR_MODE, ATTR_UID, ATTR_GID, ATTR_TIMES_SET or ATTR_TOUCH.
>     Metadata changes that the kernel performs implicitly, such as
>     timestamp updates on write(2) or size changes on truncate(2), are
>     therefore not restricted, and neither are chmod(2)/chown(2) calls
>     that do not change any attribute (e.g. chown(2) with -1/-1, which is
>     a no-op that never reaches the hook), matching the behavior of the
>     SELinux inode_setattr hook.

[Severity: Medium]
Is this description accurate for all file types?

If a restricted user invokes chown(-1, -1) on an owned SUID file,
chown_common() unconditionally sets ATTR_KILL_SUID without ATTR_FORCE when
acting on a non-directory:

fs/open.c:chown_common() {
    ...
	if (!S_ISDIR(inode->i_mode))
		newattrs.ia_valid |= ATTR_KILL_SUID | ATTR_KILL_PRIV |
				     setattr_should_drop_sgid(idmap, inode);
    ...
}

> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index a8fa8f77e775d..f56277c67a247 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
[ ... ]
> @@ -1676,6 +1678,119 @@ static int hook_path_truncate(const struct path *const path)
[ ... ]
> +static int hook_inode_setattr(const struct path *const path,
> +			      struct iattr *const attr)
> +{
> +	unsigned int ia_valid = attr->ia_valid;
> +
> +	/*
> +	 * Only explicit metadata changes are checked: implicit
> +	 * timestamp updates never go through notify_change(), and size
> +	 * changes (ATTR_SIZE) are governed by the truncate hooks
> +	 * instead.  See the "File metadata" section in the user space
> +	 * documentation for the full semantics.
> +	 *
> +	 * Clearing the setuid and setgid bits on write(2) or
> +	 * truncate(2) is also an implicit change: callers request it
> +	 * with ATTR_KILL_S[UG]ID, and do_notify_change() then adds a
> +	 * synthesized ATTR_MODE (with these bits removed) before
> +	 * calling this hook.  As do_notify_change() makes it a bug for
> +	 * callers to pass ATTR_MODE together with a kill flag, an
> +	 * ATTR_MODE seen along with ATTR_KILL_S[UG]ID is always
> +	 * kernel-synthesized and must not be checked.  An explicit
> +	 * chmod(2) never sets a kill flag, and an explicit chown(2) on
> +	 * a privileged file is still checked thanks to ATTR_UID and
> +	 * ATTR_GID.
> +	 */
> +	if (ia_valid & (ATTR_KILL_SUID | ATTR_KILL_SGID))
> +		ia_valid &= ~ATTR_MODE;
> +
> +	if (!(ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
> +			  ATTR_TIMES_SET | ATTR_TOUCH)))
> +		return 0;

[Severity: Medium]
Will this logic erroneously clear ATTR_MODE without checking for ATTR_FORCE?

When chown(-1, -1) sets ATTR_KILL_SUID, the VFS notify_change() helper then
synthesizes ATTR_MODE. Because ATTR_KILL_SUID is present, this hook clears
ATTR_MODE. Since user and group are -1, neither ATTR_UID nor ATTR_GID is set.

The final check will evaluate to true, silently bypassing the WRITE_METADATA
access check and allowing an explicit chown(2) SUID-clear request.

By comparison, SELinux avoids this regression by checking ATTR_FORCE before
stripping ATTR_MODE for implicit SUID drops:

security/selinux/hooks.c:selinux_inode_setattr() {
    ...
	/* ATTR_FORCE is just used for ATTR_KILL_S[UG]ID. */
	if (ia_valid & ATTR_FORCE) {
		ia_valid &= ~(ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_MODE |
			      ATTR_FORCE);
    ...
}

> +
> +	return current_check_access_path(path,
> +					LANDLOCK_ACCESS_FS_WRITE_METADATA);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015125425.3246251-1-caixinchen1@huawei.com?part=9