[PATCH v2 0/6] Landlock: ioctl support

[Mickaël Salaün]

Hi, thanks for the patches!


On 23/06/2023 17:20, Günther Noack wrote:
> Hello!
> 
> On Fri, Jun 23, 2023 at 04:43:23PM +0200, Günther Noack wrote:
>> Proposed approach
>> ~~~~~~~~~~~~~~~~~
>>
>> Introduce the LANDLOCK_ACCESS_FS_IOCTL right, which restricts the use
>> of ioctl(2) on file descriptors.
>>
>> We attach the LANDLOCK_ACCESS_FS_IOCTL right to opened file
>> descriptors, as we already do for LANDLOCK_ACCESS_FS_TRUNCATE.
>>
>> I believe that this approach works for the majority of use cases, and
>> offers a good trade-off between Landlock API and implementation
>> complexity and flexibility when the feature is used.
>>
>> Current limitations
>> ~~~~~~~~~~~~~~~~~~~
>>
>> With this patch set, ioctl(2) requests can *not* be filtered based on
>> file type, device number (dev_t) or on the ioctl(2) request number.
>>
>> On the initial RFC patch set [1], we have reached consensus to start
>> with this simpler coarse-grained approach, and build additional ioctl
>> restriction capabilities on top in subsequent steps.
> 
> We *could* use this opportunity to blanket disable the TIOCSTI ioctl, if a
> Landlock policy gets enabled and ioctls are handled.
> 
> TIOCSTI is a TTY ioctl which is commonly used as a sandbox escape, if the
> sandboxes system can get a hold on a TTY file descriptor from outside the
> sandbox (like STDOUT, hah).
> 
> An excellent summary of these problems was already written by Kees Cook on the
> Linux patch which removes TIOCSTI depending on a kernel config option:
> https://lore.kernel.org/lkml/20221022182828.give.717-kees@kernel.org/
> 
> Unfortunately on the distributions I have tested so far (Debian and Arch Linux),
> TIOCSTI is still enabled.
> 
> ***Proposal***:
> 
>    I am in favor of *disabling* TIOCSTI in all Landlocked processes,
>    if the Landlock policy handles the LANDLOCK_ACCESS_FS_IOCTL right.
> 
> If we want to do it in a backwards-compatible way, now would be the time to add
> it to the patch set. :)

What would that not be backward compatible?

> 
> As far as I can tell, there are no good legitimate use cases for TIOCSTI, and it
> would fix the aforementioned sandbox escaping trick for a Landlocked process.
> With the patch set as it is now, the only way to prevent that sandbox escape is
> unfortunately to either (1) close the TTY file descriptors when enabling
> Landlock, or (2) rely on an outside process to pass something else than a TTY
> for FDs 0, 1 and 2.

What about calling setsid()?

Alternatively, seccomp could be used, even if it could block overlapping 
IOCTLs as well…


> 
> Does that sound reasonable?  If yes, I'd send an update to this patch set which
> forbids TIOCSTI.

I agree that TIOCSTI is dangerous, but I don't see the rationale to add 
an exception for this specific IOCTL. I'm sure there are a lot of 
potentially dangerous IOCTLs out there, but from a kernel point of view, 
why should Landlock handle this one in a specific way?

Landlock should not define specific policies itself but let users manage 
that. Landlock should only restrict kernel features that *directly* 
enable to bypass its own restrictions (e.g. ptrace scope, FS topology 
changes when FS restrictions are in place).

I think we should instead focus on adding something like a 
landlock_inode_attr rule type to restrict IOCTLs defined by 
users/developers, and then extend it to make it possible to restrict 
already opened FDs as well.

> 
> Thanks,
> —Günther
>