[PATCH v2 0/6] Landlock: ioctl support

[Günther Noack]

Hello!

On Fri, Jul 28, 2023 at 03:52:03PM +0200, Mickaël Salaün wrote:
> On 27/07/2023 11:32, Günther Noack wrote:
> > I'm puzzled how you come to the conclusion that devices don't do such
> > checks - did you read some ioctl command implementations, or is it a
> > more underlying principle that I was not aware of so far?
> 
> I took a look at fscrypt IOCTLs for instance, and other FS IOCTLs seems to
> correctly check for FD's read/write mode according to the IOCTL behavior.
> From what I've seen, IOCTLs implemented by device drivers don't care about
> file mode.

OK - That's surprising.


> > In any case - I believe the only reason why we are discussing this is
> > to justify the DEV/NODEV split, and that one in itself is not
> > controversial to me, even when I admittedly don't fully follow your
> > reasoning.
> 
> The main reason is than I don't want applications/users to not be allowed to
> use "legitimate" IOCTL, for instance to correctly encrypt their own files.
> If we only have one IOCTL right, we cannot easily differentiate between the
> targeted file types. However, this split might be too costly, cf. my comment
> in the below summary.

I believe that for "leaf processes" like most command line utilities, the
fscrypt use case is not a problem -- these IOCTLs are only needed for setting up
fscrypt directories and unlocking them.  Processes which merely access the files
in the unlocked directories can just transparently access them without using
ioctls.

The close-on-exec ioctls might be more relevant for leaf processes, but these
ones are uncontroversial to permit.

For "parent processes" like shells, where the exact operations done underneath
are not fully known in advance, it is more difficult to define an appropriate
policy using only the step (1) patch set, but I think it's a reasonable
trade-off for now.

When a policy distinguishes between /dev and other directories based on path,
that's already a good approximation for the ..._DEV and ..._NODEV access rights
which you proposed previously.  In addition to the nodev mount flag, mknod(2)
requires CAP_MKNOD, so it should not be possible for an attacker to place these
where they want (at least not through the Landlocked process, as we have the
NO_NEW_PRIVS flag).

Step (2) discussions though :)


> > Understood - so IIUC the scenario is that a process is not permitted
> > to read file attributes, but it'll be able to infer the device ID by
> > defining a dev_t-based Landlock rule and then observing whether ioctl
> > still works.
> 
> Right. I think it should be possible to still check if this kind of file
> attribute would be allowed to be read by the process, when performing the
> IOCTL on it. We need to think about the implications, and if it's worth it.
> 
> It would be interesting to see if there are similar cover channels with
> other Linux interfaces.

OK, noted it down for step (2).


> > Summarizing this, I believe that the parts that we need for step (1)
> > are the following ones:
> > 
> > (1) Identify and blanket-permit a small list of ioctl cmds which work
> >      on all file descriptors (FIOCLEX, FIONCLEX, FIONBIO, FIOASYNC, and
> >      others?)
> > 
> >      Compare
> >      https://lore.kernel.org/linux-security-module/6dfc0198-9010-7c54-2699-d3b867249850@digikod.net/
> > 
> > (2) Split into LANDLOCK_ACCESS_FS_IOCTL into a ..._DEV and a ..._NODEV part.
> 
> I'm a bit annoyed that this access rights mix action and object property and
> I'm worried that this might be a pain for future FS-related rule types (e.g.
> reusing all/most ACCESS_FS rights).
> 
> At first glance, a cleaner way would be to add a file_type field to
> landlock_path_beneath_attr (i.e. a subset of the landlock_inode_attr) but
> that would make struct landlock_rule and landlock_layer too complex, so not
> a good approach.
> 
> Unless someone has a better idea, let's stick to your first proposal and
> only implement LANDLOCK_ACCESS_FS_IOCTL (with the FIOCLEX-like exceptions).
> We should clearly explain that IOCTLs should be allowed for non-device file
> hierarchies: containing regular/data file (e.g. /home with the fscrypt use
> case), pipe, socket…
> 
> I'm still trying to convince myself which approach is the best, but for now
> the simplest one wins.

OK, sounds good.  I'll go for the LANDLOCK_ACCESS_FS_IOCTL approach then.


> > (3) Point out in documentation that this IOCTL restriction scheme only
> >      applies to newly opened FDs and in particular point out the common
> >      use case where that is a TTY, and what to do in this case.
> > 
> > If you agree, I'd go ahead and implement that as step (1) and we can
> > discuss the more advanced ideas in the context of a follow-up.

—Günther

-- 
Sent using Mutt 🐕 Woof Woof