On Fri, Mar 08, 2024 at 05:25:21PM -0500, Paul Moore wrote: > 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: > > > > 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. FWIW, the existing IOCTL hook is used in the following places: * TOMOYO: seemingly configurable per IOCTL command? (I did not dig deeper) * SELinux: has a hardcoded switch of IOCTL commands, some with special checks. These are also a subset of the do_vfs_ioctl() commands, plus KDSKBENT, KDSKBSENT (from ioctl_console(2)). * Smack: Decomposes the IOCTL command number to look at the _IOC_WRITE and _IOC_READ bits. (This is a known problematic approach, because (1) these bits describe whether the argument is getting read or written, not whether the operation is a mutating one, and (2) some IOCTL commands do not adhere to the convention and don't use these macros) AppArmor does not use the LSM IOCTL hook. > > > 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 are concerned that we will miss a change in do_vfs_ioctl(), which we would like to reflect in the matching Landlock code. Do other LSMs have any approaches for that which go beyond just watching the do_vfs_ioctl() implementation for changes? > > 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. Thanks you for your feedback! Here is V10 with the approach where we use a new LSM hook: https://lore.kernel.org/all/20240309075320.160128-1-gnoack@xxxxxxxxxx/ I hope this helps to clarify the approach a bit. I'm explaining it in more detail again in the commit which adds the LSM hook, including a call graph, and avoiding the word "safe" this time ;-) Let me know what you think! Thanks! —Günther
