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
