Re: [RFC PATCH v3 3/3] devguard: added device guard for mknod in non-initial userns

[Michael Weiß]

On 29.12.23 23:31, Paul Moore wrote:
> On Wed, Dec 27, 2023 at 9:31 AM Michael Weiß
> <michael.weiss@xxxxxxxxxxxxxxxxxxx> wrote:
>> Hi Paul, what would you think about if we do it as shown in the
>> patch below (untested)?
>>
>> I have adapted Christians patch slightly in a way that we do let
>> all LSMs agree on if device access management should be done or not.
>> Similar to the security_task_prctl() hook.
> 
> I think it's worth taking a minute to talk about this proposed change
> and the existing security_task_prctl() hook, as there is an important
> difference between the two which is the source of my concern.
> 
> If you look at the prctl() syscall implementation, right at the top of
> the function you see the LSM hook:
> 
>   SYSCALL_DEFINE(prctl, ...)
>   {
>     ...
> 
>     error = security_task_prctl(...);
>     if (error != -ENOSYS)
>       return error;
> 
>     error = 0;
> 
>     ....
>   }
> 
> While it is true that the LSM hook returns a "special" value, -ENOSYS,
> from a practical perspective this is not significantly different from
> the much more common zero value used to indicate no restriction from
> the LSM layer.  However, the more important thing to note is that the
> return value from security_task_prctl() does not influence any other
> access controls in the caller outside of those implemented inside the
> LSM; in fact the error code is reset to zero immediately after the LSM
> hook.
> 
> More on this below ...
> 
>> diff --git a/fs/super.c b/fs/super.c
>> index 076392396e72..6510168d51ce 100644
>> --- a/fs/super.c
>> +++ b/fs/super.c
>> @@ -325,7 +325,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
>>  {
>>         struct super_block *s = kzalloc(sizeof(struct super_block),  GFP_USER);
>>         static const struct super_operations default_op;
>> -       int i;
>> +       int i, err;
>>
>>         if (!s)
>>                 return NULL;
>> @@ -362,8 +362,16 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
>>         }
>>         s->s_bdi = &noop_backing_dev_info;
>>         s->s_flags = flags;
>> -       if (s->s_user_ns != &init_user_ns)
>> +
>> +       err = security_sb_device_access(s);
>> +       if (err < 0 && err != -EOPNOTSUPP)
>> +               goto fail;
>> +
>> +       if (err && s->s_user_ns != &init_user_ns)
>>                 s->s_iflags |= SB_I_NODEV;
>> +       else
>> +               s->s_iflags |= SB_I_MANAGED_DEVICES;
> 
> This is my concern, depending on what the LSM hook returns, the
> superblock's flags are set differently, affecting much more than just
> a LSM-based security mechanism.
> 
> LSMs should not be able to undermine, shortcut, or otherwise bypass
> access controls built into other parts of the kernel.  In other words,
> a LSM should only ever be able to deny an operation, it should not be
> able to permit an operation that otherwise would have been denied.

Hmm, OK. Then I can't see to come here any further as we would directly
or indirectly set the superblock flags based on if a security hook is
implemented or not, which I understand now is against LSM architecture.
Thanks Paul for clarification.

Christian, what do you think? 
Maybe we just set the SB_I_NODEV and SB_I_MANGED_DEVICES flag based on
a sysctl at the same place for backward compatibility,
drop the additional security hook and keep the rest as is from your
proposal:

	if (sysctl_managed_devices)
		s->s_iflags |= SB_I_MANAGED_DEVICES;
	else if (s->s_user_ns != &init_user_ns)
		s->s_iflags |= SB_I_NODEV;

A device access managing LSM can then just implement the current
security_sb_alloc() hook to deny creating the super block at all.

> 
>>         INIT_HLIST_NODE(&s->s_instances);
>>         INIT_HLIST_BL_HEAD(&s->s_roots);
>>         mutex_init(&s->s_sync_lock);
>