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

[Paul Moore]

On Mon, Jan 8, 2024 at 8:45 AM Michael Weiß
<michael.weiss@xxxxxxxxxxxxxxxxxxx> wrote:
> 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.

No worries, thank you for posting to the LSM list for review and
consideration.  While it may take me a while to review something
(there always appears to be a backlog), I'm always happy to review
patches in this area and work with folks to find a solution.

-- 
paul-moore.com