From: "Mickaël Salaün" <mic@digikod.net>
To: Paul Moore <paul@paul-moore.com>
Cc: "Arnd Bergmann" <arnd@arndb.de>,
"Dave Chinner" <david@fromorbit.com>,
"Günther Noack" <gnoack@google.com>,
"Christian Brauner" <brauner@kernel.org>,
"Allen Webb" <allenwebb@google.com>,
"Dmitry Torokhov" <dtor@google.com>,
"Jeff Xu" <jeffxu@google.com>,
"Jorge Lucangeli Obes" <jorgelo@chromium.org>,
"Konstantin Meskhidze" <konstantin.meskhidze@huawei.com>,
"Matt Bobrowski" <repnop@google.com>,
linux-fsdevel@vger.kernel.org,
linux-security-module@vger.kernel.org
Subject: Re: [RFC PATCH] fs: Add vfs_masks_device_ioctl*() helpers
Date: Fri, 8 Mar 2024 21:12:07 +0100 [thread overview]
Message-ID: <20240308.eeZ1uungeeSa@digikod.net> (raw)
In-Reply-To: <CAHC9VhSjMLzfjm8re+3GN4PrAjO2qQW4Rf4o1wLchPDuqD-0Pw@mail.gmail.com>
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@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@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. ;)
>
> > 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). 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.
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. 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.
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.
next prev parent reply other threads:[~2024-03-08 20:12 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-09 17:06 [PATCH v9 0/8] Landlock: IOCTL support Günther Noack
2024-02-09 17:06 ` [PATCH v9 1/8] landlock: Add IOCTL access right Günther Noack
2024-02-10 11:06 ` Günther Noack
2024-02-10 11:49 ` Arnd Bergmann
2024-02-12 11:09 ` Christian Brauner
2024-02-12 22:10 ` Günther Noack
2024-02-10 11:18 ` Günther Noack
2024-02-16 14:11 ` Mickaël Salaün
2024-02-16 15:51 ` Mickaël Salaün
2024-02-18 8:34 ` Günther Noack
2024-02-19 21:44 ` Günther Noack
2024-02-16 17:19 ` Mickaël Salaün
2024-02-19 18:34 ` Mickaël Salaün
2024-02-19 18:35 ` [RFC PATCH] fs: Add vfs_masks_device_ioctl*() helpers Mickaël Salaün
2024-03-01 13:42 ` Mickaël Salaün
2024-03-01 16:24 ` Arnd Bergmann
2024-03-01 18:35 ` Mickaël Salaün
2024-03-05 18:13 ` Günther Noack
2024-03-06 13:47 ` Mickaël Salaün
2024-03-06 15:18 ` Arnd Bergmann
2024-03-07 12:15 ` Christian Brauner
2024-03-07 12:21 ` Arnd Bergmann
2024-03-07 12:57 ` Günther Noack
2024-03-07 20:40 ` Paul Moore
2024-03-07 23:09 ` Dave Chinner
2024-03-07 23:35 ` Paul Moore
2024-03-08 7:02 ` Arnd Bergmann
2024-03-08 9:29 ` Mickaël Salaün
2024-03-08 19:22 ` Paul Moore
2024-03-08 20:12 ` Mickaël Salaün [this message]
2024-03-08 22:04 ` Casey Schaufler
2024-03-08 22:25 ` Paul Moore
2024-03-09 8:14 ` Günther Noack
2024-03-09 17:41 ` Casey Schaufler
2024-03-11 19:04 ` Paul Moore
2024-03-08 11:03 ` Günther Noack
2024-03-11 1:03 ` Dave Chinner
2024-03-11 9:01 ` Günther Noack
2024-03-11 22:12 ` Dave Chinner
2024-03-12 10:58 ` Mickaël Salaün
2024-02-28 12:57 ` [PATCH v9 1/8] landlock: Add IOCTL access right Günther Noack
2024-03-01 12:59 ` Mickaël Salaün
2024-03-01 13:38 ` Mickaël Salaün
2024-02-09 17:06 ` [PATCH v9 2/8] selftests/landlock: Test IOCTL support Günther Noack
2024-02-09 17:06 ` [PATCH v9 3/8] selftests/landlock: Test IOCTL with memfds Günther Noack
2024-02-09 17:06 ` [PATCH v9 4/8] selftests/landlock: Test ioctl(2) and ftruncate(2) with open(O_PATH) Günther Noack
2024-02-09 17:06 ` [PATCH v9 5/8] selftests/landlock: Test IOCTLs on named pipes Günther Noack
2024-02-09 17:06 ` [PATCH v9 6/8] selftests/landlock: Check IOCTL restrictions for named UNIX domain sockets Günther Noack
2024-02-09 17:06 ` [PATCH v9 7/8] samples/landlock: Add support for LANDLOCK_ACCESS_FS_IOCTL Günther Noack
2024-02-09 17:06 ` [PATCH v9 8/8] landlock: Document IOCTL support Günther Noack
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240308.eeZ1uungeeSa@digikod.net \
--to=mic@digikod.net \
--cc=allenwebb@google.com \
--cc=arnd@arndb.de \
--cc=brauner@kernel.org \
--cc=david@fromorbit.com \
--cc=dtor@google.com \
--cc=gnoack@google.com \
--cc=jeffxu@google.com \
--cc=jorgelo@chromium.org \
--cc=konstantin.meskhidze@huawei.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=paul@paul-moore.com \
--cc=repnop@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).