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:

quoted

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