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

[Paul Moore]

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.

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

-- 
paul-moore.com