Message-ID: <CAEf4BzZaOQ+pJw+WN7KrtCxzKHSjvvRJKOue_sfpNVccoBqh6Q@mail.gmail.com>
Date: Thu, 12 Oct 2023 16:51:49 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Paul Moore <paul@...l-moore.com>
Cc: Andrii Nakryiko <andrii@...nel.org>, bpf@...r.kernel.org, netdev@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org, linux-security-module@...r.kernel.org, 
	keescook@...omium.org, brauner@...nel.org, lennart@...ttering.net, 
	kernel-team@...a.com, sargun@...gun.me, selinux@...r.kernel.org
Subject: Re: [PATCH v6 3/13] bpf: introduce BPF token object

On Thu, Oct 12, 2023 at 4:43 PM Paul Moore <paul@...l-moore.com> wrote:
>
> On Thu, Oct 12, 2023 at 5:48 PM Andrii Nakryiko
> <andrii.nakryiko@...il.com> wrote:
> > On Wed, Oct 11, 2023 at 5:31 PM Andrii Nakryiko
> > <andrii.nakryiko@...il.com> wrote:
> > >
> > > ok, so I guess I'll have to add all four variants:
> > > security_bpf_token_{cmd,map_type,prog_type,attach_type}, right?
> > >
> >
> > Thinking a bit more about this, I think this is unnecessary. All these
> > allow checks to control other BPF commands (BPF map creation, BPF
> > program load, bpf() syscall command, etc). We have dedicated LSM hooks
> > for each such operation, most importantly security_bpf_prog_load() and
> > security_bpf_map_create(). I'm extending both of those to be
> > token-aware, and struct bpf_token is one of the input arguments, so if
> > LSM need to override BPF token allow_* checks, they can do in
> > respective more specialized hooks.
> >
> > Adding so many token hooks, one for each different allow mask (or any
> > other sort of "allow something" parameter) seems to be excessive. It
> > will both add too many super-detailed LSM hooks and will unnecessarily
> > tie BPF token implementation details to LSM hook implementations, IMO.
> > I'll send v7 with just security_bpf_token_{create,free}(), please take
> > a look and let me know if you are still not convinced.
>
> I'm hoping my last email better explains why we only really need
> security_bpf_token_cmd() and security_bpf_token_capable() as opposed
> to the full list of security_bpf_token_XXX().  If not, please let me
> know and I'll try to do a better job explaining my reasoning :)
>
> One thing I didn't discuss in my last email was why there is value in
> having both security_bpf_token_{cmd,capable}() as well as
> security_bpf_prog_load(); I'll try to do that below.
>
> As we talked about previously, the reason for having
> security_bpf_prog_load() is to provide a token-aware version of
> security_bpf().  Currently the LSMs enforce their access controls
> around BPF commands using the security_bpf() hook which is
> unfortunately well before we have access to the BPF token.  If we want
> to be able to take the BPF token into account we need to have a hook
> placed after the token is retrieved and validated, hence the
> security_bpf_prog_load() hook.  In a kernel that provides BPF tokens,
> I would expect that LSMs would use security_bpf() to control access to
> BPF operations where a token is not a concern, and new token-aware
> security_bpf_OPERATION() hooks when the LSM needs to consider the BPF
> token.
>
> With the understanding that security_bpf_prog_load() is essentially a
> token-aware version of security_bpf(), I'm hopeful that you can begin
> to understand that it  serves a different purpose than
> security_bpf_token_{cmd,capable}().  The simple answer is that
> security_bpf_token_cmd() applies to more than just BPF_PROG_LOAD, but
> the better answer is that it has the ability to impact more than just
> the LSM authorization decision.  Hooking the LSM into the
> bpf_token_allow_cmd() function allows the LSM to authorize the
> individual command overrides independent of the command specific LSM
> hook, if one exists.  The security_bpf_token_cmd() hook can allow or
> disallow the use of a token for all aspects of a specific BPF
> operation including all of the token related logic outside of the LSM,
> something the security_bpf_prog_load() hook could never do.
>
> I'm hoping that makes sense :)

Yes, I think I understand what you are trying to do, but I need to
clarify something about the bpf_token_allow_cmd() check. It's
meaningless for any command besides BPF_PROG_LOAD, BPF_MAP_CREATE, and
BPF_BTF_LOAD. For any other command you cannot even specify token_fd.
So even if you create a token allowing, say, BPF_MAP_LOOKUP_ELEM, it
has no effect, because BPF_MAP_LOOKUP_ELEM is doing its own checks
based on the provided BPF map FD.

So only if the command is token-aware itself, this allowed_cmd makes
any difference. And in such a case we'll most probably have and/or
want to have an LSM hook for that specific command that accepts struct
bpf_token as an argument. Which is what I did for
security_bpf_prog_load and security_bpf_map_create.

Granted, we don't have any LSM hooks for BPF_BTF_LOAD, mostly because
BTF is just a blob of type info data, and I guess no one bothered to
control the ability to load that. But we can add that easily, if you
think it's important.

So taking everything you said, I still think we don't want a
bpf_token_capable hook, and we'll just be getting targeted LSM hooks
if we need them for some new or existing BPF commands.

Basically, bpf_token's allow_cmd doesn't give you a bypass for
non-token checks we are already doing. So security_bpf() for
everything besides BPF_PROG_LOAD/BPF_MAP_CREATE/BPF_BTF_LOAD is a
completely valid way to restrict everything. You won't miss anything.

>
> --
> paul-moore.com

Powered by blists - more mailing lists