[PATCH v3 0/5] Landlock: IOCTL support

Günther Noack gnoack at google.com
Thu Oct 19 22:09:36 UTC 2023


Hello!

On Mon, Sep 11, 2023 at 05:25:31PM +0200, Mickaël Salaün wrote:
> On Mon, Sep 11, 2023 at 12:02:46PM +0200, Günther Noack wrote:
> > Thank you for making the algorithm that explicit -- that helps to trace down the
> > differences.  I can follow the logic now, but I still don't understand what your
> > underlying rationale for that is?
> > 
> > I believe that fundamentally, a core difference is:
> > 
> > For an access right R and a file F, for these two cases:
> > 
> >  (a) the access right R is unhandled  (nothing gets restricted)
> >  (b) the access right R is handled, but R is granted for F in a rule.
> > 
> > I believe that accesses in case (a) and (b) to the file F should have the same
> > results.
> > 
> > This is at least how the existing Landlock implementation works, as far as I can
> > tell.
> > 
> > ("Refer" is an exceptional case, but we have documented that it was always
> > "implicitly handled" in ABI V1, which makes it consistent again.)
> > 
> > 
> > When I expand your code above to a boolean table, I end up with the following
> > decisions, depending on whether IOCTL and READ are handled or not, and whether
> > they are explicitly permitted for the file through a rule:
> > 
> > 
> > Mickaël's        IOCTL      IOCTL      IOCTL
> > suggestion       handled,   handled,   unhandled
> > 2023-09-04       file       file not
> >                  permitted  permitted
> > --------------------------------------------------
> > READ handled,
> > file permitted   allow      allow      allow
> > 
> > READ handled,
> > f not permitted  deny       deny       allow
> > 
> > READ unhandled   allow      deny       allow
> > 
> > 
> > In patch set V3, this is different: Because I think that cases (a) and (b) from
> > above should always behave the same, the first and third column and row must be
> > symmetric and have the same entries.  So, in patch set V3, it is sufficient if
> > *one of* the two rights IOCTL and READ_FILE are present, in order to use the
> > FIONREAD IOCTL:
> > 
> > 
> > Günther's        IOCTL      IOCTL      IOCTL
> > patch set V3     handled,   handled,   unhandled
> > 2023-08-14       file       file not
> >                  permitted  permitted
> > --------------------------------------------------
> > READ handled,
> > file permitted   allow      allow      allow
> > 
> > READ handled,
> > f not permitted  allow      deny       allow
> > 
> > READ unhandled   allow      allow      allow
> 
> A first difference is about (READ unhandled) AND (IOCTL handled +
> file not permitted). It will not be possible to follow the same logic
> with new Landlock access right (e.g. LANDLOCK_ACCESS_FS_READ_METADATA
> that should also allow FS_IOC_FSGETXATTR), and I'd like to keep it
> consistent.
> 
> A second difference is about (READ handled + f not permitted) AND
> (IOCTL handled + file permitted). The reasoning was to avoid giving too
> much power to LANDLOCK_ACCESS_FS_IOCTL and dowgrade it as new access
> rights are implemented. This looks quite similar to the CAP_SYS_ADMIN
> right that can basically do anything, and new capabilites are mainly a
> subset of this one. My proposal was to incrementally downgrade the power
> given by LANDLOCK_ACCESS_FS_IOCTL while still being compatible. On the
> I was thinking that, if we make a requirement to have the "new correct"
> access right, the application update might drop the IOCTL access right.
> I now think this reasoning is flawed.
> 
> Indeed, this comparaison doesn't fit well because IOCTLs are not a
> superset of all access rights, and because nothing can force developers
> that already gave access to all IOCTLs for a file to not just add
> another access right (instead of swapping them).
> 
> Instead, I think user space libraries should manage this kind of access
> right swapping when possible and have a fallback mechanism relying on
> the LANDLOCK_ACCESS_FS_IOCTL right. This would be valuable because they
> may be updated before the (stable system) kernel, and this would be
> easier for developers to manage.
> 
> In a nutshell, it is about giving control for an action (e.g. FIONREAD)
> to either a unique access right or to a set of access rights. At first,
> I would have preferred to have a unique access right to control an
> action, because it is simpler (e.g. for audit/debug). On the other hand,
> we need to handle access rights that allow the same action (e.g. file
> read OR write for FIOQSIZE). I now think your approach (i.e. set of
> access rights to control an action) could make more sense. Another good
> point is to not downgrade the power of LANDLOCK_ACCESS_FS_IOCTL, which
> could in fact be difficult to understand for users. Nested Landlock
> domains should also be easier to manage with this logic.

After we discussed this difficult topic briefly off-list, let me try to
summarize my takeaways and write it up here for reference.

I think the requirements for the logic of the IOCTL right are as follows:

 (1) In the future, if a new FOO access right is introduced, this right should
     implicitly give access to FOO-related IOCTLs on the affected same files,
     *without requiring the LANDLOCK_ACCESS_FS_IOCTL right*.

     Example: If in Landlock version 10, we introduce LANDLOCK_ACCESS_FS_GFX for
     graphics-related functionality, this access right should potentially give
     access to graphics-related ioctl commands.  I'll use the "GFX" example
     below as a stand-in for a generic future access right which should give
     access to a set of IOCTL commands.

and then the ones which are a bit more obvious:

 (2) When stacking additional Landlock layers, the thread's available access can
     only be restricted further (it should not accidentally be able to do more
     than before).

 (3) Landlock usages need to stay compatible across kernel versions.
     The Landlock usages that are in use today need to do the same thing
     in future kernel versions.

