[RFC PATCH] fs: Add vfs_masks_device_ioctl*() helpers

[Paul Moore]

On Fri, Mar 8, 2024 at 3:12 PM Mickaël Salaün <mic at digikod.net> wrote:
> On Fri, Mar 08, 2024 at 02:22:58PM -0500, Paul Moore wrote:
> > On Fri, Mar 8, 2024 at 4:29 AM Mickaël Salaün <mic at digikod.net> wrote:
> > > On Fri, Mar 08, 2024 at 08:02:13AM +0100, Arnd Bergmann wrote:
> > > > On Fri, Mar 8, 2024, at 00:09, Dave Chinner wrote:
> > > > > On Thu, Mar 07, 2024 at 03:40:44PM -0500, Paul Moore wrote:
> > > > >> On Thu, Mar 7, 2024 at 7:57 AM Günther Noack <gnoack at google.com> wrote:
> > > > >> I need some more convincing as to why we need to introduce these new
> > > > >> hooks, or even the vfs_masked_device_ioctl() classifier as originally
> > > > >> proposed at the top of this thread.  I believe I understand why
> > > > >> Landlock wants this, but I worry that we all might have different
> > > > >> definitions of a "safe" ioctl list, and encoding a definition into the
> > > > >> LSM hooks seems like a bad idea to me.
> > > > >
> > > > > I have no idea what a "safe" ioctl means here. Subsystems already
> > > > > restrict ioctls that can do damage if misused to CAP_SYS_ADMIN, so
> > > > > "safe" clearly means something different here.
> > > >
> > > > That was my problem with the first version as well, but I think
> > > > drawing the line between "implemented in fs/ioctl.c" and
> > > > "implemented in a random device driver fops->unlock_ioctl()"
> > > > seems like a more helpful definition.
> > > >
> > > > This won't just protect from calling into drivers that are lacking
> > > > a CAP_SYS_ADMIN check, but also from those that end up being
> > > > harmful regardless of the ioctl command code passed into them
> > > > because of stupid driver bugs.
> > >
> > > Indeed.
> > >
> > > "safe" is definitely not the right word, it is too broad, relative to
> > > use cases and threat models.  There is no "safe" IOCTL.
> > >
> > > Let's replace "safe IOCTL" with "IOCTL always allowed in a Landlock
> > > sandbox".
> >
> > Which is a problem from a LSM perspective as we want to avoid hooks
> > which are tightly bound to a single LSM or security model.  It's okay
> > if a new hook only has a single LSM implementation, but the hook's
> > definition should be such that it is reasonably generalized to support
> > multiple LSM/models.
>
> As any new hook, there is a first user.  Obviously this new hook would
> not be restricted to Landlock, it is a generic approach.  I'm pretty
> sure a few hooks are only used by one LSM though. ;)

Sure, as I said above, it's okay for there to only be a single LSM
implementation, but the basic idea behind the hook needs to have some
hope of being generic.  Your "let's redefine a safe ioctl as 'IOCTL
always allowed in a Landlock sandbox'" doesn't fill me with confidence
about the hook being generic; who knows, maybe it will be, but in the
absence of a patch, I'm left with descriptions like those.

