| Message ID | 20230502171755.9788-3-gnoack3000@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Landlock: ioctl support | expand |
On 02/05/2023 19:17, Günther Noack wrote: > Like the truncate right, this right is associated with a file > descriptor at the time of open(2), and gets respected even when the > file descriptor is used outside of the thread which it was originally > created in. > > In particular, this happens for the commonly inherited file > descriptors stdin, stdout and stderr, if these are bound to a tty. s/tty/TTY/ > This means that programs using tty ioctls can drop the ioctl access s/ioctl/IOCTLs/ > right, but continue using these ioctls on the already opened input and > output file descriptors. I'd like a new documentation paragraph explaining the limitation of LANDLOCK_ACCESS_FS_IOCTL (not fine-grained; should be careful about fscrypt-like features for regular files; compatibility with TTY and other common IOCTLs), a way to get more guarantees (e.g. using nodev mount points when possible), and a sentence explaining that future work will enable a more fine-grained access control. > > Signed-off-by: Günther Noack <gnoack3000@gmail.com> > --- > include/uapi/linux/landlock.h | 19 ++++++++++++------- > security/landlock/fs.c | 20 ++++++++++++++++++-- > security/landlock/limits.h | 2 +- > tools/testing/selftests/landlock/fs_test.c | 5 +++-- > 4 files changed, 34 insertions(+), 12 deletions(-) > > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h > index f3223f96469..d87457a1c22 100644 > --- a/include/uapi/linux/landlock.h > +++ b/include/uapi/linux/landlock.h > @@ -102,12 +102,16 @@ struct landlock_path_beneath_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. > + * - %LANDLOCK_ACCESS_FS_IOCTL: Invoke :manpage:`ioctl(2)` on the opened file. > + * This access right is available since the fourth 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 > @@ -152,7 +156,7 @@ struct landlock_path_beneath_attr { > * 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 */ > @@ -171,6 +175,7 @@ struct landlock_path_beneath_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 */ > > #endif /* _UAPI_LINUX_LANDLOCK_H */ > diff --git a/security/landlock/fs.c b/security/landlock/fs.c > index adcea0fe7e6..b13c765733c 100644 > --- a/security/landlock/fs.c > +++ b/security/landlock/fs.c > @@ -147,7 +147,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 */ > > /* > @@ -1207,7 +1208,8 @@ 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; > const struct landlock_ruleset *const dom = > landlock_get_current_domain(); > > @@ -1280,6 +1282,19 @@ 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) > +{ > + /* > + * 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 (landlock_file(file)->allowed_access & LANDLOCK_ACCESS_FS_IOCTL) > + return 0; Please add a new line here. > + return -EACCES; > +} > + > static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(inode_free_security, hook_inode_free_security), > > @@ -1302,6 +1317,7 @@ static struct security_hook_list landlock_hooks[] __lsm_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/limits.h b/security/landlock/limits.h > index 82288f0e9e5..40d8f17698b 100644 > --- a/security/landlock/limits.h > +++ b/security/landlock/limits.h > @@ -18,7 +18,7 @@ > #define LANDLOCK_MAX_NUM_LAYERS 16 > #define LANDLOCK_MAX_NUM_RULES U32_MAX > > -#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_TRUNCATE > +#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_IOCTL > #define LANDLOCK_MASK_ACCESS_FS ((LANDLOCK_LAST_ACCESS_FS << 1) - 1) > #define LANDLOCK_NUM_ACCESS_FS __const_hweight64(LANDLOCK_MASK_ACCESS_FS) > > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c > index b6c4be3faf7..fdd7d439ce4 100644 > --- a/tools/testing/selftests/landlock/fs_test.c > +++ b/tools/testing/selftests/landlock/fs_test.c > @@ -446,9 +446,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 | \
Hi! On Mon, Jun 19, 2023 at 04:42:07PM +0200, Mickaël Salaün wrote: > I'd like a new documentation paragraph explaining the limitation of > LANDLOCK_ACCESS_FS_IOCTL (not fine-grained; should be careful about > fscrypt-like features for regular files; compatibility with TTY and other > common IOCTLs), a way to get more guarantees (e.g. using nodev mount points > when possible), and a sentence explaining that future work will enable a > more fine-grained access control. I tried to add this comment but realized that I don't understand it well enough - Regarding fscrypt: If a process is not the fscrypt user space tool itself, in which ways do the fscrypt ioctls matter for that process? I dug up a list of ioctls in tools/include/uapi/linux/fscrypt.h which look related, but these look like they are only needed for the set up of encrypted files and directories, but not for using these files later from other processes? Am I misunderstanding that? The one thing that seems to stand out with the fscrypt ioctls is that the same ioctl numbers are implemented by multiple different file systems. Regarding nodev mount points: I guess this is not relevant any more if we split the IOCTL right into a device-only and non-device-only flag? (I prefer that solution over nodev mounts as well, because that solution works unprivileged from the perspective of the process that defines the Landlock policy. Re-mounting with different options requires more rights and can often not be influenced by small utilities.) Thanks, —Günther
On Fri, Jul 14, 2023 at 02:46:09PM +0200, Günther Noack wrote: > Hi! > > On Mon, Jun 19, 2023 at 04:42:07PM +0200, Mickaël Salaün wrote: > > I'd like a new documentation paragraph explaining the limitation of > > LANDLOCK_ACCESS_FS_IOCTL (not fine-grained; should be careful about > > fscrypt-like features for regular files; compatibility with TTY and other > > common IOCTLs), a way to get more guarantees (e.g. using nodev mount points > > when possible), and a sentence explaining that future work will enable a > > more fine-grained access control. > > I tried to add this comment but realized that I don't understand it well enough - > > Regarding fscrypt: > > If a process is not the fscrypt user space tool itself, in which ways do the > fscrypt ioctls matter for that process? These IOCTLs may be only used by the fscrypt tool on most distros but there are of course no guarantee any application could use it to encrypt its own files according to various use cases. > > I dug up a list of ioctls in tools/include/uapi/linux/fscrypt.h which look > related, but these look like they are only needed for the set up of encrypted > files and directories, but not for using these files later from other > processes? Correct > > Am I misunderstanding that? > > The one thing that seems to stand out with the fscrypt ioctls is that the same > ioctl numbers are implemented by multiple different file systems. My point was to highlight that nodev IOCTLs might be used by user space on regular files and directories, not only /dev files, and we should be careful to not inadvertently deny legitimate IOCTLs. I think nodev-IOCTLs are legitimate for unprivileged users, taking into account capability checks and file mode checks, which might not be the case for IOCTLs implemented by drivers. You can take a look ad do_vfs_ioctl() to get a hint about other FS-related IOCTLs (i.e. FIGETBSZ, FIONREAD), and all other nodev-files (e.g. pipe, sockets) IOCTLs that may be required to properly use them. We should be careful to find the good balance between restricting as much as possible but without breaking user space and bother users that would then disable security features. ;) > > Regarding nodev mount points: > > I guess this is not relevant any more if we split the IOCTL right into a > device-only and non-device-only flag? > > (I prefer that solution over nodev mounts as well, because that solution works > unprivileged from the perspective of the process that defines the Landlock > policy. Re-mounting with different options requires more rights and can often > not be influenced by small utilities.) We're now going withouth the split, but this nodev property could in fact be part of step 2 (or 3). Indeed, being able to differenciate file type would enable to enforce a nodev-like control over a file hierarchy. > > Thanks, > —Günther > > -- > Sent using Mutt
diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h index f3223f96469..d87457a1c22 100644 --- a/include/uapi/linux/landlock.h +++ b/include/uapi/linux/landlock.h @@ -102,12 +102,16 @@ struct landlock_path_beneath_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. + * - %LANDLOCK_ACCESS_FS_IOCTL: Invoke :manpage:`ioctl(2)` on the opened file. + * This access right is available since the fourth 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 @@ -152,7 +156,7 @@ struct landlock_path_beneath_attr { * 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 */ @@ -171,6 +175,7 @@ struct landlock_path_beneath_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 */ #endif /* _UAPI_LINUX_LANDLOCK_H */ diff --git a/security/landlock/fs.c b/security/landlock/fs.c index adcea0fe7e6..b13c765733c 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -147,7 +147,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 */ /* @@ -1207,7 +1208,8 @@ 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; const struct landlock_ruleset *const dom = landlock_get_current_domain(); @@ -1280,6 +1282,19 @@ 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) +{ + /* + * 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 (landlock_file(file)->allowed_access & LANDLOCK_ACCESS_FS_IOCTL) + return 0; + return -EACCES; +} + static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(inode_free_security, hook_inode_free_security), @@ -1302,6 +1317,7 @@ static struct security_hook_list landlock_hooks[] __lsm_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/limits.h b/security/landlock/limits.h index 82288f0e9e5..40d8f17698b 100644 --- a/security/landlock/limits.h +++ b/security/landlock/limits.h @@ -18,7 +18,7 @@ #define LANDLOCK_MAX_NUM_LAYERS 16 #define LANDLOCK_MAX_NUM_RULES U32_MAX -#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_TRUNCATE +#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_IOCTL #define LANDLOCK_MASK_ACCESS_FS ((LANDLOCK_LAST_ACCESS_FS << 1) - 1) #define LANDLOCK_NUM_ACCESS_FS __const_hweight64(LANDLOCK_MASK_ACCESS_FS) diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index b6c4be3faf7..fdd7d439ce4 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -446,9 +446,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 | \
Like the truncate right, this right is associated with a file descriptor at the time of open(2), and gets respected even when the file descriptor is used outside of the thread which it was originally created in. In particular, this happens for the commonly inherited file descriptors stdin, stdout and stderr, if these are bound to a tty. This means that programs using tty ioctls can drop the ioctl access right, but continue using these ioctls on the already opened input and output file descriptors. Signed-off-by: Günther Noack <gnoack3000@gmail.com> --- include/uapi/linux/landlock.h | 19 ++++++++++++------- security/landlock/fs.c | 20 ++++++++++++++++++-- security/landlock/limits.h | 2 +- tools/testing/selftests/landlock/fs_test.c | 5 +++-- 4 files changed, 34 insertions(+), 12 deletions(-)