| Message ID | 20231208155121.1943775-5-gnoack@google.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Landlock: IOCTL support | expand |
On Fri, Dec 08, 2023 at 04:51:16PM +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 support IOCTLs like TIOCSTI and TIOCLINUX, which can be > used to control shell processes on the same terminal which run at > different privilege levels, which may make it possible to escape a > sandbox. Because stdin, stdout and stderr are normally inherited > rather than newly opened, IOCTLs are usually permitted on them even > after the Landlock policy is enforced. > > 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. > > 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. > > Signed-off-by: Günther Noack <gnoack@google.com> > --- > include/uapi/linux/landlock.h | 58 +++++- > security/landlock/fs.c | 176 ++++++++++++++++++- > security/landlock/fs.h | 2 + > 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, 253 insertions(+), 22 deletions(-) > > diff --git a/security/landlock/fs.c b/security/landlock/fs.c > index 9ba989ef46a5..81ce41e9e6db 100644 > --- a/security/landlock/fs.c > +++ b/security/landlock/fs.c > @@ -7,12 +7,14 @@ > * Copyright © 2021-2022 Microsoft Corporation > */ > > +#include <asm/ioctls.h> > #include <linux/atomic.h> > #include <linux/bitops.h> > #include <linux/bits.h> > #include <linux/compiler_types.h> > #include <linux/dcache.h> > #include <linux/err.h> > +#include <linux/falloc.h> > #include <linux/fs.h> > #include <linux/init.h> > #include <linux/kernel.h> > @@ -28,6 +30,7 @@ > #include <linux/types.h> > #include <linux/wait_bit.h> > #include <linux/workqueue.h> > +#include <uapi/linux/fiemap.h> > #include <uapi/linux/landlock.h> > > #include "common.h" > @@ -83,6 +86,145 @@ 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 required_ioctl_access() helper function. > + */ > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP1 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 1) > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP2 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 2) > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP3 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 3) > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP4 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 4) > + > +/* ioctl_groups - all synthetic access rights for IOCTL command groups */ > +/* clang-format off */ > +#define IOCTL_GROUPS ( \ > + LANDLOCK_ACCESS_FS_IOCTL_GROUP1 | \ > + LANDLOCK_ACCESS_FS_IOCTL_GROUP2 | \ > + LANDLOCK_ACCESS_FS_IOCTL_GROUP3 | \ > + LANDLOCK_ACCESS_FS_IOCTL_GROUP4) > +/* clang-format on */ > + > +static_assert((IOCTL_GROUPS & LANDLOCK_MASK_ACCESS_FS) == IOCTL_GROUPS); > + > +/** > + * required_ioctl_access(): Determine required IOCTL access rights. > + * > + * @cmd: The IOCTL command that is supposed to be run. > + * > + * Returns: The access rights that must be granted on an opened file in order to > + * use the given @cmd. > + */ > +static access_mask_t required_ioctl_access(unsigned int cmd) You can add __attribute_const__ after "static", and also constify 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 FIOQSIZE: > + return LANDLOCK_ACCESS_FS_IOCTL_GROUP1; > + case FS_IOC_FIEMAP: > + case FIBMAP: > + case FIGETBSZ: > + return LANDLOCK_ACCESS_FS_IOCTL_GROUP2; > + case FIONREAD: > + case FIDEDUPERANGE: > + return LANDLOCK_ACCESS_FS_IOCTL_GROUP3; > + case FICLONE: > + case FICLONERANGE: > + case FS_IOC_RESVSP: > + case FS_IOC_RESVSP64: > + case FS_IOC_UNRESVSP: > + case FS_IOC_UNRESVSP64: > + case FS_IOC_ZERO_RANGE: > + return LANDLOCK_ACCESS_FS_IOCTL_GROUP4; > + 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 access_mask_t expand_ioctl(const access_mask_t handled, static __attribute_const__ > + 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 access_mask_t landlock_expand_access_fs(const access_mask_t handled, static __attribute_const__ > + const access_mask_t access) > +{ > + return access | > + expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_WRITE_FILE, > + LANDLOCK_ACCESS_FS_IOCTL_GROUP1 | > + LANDLOCK_ACCESS_FS_IOCTL_GROUP2 | > + LANDLOCK_ACCESS_FS_IOCTL_GROUP4) | > + expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_READ_FILE, > + LANDLOCK_ACCESS_FS_IOCTL_GROUP1 | > + LANDLOCK_ACCESS_FS_IOCTL_GROUP2 | > + LANDLOCK_ACCESS_FS_IOCTL_GROUP3) | > + expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_READ_DIR, > + LANDLOCK_ACCESS_FS_IOCTL_GROUP1); > +} > + > +/** > + * 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. > + */ > +access_mask_t landlock_expand_handled_access_fs(const access_mask_t handled) __attribute_const__ access_mask_t > +{ > + return landlock_expand_access_fs(handled, handled); > +} > + > diff --git a/security/landlock/fs.h b/security/landlock/fs.h > index 488e4813680a..c88fe7bda37b 100644 > --- a/security/landlock/fs.h > +++ b/security/landlock/fs.h > @@ -92,4 +92,6 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset, > const struct path *const path, > access_mask_t access_hierarchy); > > +access_mask_t landlock_expand_handled_access_fs(const access_mask_t handled); __attribute_const__ access_mask_t > + > #endif /* _SECURITY_LANDLOCK_FS_H */
On Thu, Dec 14, 2023 at 10:26:49AM +0100, Mickaël Salaün wrote: > On Fri, Dec 08, 2023 at 04:51:16PM +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 support IOCTLs like TIOCSTI and TIOCLINUX, which can be > > used to control shell processes on the same terminal which run at > > different privilege levels, which may make it possible to escape a > > sandbox. Because stdin, stdout and stderr are normally inherited > > rather than newly opened, IOCTLs are usually permitted on them even > > after the Landlock policy is enforced. > > > > 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. > > > > 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. > > > > Signed-off-by: Günther Noack <gnoack@google.com> > > --- > > include/uapi/linux/landlock.h | 58 +++++- > > security/landlock/fs.c | 176 ++++++++++++++++++- > > security/landlock/fs.h | 2 + > > 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, 253 insertions(+), 22 deletions(-) > > > > > diff --git a/security/landlock/fs.c b/security/landlock/fs.c > > index 9ba989ef46a5..81ce41e9e6db 100644 > > --- a/security/landlock/fs.c > > +++ b/security/landlock/fs.c > > @@ -7,12 +7,14 @@ > > * Copyright © 2021-2022 Microsoft Corporation > > */ > > > > +#include <asm/ioctls.h> > > #include <linux/atomic.h> > > #include <linux/bitops.h> > > #include <linux/bits.h> > > #include <linux/compiler_types.h> > > #include <linux/dcache.h> > > #include <linux/err.h> > > +#include <linux/falloc.h> > > #include <linux/fs.h> > > #include <linux/init.h> > > #include <linux/kernel.h> > > @@ -28,6 +30,7 @@ > > #include <linux/types.h> > > #include <linux/wait_bit.h> > > #include <linux/workqueue.h> > > +#include <uapi/linux/fiemap.h> > > #include <uapi/linux/landlock.h> > > > > #include "common.h" > > @@ -83,6 +86,145 @@ 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 required_ioctl_access() helper function. > > + */ > > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP1 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 1) > > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP2 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 2) > > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP3 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 3) > > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP4 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 4) > > + > > +/* ioctl_groups - all synthetic access rights for IOCTL command groups */ > > +/* clang-format off */ > > +#define IOCTL_GROUPS ( \ > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP1 | \ > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP2 | \ > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP3 | \ > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP4) > > +/* clang-format on */ > > + > > +static_assert((IOCTL_GROUPS & LANDLOCK_MASK_ACCESS_FS) == IOCTL_GROUPS); > > + > > +/** > > + * required_ioctl_access(): Determine required IOCTL access rights. > > + * > > + * @cmd: The IOCTL command that is supposed to be run. > > + * > > + * Returns: The access rights that must be granted on an opened file in order to > > + * use the given @cmd. > > + */ > > +static access_mask_t required_ioctl_access(unsigned int cmd) Please use a verb for functions, something like get_required_ioctl_access(). > > You can add __attribute_const__ after "static", and also constify 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 FIOQSIZE: > > + return LANDLOCK_ACCESS_FS_IOCTL_GROUP1; > > + case FS_IOC_FIEMAP: > > + case FIBMAP: > > + case FIGETBSZ: > > + return LANDLOCK_ACCESS_FS_IOCTL_GROUP2; > > + case FIONREAD: > > + case FIDEDUPERANGE: > > + return LANDLOCK_ACCESS_FS_IOCTL_GROUP3; > > + case FICLONE: > > + case FICLONERANGE: > > + case FS_IOC_RESVSP: > > + case FS_IOC_RESVSP64: > > + case FS_IOC_UNRESVSP: > > + case FS_IOC_UNRESVSP64: > > + case FS_IOC_ZERO_RANGE: > > + return LANDLOCK_ACCESS_FS_IOCTL_GROUP4; > > + 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 access_mask_t expand_ioctl(const access_mask_t handled, > > static __attribute_const__ > > > + 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 access_mask_t landlock_expand_access_fs(const access_mask_t handled, > > static __attribute_const__ > > > + const access_mask_t access) > > +{ > > + return access | > > + expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_WRITE_FILE, > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP1 | > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP2 | > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP4) | > > + expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_READ_FILE, > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP1 | > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP2 | > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP3) | > > + expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_READ_DIR, > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP1); > > +} > > + > > +/** > > + * 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. > > + */ > > +access_mask_t landlock_expand_handled_access_fs(const access_mask_t handled) > > __attribute_const__ access_mask_t > > > +{ > > + return landlock_expand_access_fs(handled, handled); > > +} > > + > > > diff --git a/security/landlock/fs.h b/security/landlock/fs.h > > index 488e4813680a..c88fe7bda37b 100644 > > --- a/security/landlock/fs.h > > +++ b/security/landlock/fs.h > > @@ -92,4 +92,6 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset, > > const struct path *const path, > > access_mask_t access_hierarchy); > > > > +access_mask_t landlock_expand_handled_access_fs(const access_mask_t handled); > > __attribute_const__ access_mask_t > > > + > > #endif /* _SECURITY_LANDLOCK_FS_H */
Christian, what do you think about the following IOCTL groups? On Thu, Dec 14, 2023 at 11:14:10AM +0100, Mickaël Salaün wrote: > On Thu, Dec 14, 2023 at 10:26:49AM +0100, Mickaël Salaün wrote: > > On Fri, Dec 08, 2023 at 04:51:16PM +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 support IOCTLs like TIOCSTI and TIOCLINUX, which can be > > > used to control shell processes on the same terminal which run at > > > different privilege levels, which may make it possible to escape a > > > sandbox. Because stdin, stdout and stderr are normally inherited > > > rather than newly opened, IOCTLs are usually permitted on them even > > > after the Landlock policy is enforced. > > > > > > 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. > > > > > > 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. > > > > > > Signed-off-by: Günther Noack <gnoack@google.com> > > > --- > > > include/uapi/linux/landlock.h | 58 +++++- > > > security/landlock/fs.c | 176 ++++++++++++++++++- > > > security/landlock/fs.h | 2 + > > > 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, 253 insertions(+), 22 deletions(-) > > > > > > > > diff --git a/security/landlock/fs.c b/security/landlock/fs.c > > > index 9ba989ef46a5..81ce41e9e6db 100644 > > > --- a/security/landlock/fs.c > > > +++ b/security/landlock/fs.c > > > @@ -7,12 +7,14 @@ > > > * Copyright © 2021-2022 Microsoft Corporation > > > */ > > > > > > +#include <asm/ioctls.h> > > > #include <linux/atomic.h> > > > #include <linux/bitops.h> > > > #include <linux/bits.h> > > > #include <linux/compiler_types.h> > > > #include <linux/dcache.h> > > > #include <linux/err.h> > > > +#include <linux/falloc.h> > > > #include <linux/fs.h> > > > #include <linux/init.h> > > > #include <linux/kernel.h> > > > @@ -28,6 +30,7 @@ > > > #include <linux/types.h> > > > #include <linux/wait_bit.h> > > > #include <linux/workqueue.h> > > > +#include <uapi/linux/fiemap.h> > > > #include <uapi/linux/landlock.h> > > > > > > #include "common.h" > > > @@ -83,6 +86,145 @@ 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 required_ioctl_access() helper function. > > > + */ > > > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP1 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 1) > > > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP2 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 2) > > > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP3 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 3) > > > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP4 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 4) > > > + > > > +/* ioctl_groups - all synthetic access rights for IOCTL command groups */ > > > +/* clang-format off */ > > > +#define IOCTL_GROUPS ( \ > > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP1 | \ > > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP2 | \ > > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP3 | \ > > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP4) > > > +/* clang-format on */ > > > + > > > +static_assert((IOCTL_GROUPS & LANDLOCK_MASK_ACCESS_FS) == IOCTL_GROUPS); > > > + > > > +/** > > > + * required_ioctl_access(): Determine required IOCTL access rights. > > > + * > > > + * @cmd: The IOCTL command that is supposed to be run. > > > + * > > > + * Returns: The access rights that must be granted on an opened file in order to > > > + * use the given @cmd. > > > + */ > > > +static access_mask_t required_ioctl_access(unsigned int cmd) > > Please use a verb for functions, something like > get_required_ioctl_access(). > > > > > You can add __attribute_const__ after "static", and also constify 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; Could you please add comments for the following IOCTL commands explaining why they make sense for the related file/dir read/write mapping? We discussed about that in the ML but it would be much easier to put that doc here for future changes, and for reviewers to understand the rationale. Some of this doc is already in the cover letter. To make this easier to follow, what about renaming the IOCTL groups to something like this: * LANDLOCK_ACCESS_FS_IOCTL_GROUP1: LANDLOCK_ACCESS_FS_IOCTL_GET_SIZE * LANDLOCK_ACCESS_FS_IOCTL_GROUP2: LANDLOCK_ACCESS_FS_IOCTL_GET_INNER * LANDLOCK_ACCESS_FS_IOCTL_GROUP3: LANDLOCK_ACCESS_FS_IOCTL_READ_FILE * LANDLOCK_ACCESS_FS_IOCTL_GROUP4: LANDLOCK_ACCESS_FS_IOCTL_WRITE_FILE > > > + case FIOQSIZE: > > > + return LANDLOCK_ACCESS_FS_IOCTL_GROUP1; > > > + case FS_IOC_FIEMAP: > > > + case FIBMAP: > > > + case FIGETBSZ: Does it make sense to not include FIGETBSZ in LANDLOCK_ACCESS_FS_IOCTL_GROUP1? I think it's OK like this as previously explained but I'd like to get confirmation: https://lore.kernel.org/r/20230904.aiWae8eineo4@digikod.net > > > + return LANDLOCK_ACCESS_FS_IOCTL_GROUP2; > > > + case FIONREAD: > > > + case FIDEDUPERANGE: > > > + return LANDLOCK_ACCESS_FS_IOCTL_GROUP3; > > > + case FICLONE: > > > + case FICLONERANGE: The FICLONE* commands seems to already check read/write permissions with generic_file_rw_checks(). Always allowing them should then be OK (and the current tests should still pass), but we can still keep them here to make the required access right explicit and test with and without Landlock restrictions to make sure this is consistent with the VFS access checks. See https://lore.kernel.org/r/20230904.aiWae8eineo4@digikod.net If this is correct, a new test should check that Landlock restrictions are the same as the VFS checks and then don't impact such IOCTLs. > > > + case FS_IOC_RESVSP: > > > + case FS_IOC_RESVSP64: > > > + case FS_IOC_UNRESVSP: > > > + case FS_IOC_UNRESVSP64: > > > + case FS_IOC_ZERO_RANGE: > > > + return LANDLOCK_ACCESS_FS_IOCTL_GROUP4; > > > + default: > > > + /* > > > + * Other commands are guarded by the catch-all access right. > > > + */ > > > + return LANDLOCK_ACCESS_FS_IOCTL; > > > + } > > > +} We previously talked about allowing all IOCTLs on unix sockets and named pipes: https://lore.kernel.org/r/ZP7lxmXklksadvz+@google.com I think the remaining issue with this grouping is that if the VFS implementation returns -ENOIOCTLCMD, then the IOCTL command can be forwarded to the device driver (for character or block devices). For instance, FIONREAD on a character device could translate to unknown action (on this device), which should then be considered dangerous and denied unless explicitly allowed with LANDLOCK_ACCESS_FS_IOCTL (but not any IOCTL_GROUP*). For instance, FIONREAD on /dev/null should return -ENOTTY, which should then also be the case if LANDLOCK_ACCESS_FS_IOCTL is allowed (even if LANDLOCK_ACCESS_FS_READ_FILE is denied). This is also the case for file_ioctl()'s commands. One solution to implement this logic would be to add an additional check in hook_file_ioctl() for specific file types (!S_ISREG or socket or pipe exceptions) and IOCTL commands. Christian, is it correct to say that device drivers are not "required" to follow the same semantic as the VFS's IOCTLs and that (for whatever reason) collisions may occur? I guess this is not the case for filesystems, which should implement similar semantic for the same IOCTLs. > > > + > > > +/** > > > + * 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 access_mask_t expand_ioctl(const access_mask_t handled, > > > > static __attribute_const__ > > > > > + 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 access_mask_t landlock_expand_access_fs(const access_mask_t handled, > > > > static __attribute_const__ > > > > > + const access_mask_t access) > > > +{ > > > + return access | > > > + expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_WRITE_FILE, > > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP1 | > > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP2 | > > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP4) | > > > + expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_READ_FILE, > > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP1 | > > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP2 | > > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP3) | > > > + expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_READ_DIR, > > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP1); > > > +} > > > + > > > +/** > > > + * 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. > > > + */ > > > +access_mask_t landlock_expand_handled_access_fs(const access_mask_t handled) > > > > __attribute_const__ access_mask_t > > > > > +{ > > > + return landlock_expand_access_fs(handled, handled); > > > +} > > > + > > > > > diff --git a/security/landlock/fs.h b/security/landlock/fs.h > > > index 488e4813680a..c88fe7bda37b 100644 > > > --- a/security/landlock/fs.h > > > +++ b/security/landlock/fs.h > > > @@ -92,4 +92,6 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset, > > > const struct path *const path, > > > access_mask_t access_hierarchy); > > > > > > +access_mask_t landlock_expand_handled_access_fs(const access_mask_t handled); > > > > __attribute_const__ access_mask_t > > > > > + > > > #endif /* _SECURITY_LANDLOCK_FS_H */
Hello! On Thu, Dec 14, 2023 at 03:28:10PM +0100, Mickaël Salaün wrote: > Christian, what do you think about the following IOCTL groups? > > On Thu, Dec 14, 2023 at 11:14:10AM +0100, Mickaël Salaün wrote: > > On Thu, Dec 14, 2023 at 10:26:49AM +0100, Mickaël Salaün wrote: > > > On Fri, Dec 08, 2023 at 04:51:16PM +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 support IOCTLs like TIOCSTI and TIOCLINUX, which can be > > > > used to control shell processes on the same terminal which run at > > > > different privilege levels, which may make it possible to escape a > > > > sandbox. Because stdin, stdout and stderr are normally inherited > > > > rather than newly opened, IOCTLs are usually permitted on them even > > > > after the Landlock policy is enforced. > > > > > > > > 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. > > > > > > > > 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. > > > > > > > > Signed-off-by: Günther Noack <gnoack@google.com> > > > > --- > > > > include/uapi/linux/landlock.h | 58 +++++- > > > > security/landlock/fs.c | 176 ++++++++++++++++++- > > > > security/landlock/fs.h | 2 + > > > > 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, 253 insertions(+), 22 deletions(-) > > > > > > > > > > > diff --git a/security/landlock/fs.c b/security/landlock/fs.c > > > > index 9ba989ef46a5..81ce41e9e6db 100644 > > > > --- a/security/landlock/fs.c > > > > +++ b/security/landlock/fs.c > > > > @@ -7,12 +7,14 @@ > > > > * Copyright © 2021-2022 Microsoft Corporation > > > > */ > > > > > > > > +#include <asm/ioctls.h> > > > > #include <linux/atomic.h> > > > > #include <linux/bitops.h> > > > > #include <linux/bits.h> > > > > #include <linux/compiler_types.h> > > > > #include <linux/dcache.h> > > > > #include <linux/err.h> > > > > +#include <linux/falloc.h> > > > > #include <linux/fs.h> > > > > #include <linux/init.h> > > > > #include <linux/kernel.h> > > > > @@ -28,6 +30,7 @@ > > > > #include <linux/types.h> > > > > #include <linux/wait_bit.h> > > > > #include <linux/workqueue.h> > > > > +#include <uapi/linux/fiemap.h> > > > > #include <uapi/linux/landlock.h> > > > > > > > > #include "common.h" > > > > @@ -83,6 +86,145 @@ 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 required_ioctl_access() helper function. > > > > + */ > > > > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP1 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 1) > > > > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP2 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 2) > > > > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP3 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 3) > > > > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP4 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 4) > > > > + > > > > +/* ioctl_groups - all synthetic access rights for IOCTL command groups */ > > > > +/* clang-format off */ > > > > +#define IOCTL_GROUPS ( \ > > > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP1 | \ > > > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP2 | \ > > > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP3 | \ > > > > + LANDLOCK_ACCESS_FS_IOCTL_GROUP4) > > > > +/* clang-format on */ > > > > + > > > > +static_assert((IOCTL_GROUPS & LANDLOCK_MASK_ACCESS_FS) == IOCTL_GROUPS); > > > > + > > > > +/** > > > > + * required_ioctl_access(): Determine required IOCTL access rights. > > > > + * > > > > + * @cmd: The IOCTL command that is supposed to be run. > > > > + * > > > > + * Returns: The access rights that must be granted on an opened file in order to > > > > + * use the given @cmd. > > > > + */ > > > > +static access_mask_t required_ioctl_access(unsigned int cmd) > > > > Please use a verb for functions, something like > > get_required_ioctl_access(). > > > > > > > > You can add __attribute_const__ after "static", and also constify 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; > > Could you please add comments for the following IOCTL commands > explaining why they make sense for the related file/dir read/write > mapping? We discussed about that in the ML but it would be much easier > to put that doc here for future changes, and for reviewers to understand > the rationale. Some of this doc is already in the cover letter. Done, I'm adding documentation inline here. > > To make this easier to follow, what about renaming the IOCTL groups to > something like this: > * LANDLOCK_ACCESS_FS_IOCTL_GROUP1: > LANDLOCK_ACCESS_FS_IOCTL_GET_SIZE > * LANDLOCK_ACCESS_FS_IOCTL_GROUP2: > LANDLOCK_ACCESS_FS_IOCTL_GET_INNER > * LANDLOCK_ACCESS_FS_IOCTL_GROUP3: > LANDLOCK_ACCESS_FS_IOCTL_READ_FILE > * LANDLOCK_ACCESS_FS_IOCTL_GROUP4: > LANDLOCK_ACCESS_FS_IOCTL_WRITE_FILE Agreed that better names are in order here. I renamed them as you suggested. In principle, it would have been nice to name them after the access rights which enable them, but LANDLOCK_ACCESS_FS_IOCTL_READ_DIR_OR_READ_FILE_OR_WRITE_FILE is a bit too long for my taste. o_O > > > > + case FIOQSIZE: > > > > + return LANDLOCK_ACCESS_FS_IOCTL_GROUP1; > > > > + case FS_IOC_FIEMAP: > > > > + case FIBMAP: > > > > + case FIGETBSZ: > > Does it make sense to not include FIGETBSZ in > LANDLOCK_ACCESS_FS_IOCTL_GROUP1? I think it's OK like this as previously > explained but I'd like to get confirmation: > https://lore.kernel.org/r/20230904.aiWae8eineo4@digikod.net It seems that the more standardized way to get file system block sizes is to use POSIX' statvfs(3) interface, whose functionality is provided through the statfs(2) syscall. These functions have the usual path-based and fd-based variants. Landlock does not currently restrict statfs(2) at all, but there is an existing LSM security hook for it. We should probably introduce an access right to restrict statfs(2) in the future, because this otherwise lets callers probe for the existence of files. I filed https://github.com/landlock-lsm/linux/issues/18 for it. I am not sure how to group this best. It seems like a very harmless thing to allow. (What is to be learned from the filesystem blocksize anyway?) If we are unsure about it, we could do the following though: - disallow FIGETBSZ unless LANDLOCK_ACCESS_FS_IOCTL ("misc") is granted - allow FIGETBSZ together with a future access right which controls statfs(2) In that case, the use of FIGETBSZ would be nicely separable from regular read access for files, and it would be associated with the same right. (We could also potentially group FS_IOC_FIEMAP and FIBMAP in the same way. These ones give information about file extents and a file's block numbers. (You can check whether your file is stored in a continuous area on disk.)) This would simplify the story somewhat for the IOCTLs that we need to immediately give access to. What do you think? > > > > + return LANDLOCK_ACCESS_FS_IOCTL_GROUP2; > > > > + case FIONREAD: > > > > + case FIDEDUPERANGE: > > > > + return LANDLOCK_ACCESS_FS_IOCTL_GROUP3; > > > > + case FICLONE: > > > > + case FICLONERANGE: > > The FICLONE* commands seems to already check read/write permissions with > generic_file_rw_checks(). Always allowing them should then be OK (and > the current tests should still pass), but we can still keep them here to > make the required access right explicit and test with and without > Landlock restrictions to make sure this is consistent with the VFS > access checks. See > https://lore.kernel.org/r/20230904.aiWae8eineo4@digikod.net > If this is correct, a new test should check that Landlock restrictions > are the same as the VFS checks and then don't impact such IOCTLs. Noted. I'll look into it. (My understanding of FICLONE, FIDEDUPRANGE and FICLONERANGE is that they let files share the same underlying storage, on a per-range basis ("reflink"). The IOCTL man pages for these do not explain that as explicitly, but the key point is that the two resulting files still behave like a regular copy, because this feature exists on COW file systems only. So that reinforces the approach of using READ_FILE and WRITE_FILE access rights for these IOCTL commands (because it behaves just as if we had called read() on one file and written the results to the other file with write()).) > > > > + case FS_IOC_RESVSP: > > > > + case FS_IOC_RESVSP64: > > > > + case FS_IOC_UNRESVSP: > > > > + case FS_IOC_UNRESVSP64: > > > > + case FS_IOC_ZERO_RANGE: > > > > + return LANDLOCK_ACCESS_FS_IOCTL_GROUP4; > > > > + default: > > > > + /* > > > > + * Other commands are guarded by the catch-all access right. > > > > + */ > > > > + return LANDLOCK_ACCESS_FS_IOCTL; > > > > + } > > > > +} > We previously talked about allowing all IOCTLs on unix sockets and named > pipes: https://lore.kernel.org/r/ZP7lxmXklksadvz+@google.com Thanks for the reminder, I missed that. Putting it on the TODO list. > I think the remaining issue with this grouping is that if the VFS > implementation returns -ENOIOCTLCMD, then the IOCTL command can be > forwarded to the device driver (for character or block devices). > For instance, FIONREAD on a character device could translate to unknown > action (on this device), which should then be considered dangerous and > denied unless explicitly allowed with LANDLOCK_ACCESS_FS_IOCTL (but not > any IOCTL_GROUP*). > > For instance, FIONREAD on /dev/null should return -ENOTTY, which should > then also be the case if LANDLOCK_ACCESS_FS_IOCTL is allowed (even if > LANDLOCK_ACCESS_FS_READ_FILE is denied). This is also the case for > file_ioctl()'s commands. > > One solution to implement this logic would be to add an additional check > in hook_file_ioctl() for specific file types (!S_ISREG or socket or pipe > exceptions) and IOCTL commands. In my view this seems OK, because we are primarily protecting access to resources (files), and only secondarily reducing the exposed kernel attack surface. I agree there is a certain risk associated with calling ioctl(fd, FIONREAD, ...) on a buggy device driver. But then again, that risk is comparable to the risk of calling read(fd, &buf, buflen) on the same buggy device driver. So the LANDLOCK_ACCESS_FS_READ_FILE right grants access to both. Users who are concerned about the security of specific device drivers can enforce a policy where only the necessary device files can be opened. Does that make sense? (Otherwise, if it makes you feel better, we can also change it so that these IOCTL commands require LANDLOCK_ACCESS_FS_IOCTL if they are used on non-S_ISREG files. But it would complicate the IOCTL logic a bit, which we are exposing to users.) > Christian, is it correct to say that device drivers are not "required" > to follow the same semantic as the VFS's IOCTLs and that (for whatever > reason) collisions may occur? I guess this is not the case for > filesystems, which should implement similar semantic for the same > IOCTLs. Christian, friendly ping! :) Do you have opinions on this? If the Landlock LSM makes decisions based on the IOCTL command numbers, do we have to assume that underlying device drivers might expose different functionality under the same IOCTL command numbers? Thanks, —Günther
On Tue, Jan 30, 2024 at 07:13:25PM +0100, Günther Noack wrote: > Hello! > > On Thu, Dec 14, 2023 at 03:28:10PM +0100, Mickaël Salaün wrote: > > Christian, what do you think about the following IOCTL groups? > > > > On Thu, Dec 14, 2023 at 11:14:10AM +0100, Mickaël Salaün wrote: > > > On Thu, Dec 14, 2023 at 10:26:49AM +0100, Mickaël Salaün wrote: > > > > On Fri, Dec 08, 2023 at 04:51:16PM +0100, Günther Noack wrote: > > > > > + 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; > > > > Could you please add comments for the following IOCTL commands > > explaining why they make sense for the related file/dir read/write > > mapping? We discussed about that in the ML but it would be much easier > > to put that doc here for future changes, and for reviewers to understand > > the rationale. Some of this doc is already in the cover letter. > > Done, I'm adding documentation inline here. > > > > > To make this easier to follow, what about renaming the IOCTL groups to > > something like this: > > * LANDLOCK_ACCESS_FS_IOCTL_GROUP1: > > LANDLOCK_ACCESS_FS_IOCTL_GET_SIZE Well, this looks better: LANDLOCK_ACCESS_FS_IOCTL_RW We could think that it includes LANDLOCK_ACCESS_FS_MAKE_* though (which is not the case), but it looks like the least worst... These synthetic access rights are not public anyway. > > * LANDLOCK_ACCESS_FS_IOCTL_GROUP2: > > LANDLOCK_ACCESS_FS_IOCTL_GET_INNER LANDLOCK_ACCESS_FS_IOCTL_RW_FILE > > * LANDLOCK_ACCESS_FS_IOCTL_GROUP3: > > LANDLOCK_ACCESS_FS_IOCTL_READ_FILE LANDLOCK_ACCESS_FS_IOCTL_R_FILE > > * LANDLOCK_ACCESS_FS_IOCTL_GROUP4: > > LANDLOCK_ACCESS_FS_IOCTL_WRITE_FILE LANDLOCK_ACCESS_FS_IOCTL_W_FILE > > Agreed that better names are in order here. > I renamed them as you suggested. > > In principle, it would have been nice to name them after the access rights which > enable them, but LANDLOCK_ACCESS_FS_IOCTL_READ_DIR_OR_READ_FILE_OR_WRITE_FILE is > a bit too long for my taste. o_O > > > > > > > + case FIOQSIZE: > > > > > + return LANDLOCK_ACCESS_FS_IOCTL_GROUP1; > > > > > + case FS_IOC_FIEMAP: > > > > > + case FIBMAP: > > > > > + case FIGETBSZ: > > > > Does it make sense to not include FIGETBSZ in > > LANDLOCK_ACCESS_FS_IOCTL_GROUP1? I think it's OK like this as previously I guess I meant "Does it make sense *to include* FIGETBSZ in LANDLOCK_ACCESS_FS_IOCTL_GROUP1?" Which means to allow it for the write_file, read_file and read_dir access rights: LANDLOCK_ACCESS_FS_IOCTL_RW. > > explained but I'd like to get confirmation: > > https://lore.kernel.org/r/20230904.aiWae8eineo4@digikod.net > > It seems that the more standardized way to get file system block sizes is to use > POSIX' statvfs(3) interface, whose functionality is provided through the > statfs(2) syscall. These functions have the usual path-based and fd-based > variants. Landlock does not currently restrict statfs(2) at all, but there is > an existing LSM security hook for it. > > We should probably introduce an access right to restrict statfs(2) in the > future, because this otherwise lets callers probe for the existence of files. I > filed https://github.com/landlock-lsm/linux/issues/18 for it. According to the struct statfs fields, most of them seems to be useful for file writes (e.g. f_bsize) and file creations (e.g. f_ffree) but potentially for file read too (e.g. f_bsize). I'm not sure how statfs is used in practice though. > > I am not sure how to group this best. It seems like a very harmless thing to > allow. (What is to be learned from the filesystem blocksize anyway?) If we are > unsure about it, we could do the following though: I agree that it seems to be harmless. When we'll be able to control statfs(2), following the same logic, LANDLOCK_ACCESS_FS_IOCTL_RW should allows to use it. To said it another way, implementing the statfs LSM hook doesn't seem to be useful once we get the ability to restrict path walks: https://github.com/landlock-lsm/linux/issues/9 Until then, there are other ways to probe for file existence anyway. > > - disallow FIGETBSZ unless LANDLOCK_ACCESS_FS_IOCTL ("misc") is granted > - allow FIGETBSZ together with a future access right which controls statfs(2) > > In that case, the use of FIGETBSZ would be nicely separable from regular read > access for files, and it would be associated with the same right. If this FIGETBSZ is legitimately used by applications to optimize their FS interactions, then we should not mask it under FS_IOCTL because this would result of applications allowing FS_IOCTL everywhere, which is not what we want. > > (We could also potentially group FS_IOC_FIEMAP and FIBMAP in the same way. > These ones give information about file extents and a file's block numbers. (You > can check whether your file is stored in a continuous area on disk.)) > > This would simplify the story somewhat for the IOCTLs that we need to > immediately give access to. > > What do you think? It would help to trace a lot of generic applications and see which IOCTL command are used in practice, we might discover new ones. > > > > > > > + return LANDLOCK_ACCESS_FS_IOCTL_GROUP2; > > > > > + case FIONREAD: > > > > > + case FIDEDUPERANGE: > > > > > + return LANDLOCK_ACCESS_FS_IOCTL_GROUP3; > > > > > + case FICLONE: > > > > > + case FICLONERANGE: > > > > The FICLONE* commands seems to already check read/write permissions with > > generic_file_rw_checks(). Always allowing them should then be OK (and > > the current tests should still pass), but we can still keep them here to > > make the required access right explicit and test with and without > > Landlock restrictions to make sure this is consistent with the VFS > > access checks. See > > https://lore.kernel.org/r/20230904.aiWae8eineo4@digikod.net > > If this is correct, a new test should check that Landlock restrictions > > are the same as the VFS checks and then don't impact such IOCTLs. > > Noted. I'll look into it. > > (My understanding of FICLONE, FIDEDUPRANGE and FICLONERANGE is that they let > files share the same underlying storage, on a per-range basis ("reflink"). The > IOCTL man pages for these do not explain that as explicitly, but the key point > is that the two resulting files still behave like a regular copy, because this > feature exists on COW file systems only. So that reinforces the approach of > using READ_FILE and WRITE_FILE access rights for these IOCTL commands (because > it behaves just as if we had called read() on one file and written the results > to the other file with write()).) > > > > > > > + case FS_IOC_RESVSP: > > > > > + case FS_IOC_RESVSP64: > > > > > + case FS_IOC_UNRESVSP: > > > > > + case FS_IOC_UNRESVSP64: > > > > > + case FS_IOC_ZERO_RANGE: > > > > > + return LANDLOCK_ACCESS_FS_IOCTL_GROUP4; > > > > > + default: > > > > > + /* > > > > > + * Other commands are guarded by the catch-all access right. > > > > > + */ > > > > > + return LANDLOCK_ACCESS_FS_IOCTL; > > > > > + } > > > > > +} > > > We previously talked about allowing all IOCTLs on unix sockets and named > > pipes: https://lore.kernel.org/r/ZP7lxmXklksadvz+@google.com > > Thanks for the reminder, I missed that. Putting it on the TODO list. > > > > I think the remaining issue with this grouping is that if the VFS > > implementation returns -ENOIOCTLCMD, then the IOCTL command can be > > forwarded to the device driver (for character or block devices). > > For instance, FIONREAD on a character device could translate to unknown > > action (on this device), which should then be considered dangerous and > > denied unless explicitly allowed with LANDLOCK_ACCESS_FS_IOCTL (but not > > any IOCTL_GROUP*). > > > > For instance, FIONREAD on /dev/null should return -ENOTTY, which should > > then also be the case if LANDLOCK_ACCESS_FS_IOCTL is allowed (even if > > LANDLOCK_ACCESS_FS_READ_FILE is denied). This is also the case for > > file_ioctl()'s commands. > > > > One solution to implement this logic would be to add an additional check > > in hook_file_ioctl() for specific file types (!S_ISREG or socket or pipe > > exceptions) and IOCTL commands. > > In my view this seems OK, because we are primarily protecting access to > resources (files), and only secondarily reducing the exposed kernel attack > surface. Correct, but of course block and character devices need to be handled. seccomp-bpf is the main tool to protect the kernel in this case. > > I agree there is a certain risk associated with calling ioctl(fd, FIONREAD, ...) > on a buggy device driver. But then again, that risk is comparable to the risk > of calling read(fd, &buf, buflen) on the same buggy device driver. So the > LANDLOCK_ACCESS_FS_READ_FILE right grants access to both. Users who are > concerned about the security of specific device drivers can enforce a policy > where only the necessary device files can be opened. I'm thinking about the case where the FIONREAD value is used by a device with a completely different semantic. This should be a bad practice but this could happen, right Christian? > > Does that make sense? > > (Otherwise, if it makes you feel better, we can also change it so that these > IOCTL commands require LANDLOCK_ACCESS_FS_IOCTL if they are used on non-S_ISREG > files. But it would complicate the IOCTL logic a bit, which we are exposing to > users.) I think I'd prefer this approach. I'm not sure how special files (but still S_ISREG) are handled though, nor if this could be an issue. > > > > Christian, is it correct to say that device drivers are not "required" > > to follow the same semantic as the VFS's IOCTLs and that (for whatever > > reason) collisions may occur? I guess this is not the case for > > filesystems, which should implement similar semantic for the same > > IOCTLs. > > Christian, friendly ping! :) Do you have opinions on this? > > If the Landlock LSM makes decisions based on the IOCTL command numbers, do we > have to assume that underlying device drivers might expose different > functionality under the same IOCTL command numbers? > > Thanks, > —Günther >
diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h index 25c8d7677539..578f268b084b 100644 --- a/include/uapi/linux/landlock.h +++ b/include/uapi/linux/landlock.h @@ -128,7 +128,7 @@ struct landlock_net_port_attr { * files and directories. Files or directories opened before the sandboxing * are not subject to these restrictions. * - * A file can only receive these access rights: + * The following access rights apply only to files: * * - %LANDLOCK_ACCESS_FS_EXECUTE: Execute a file. * - %LANDLOCK_ACCESS_FS_WRITE_FILE: Open a file with write access. Note that @@ -138,12 +138,13 @@ struct landlock_net_port_attr { * - %LANDLOCK_ACCESS_FS_READ_FILE: Open a file with read access. * - %LANDLOCK_ACCESS_FS_TRUNCATE: Truncate a file with :manpage:`truncate(2)`, * :manpage:`ftruncate(2)`, :manpage:`creat(2)`, or :manpage:`open(2)` with - * ``O_TRUNC``. Whether an opened file can be truncated with - * :manpage:`ftruncate(2)` is determined during :manpage:`open(2)`, in the - * same way as read and write permissions are checked during - * :manpage:`open(2)` using %LANDLOCK_ACCESS_FS_READ_FILE and - * %LANDLOCK_ACCESS_FS_WRITE_FILE. This access right is available since the - * third version of the Landlock ABI. + * ``O_TRUNC``. This access right is available since the third version of the + * Landlock ABI. + * + * Whether an opened file can be truncated with :manpage:`ftruncate(2)` or used + * with `ioctl(2)` is determined during :manpage:`open(2)`, in the same way as + * read and write permissions are checked during :manpage:`open(2)` using + * %LANDLOCK_ACCESS_FS_READ_FILE and %LANDLOCK_ACCESS_FS_WRITE_FILE. * * A directory can receive access rights related to files or directories. The * following access right is applied to the directory itself, and the @@ -198,13 +199,53 @@ struct landlock_net_port_attr { * If multiple requirements are not met, the ``EACCES`` error code takes * precedence over ``EXDEV``. * + * The following access right applies both to files and directories: + * + * - %LANDLOCK_ACCESS_FS_IOCTL: Invoke :manpage:`ioctl(2)` commands on an opened + * file or directory. + * + * This access right applies to all :manpage:`ioctl(2)` commands, except of + * ``FIOCLEX``, ``FIONCLEX``, ``FIONBIO`` and ``FIOASYNC``. These commands + * continue to be invokable independent of the %LANDLOCK_ACCESS_FS_IOCTL + * access right. + * + * When certain other access rights are handled in the ruleset, in addition to + * %LANDLOCK_ACCESS_FS_IOCTL, granting these access rights will unlock access + * to additional groups of IOCTL commands, on the affected files: + * + * * %LANDLOCK_ACCESS_FS_READ_FILE unlocks access to ``FIOQSIZE``, + * ``FS_IOC_FIEMAP``, ``FIBMAP``, ``FIGETBSZ``, ``FIONREAD``, + * ``FIDEDUPRANGE``. + * + * * %LANDLOCK_ACCESS_FS_WRITE_FILE unlocks access to ``FIOQSIZE``, + * ``FS_IOC_FIEMAP``, ``FIBMAP``, ``FIGETBSZ``, ``FICLONE``, + * ``FICLONERANGE``, ``FS_IOC_RESVSP``, ``FS_IOC_RESVSP64``, + * ``FS_IOC_UNRESVSP``, ``FS_IOC_UNRESVSP64``, ``FS_IOC_ZERO_RANGE``. + * + * * %LANDLOCK_ACCESS_FS_READ_DIR unlocks access to ``FIOQSIZE``, + * ``FS_IOC_FIEMAP``, ``FIBMAP``, ``FIGETBSZ``. + * + * When these access rights are handled in the ruleset, the availability of + * the affected IOCTL commands is not governed by %LANDLOCK_ACCESS_FS_IOCTL + * any more, but by the respective access right. + * + * All other IOCTL commands are not handled specially, and are governed by + * %LANDLOCK_ACCESS_FS_IOCTL. This includes %FS_IOC_GETFLAGS and + * %FS_IOC_SETFLAGS for manipulating inode flags (:manpage:`ioctl_iflags(2)`), + * %FS_IOC_FSFETXATTR and %FS_IOC_FSSETXATTR for manipulating extended + * attributes, as well as %FIFREEZE and %FITHAW for freezing and thawing file + * systems. + * + * This access right is available since the fifth version of the Landlock + * ABI. + * * .. warning:: * * It is currently not possible to restrict some file-related actions * accessible through these syscall families: :manpage:`chdir(2)`, * :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`, * :manpage:`chown(2)`, :manpage:`setxattr(2)`, :manpage:`utime(2)`, - * :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, :manpage:`access(2)`. + * :manpage:`fcntl(2)`, :manpage:`access(2)`. * Future Landlock evolutions will enable to restrict them. */ /* clang-format off */ @@ -223,6 +264,7 @@ struct landlock_net_port_attr { #define LANDLOCK_ACCESS_FS_MAKE_SYM (1ULL << 12) #define LANDLOCK_ACCESS_FS_REFER (1ULL << 13) #define LANDLOCK_ACCESS_FS_TRUNCATE (1ULL << 14) +#define LANDLOCK_ACCESS_FS_IOCTL (1ULL << 15) /* clang-format on */ /** diff --git a/security/landlock/fs.c b/security/landlock/fs.c index 9ba989ef46a5..81ce41e9e6db 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -7,12 +7,14 @@ * Copyright © 2021-2022 Microsoft Corporation */ +#include <asm/ioctls.h> #include <linux/atomic.h> #include <linux/bitops.h> #include <linux/bits.h> #include <linux/compiler_types.h> #include <linux/dcache.h> #include <linux/err.h> +#include <linux/falloc.h> #include <linux/fs.h> #include <linux/init.h> #include <linux/kernel.h> @@ -28,6 +30,7 @@ #include <linux/types.h> #include <linux/wait_bit.h> #include <linux/workqueue.h> +#include <uapi/linux/fiemap.h> #include <uapi/linux/landlock.h> #include "common.h" @@ -83,6 +86,145 @@ 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 required_ioctl_access() helper function. + */ +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP1 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 1) +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP2 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 2) +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP3 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 3) +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP4 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 4) + +/* ioctl_groups - all synthetic access rights for IOCTL command groups */ +/* clang-format off */ +#define IOCTL_GROUPS ( \ + LANDLOCK_ACCESS_FS_IOCTL_GROUP1 | \ + LANDLOCK_ACCESS_FS_IOCTL_GROUP2 | \ + LANDLOCK_ACCESS_FS_IOCTL_GROUP3 | \ + LANDLOCK_ACCESS_FS_IOCTL_GROUP4) +/* clang-format on */ + +static_assert((IOCTL_GROUPS & LANDLOCK_MASK_ACCESS_FS) == IOCTL_GROUPS); + +/** + * required_ioctl_access(): Determine required IOCTL access rights. + * + * @cmd: The IOCTL command that is supposed to be run. + * + * Returns: The access rights that must be granted on an opened file in order to + * use the given @cmd. + */ +static access_mask_t required_ioctl_access(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 FIOQSIZE: + return LANDLOCK_ACCESS_FS_IOCTL_GROUP1; + case FS_IOC_FIEMAP: + case FIBMAP: + case FIGETBSZ: + return LANDLOCK_ACCESS_FS_IOCTL_GROUP2; + case FIONREAD: + case FIDEDUPERANGE: + return LANDLOCK_ACCESS_FS_IOCTL_GROUP3; + case FICLONE: + case FICLONERANGE: + case FS_IOC_RESVSP: + case FS_IOC_RESVSP64: + case FS_IOC_UNRESVSP: + case FS_IOC_UNRESVSP64: + case FS_IOC_ZERO_RANGE: + return LANDLOCK_ACCESS_FS_IOCTL_GROUP4; + 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 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 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_GROUP1 | + LANDLOCK_ACCESS_FS_IOCTL_GROUP2 | + LANDLOCK_ACCESS_FS_IOCTL_GROUP4) | + expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_READ_FILE, + LANDLOCK_ACCESS_FS_IOCTL_GROUP1 | + LANDLOCK_ACCESS_FS_IOCTL_GROUP2 | + LANDLOCK_ACCESS_FS_IOCTL_GROUP3) | + expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_READ_DIR, + LANDLOCK_ACCESS_FS_IOCTL_GROUP1); +} + +/** + * 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. + */ +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) @@ -147,7 +289,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 */ /* @@ -157,6 +300,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, @@ -169,9 +313,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); @@ -1123,7 +1269,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) @@ -1196,6 +1344,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 = required_ioctl_access(cmd); + 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; + + return -EACCES; +} + static struct security_hook_list landlock_hooks[] __ro_after_init = { LSM_HOOK_INIT(inode_free_security, hook_inode_free_security), @@ -1218,6 +1385,7 @@ static struct security_hook_list landlock_hooks[] __ro_after_init = { LSM_HOOK_INIT(file_alloc_security, hook_file_alloc_security), LSM_HOOK_INIT(file_open, hook_file_open), LSM_HOOK_INIT(file_truncate, hook_file_truncate), + LSM_HOOK_INIT(file_ioctl, hook_file_ioctl), }; __init void landlock_add_fs_hooks(void) diff --git a/security/landlock/fs.h b/security/landlock/fs.h index 488e4813680a..c88fe7bda37b 100644 --- a/security/landlock/fs.h +++ b/security/landlock/fs.h @@ -92,4 +92,6 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset, const struct path *const path, access_mask_t access_hierarchy); +access_mask_t landlock_expand_handled_access_fs(const access_mask_t handled); + #endif /* _SECURITY_LANDLOCK_FS_H */ diff --git a/security/landlock/limits.h b/security/landlock/limits.h index 93c9c6f91556..296795f8a5c1 100644 --- a/security/landlock/limits.h +++ b/security/landlock/limits.h @@ -18,7 +18,16 @@ #define LANDLOCK_MAX_NUM_LAYERS 16 #define LANDLOCK_MAX_NUM_RULES U32_MAX -#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_TRUNCATE +/* + * For file system access rights, Landlock distinguishes between the publicly + * visible access rights (1 to LANDLOCK_LAST_PUBLIC_ACCESS_FS) and the private + * ones which are not exposed to userspace (LANDLOCK_LAST_PUBLIC_ACCESS_FS + 1 + * to LANDLOCK_LAST_ACCESS_FS). The private access rights are defined in fs.c. + */ +#define LANDLOCK_LAST_PUBLIC_ACCESS_FS LANDLOCK_ACCESS_FS_IOCTL +#define LANDLOCK_MASK_PUBLIC_ACCESS_FS ((LANDLOCK_LAST_PUBLIC_ACCESS_FS << 1) - 1) + +#define LANDLOCK_LAST_ACCESS_FS (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 4) #define LANDLOCK_MASK_ACCESS_FS ((LANDLOCK_LAST_ACCESS_FS << 1) - 1) #define LANDLOCK_NUM_ACCESS_FS __const_hweight64(LANDLOCK_MASK_ACCESS_FS) #define LANDLOCK_SHIFT_ACCESS_FS 0 diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h index c7f1526784fd..5a28ea8e1c3d 100644 --- a/security/landlock/ruleset.h +++ b/security/landlock/ruleset.h @@ -30,7 +30,7 @@ LANDLOCK_ACCESS_FS_REFER) /* clang-format on */ -typedef u16 access_mask_t; +typedef u32 access_mask_t; /* Makes sure all filesystem access rights can be stored. */ static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS); /* Makes sure all network access rights can be stored. */ diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c index 898358f57fa0..f0bc50003b46 100644 --- a/security/landlock/syscalls.c +++ b/security/landlock/syscalls.c @@ -137,7 +137,7 @@ static const struct file_operations ruleset_fops = { .write = fop_dummy_write, }; -#define LANDLOCK_ABI_VERSION 4 +#define LANDLOCK_ABI_VERSION 5 /** * sys_landlock_create_ruleset - Create a new ruleset @@ -192,8 +192,8 @@ SYSCALL_DEFINE3(landlock_create_ruleset, return err; /* Checks content (and 32-bits cast). */ - if ((ruleset_attr.handled_access_fs | LANDLOCK_MASK_ACCESS_FS) != - LANDLOCK_MASK_ACCESS_FS) + if ((ruleset_attr.handled_access_fs | LANDLOCK_MASK_PUBLIC_ACCESS_FS) != + LANDLOCK_MASK_PUBLIC_ACCESS_FS) return -EINVAL; /* Checks network content (and 32-bits cast). */ @@ -201,6 +201,10 @@ SYSCALL_DEFINE3(landlock_create_ruleset, LANDLOCK_MASK_ACCESS_NET) return -EINVAL; + /* Expands synthetic IOCTL groups. */ + ruleset_attr.handled_access_fs = landlock_expand_handled_access_fs( + ruleset_attr.handled_access_fs); + /* Checks arguments and transforms to kernel struct. */ ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs, ruleset_attr.handled_access_net); @@ -309,8 +313,13 @@ static int add_rule_path_beneath(struct landlock_ruleset *const ruleset, if (!path_beneath_attr.allowed_access) return -ENOMSG; - /* Checks that allowed_access matches the @ruleset constraints. */ - mask = landlock_get_raw_fs_access_mask(ruleset, 0); + /* + * Checks that allowed_access matches the @ruleset constraints and only + * consists of publicly visible access rights (as opposed to synthetic + * ones). + */ + mask = landlock_get_raw_fs_access_mask(ruleset, 0) & + LANDLOCK_MASK_PUBLIC_ACCESS_FS; if ((path_beneath_attr.allowed_access | mask) != mask) return -EINVAL; diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c index 646f778dfb1e..d292b419ccba 100644 --- a/tools/testing/selftests/landlock/base_test.c +++ b/tools/testing/selftests/landlock/base_test.c @@ -75,7 +75,7 @@ TEST(abi_version) const struct landlock_ruleset_attr ruleset_attr = { .handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE, }; - ASSERT_EQ(4, landlock_create_ruleset(NULL, 0, + ASSERT_EQ(5, landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION)); ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0, diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index 50818904397c..192608c3e840 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -525,9 +525,10 @@ TEST_F_FORK(layout1, inval) 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) -#define ACCESS_LAST LANDLOCK_ACCESS_FS_TRUNCATE +#define ACCESS_LAST LANDLOCK_ACCESS_FS_IOCTL #define ACCESS_ALL ( \ ACCESS_FILE | \
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 support IOCTLs like TIOCSTI and TIOCLINUX, which can be used to control shell processes on the same terminal which run at different privilege levels, which may make it possible to escape a sandbox. Because stdin, stdout and stderr are normally inherited rather than newly opened, IOCTLs are usually permitted on them even after the Landlock policy is enforced. 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. 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. Signed-off-by: Günther Noack <gnoack@google.com> --- include/uapi/linux/landlock.h | 58 +++++- security/landlock/fs.c | 176 ++++++++++++++++++- security/landlock/fs.h | 2 + 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, 253 insertions(+), 22 deletions(-)