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 <[email protected]> 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/[email protected]/ > 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/[email protected]?part=9
