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]

Message-ID: <CAHC9VhSwgY8cCX+eR7=+gb=-Q2pC9Z_jstf0xHD4kMA7vpiDOw@mail.gmail.com>
Date: Wed, 10 Jan 2024 14:29:01 -0500
From: Paul Moore <paul@...l-moore.com>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: Linus Torvalds <torvalds@...uxfoundation.org>, Andrii Nakryiko <andrii@...nel.org>, bpf@...r.kernel.org, 
	netdev@...r.kernel.org, brauner@...nel.org, 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 Mon, Jan 8, 2024 at 7:07 PM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
> On Mon, Jan 8, 2024 at 8:45 AM Paul Moore <paul@...l-moore.com> wrote:
> >
> > On Fri, Jan 5, 2024 at 4:45 PM Linus Torvalds
> > <torvalds@...uxfoundation.org> wrote:
> > > On Wed, 3 Jan 2024 at 14:21, Andrii Nakryiko <andrii@...nel.org> wrote:
> > > >
> > > > +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))
> > > > +                       return true;
> > > > +               if (cap != CAP_SYS_ADMIN && ns_capable(token->userns, CAP_SYS_ADMIN))
> > > > +                       return true;
> > > > +       }
> > > > +       /* otherwise fallback to capable() checks */
> > > > +       return capable(cap) || (cap != CAP_SYS_ADMIN && capable(CAP_SYS_ADMIN));
> > > > +}
> > >
> > > This *feels* like it should be written as
> > >
> > >     bool bpf_token_capable(const struct bpf_token *token, int cap)
> > >     {
> > >         struct user_namespace *ns = &init_ns;
> > >
> > >         /* 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)
> > >                 ns = token->userns;
> > >         return ns_capable(ns, cap) ||
> > >                 (cap != CAP_SYS_ADMIN && capable(CAP_SYS_ADMIN));
> > >     }
> > >
> > > And yes, I realize that the function will end up later growing a
> > >
> > >         security_bpf_token_capable(token, cap)
> > >
> > > test inside that 'if (token ..)' statement, and this would change the
> > > order of that test so that the LSM hook would now be done before the
> > > capability checks are done, but that all still seems just more of an
> > > argument for the simplification.
> >
> > I have no problem with rewriting things, my only ask is that we stick
> > with the idea of doing the capability checks before the LSM hook.  The
> > DAC-before-MAC (capability-before-LSM) pattern is one we try to stick
> > to most everywhere in the kernel and deviating from it here could
> > potentially result in some odd/unexpected behavior from a user
> > perspective.
>
> Makes sense, Paul. With the suggested rewrite we'll get an LSM call
> before we get to ns_capable() (which we avoid doing in BPF code base,
> generally speaking, after someone called this out earlier). Hmm...
>
> I guess it will be better to keep this logic as is then, I believe it
> was more of a subjective stylistical nit from Linus, so it probably is
> ok to keep existing code.

I didn't read Linus' reply as a mandate, more as a
this-would-be-nice-to-have, and considering the access control
ordering I would just stick with what you have (ignoring Christian's
concerns, I'm only commenting on the LSM related stuff here).

If Linus is *really* upset with how the code is written I suspect
we'll hear from him on that.

-- 
paul-moore.com