On Mon, Jan 8, 2024 at 4:02 AM Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> On Fri, Jan 05, 2024 at 02:18:40PM -0800, Andrii Nakryiko wrote:
> > On Fri, Jan 5, 2024 at 1:45 PM Linus Torvalds
> > <torvalds@xxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > Ok, I've gone through the whole series now, and I don't find anything
> > > objectionable.
> >
> > That's great, thanks for reviewing!
> >
> > >
> > > Which may only mean that I didn't notice something, of course, but at
> > > least there's nothing I'd consider obvious.
> > >
> > > I keep coming back to this 03/29 patch, because it's kind of the heart
> > > of it, and I have one more small nit, but it's also purely stylistic:
> > >
> > > On Wed, 3 Jan 2024 at 14:21, Andrii Nakryiko <andrii@xxxxxxxxxx> 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.
> > >
> > > So the end result would be something like
> > >
> > > bool bpf_token_capable(const struct bpf_token *token, int cap)
> > > {
> > > struct user_namespace *ns = &init_ns;
> > >
> > > if (token && current_user_ns() == token->userns) {
> > > if (security_bpf_token_capable(token, cap) < 0)
> > > return false;
> > > ns = token->userns;
> > > }
> > > return ns_capable(ns, cap) ||
> > > (cap != CAP_SYS_ADMIN && capable(CAP_SYS_ADMIN));
> > > }
> >
> > Yep, it makes sense to use ns_capable with init_ns. I'll change those
> > two patches to end up with something like what you suggested here.
> >
> > >
> > > although I feel that with that LSM hook, maybe this all should return
> > > the error code (zero or negative), not a bool for success?
> > >
> > > Also, should "current_user_ns() != token->userns" perhaps be an error
> > > condition, rather than a "fall back to init_ns" condition?
> > >
> > > Again, none of this is a big deal. I do think you're dropping the LSM
> > > error code on the floor, and are duplicating the "ns_capable()" vs
> > > "capable()" logic as-is, but none of this is a deal breaker, just more
> > > of my commentary on the patch and about the logic here.
> > >
> > > And yeah, I don't exactly love how you say "ok, if there's a token and
> > > it doesn't match, I'll not use it" rather than "if the token namespace
> > > doesn't match, it's an error", but maybe there's some usability issue
> > > here?
> >
> > Yes, usability was the primary concern. The overall idea with BPF
>
> NAK on not restricting this to not erroring out on current_user_ns()
> != token->user_ns. I've said this multiple times before.
I do restrict token usage to *exact* userns in which the token was
created. See bpf_token_capable()'s
if (token && current_user_ns() == token->userns) { ... }
and in bpf_token_allow_cmd():
if (!token || current_user_ns() != token->userns)
return false;
So I followed what you asked in [1] (just like I said I will in [2]),
unless I made some stupid mistake which I cannot even see.
What we are discussing here is a different question. It's the
difference between erroring out (that is, failing whatever BPF
operation was attempted with such token, i.e., program loading or map
creation) vs ignoring the token altogether and just using
init_ns-based capable() checks. And the latter is vastly more user
friendly when considering end-to-end integration with user-space
applications and tooling. And doesn't seem to open any security holes.
[1] https://lore.kernel.org/r/20231130-katzen-anhand-7ad530f187da@brauner
[2] https://lore.kernel.org/all/CAEf4BzZA2or352VkAaBsr+fsWAGO1Cs_gonH7Ffm5emXGE+2Ug@xxxxxxxxxxxxxx/
