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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAHC9VhSZDMWJ_kh+RaB6dsPLQjkrjDY4bVkqsFDG3JtjinT_bQ@mail.gmail.com>
Date: Fri, 22 Dec 2023 18:39:53 -0500
From: Paul Moore <paul@...l-moore.com>
To: Christian Brauner <brauner@...nel.org>
Cc: Alexei Starovoitov <alexei.starovoitov@...il.com>, 
	Michael Weiß <michael.weiss@...ec.fraunhofer.de>, 
	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, Dec 18, 2023 at 7:30 AM Christian Brauner <brauner@...nel.org> wrote:
> I'm not generally opposed to kfuncs ofc but here it just seems a bit
> pointless. What we want is to keep SB_I_{NODEV,MANAGED_DEVICES} confined
> to alloc_super(). The only central place it's raised where we control
> all locking and logic. So it doesn't even have to appear in any
> security_*() hooks.
>
> diff --git a/security/security.c b/security/security.c
> index 088a79c35c26..bf440d15615d 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1221,6 +1221,33 @@ int security_sb_alloc(struct super_block *sb)
>         return rc;
>  }
>
> +/*
> + * security_sb_device_access() - Let LSMs handle device access
> + * @sb: filesystem superblock
> + *
> + * Let an LSM take over device access management for this superblock.
> + *
> + * Return: Returns 1 if LSMs handle device access, 0 if none does and -ERRNO on
> + *         failure.
> + */
> +int security_sb_device_access(struct super_block *sb)
> +{
> +       int thisrc;
> +       int rc = LSM_RET_DEFAULT(sb_device_access);
> +       struct security_hook_list *hp;
> +
> +       hlist_for_each_entry(hp, &security_hook_heads.sb_device_access, list) {
> +               thisrc = hp->hook.sb_device_access(sb);
> +               if (thisrc < 0)
> +                       return thisrc;
> +               /* At least one LSM claimed device access management. */
> +               if (thisrc == 1)
> +                       rc = 1;
> +       }
> +
> +       return rc;
> +}

I worry that this hook, and the way it is plumbed into alloc_super()
below, brings us back to the problem of authoritative LSM hooks which
is something I can't support at this point in time.  The same can be
said for a LSM directly flipping bits in the superblock struct.

The LSM should not grant any additional privilege, either directly in
the LSM code, or indirectly via the caller; the LSM should only
restrict operations which would have otherwise been allowed.

The LSM should also refrain from modifying any kernel data structures
that do not belong directly to the LSM.  A LSM caller may modify
kernel data structures that it owns based on the result of the LSM
hook, so long as those modifications do not grant additional privilege
as described above.

> diff --git a/fs/super.c b/fs/super.c
> index 076392396e72..2295c0f76e56 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 err, i;
>
>         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)
> +               goto fail;
> +
> +       if (err)
> +               s->s_iflags |= SB_I_MANAGED_DEVICES;
> +       else if (s->s_user_ns != &init_user_ns)
>                 s->s_iflags |= SB_I_NODEV;
> +
>         INIT_HLIST_NODE(&s->s_instances);
>         INIT_HLIST_BL_HEAD(&s->s_roots);
>         mutex_init(&s->s_sync_lock);

-- 
paul-moore.com