On Fri, Mar 08, 2024 at 05:25:21PM -0500, Paul Moore wrote:

On Fri, Mar 8, 2024 at 3:12 PM Mickaël Salaün [off-list ref] wrote:

quoted

On Fri, Mar 08, 2024 at 02:22:58PM -0500, Paul Moore wrote:

quoted

On Fri, Mar 8, 2024 at 4:29 AM Mickaël Salaün [off-list ref] wrote:

quoted

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.

FWIW, the existing IOCTL hook is used in the following places:

* TOMOYO: seemingly configurable per IOCTL command?  (I did not dig deeper)
* SELinux: has a hardcoded switch of IOCTL commands, some with special checks.
  These are also a subset of the do_vfs_ioctl() commands,
  plus KDSKBENT, KDSKBSENT (from ioctl_console(2)).
* Smack: Decomposes the IOCTL command number to look at the _IOC_WRITE and
  _IOC_READ bits. (This is a known problematic approach, because (1) these bits
  describe whether the argument is getting read or written, not whether the
  operation is a mutating one, and (2) some IOCTL commands do not adhere to the
  convention and don't use these macros)

AppArmor does not use the LSM IOCTL hook.

quoted

quoted

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 are concerned that we will miss a change in do_vfs_ioctl(), which we would
like to reflect in the matching Landlock code.  Do other LSMs have any
approaches for that which go beyond just watching the do_vfs_ioctl()
implementation for changes?

quoted

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.

Thanks you for your feedback!

Here is V10 with the approach where we use a new LSM hook:
https://lore.kernel.org/all/20240309075320.160128-1-gnoack@google.com/ (local)

I hope this helps to clarify the approach a bit.  I'm explaining it in more
detail again in the commit which adds the LSM hook, including a call graph, and
avoiding the word "safe" this time ;-)

Let me know what you think!

Thanks!
—Günther