[RFC PATCH] fs: Add vfs_masks_device_ioctl*() helpers

[Casey Schaufler]

On 3/8/2024 12:12 PM, Mickaël Salaün 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 at digikod.net> 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 at google.com> 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.

I've been watching this thread with some interest, as one of my side projects
has been an attempt to address the "CAP_SYS_ADMIN problem", and there looks
to be a lot of similarity between that and the "ioctl problem". In both cases
it comes down to a matter of:
	1. uniquely identifying the action
	2. providing the information to code that can act upon it
	3. providing "policy" to determine what to do about it

My thought for the CAP_SYS_ADMIN case was to provide a new LSM hook
security_sysadmin() that takes a single parameter which is the action ID.
I called the action ID a "chit", because it's short and all the good,
more descriptive words where taken. Calls to cap_able(CAP_SYS_ADMIN) could
be replaced by calls to security_sysadmin(chit). security_sysadmin() would
first call cap_able(CAP_SYSADMIN) and, if that succeeded, allow LSMs with
registered hooks (selinux_sysadmin() etc) the opportunity to disallow the
operation. I planned to include a small LSM (chits) that would allow the
operation only if the process had the chit on its chit list. Landlock could
add policy to deal with chits if so inclined.

A generalization of this scheme would be to leave the cap_able(CAP_SYS_ADMIN)
checks as they are and add an optional security_chit() hook for places where
additional enforcement information is desired. Adding

	security_chit(CHIT_IOCTL_TTY_SOMETHING)

to an ioctl would allow any LSM to make policy decisions about that ioctl
operation. Adding

	security_chit(CHIT_ERASE_TAPE_REGISTERS)

after a cap_able(CAP_SYS_ADMIN) could appease the driver writer who would
otherwise be begging for CAP_ERASE_TAPE_REGISTERS. My biggest concern with
this scheme is the management of chit values, which would have to be kept
in a uapi header.

A major advantage of this is that the security_chit() calls would only have
to be added where someone wants to take advantage of the mechanism. People
who are happy with CAP_SYS_ADMIN or ioctl as it is don't have to do anything,
and their code won't get churned for the new world order. The downside is the
potentially onerous process of deciding if an LSM cares about an action known
only by its number.

I have patches close to ready. The chit LSM isn't passing the laugh test quite
yet, so I'm holding it back for now. I wanted to bring this up before we go too
far down a more complicated path.