On Fri, Mar 27, 2020 at 09:36:15AM -0700, Casey Schaufler wrote:

On 3/27/2020 6:43 AM, Stephen Smalley wrote:

quoted

On 3/27/20 8:41 AM, KP Singh wrote:

quoted

On 27-Mär 08:27, Stephen Smalley wrote:

quoted

On 3/26/20 8:24 PM, James Morris wrote:

quoted

On Thu, 26 Mar 2020, KP Singh wrote:

quoted

+int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
+            const struct bpf_prog *prog)
+{
+    /* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks
+     */
+    if (!capable(CAP_MAC_ADMIN))
+        return -EPERM;
+

Stephen, can you confirm that your concerns around this are resolved
(IIRC, by SELinux implementing a bpf_prog callback) ?

I guess the only residual concern I have is that CAP_MAC_ADMIN means
something different to SELinux (ability to get/set file security contexts
unknown to the currently loaded policy), so leaving the CAP_MAC_ADMIN check
here (versus calling a new security hook here and checking CAP_MAC_ADMIN in
the implementation of that hook for the modules that want that) conflates
two very different things.  Prior to this patch, there are no users of
CAP_MAC_ADMIN outside of individual security modules; it is only checked in
module-specific logic within apparmor, safesetid, selinux, and smack, so the
meaning was module-specific.

As we had discussed, We do have a security hook as well:

https://lore.kernel.org/bpf/20200324180652.GA11855@chromium.org/ (local)

The bpf_prog hook which can check for BPF_PROG_TYPE_LSM and implement
module specific logic for LSM programs. I thougt that was okay?

Kees was in favor of keeping the CAP_MAC_ADMIN check here:

https://lore.kernel.org/bpf/202003241133.16C02BE5B@keescook (local)

If you feel strongly and Kees agrees, we can remove the CAP_MAC_ADMIN
check here, but given that we already have a security hook that meets
the requirements, we probably don't need another one.

I would favor removing the CAP_MAC_ADMIN check here, and implementing it in a bpf_prog hook for Smack and AppArmor if they want that.  SELinux would implement its own check in its existing bpf_prog hook.

The whole notion of one security module calling into another for permission
to do something still gives me the heebee jeebees, but if more nimble minds
than mine think this is a good idea I won't nack it.

Well, it's a hook into BPF prog creation, not the BPF LSM specifically,
so that's why I think it's general enough control without it being
directly weird. :)

As far as dropping CAP_MAC_ADMIN, yeah, that should be fine. Creating LSM
BPF programs already requires CAP_SYS_ADMIN, so for SELinux-less systems,
that's likely fine. If we need to change the BPF program creation access
control in the future we can revisit it then.

-- 
Kees Cook