On 24-Mär 19:27, KP Singh wrote:

On 24-Mär 14:21, Stephen Smalley wrote:

quoted

On Tue, Mar 24, 2020 at 2:06 PM KP Singh [off-list ref] wrote:

quoted

On 24-Mär 11:01, Kees Cook wrote:

quoted

On Tue, Mar 24, 2020 at 01:49:34PM -0400, Stephen Smalley wrote:

quoted

On Tue, Mar 24, 2020 at 12:25 PM Casey Schaufler [off-list ref] wrote:

quoted

On 3/24/2020 7:58 AM, Stephen Smalley wrote:

quoted

On Tue, Mar 24, 2020 at 10:50 AM KP Singh [off-list ref] wrote:

quoted

On 24-Mär 10:35, Stephen Smalley wrote:

quoted

On Mon, Mar 23, 2020 at 12:46 PM KP Singh [off-list ref] wrote:

quoted

From: KP Singh <redacted>

diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index 530d137f7a84..2a8131b640b8 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c

@@ -9,6 +9,9 @@
 #include <linux/btf.h>
 #include <linux/lsm_hooks.h>
 #include <linux/bpf_lsm.h>
+#include <linux/jump_label.h>
+#include <linux/kallsyms.h>
+#include <linux/bpf_verifier.h>

 /* For every LSM hook  that allows attachment of BPF programs, declare a NOP
  * function where a BPF program can be attached as an fexit trampoline.

@@ -27,6 +30,32 @@ noinline __weak void bpf_lsm_##NAME(__VA_ARGS__) {}
 #include <linux/lsm_hook_names.h>
 #undef LSM_HOOK

+#define BPF_LSM_SYM_PREFX  "bpf_lsm_"
+
+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;

I had asked before, and will ask again: please provide an explicit LSM
hook for mediating whether one can make changes to the LSM hooks.
Neither CAP_MAC_ADMIN nor CAP_SYS_ADMIN suffices to check this for SELinux.

What do you think about:

  int security_check_mutable_hooks(void)

Do you have any suggestions on the signature of this hook? Does this
hook need to be BPF specific?

I'd do something like int security_bpf_prog_attach_security(const
struct bpf_prog *prog) or similar.
Then the security module can do a check based on the current task
and/or the prog.  We already have some bpf-specific hooks.

I *strongly* disagree with Stephen on this. KRSI and SELinux are peers.
Just as Yama policy is independent of SELinux policy so KRSI policy should
be independent of SELinux policy. I understand the argument that BDF programs
ought to be constrained by SELinux, but I don't think it's right. Further,
we've got unholy layering when security modules call security_ functions.
I'm not saying there is no case where it would be appropriate, but this is not
one of them.

I explained this previously.  The difference is that the BPF programs
are loaded from a userspace
process, not a kernel-resident module.  They already recognize there
is a difference here or
they wouldn't have the CAP_MAC_ADMIN check above in their patch.  The
problem with that
check is just that CAP_MAC_ADMIN doesn't necessarily mean fully
privileged with respect to
SELinux, which is why I want an explicit hook.  This gets a NAK from
me until there is such a hook.

Doesn't the existing int (*bpf_prog)(struct bpf_prog *prog); cover
SELinux's need here? I.e. it can already examine that a hook is being
created for the LSM (since it has a distinct type, etc)?

I was about to say the same, specifically for the BPF use-case, we do
have the "bpf_prog" i.e. :

"Do a check when the kernel generate and return a file descriptor for
eBPF programs."

SELinux can implement its policy logic for BPF_PROG_TYPE_LSM by
providing a callback for this hook.

Ok.  In that case do we really need the capable() check here at all?

We do not have a specific capable check for BPF_PROG_TYPE_LSM programs
now. There is a general check which requires CAP_SYS_ADMIN when
unprivileged BPF is disabled:

in kernel/bpf/sycall.c:

        if (sysctl_unprivileged_bpf_disabled && !capable(CAP_SYS_ADMIN))
	        return -EPERM;

AFAIK, Most distros disable unprivileged eBPF.

Now that I look at this, I think we might need a CAP_MAC_ADMIN check
though as unprivileged BPF being enabled will result in an
unprivileged user being able to load MAC policies.

Actually we do have an extra check for loading BPF programs:


in kernel/bpf/syscall.c:bpf_prog_load

	if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
	    type != BPF_PROG_TYPE_CGROUP_SKB &&
	    !capable(CAP_SYS_ADMIN))
		return -EPERM;

Do you think we still need a CAP_MAC_ADMIN check for LSM programs?

- KP

- KP