Arn, Christian, please take a look at the following RFC patch and the
rationale explained here.
On Fri, Feb 09, 2024 at 06:06:05PM +0100, Günther Noack wrote:
> Introduces the LANDLOCK_ACCESS_FS_IOCTL access right
> and increments the Landlock ABI version to 5.
>
> Like the truncate right, these rights are associated with a file
> descriptor at the time of open(2), and get respected even when the
> file descriptor is used outside of the thread which it was originally
> opened in.
>
> A newly enabled Landlock policy therefore does not apply to file
> descriptors which are already open.
>
> If the LANDLOCK_ACCESS_FS_IOCTL right is handled, only a small number
> of safe IOCTL commands will be permitted on newly opened files. The
> permitted IOCTLs can be configured through the ruleset in limited ways
> now. (See documentation for details.)
>
> Specifically, when LANDLOCK_ACCESS_FS_IOCTL is handled, granting this
> right on a file or directory will *not* permit to do all IOCTL
> commands, but only influence the IOCTL commands which are not already
> handled through other access rights. The intent is to keep the groups
> of IOCTL commands more fine-grained.
>
> Noteworthy scenarios which require special attention:
>
> TTY devices are often passed into a process from the parent process,
> and so a newly enabled Landlock policy does not retroactively apply to
> them automatically. In the past, TTY devices have often supported
> IOCTL commands like TIOCSTI and some TIOCLINUX subcommands, which were
> letting callers control the TTY input buffer (and simulate
> keypresses). This should be restricted to CAP_SYS_ADMIN programs on
> modern kernels though.
>
> Some legitimate file system features, like setting up fscrypt, are
> exposed as IOCTL commands on regular files and directories -- users of
> Landlock are advised to double check that the sandboxed process does
> not need to invoke these IOCTLs.
I think we really need to allow fscrypt and fs-verity IOCTLs.
>
> Known limitations:
>
> The LANDLOCK_ACCESS_FS_IOCTL access right is a coarse-grained control
> over IOCTL commands. Future work will enable a more fine-grained
> access control for IOCTLs.
>
> In the meantime, Landlock users may use path-based restrictions in
> combination with their knowledge about the file system layout to
> control what IOCTLs can be done. Mounting file systems with the nodev
> option can help to distinguish regular files and devices, and give
> guarantees about the affected files, which Landlock alone can not give
> yet.
I had a second though about our current approach, and it looks like we
can do simpler, more generic, and with less IOCTL commands specific
handling.
What we didn't take into account is that an IOCTL needs an opened file,
which means that the caller must already have been allowed to open this
file in read or write mode.
I think most FS-specific IOCTL commands check access rights (i.e. access
mode or required capability), other than implicit ones (at least read or
write), when appropriate. We don't get such guarantee with device
drivers.
The main threat is IOCTLs on character or block devices because their
impact may be unknown (if we only look at the IOCTL command, not the
backing file), but we should allow IOCTLs on filesystems (e.g. fscrypt,
fs-verity, clone extents). I think we should only implement a
LANDLOCK_ACCESS_FS_IOCTL_DEV right, which would be more explicit. This
change would impact the IOCTLs grouping (not required anymore), but
we'll still need the list of VFS IOCTLs.
>
> Signed-off-by: Günther Noack <gnoack@xxxxxxxxxx>
> ---
> include/uapi/linux/landlock.h | 55 ++++-
> security/landlock/fs.c | 227 ++++++++++++++++++-
> security/landlock/fs.h | 3 +
> security/landlock/limits.h | 11 +-
> security/landlock/ruleset.h | 2 +-
> security/landlock/syscalls.c | 19 +-
> tools/testing/selftests/landlock/base_test.c | 2 +-
> tools/testing/selftests/landlock/fs_test.c | 5 +-
> 8 files changed, 302 insertions(+), 22 deletions(-)
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 73997e63734f..84efea3f7c0f 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -84,6 +87,186 @@ static const struct landlock_object_underops landlock_fs_underops = {
> .release = release_inode
> };
>
> +/* IOCTL helpers */
> +
> +/*
> + * These are synthetic access rights, which are only used within the kernel, but
> + * not exposed to callers in userspace. The mapping between these access rights
> + * and IOCTL commands is defined in the get_required_ioctl_access() helper function.
> + */
> +#define LANDLOCK_ACCESS_FS_IOCTL_RW (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 1)
> +#define LANDLOCK_ACCESS_FS_IOCTL_RW_FILE (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 2)
> +
> +/* ioctl_groups - all synthetic access rights for IOCTL command groups */
> +/* clang-format off */
> +#define IOCTL_GROUPS ( \
> + LANDLOCK_ACCESS_FS_IOCTL_RW | \
> + LANDLOCK_ACCESS_FS_IOCTL_RW_FILE)
> +/* clang-format on */
> +
> +static_assert((IOCTL_GROUPS & LANDLOCK_MASK_ACCESS_FS) == IOCTL_GROUPS);
> +
> +/**
> + * get_required_ioctl_access(): Determine required IOCTL access rights.
> + *
> + * @cmd: The IOCTL command that is supposed to be run.
> + *
> + * Any new IOCTL commands that are implemented in fs/ioctl.c's do_vfs_ioctl()
> + * should be considered for inclusion here.
> + *
> + * Returns: The access rights that must be granted on an opened file in order to
> + * use the given @cmd.
> + */
> +static __attribute_const__ access_mask_t
> +get_required_ioctl_access(const unsigned int cmd)
> +{
> + switch (cmd) {
> + case FIOCLEX:
> + case FIONCLEX:
> + case FIONBIO:
> + case FIOASYNC:
> + /*
> + * FIOCLEX, FIONCLEX, FIONBIO and FIOASYNC manipulate the FD's
> + * close-on-exec and the file's buffered-IO and async flags.
> + * These operations are also available through fcntl(2), and are
> + * unconditionally permitted in Landlock.
> + */
> + return 0;
> + case FIONREAD:
> + case FIOQSIZE:
> + case FIGETBSZ:
> + /*
> + * FIONREAD returns the number of bytes available for reading.
> + * FIONREAD returns the number of immediately readable bytes for
> + * a file.
> + *
> + * FIOQSIZE queries the size of a file or directory.
> + *
> + * FIGETBSZ queries the file system's block size for a file or
> + * directory.
> + *
> + * These IOCTL commands are permitted for files which are opened
> + * with LANDLOCK_ACCESS_FS_READ_DIR,
> + * LANDLOCK_ACCESS_FS_READ_FILE, or
> + * LANDLOCK_ACCESS_FS_WRITE_FILE.
> + */
Because files or directories can only be opened with
LANDLOCK_ACCESS_FS_{READ,WRITE}_{FILE,DIR}, and because IOCTLs can only
be sent on a file descriptor, this means that we can always allow these
3 commands (for opened files).
> + return LANDLOCK_ACCESS_FS_IOCTL_RW;
> + case FS_IOC_FIEMAP:
> + case FIBMAP:
> + /*
> + * FS_IOC_FIEMAP and FIBMAP query information about the
> + * allocation of blocks within a file. They are permitted for
> + * files which are opened with LANDLOCK_ACCESS_FS_READ_FILE or
> + * LANDLOCK_ACCESS_FS_WRITE_FILE.
> + */
> + fallthrough;
> + case FIDEDUPERANGE:
> + case FICLONE:
> + case FICLONERANGE:
> + /*
> + * FIDEDUPERANGE, FICLONE and FICLONERANGE make files share
> + * their underlying storage ("reflink") between source and
> + * destination FDs, on file systems which support that.
> + *
> + * The underlying implementations are already checking whether
> + * the involved files are opened with the appropriate read/write
> + * modes. We rely on this being implemented correctly.
> + *
> + * These IOCTLs are permitted for files which are opened with
> + * LANDLOCK_ACCESS_FS_READ_FILE or
> + * LANDLOCK_ACCESS_FS_WRITE_FILE.
> + */
> + fallthrough;
> + case FS_IOC_RESVSP:
> + case FS_IOC_RESVSP64:
> + case FS_IOC_UNRESVSP:
> + case FS_IOC_UNRESVSP64:
> + case FS_IOC_ZERO_RANGE:
> + /*
> + * These IOCTLs reserve space, or create holes like
> + * fallocate(2). We rely on the implementations checking the
> + * files' read/write modes.
> + *
> + * These IOCTLs are permitted for files which are opened with
> + * LANDLOCK_ACCESS_FS_READ_FILE or
> + * LANDLOCK_ACCESS_FS_WRITE_FILE.
> + */
These 10 commands only make sense on directories, so we could also
always allow them on file descriptors.
> + return LANDLOCK_ACCESS_FS_IOCTL_RW_FILE;
> + default:
> + /*
> + * Other commands are guarded by the catch-all access right.
> + */
> + return LANDLOCK_ACCESS_FS_IOCTL;
> + }
> +}
> +
> +/**
> + * expand_ioctl() - Return the dst flags from either the src flag or the
> + * %LANDLOCK_ACCESS_FS_IOCTL flag, depending on whether the
> + * %LANDLOCK_ACCESS_FS_IOCTL and src access rights are handled or not.
> + *
> + * @handled: Handled access rights.
> + * @access: The access mask to copy values from.
> + * @src: A single access right to copy from in @access.
> + * @dst: One or more access rights to copy to.
> + *
> + * Returns: @dst, or 0.
> + */
> +static __attribute_const__ access_mask_t
> +expand_ioctl(const access_mask_t handled, const access_mask_t access,
> + const access_mask_t src, const access_mask_t dst)
> +{
> + access_mask_t copy_from;
> +
> + if (!(handled & LANDLOCK_ACCESS_FS_IOCTL))
> + return 0;
> +
> + copy_from = (handled & src) ? src : LANDLOCK_ACCESS_FS_IOCTL;
> + if (access & copy_from)
> + return dst;
> +
> + return 0;
> +}
> +
> +/**
> + * landlock_expand_access_fs() - Returns @access with the synthetic IOCTL group
> + * flags enabled if necessary.
> + *
> + * @handled: Handled FS access rights.
> + * @access: FS access rights to expand.
> + *
> + * Returns: @access expanded by the necessary flags for the synthetic IOCTL
> + * access rights.
> + */
> +static __attribute_const__ access_mask_t landlock_expand_access_fs(
> + const access_mask_t handled, const access_mask_t access)
> +{
> + return access |
> + expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_WRITE_FILE,
> + LANDLOCK_ACCESS_FS_IOCTL_RW |
> + LANDLOCK_ACCESS_FS_IOCTL_RW_FILE) |
> + expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_READ_FILE,
> + LANDLOCK_ACCESS_FS_IOCTL_RW |
> + LANDLOCK_ACCESS_FS_IOCTL_RW_FILE) |
> + expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_READ_DIR,
> + LANDLOCK_ACCESS_FS_IOCTL_RW);
> +}
> +
> +/**
> + * landlock_expand_handled_access_fs() - add synthetic IOCTL access rights to an
> + * access mask of handled accesses.
> + *
> + * @handled: The handled accesses of a ruleset that is being created.
> + *
> + * Returns: @handled, with the bits for the synthetic IOCTL access rights set,
> + * if %LANDLOCK_ACCESS_FS_IOCTL is handled.
> + */
> +__attribute_const__ access_mask_t
> +landlock_expand_handled_access_fs(const access_mask_t handled)
> +{
> + return landlock_expand_access_fs(handled, handled);
> +}
> +
> /* Ruleset management */
>
> static struct landlock_object *get_inode_object(struct inode *const inode)
> @@ -148,7 +331,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
> LANDLOCK_ACCESS_FS_EXECUTE | \
> LANDLOCK_ACCESS_FS_WRITE_FILE | \
> LANDLOCK_ACCESS_FS_READ_FILE | \
> - LANDLOCK_ACCESS_FS_TRUNCATE)
> + LANDLOCK_ACCESS_FS_TRUNCATE | \
> + LANDLOCK_ACCESS_FS_IOCTL)
> /* clang-format on */
>
> /*
> @@ -158,6 +342,7 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
> const struct path *const path,
> access_mask_t access_rights)
> {
> + access_mask_t handled;
> int err;
> struct landlock_id id = {
> .type = LANDLOCK_KEY_INODE,
> @@ -170,9 +355,11 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
> if (WARN_ON_ONCE(ruleset->num_layers != 1))
> return -EINVAL;
>
> + handled = landlock_get_fs_access_mask(ruleset, 0);
> + /* Expands the synthetic IOCTL groups. */
> + access_rights |= landlock_expand_access_fs(handled, access_rights);
> /* Transforms relative access rights to absolute ones. */
> - access_rights |= LANDLOCK_MASK_ACCESS_FS &
> - ~landlock_get_fs_access_mask(ruleset, 0);
> + access_rights |= LANDLOCK_MASK_ACCESS_FS & ~handled;
> id.key.object = get_inode_object(d_backing_inode(path->dentry));
> if (IS_ERR(id.key.object))
> return PTR_ERR(id.key.object);
> @@ -1333,7 +1520,9 @@ static int hook_file_open(struct file *const file)
> {
> layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
> access_mask_t open_access_request, full_access_request, allowed_access;
> - const access_mask_t optional_access = LANDLOCK_ACCESS_FS_TRUNCATE;
> + const access_mask_t optional_access = LANDLOCK_ACCESS_FS_TRUNCATE |
> + LANDLOCK_ACCESS_FS_IOCTL |
> + IOCTL_GROUPS;
> const struct landlock_ruleset *const dom = get_current_fs_domain();
>
> if (!dom)
We should set optional_access according to the file type before
`full_access_request = open_access_request | optional_access;`
const bool is_device = S_ISBLK(inode->i_mode) || S_ISCHR(inode->i_mode);
optional_access = LANDLOCK_ACCESS_FS_TRUNCATE;
if (is_device)
optional_access |= LANDLOCK_ACCESS_FS_IOCTL_DEV;
Because LANDLOCK_ACCESS_FS_IOCTL_DEV is dedicated to character or block
devices, we may want landlock_add_rule() to only allow this access right
to be tied to directories, or character devices, or block devices. Even
if it would be more consistent with constraints on directory-only access
rights, I'm not sure about that.
> @@ -1375,6 +1564,16 @@ static int hook_file_open(struct file *const file)
> }
> }
>
> + /*
> + * Named pipes should be treated just like anonymous pipes.
> + * Therefore, we permit all IOCTLs on them.
> + */
> + if (S_ISFIFO(file_inode(file)->i_mode)) {
> + allowed_access |= LANDLOCK_ACCESS_FS_IOCTL |
> + LANDLOCK_ACCESS_FS_IOCTL_RW |
> + LANDLOCK_ACCESS_FS_IOCTL_RW_FILE;
> + }
Instead of this S_ISFIFO check:
if (!is_device)
allowed_access |= LANDLOCK_ACCESS_FS_IOCTL_DEV;
> +
> /*
> * For operations on already opened files (i.e. ftruncate()), it is the
> * access rights at the time of open() which decide whether the
> @@ -1406,6 +1605,25 @@ static int hook_file_truncate(struct file *const file)
> return -EACCES;
> }
>
> +static int hook_file_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + const access_mask_t required_access = get_required_ioctl_access(cmd);
const access_mask_t required_access = LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + const access_mask_t allowed_access =
> + landlock_file(file)->allowed_access;
> +
> + /*
> + * It is the access rights at the time of opening the file which
> + * determine whether IOCTL can be used on the opened file later.
> + *
> + * The access right is attached to the opened file in hook_file_open().
> + */
> + if ((allowed_access & required_access) == required_access)
> + return 0;
We could then check against the do_vfs_ioctl()'s commands, excluding
FIONREAD and file_ioctl()'s commands, to always allow VFS-related
commands:
if (vfs_masked_device_ioctl(cmd))
return 0;
As a safeguard, we could define vfs_masked_device_ioctl(cmd) in
fs/ioctl.c and make it called by do_vfs_ioctl() as a safeguard to make
sure we keep an accurate list of VFS IOCTL commands (see next RFC patch).
The compat IOCTL hook must also be implemented.
What do you think? Any better idea?
> +
> + return -EACCES;
> +}
> +
> static struct security_hook_list landlock_hooks[] __ro_after_init = {
> LSM_HOOK_INIT(inode_free_security, hook_inode_free_security),
>
> @@ -1428,6 +1646,7 @@ static struct security_hook_list landlock_hooks[] __ro_after_init = {