> > > Our assumptions are (in the context of Landlock):
> > >
> > > 1. There are IOCTLs tied to file types (e.g. block device with
> > >    major/minor) that can easily be identified from user space (e.g. with
> > >    the path name and file's metadata).  /dev/* files make sense for user
> > >    space and they scope to a specific use case (with relative
> > >    privileges).  This category of IOCTLs is implemented in standalone
> > >    device drivers (for most of them).
> > >
> > > 2. Most user space processes should not be denied access to IOCTLs that
> > >    are managed by the VFS layer or the underlying filesystem
> > >    implementations.  For instance, the do_vfs_ioctl()'s ones (e.g.
> > >    FIOCLEX, FIONREAD) should always be allowed because they may be
> > >    required to legitimately use files, and for performance and security
> > >    reasons (e.g. fs-crypt, fsverity implemented at the filesystem layer).
> > >    Moreover, these IOCTLs should already check the read/write permission
> > >    (on the related FD), which is not the case for most block/char device
> > >    IOCTL.
> > >
> > > 3. IOCTLs to pipes and sockets are out of scope.  They should always be
> > >    allowed for now because they don't directly expose files' data but
> > >    IPCs instead, and we are focusing on FS access rights for now.
> > >
> > > We want to add a new LANDLOCK_ACCESS_FS_IOCTL_DEV right that could match
> > > on char/block device's specific IOCTLs, but it would not have any impact
> > > on other IOCTLs which would then always be allowed (if the sandboxed
> > > process is allowed to open the file).
> > >
> > > Because IOCTLs are implemented in layers and all IOCTLs commands live in
> > > the same 32-bit namespace, we need a way to identify the layer
> > > implemented by block and character devices.  The new LSM hook proposal
> > > enables us to cleanly and efficiently identify the char/block device
> > > IOCTL layer with an additional check on the file type.
> >
> > I guess I should wait until there is an actual patch, but as of right
> > now a VFS ioctl specific LSM hook looks far too limited to me and
> > isn't something I can support at this point in time.  It's obviously
> > limited to only a subset of the ioctls, meaning that in order to have
> > comprehensive coverage we would either need to implement a full range
> > of subsystem ioctl hooks (ugh), or just use the existing
> > security_file_ioctl().
>
> I think there is a misunderstanding.  The subset of IOCTL commands the
> new hook will see would be 99% of them (i.e. all except those
> implemented in fs/ioctl.c).

*cough* 99% != 100% *cough*

> Being able to only handle this (big) subset
> would empower LSMs to control IOCTL commands without collision (e.g. the
> same command/value may have different meanings according to the
> implementation/layer), which is not currently possible (without manual
> tweaking).
>
> This proposal is to add a new hook for the layer just beneath the VFS
> catch-all IOCTL implementation.  This layer can then differentiate
> between the underlying implementation according to the file properties.
> There is no need for additional hooks for other layers/subsystems.

I'm not sure how you reconcile less than 100% coverage, the need for a
generic hook, and the idea that there will not be a need for
additional hooks.  That still seems like a problem to me.

> The existing security_file_ioctl() hook is useful to catch all IOCTL
> commands, but it doesn't enable to identify the underlying target and
> then the semantic of the command.

The LSM hook gets the file pointer, the command, and the argument, how
is a LSM not able to identify the underlying target?

> Furthermore, as Günther said, an
> IOCTL call can already do kernel operations without looking at the
> command, but we would then be able to identify that by looking at the
> char/block device file for instance.
>
> > I understand that this makes things a bit more
> > complicated for Landlock's initial ioctl implementation, but
> > considering my thoughts above and the fact that Landlock's ioctl
> > protections are still evolving I'd rather not add a lot of extra hooks
> > right now.
>
> Without this hook, we'll need to rely on a list of allowed IOCTLs, which
> will be out-of-sync eventually.  It would be a maintenance burden and an
> hacky approach.

Welcome to the painful world of a LSM developer, ioctls are not the
only place where this is a problem, and it should be easy enough to
watch for changes in the ioctl list and update your favorite LSM
accordingly.  Honestly, I think that is kinda the right thing anyway,
I'm skeptical that one could have a generic solution that would
automatically allow or disallow a new ioctl without potentially
breaking your favorite LSM's security model.  If a new ioctl is
introduced it seems like having someone manually review it's impact on
your LSM would be a good idea.

> We're definitely open to new proposals, but until now this is the best
> approach we found from a maintenance, performance, and security point of
> view.

At this point it's probably a good idea to post another RFC patch with
your revised idea, if nothing else it will help rule out any
confusion.  While I remain skeptical, perhaps I am misunderstanding
the design and you'll get my apology and an ACK, but be warned that as
of right now I'm not convinced.

--
paul-moore.com