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@xxxxxxxxxx/
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
