netdev - Re: [PATCH bpf-next 03/29] bpf: introduce BPF token object

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]
Date: Sat, 13 Jan 2024 18:29:33 -0800
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Christian Brauner <brauner@...nel.org>
Cc: Linus Torvalds <torvalds@...uxfoundation.org>, Andrii Nakryiko <andrii@...nel.org>, bpf@...r.kernel.org, 
	netdev@...r.kernel.org, paul@...l-moore.com, linux-fsdevel@...r.kernel.org, 
	linux-security-module@...r.kernel.org, kernel-team@...a.com
Subject: Re: [PATCH bpf-next 03/29] bpf: introduce BPF token object

On Fri, Jan 12, 2024 at 11:17 AM Christian Brauner <brauner@...nel.org> wrote:
>
> > > My point is that the capable logic will walk upwards the user namespace
> > > hierarchy from the token->userns until the user namespace of the caller
> > > and terminate when it reached the init_user_ns.
> > >
> > > A caller is located in some namespace at the point where they call this
> > > function. They provided a token. The caller isn't capable in the
> > > namespace of the token so the function falls back to init_user_ns. Two
> > > interesting cases:
> > >
> > > (1) The caller wasn't in an ancestor userns of the token. If that's the
> > >     case then it follows that the caller also wasn't in the init_user_ns
> > >     because the init_user_ns is a descendant of all other user
> > >     namespaces. So falling back will fail.
> >
> > agreed
> >
> > >
> > > (2) The caller was in the same or an ancestor user namespace of the
> > >     token but didn't have the capability in that user namespace:
> > >
> > >      (i) They were in a non-init_user_ns. Therefore they can't be
> > >          privileged in init_user_ns.
> > >     (ii) They were in init_user_ns. Therefore, they lacked privileges in
> > >          the init_user_ns.
> > >
> > > In both cases your fallback will do nothing iiuc.
> >
> > agreed as well
> >
> > And I agree in general that there isn't a *practically useful* case
> > where this would matter much. But there is still (at least one) case
> > where there could be a regression: if token is created in
> > init_user_ns, caller has CAP_BPF in init_user_ns, caller passes that
> > token to BPF_PROG_LOAD, and LSM policy rejects that token in
> > security_bpf_token_capable(). Without the above implementation such
> > operation will be rejected, even though if there was no token passed
> > it would succeed. With my implementation above it will succeed as
> > expected.
>
> If that's the case then prevent the creation of tokens in the
> init_user_ns and be done with it. If you fallback anyway then this is
> the correct solution.
>
> Make this change, please. I'm not willing to support this weird fallback
> stuff which is even hard to reason about.

Alright, added an extra check. Ok, so in summary I have the changes
below compared to v1 (plus a few extra LSM-related test cases added):

diff --git a/kernel/bpf/token.c b/kernel/bpf/token.c
index a86fccd57e2d..7d04378560fd 100644
--- a/kernel/bpf/token.c
+++ b/kernel/bpf/token.c
@@ -9,18 +9,22 @@
 #include <linux/user_namespace.h>
 #include <linux/security.h>

+static bool bpf_ns_capable(struct user_namespace *ns, int cap)
+{
+       return ns_capable(ns, cap) || (cap != CAP_SYS_ADMIN &&
ns_capable(ns, CAP_SYS_ADMIN));
+}
+
 bool bpf_token_capable(const struct bpf_token *token, int cap)
 {
-       /* BPF token allows ns_capable() level of capabilities, but only if
-        * token's userns is *exactly* the same as current user's userns
-        */
-       if (token && current_user_ns() == token->userns) {
-               if (ns_capable(token->userns, cap) ||
-                   (cap != CAP_SYS_ADMIN && ns_capable(token->userns,
CAP_SYS_ADMIN)))
-                       return security_bpf_token_capable(token, cap) == 0;
-       }
-       /* otherwise fallback to capable() checks */
-       return capable(cap) || (cap != CAP_SYS_ADMIN && capable(CAP_SYS_ADMIN));
+       struct user_namespace *userns;
+
+       /* BPF token allows ns_capable() level of capabilities */
+       userns = token ? token->userns : &init_user_ns;
+       if (!bpf_ns_capable(userns, cap))
+               return false;
+       if (token && security_bpf_token_capable(token, cap) < 0)
+               return false;
+       return true;
 }

 void bpf_token_inc(struct bpf_token *token)
@@ -32,7 +36,7 @@ static void bpf_token_free(struct bpf_token *token)
 {
        security_bpf_token_free(token);
        put_user_ns(token->userns);
-       kvfree(token);
+       kfree(token);
 }

 static void bpf_token_put_deferred(struct work_struct *work)
@@ -152,6 +156,12 @@ int bpf_token_create(union bpf_attr *attr)
                goto out_path;
        }

+       /* Creating BPF token in init_user_ns doesn't make much sense. */
+       if (current_user_ns() == &init_user_ns) {
+               err = -EOPNOTSUPP;
+               goto out_path;
+       }
+
        mnt_opts = path.dentry->d_sb->s_fs_info;
        if (mnt_opts->delegate_cmds == 0 &&
            mnt_opts->delegate_maps == 0 &&
@@ -179,7 +189,7 @@ int bpf_token_create(union bpf_attr *attr)
                goto out_path;
        }

-       token = kvzalloc(sizeof(*token), GFP_USER);
+       token = kzalloc(sizeof(*token), GFP_USER);
        if (!token) {
                err = -ENOMEM;
                goto out_file;