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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAHC9VhRcRT9DKtxmtamBCRvNF+dWW4MGx2KtS3CA3xcCGZf+ww@mail.gmail.com>
Date: Mon, 8 Jan 2024 11:34:15 -0500
From: Paul Moore <paul@...l-moore.com>
To: Michael Weiß <michael.weiss@...ec.fraunhofer.de>
Cc: Christian Brauner <brauner@...nel.org>, Alexei Starovoitov <alexei.starovoitov@...il.com>, 
	Alexander Mikhalitsyn <alexander@...alicyn.com>, Alexei Starovoitov <ast@...nel.org>, 
	Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, 
	Martin KaFai Lau <martin.lau@...ux.dev>, Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>, 
	John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>, 
	Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>, 
	Quentin Monnet <quentin@...valent.com>, Alexander Viro <viro@...iv.linux.org.uk>, 
	Miklos Szeredi <miklos@...redi.hu>, Amir Goldstein <amir73il@...il.com>, 
	"Serge E. Hallyn" <serge@...lyn.com>, bpf <bpf@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, 
	Linux-Fsdevel <linux-fsdevel@...r.kernel.org>, 
	LSM List <linux-security-module@...r.kernel.org>, gyroidos@...ec.fraunhofer.de
Subject: Re: [RFC PATCH v3 3/3] devguard: added device guard for mknod in
 non-initial userns

On Mon, Jan 8, 2024 at 8:45 AM Michael Weiß
<michael.weiss@...ec.fraunhofer.de> wrote:
> On 29.12.23 23:31, Paul Moore wrote:
> > On Wed, Dec 27, 2023 at 9:31 AM Michael Weiß
> > <michael.weiss@...ec.fraunhofer.de> 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