On Sat, Jan 13, 2024 at 06:29:33PM -0800, Andrii Nakryiko wrote:
> On Fri, Jan 12, 2024 at 11:17 AM Christian Brauner <brauner@xxxxxxxxxx> 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;
Thank you! Looks good,
Acked-by: Christian Brauner <brauner@xxxxxxxxxx>
