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] [thread-next>] [day] [month] [year] [list]

Message-ID: <6c2eb494-0cbf-4493-ae31-6dea519c4715@aisec.fraunhofer.de>
Date: Mon, 8 Jan 2024 14:44:59 +0100
From: Michael Weiß <michael.weiss@...ec.fraunhofer.de>
To: Paul Moore <paul@...l-moore.com>, Christian Brauner <brauner@...nel.org>
CC: 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 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.

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);
>