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: > > Hello Mickaël! > > > > On Mon, Feb 19, 2024 at 07:34:42PM +0100, Mickaël Salaün wrote: > > > 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: > > > > 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. > > > > > > > > > > > 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.
