Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] landlock: missing ATTR_KILL_PRIV check in hook_path_setattr() - [Medium] landlock: hook_path_getattr() misses file check in io_statx() AT_EMPTY_PATH - [Medium] landlock: unguarded second metadata hook check on ecryptfs lower_path --
commit c5b94acf0094997d0231e58b264ebd0f262e2631 Author: Cai Xinchen <[email protected]> landlock: Add READ_METADATA and WRITE_METADATA access rights This commit introduces the LANDLOCK_ACCESS_FS_READ_METADATA and LANDLOCK_ACCESS_FS_WRITE_METADATA filesystem access rights to control reading and modifying file and directory metadata through path and file LSM hooks, bumping the Landlock ABI version to 13. > diff --git a/security/landlock/fs.c b/security/landlock/fs.c > index 2ab67bcad1983..73c6353ecbf85 100644 > --- a/security/landlock/fs.c > +++ b/security/landlock/fs.c [ ... ] > @@ -1676,6 +1678,192 @@ static int hook_path_truncate(const struct path > *const path) > return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE); > } > > +static int check_access_file(const struct file *const file, > + const access_mask_t access) > +{ [ ... ] > +} > + > +static int check_access_path_or_file(const struct path *const path, > + const struct file *const file, > + const access_mask_t access) > +{ > + if (file) > + return check_access_file(file, access); > + > + return current_check_access_path(path, access); > +} [Severity: Medium] How does check_access_path_or_file() interact with eCryptfs when forwarding metadata operations to the lower filesystem? Unlike overlayfs, eCryptfs forwards operations under current_cred(). While ecryptfs_getattr(), ecryptfs_getxattr(), ecryptfs_listxattr(), ecryptfs_removexattr(), ecryptfs_fileattr_get(), and ecryptfs_fileattr_set() call unchecked VFS helpers so the LSM hooks run only once on the upper path, four operations in fs/ecryptfs/inode.c call LSM-checked VFS helpers on lower_path with file set to NULL: fs/ecryptfs/inode.c:ecryptfs_setattr() notify_change(&nop_mnt_idmap, &lower_path, &lower_ia, NULL, NULL); fs/ecryptfs/inode.c:ecryptfs_setxattr() __vfs_setxattr_locked(&nop_mnt_idmap, &lower_path, name, value, size, flags, NULL, NULL); fs/ecryptfs/inode.c:ecryptfs_get_acl() vfs_get_acl(idmap, &lower_path, posix_acl_xattr_name(type), NULL); fs/ecryptfs/inode.c:ecryptfs_set_acl() vfs_set_acl(&nop_mnt_idmap, &lower_path, posix_acl_xattr_name(type), acl, NULL); When a sandboxed task invokes fchmod(), fchown(), futimens(), fsetxattr(), or POSIX ACL operations on an eCryptfs file descriptor opened before landlock_restrict_self(), the upper check succeeds via check_access_file(), but the internal forward reaches hook_path_setattr(), hook_path_setxattr(), hook_path_get_acl(), or hook_path_set_acl() with file == NULL and evaluates current_check_access_path(&lower_path, access) against the current domain. Path-based chmod(), chown(), utimensat(), setxattr(), and ACL operations allowed by a Landlock rule on the upper eCryptfs mount point also fail with -EACCES unless the current domain additionally grants metadata rights on the underlying lower path. > + > +static int hook_path_getattr(const struct path *const path, > + struct file *const file) > +{ > + return check_access_path_or_file(path, file, > + LANDLOCK_ACCESS_FS_READ_METADATA); > +} [Severity: Medium] When IORING_OP_STATX is submitted with AT_EMPTY_PATH and an empty pathname string on an open file descriptor, will hook_path_getattr() check the current task's domain instead of the file's open-time access rights? For the statx() syscall, sys_statx() uses filename_maybe_null, which turns an empty string with AT_EMPTY_PATH into name == NULL and dispatches to do_statx_fd() -> vfs_statx_fd(), passing the struct file to vfs_statx_path() -> security_path_getattr(). In contrast, io_uring's io_statx_prep() calls delayed_getname_uflags() -> do_getname() with LOOKUP_EMPTY, producing a non-NULL struct filename where name->name[0] == '\0', and io_statx() then calls do_statx() -> vfs_statx(): io_uring/statx.c:io_statx() ret = do_statx(sx->dfd, name, sx->flags, sx->mask, sx->buffer); fs/stat.c:vfs_statx() error = filename_lookup(dfd, filename, lookup_flags, &path, NULL); if (error) return error; error = vfs_statx_path(&path, flags, stat, request_mask, NULL); Because file is passed as NULL to vfs_statx_path(), hook_path_getattr() falls back to current_check_access_path() instead of check_access_file(). Wouldn't this allow a file descriptor opened without LANDLOCK_ACCESS_FS_READ_METADATA to leak metadata when passed to an unrestricted process that uses IORING_OP_STATX with AT_EMPTY_PATH, while rejecting IORING_OP_STATX with AT_EMPTY_PATH in a sandboxed process using a file descriptor opened before sandboxing? > + > +static int hook_path_file_getattr(const struct path *const path, > + struct file_kattr *const fa, > + struct file *const file) > +{ > + return check_access_path_or_file(path, file, > + LANDLOCK_ACCESS_FS_READ_METADATA); > +} > + > +static int hook_path_setattr(const struct mnt_idmap *const idmap, > + const struct path *const path, > + struct iattr *const attr, > + struct file *const file) > +{ > + 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. > + * > + * The setuid/setgid bit clearing that the kernel performs > + * after write(2) or truncate(2) is an implicit change as > + * well: it comes through __remove_privs() and do_truncate() > + * with ATTR_FORCE set, 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_FORCE is always > + * kernel-synthesized and must not be checked. An explicit > + * chown(2) does not set ATTR_FORCE, so the same clearing > + * requested through chown(2), including chown(2) with > + * (-1, -1) on a privileged file, is still checked through the > + * synthesized ATTR_MODE, matching the SELinux behavior. A > + * chown(2) that changes nothing (e.g. with (-1, -1) on a file > + * without privilege bits) sets neither a changed attribute > + * nor a synthesized ATTR_MODE, and is not checked. > + */ > + if (ia_valid & ATTR_FORCE) > + ia_valid &= ~(ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_MODE); > + > + if (!(ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID | > + ATTR_TIMES_SET | ATTR_TOUCH))) > + return 0; > + > + return check_access_path_or_file(path, file, > + LANDLOCK_ACCESS_FS_WRITE_METADATA); > +} [Severity: Medium] What happens in hook_path_setattr() when chown(path, -1, -1) or fchown(fd, -1, -1) is called on a file that has file capabilities (security.capability) but neither S_ISUID nor S_ISGID set? In chown_common(), when user == -1 and group == -1 on a non-directory, newattrs.ia_valid is set to ATTR_CTIME | ATTR_KILL_SUID | ATTR_KILL_PRIV (plus potentially ATTR_KILL_SGID) without ATTR_UID, ATTR_GID, or ATTR_FORCE: 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); In do_notify_change(), security_inode_need_killpriv() returns 1 when security.capability is present, so ATTR_KILL_PRIV remains set in attr->ia_valid, while ATTR_MODE is not synthesized because S_ISUID and S_ISGID are not set: fs/attr.c:do_notify_change() if (ia_valid & ATTR_KILL_PRIV) { error = security_inode_need_killpriv(dentry); if (error < 0) return error; if (error == 0) ia_valid = attr->ia_valid &= ~ATTR_KILL_PRIV; } When do_notify_change() then calls security_path_setattr() -> hook_path_setattr(), ia_valid only contains ATTR_CTIME | ATTR_KILL_PRIV, so hook_path_setattr() returns 0 without checking LANDLOCK_ACCESS_FS_WRITE_METADATA. Execution then proceeds to setattr_prepare(), which strips the security.capability xattr via security_inode_killpriv() -> cap_inode_killpriv() -> __vfs_removexattr() and updates inode ctime. Should hook_path_setattr() also clear ATTR_KILL_PRIV when ATTR_FORCE is set (for implicit capability removal during write() or truncate()) and include ATTR_KILL_PRIV in the checked ia_valid mask alongside ATTR_MODE, ATTR_UID, ATTR_GID, ATTR_TIMES_SET, and ATTR_TOUCH? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=15
