On Fri, Mar 01, 2024 at 01:59:13PM +0100, Mickaël Salaün wrote:

On Wed, Feb 28, 2024 at 01:57:42PM +0100, Günther Noack wrote:

quoted

Hello Mickaël!

On Mon, Feb 19, 2024 at 07:34:42PM +0100, Mickaël Salaün wrote:

quoted

Arn, Christian, please take a look at the following RFC patch and the
rationale explained here.

On Fri, Feb 09, 2024 at 06:06:05PM +0100, Günther Noack wrote:

quoted

Introduces the LANDLOCK_ACCESS_FS_IOCTL access right
and increments the Landlock ABI version to 5.

Like the truncate right, these rights are associated with a file
descriptor at the time of open(2), and get respected even when the
file descriptor is used outside of the thread which it was originally
opened in.

A newly enabled Landlock policy therefore does not apply to file
descriptors which are already open.

If the LANDLOCK_ACCESS_FS_IOCTL right is handled, only a small number
of safe IOCTL commands will be permitted on newly opened files.  The
permitted IOCTLs can be configured through the ruleset in limited ways
now.  (See documentation for details.)

Specifically, when LANDLOCK_ACCESS_FS_IOCTL is handled, granting this
right on a file or directory will *not* permit to do all IOCTL
commands, but only influence the IOCTL commands which are not already
handled through other access rights.  The intent is to keep the groups
of IOCTL commands more fine-grained.

Noteworthy scenarios which require special attention:

TTY devices are often passed into a process from the parent process,
and so a newly enabled Landlock policy does not retroactively apply to
them automatically.  In the past, TTY devices have often supported
IOCTL commands like TIOCSTI and some TIOCLINUX subcommands, which were
letting callers control the TTY input buffer (and simulate
keypresses).  This should be restricted to CAP_SYS_ADMIN programs on
modern kernels though.

Some legitimate file system features, like setting up fscrypt, are
exposed as IOCTL commands on regular files and directories -- users of
Landlock are advised to double check that the sandboxed process does
not need to invoke these IOCTLs.

I think we really need to allow fscrypt and fs-verity IOCTLs.

quoted

Known limitations:

The LANDLOCK_ACCESS_FS_IOCTL access right is a coarse-grained control
over IOCTL commands.  Future work will enable a more fine-grained
access control for IOCTLs.

In the meantime, Landlock users may use path-based restrictions in
combination with their knowledge about the file system layout to
control what IOCTLs can be done.  Mounting file systems with the nodev
option can help to distinguish regular files and devices, and give
guarantees about the affected files, which Landlock alone can not give
yet.

I had a second though about our current approach, and it looks like we
can do simpler, more generic, and with less IOCTL commands specific
handling.

What we didn't take into account is that an IOCTL needs an opened file,
which means that the caller must already have been allowed to open this
file in read or write mode.

I think most FS-specific IOCTL commands check access rights (i.e. access
mode or required capability), other than implicit ones (at least read or
write), when appropriate.  We don't get such guarantee with device
drivers.

The main threat is IOCTLs on character or block devices because their
impact may be unknown (if we only look at the IOCTL command, not the
backing file), but we should allow IOCTLs on filesystems (e.g. fscrypt,
fs-verity, clone extents).  I think we should only implement a
LANDLOCK_ACCESS_FS_IOCTL_DEV right, which would be more explicit.  This
change would impact the IOCTLs grouping (not required anymore), but
we'll still need the list of VFS IOCTLs.

I am fine with dropping the IOCTL grouping and going for this simpler approach.

This must have been a misunderstanding - I thought you wanted to align the
access checks in Landlock with the ones done by the kernel already, so that we
can reason about it more locally.  But I'm fine with doing it just for device
files as well, if that is what it takes.  It's definitely simpler.

I still think we should align existing Landlock access rights with the VFS IOCTL
semantic (i.e. mostly defined in do_vfs_ioctl(), but also in the compat
ioctl syscall).  However, according to our investigations and
discussions, it looks like the groups we defined should already be
enforced by the VFS code, which means we should not need such groups
after all.  My last proposal is to still delegate access for VFS IOCTLs
to the current Landlock access rights, but it doesn't seem required to
add specific access check if we are able to identify these VFS IOCTLs.

To say it another way, at least one of the read/write Landlock rights
are already required to open a file/directory, and according to the new
get_required_ioctl_access() grouping we can simplifying it further to
fully rely on the meta "open" access right, and then replace
get_required_ioctl_access() with the file type and
vfs_masked_device_ioctl() checks.

For now, the only "optional" access right is
LANDLOCK_ACCESS_FS_TRUNCATE, and I don't think it needs to be tied to
any VFS IOCTLs.

Because the IOCTL_DEV access right comes now, future access rights that
may need to also check IOCTL (e.g. change file attribute) should be much
simpler to implement.  Indeed, they will only impact VFS IOCTLs which
would always be allowed with LANDLOCK_ACCESS_FS_IOCTL_DEV.  It should
then be trivial to add a new control layer for a subset of the VFS
IOCTLs.