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
