On Thu, Jan 11, 2024 at 2:38 AM Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> > > The current check is inconsisent. It special-cases init_user_ns. The
> > > correct thing to do for what you're intending imho is:
> > >
> > > bool bpf_token_capable(const struct bpf_token *token, int cap)
> > > {
> > > struct user_namespace *userns = &init_user_ns;
> > >
> > > if (token)
> > > userns = token->userns;
> > > if (ns_capable(userns, cap))
> > > return true;
> > > return cap != CAP_SYS_ADMIN && ns_capable(userns, CAP_SYS_ADMIN))
> > >
> > > }
> >
> > Unfortunately the above becomes significantly more hairy when LSM
> > (security_bpf_token_capable) gets involved, while preserving the rule
> > "if token doesn't give rights, fall back to init userns checks".
>
> Why? Please explain your reasoning in detail.
Why which part? About LSM interaction making this much hairier? Then see below.
But if your "why?" is about "pretend no token, if token doesn't give
rights", then that's what I tried to explain in my last email(s). It
significantly alters (for the worse) user-space integration story
(providing a token can be a regression, so now it's not safe to
opportunistically try to create and use BPF token; on the other hand,
automatically retrying inside libbpf makes for confusing user
experience and inefficiencies). Please let me know which parts are not
clear.
>
> >
> > I'm happy to accommodate any implementation of bpf_token_capable() as
> > long as it behaves as discussed above and also satisfies Paul's
> > requirement that capability checks should happen before LSM checks.
> >
> > >
> > > Because any caller located in an ancestor user namespace of
> > > token->user_ns will be privileged wrt to the token's userns as long as
> > > they have that capability in their user namespace.
> >
> > And with `current_user_ns() == token->userns` check we won't be using
> > token->userns while the caller is in ancestor user namespace, we'll
> > use capable() check, which will succeed only in init user_ns, assuming
> > corresponding CAP_xxx is actually set.
>
> Why? This isn't how any of our ns_capable() logic works.
>
> This basically argues that anyone in an ancestor user namespace is not
> allowed to operate on any of their descendant child namespaces unless
> they are in the init_user_ns.
>
> But that's nonsense as I'm trying to tell you. Any process in an
> ancestor user namespace that is privileged over the child namespace can
> just setns() into it and then pass that bpf_token_capable() check by
> supplying the token.
>
> At this point just do it properly and allow callers that are privileged
> in the token user namespace to load bpf programs. It also means you get
> user namespace nesting done properly.
Ok, I see. This `current_user_ns() == token->userns` check prevents
this part of cap_capable() to ever be exercised:
if ((ns->parent == cred->user_ns) && uid_eq(ns->owner, cred->euid))
return 0;
Got it. I'm all for not adding any unnecessary restrictions.
>
> >
> > >
> > > For example, if the caller is in the init_user_ns and permissions
> > > for CAP_WHATEVER is checked for in token->user_ns and the caller has
> > > CAP_WHATEVER in init_user_ns then they also have it in all
> > > descendant user namespaces.
> >
> > Right, so if they didn't use a token they would still pass
> > capable(CAP_WHATEVER), right?
>
> Yes, I'm trying to accomodate your request but making it work
> consistently.
>
> >
> > >
> > > The original intention had been to align with what we require during
> > > token creation meaning that once a token has been created interacting
> > > with this token is specifically confined to caller's located in the
> > > token's user namespace.
> > >
> > > If that's not the case then it doesn't make sense to not allow
> > > permission checking based on regular capability semantics. IOW, why
> > > special case init_user_ns if you're breaking the confinement restriction
> > > anyway.
> >
> > I'm sorry, perhaps I'm dense, but with `current_user_ns() ==
> > token->userns` check I think we do fulfill the intention to not allow
> > using a token in a userns different from the one in which it was
> > created. If that condition isn't satisfied, the token is immediately
>
> My request originally was about never being able to interact with a
> token outside of that userns. This is different as you provide an escape
> hatch to init_user_ns. But if you need that and ignore the token then
> please do it properly. That's what I'm trying to tell you. See below.
Yes, I do need that. Thanks for providing the full code implementation
(including LSM), it's much easier this way to converge. Let's see
below.
>
> > ignored. So you can't use a token from another userns for anything,
> > it's just not there, effectively.
> >
> > And as I tried to explain above, I do think that ignoring the token
> > instead of erroring out early is what we want to provide good
> > user-space ecosystem integration of BPF token.
>
> There is no erroring out early in. It's:
>
> (1) Has a token been provided and is the caller capable wrt to the
> namespace of the token? Any caller in an ancestor user namespace
> that has the capability in that user namespace is capable wrt to
> that token. That __includes__ a callers in the init_user_ns. IOW,
> you don't need to fallback to any special checking for init_user_ns.
> It is literally covered in the if (token) branch with the added
> consistency that a process in an ancestor user namespace is
> privileged wrt to that token as well.
>
> (2) No token has been provided. Then do what we always did and perform
> the capability checks based on the initial user namespace.
>
> The only thing that you then still need is add that token_capable() hook
> in there:
>
> bool bpf_token_capable(const struct bpf_token *token, int cap)
> {
> bool has_cap;
> struct user_namespace *userns = &init_user_ns;
>
> if (token)
> userns = token->userns;
> if (ns_capable(userns, cap))
Here, we still need to check security_bpf_token_capable(token, cap)
result (and only if token != NULL). And if LSM returns < 0, then drop
the token and do the original init userns check.
And I just realized that my original implementation has the same
problem. In my current implementation if we have a token we will
terminate at LSM call, regardless if LSM allows or disallows the
token. But that's inconsistent behavior and shouldn't be like that.
I will add new tests that validate LSM interactions in the next revision.
> return true;
> if (cap != CAP_SYS_ADMIN && ns_capable(userns, CAP_SYS_ADMIN))
> return token ? security_bpf_token_capable(token, cap) == 0 : true;
here as well, even if we have a token which passes ns_capable() check,
but LSM rejects this token, we still need to forget about the token
and do capable() checks in init userns.
> return false;
> }
>
> Or write it however you like. I think this is way more consistent and
> gives you a more flexible permission model.
Yes, I like it, thanks. Taking into account fixed LSM interactions,
here's what I came up with. Yell if you can spot anything wrong (or
just hate the style). I did have a version without extra function,
just setting the token to NULL and "goto again" approach, but I think
it's way less readable and harder to follow. So this is my version
right now:
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));
}
static bool token_capable(const struct bpf_token *token, int cap)
{
struct user_namespace *userns;
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;
}
bool bpf_token_capable(const struct bpf_token *token, int cap)
{
/* BPF token allows ns_capable() level of capabilities, but if it
* doesn't grant required capabilities, ignore token and fallback to
* init userns-based checks
*/
if (token && token_capable(token, cap))
return true;
return token_capable(NULL, cap);
}
