On Fri, Mar 8, 2024 at 4:29 AM Mickaël Salaün <mic@xxxxxxxxxxx> wrote: > On Fri, Mar 08, 2024 at 08:02:13AM +0100, Arnd Bergmann wrote: > > On Fri, Mar 8, 2024, at 00:09, Dave Chinner wrote: > > > On Thu, Mar 07, 2024 at 03:40:44PM -0500, Paul Moore wrote: > > >> On Thu, Mar 7, 2024 at 7:57 AM Günther Noack <gnoack@xxxxxxxxxx> wrote: > > >> I need some more convincing as to why we need to introduce these new > > >> hooks, or even the vfs_masked_device_ioctl() classifier as originally > > >> proposed at the top of this thread. I believe I understand why > > >> Landlock wants this, but I worry that we all might have different > > >> definitions of a "safe" ioctl list, and encoding a definition into the > > >> LSM hooks seems like a bad idea to me. > > > > > > I have no idea what a "safe" ioctl means here. Subsystems already > > > restrict ioctls that can do damage if misused to CAP_SYS_ADMIN, so > > > "safe" clearly means something different here. > > > > That was my problem with the first version as well, but I think > > drawing the line between "implemented in fs/ioctl.c" and > > "implemented in a random device driver fops->unlock_ioctl()" > > seems like a more helpful definition. > > > > This won't just protect from calling into drivers that are lacking > > a CAP_SYS_ADMIN check, but also from those that end up being > > harmful regardless of the ioctl command code passed into them > > because of stupid driver bugs. > > Indeed. > > "safe" is definitely not the right word, it is too broad, relative to > use cases and threat models. There is no "safe" IOCTL. > > Let's replace "safe IOCTL" with "IOCTL always allowed in a Landlock > sandbox". Which is a problem from a LSM perspective as we want to avoid hooks which are tightly bound to a single LSM or security model. It's okay if a new hook only has a single LSM implementation, but the hook's definition should be such that it is reasonably generalized to support multiple LSM/models. > Our assumptions are (in the context of Landlock): > > 1. There are IOCTLs tied to file types (e.g. block device with > major/minor) that can easily be identified from user space (e.g. with > the path name and file's metadata). /dev/* files make sense for user > space and they scope to a specific use case (with relative > privileges). This category of IOCTLs is implemented in standalone > device drivers (for most of them). > > 2. Most user space processes should not be denied access to IOCTLs that > are managed by the VFS layer or the underlying filesystem > implementations. For instance, the do_vfs_ioctl()'s ones (e.g. > FIOCLEX, FIONREAD) should always be allowed because they may be > required to legitimately use files, and for performance and security > reasons (e.g. fs-crypt, fsverity implemented at the filesystem layer). > Moreover, these IOCTLs should already check the read/write permission > (on the related FD), which is not the case for most block/char device > IOCTL. > > 3. IOCTLs to pipes and sockets are out of scope. They should always be > allowed for now because they don't directly expose files' data but > IPCs instead, and we are focusing on FS access rights for now. > > We want to add a new LANDLOCK_ACCESS_FS_IOCTL_DEV right that could match > on char/block device's specific IOCTLs, but it would not have any impact > on other IOCTLs which would then always be allowed (if the sandboxed > process is allowed to open the file). > > Because IOCTLs are implemented in layers and all IOCTLs commands live in > the same 32-bit namespace, we need a way to identify the layer > implemented by block and character devices. The new LSM hook proposal > enables us to cleanly and efficiently identify the char/block device > IOCTL layer with an additional check on the file type. I guess I should wait until there is an actual patch, but as of right now a VFS ioctl specific LSM hook looks far too limited to me and isn't something I can support at this point in time. It's obviously limited to only a subset of the ioctls, meaning that in order to have comprehensive coverage we would either need to implement a full range of subsystem ioctl hooks (ugh), or just use the existing security_file_ioctl(). I understand that this makes things a bit more complicated for Landlock's initial ioctl implementation, but considering my thoughts above and the fact that Landlock's ioctl protections are still evolving I'd rather not add a lot of extra hooks right now. -- paul-moore.com
