lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
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