On Fri, Mar 8, 2024 at 3:12 PM Mickaël Salaün <mic@xxxxxxxxxxx> wrote: > On Fri, Mar 08, 2024 at 02:22:58PM -0500, Paul Moore wrote: > > 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. > > As any new hook, there is a first user. Obviously this new hook would > not be restricted to Landlock, it is a generic approach. I'm pretty > sure a few hooks are only used by one LSM though. ;) Sure, as I said above, it's okay for there to only be a single LSM implementation, but the basic idea behind the hook needs to have some hope of being generic. Your "let's redefine a safe ioctl as 'IOCTL always allowed in a Landlock sandbox'" doesn't fill me with confidence about the hook being generic; who knows, maybe it will be, but in the absence of a patch, I'm left with descriptions like those. > > > 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 think there is a misunderstanding. The subset of IOCTL commands the > new hook will see would be 99% of them (i.e. all except those > implemented in fs/ioctl.c). *cough* 99% != 100% *cough* > Being able to only handle this (big) subset > would empower LSMs to control IOCTL commands without collision (e.g. the > same command/value may have different meanings according to the > implementation/layer), which is not currently possible (without manual > tweaking). > > This proposal is to add a new hook for the layer just beneath the VFS > catch-all IOCTL implementation. This layer can then differentiate > between the underlying implementation according to the file properties. > There is no need for additional hooks for other layers/subsystems. I'm not sure how you reconcile less than 100% coverage, the need for a generic hook, and the idea that there will not be a need for additional hooks. That still seems like a problem to me. > The existing security_file_ioctl() hook is useful to catch all IOCTL > commands, but it doesn't enable to identify the underlying target and > then the semantic of the command. The LSM hook gets the file pointer, the command, and the argument, how is a LSM not able to identify the underlying target? > Furthermore, as Günther said, an > IOCTL call can already do kernel operations without looking at the > command, but we would then be able to identify that by looking at the > char/block device file for instance. > > > 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. > > Without this hook, we'll need to rely on a list of allowed IOCTLs, which > will be out-of-sync eventually. It would be a maintenance burden and an > hacky approach. Welcome to the painful world of a LSM developer, ioctls are not the only place where this is a problem, and it should be easy enough to watch for changes in the ioctl list and update your favorite LSM accordingly. Honestly, I think that is kinda the right thing anyway, I'm skeptical that one could have a generic solution that would automatically allow or disallow a new ioctl without potentially breaking your favorite LSM's security model. If a new ioctl is introduced it seems like having someone manually review it's impact on your LSM would be a good idea. > We're definitely open to new proposals, but until now this is the best > approach we found from a maintenance, performance, and security point of > view. At this point it's probably a good idea to post another RFC patch with your revised idea, if nothing else it will help rule out any confusion. While I remain skeptical, perhaps I am misunderstanding the design and you'll get my apology and an ACK, but be warned that as of right now I'm not convinced. -- paul-moore.com
