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@xxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxxx
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@xxxxxxxxxxx
> 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+@xxxxxxxxxx
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
