[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAEf4BzYMrvtTjkBUWOk1TKi8qiBbwv1xv=eJeF3j3QrY1M=h3g@mail.gmail.com>
Date: Mon, 8 Jan 2024 16:07:25 -0800
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Paul Moore <paul@...l-moore.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 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.
Alternatively we could do something like:
struct user_namespace *ns = &init_ns;
if (token && current_user_ns() == token->userns)
ns = token->user_ns;
else
token = NULL;
if (ns_capable(ns, cap) || (cap != CAP_SYS_ADMIN && ns_capable(ns,
CAP_SYS_ADMIN)) {
if (token)
return security_bpf_token_capable(token, cap) == 0;
return true;
}
return false;
Or something along those lines? I don't particularly care (though the
latter seems a bit more ceremonious), so please let me know the
preference, if any.
>
> --
> paul-moore.com
Powered by blists - more mailing lists