I had indeed overlooked requirement (1) and did not realize that my proposal was
going to be at odds with that.



## Some counterexamples for approaches that don't work

So: Counterexample for why my earlier proposal (OR-combination) does not work:

  In my proposal, a GFX-related IOCTL would be permitted when *either one* of
  the ..._GFX or the ..._IOCTL rights are available for the file.  (The READ
  right in the tables above should work the same as the GFX or FOO rights from
  requirement (1), for consistency).

  So a user who today uses

    handled: LANDLOCK_ACCESS_FS_IOCTL
    allowed: (nothing)

  will expect that GFX-related IOCTL operations are forbidden.  (We do not know
  yet whether the "GFX" access right will ever exist, therefore it is covered by
  LANDLOCK_ACCESS_FS_IOCTL.)

  Now we introduce the LANDLOCK_ACCESS_FS_GFX right, and suddenly, GFX-related
  IOCTL commands are checked with a new logic: You *either* need to have the
  LANDLOCK_ACCESS_FS_IOCTL right, *or* the LANDLOCK_ACCESS_FS_GFX right.  So
  when the user again uses

    handled: LANDLOCK_ACCESS_FS_IOCTL
    allowed: (nothing)

  the user would according to the new logic suddenly *have* the
  LANDLOCK_ACCESS_FS_GFX right, and these IOCTL commands would be permitted.

  This is a change of how Landlock behaves compared to the earlier version,
  and that is at odds with rule (3).


The other obvious bitwise combination (AND) does not work either -- that one
would violate requirement (1).



## A new proposal

We have discussed above that one option would be to start distinguishing between
the case where a right is "not handled" and the case where the right is
"handled, but allowed on the file".

This is not very nice, because it would be inconsistent with the semantics which
we had before for all other rights.

After thinking a bit more about it, one way to look at it is that we are using
the "handled" flags to control how the IOCTLs are grouped.  I agree that we have
to control the IOCTL grouping, but I am not sure whether the "handled" flags are
the right place to do that. -- We could just as well pass instructions about the
IOCTL grouping out of band, and I think it might make that logic clearer:

To put forward something concrete, how about this:

* LANDLOCK_ACCESS_FS_IOCTL: This access right controls the invocation of IOCTL
  commands, unless these commands are controlled by another access right.

  In every layer, each IOCTL command is only controlled through one access right.

* LANDLOCK_ACCESS_FS_READ_FILE: This access right controls opening files for
  reading, and additionally the use of the FIONREAD ioctl command.

* We introduce a flag in struct landlock_ruleset_attr which controls whether the
  graphics-related IOCTLs are controlled through the LANDLOCK_ACCESS_FS_GFX
  access right, rather than through LANDLOCK_ACCESS_FS_IOCTL.

  (This could potentially also be put in the "flags" argument to
  landlock_create_ruleset(), but it feels a bit more appropriate in the struct I
  think, as it influences the interpretation of the logic.  But I'm open to
  suggestions.)


Example: Without the flag, the IOCTL groups will be:

  These are always permitted:   FIOCLEX, FIONCLEX, FIONBIO, etc.
  LANDLOCK_ACCESS_FS_READ_FILE: controls FIONREAD
  LANDLOCK_ACCESS_FS_IOCTL:     controls all other IOCTL commands

but when users set the flag, the IOCTL groups will be:

  These are always permitted:   FIOCLEX, FIONCLEX, FIONBIO, etc.
  LANDLOCK_ACCESS_FS_READ_FILE: controls FIONREAD
  LANDLOCK_ACCESS_FS_GFX:       controls (list of gfx-related IOCTLs)
  LANDLOCK_ACCESS_FS_IOCTL:     controls all other IOCTL commands


Implementation-wise, I think it would actually look very similar to what would
be needed for your proposal of having a new special meaning for "handled".  It
would have the slight advantage that the new flag is actually only needed at the
time when we introduce a new way of grouping the IOCTL commands, so we would
only burden users with the additional complexity when it's actually required.

One implementation approach that I find reasonable to think about is to create
"synthetic" access rights when rulesets are enabled.  That is, we introduce
LANDLOCK_ACCESS_FS_SYNTHETIC_GFX_IOCTL (name TBD), but we keep this constant
private to the kernel.

* *At ruleset enablement time*, we populate the bit for this access right either
  from the LANDLOCK_ACCESS_FS_GFX or the LANDLOCK_ACCESS_FS_IOCTL bit from the
  same access_mask_t, depending on the IOCTL grouping which the ruleset is
  configured with.

* *In hook_file_open*, we then check for LANDLOCK_ACCESS_FS_SYNTHETIC_GFX_IOCTL
  for the GFX-related IOCTL commands.

I'm in favor of using the synthetic access rights, because I find it clearer to
understand that the effective access rights for a file from different layers are
just combined with a bitwise AND, and will give the right results.  We could
probably also make these path walk helpers aware of the special cases and only
have the synthetic right in layer_masks_dom, but I'd prefer not to complicate
these helpers even further.


Sorry for the long mail, I hope that the examples clarify it a bit. :)

In summary, it seems conceptually cleaner to me to control every IOCTL command
with only one access right, and let users control which one that should be with
a separate flag, so that "handled" keeps its original semantics.  It would also
have the upside that we can delay that implementation until the time where we
actually introduce new IOCTL-aware access rights on top of the current patch st.

I'd be interested to hear your thoughts on it.
—Günther

-- 
Sent using Mutt 🐕 Woof Woof



More information about the Linux-security-module-archive mailing list