[RFC][PATCH kernel_bpf] honor CAP_NET_ADMIN for BPF_PROG_LOAD

[RFC][PATCH kernel_bpf] honor CAP_NET_ADMIN for BPF_PROG_LOAD

[Andreas Steinmetz]

[sorry for crossposting but this affects both lists]

BPF_PROG_TYPE_SCHED_CLS and BPF_PROG_TYPE_XDP should be allowed
for CAP_NET_ADMIN capability. Nearly everything one can do with
these program types can be done some other way with CAP_NET_ADMIN
capability (e.g. NFQUEUE), but only slower.

This change is similar in behaviour to the /proc/sys/net
CAP_NET_ADMIN exemption.

Overall chances are of increased security as network related
applications do no longer require to keep CAP_SYS_ADMIN
admin capability for network related eBPF operations.

It may well be that other program types than BPF_PROG_TYPE_XDP
and BPF_PROG_TYPE_SCHED_CLS do need the same exemption, though
I do not have sufficient knowledge of other program types
to be able to decide this.

Preloading BPF programs is not possible in case of application
modified or generated BPF programs, so this is no alternative.
The verifier does prevent the BPF program from doing harmful
things anyway.

Signed-off-by: Andreas Steinmetz <ast@domdv.de>

--- a/kernel/bpf/syscall.c	2019-05-28 18:00:40.472841432 +0200
+++ b/kernel/bpf/syscall.c	2019-05-28 18:17:50.162811510 +0200
@@ -1561,8 +1561,13 @@ static int bpf_prog_load(union bpf_attr
 		return -E2BIG;
 	if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
 	    type != BPF_PROG_TYPE_CGROUP_SKB &&
-	    !capable(CAP_SYS_ADMIN))
-		return -EPERM;
+	    !capable(CAP_SYS_ADMIN)) {
+		if (type != BPF_PROG_TYPE_SCHED_CLS &&
+		    type != BPF_PROG_TYPE_XDP)
+			return -EPERM;
+		if(!capable(CAP_NET_ADMIN))
+			return -EPERM;
+	}
 
 	bpf_prog_load_fixup_attach_type(attr);
 	if (bpf_prog_load_check_attach_type(type, attr->expected_attach_type))

Re: [RFC][PATCH kernel_bpf] honor CAP_NET_ADMIN for BPF_PROG_LOAD

[Song Liu]

On Tue, May 28, 2019 at 9:59 AM Andreas Steinmetz <ast@domdv.de> wrote:
>
> [sorry for crossposting but this affects both lists]
>
> BPF_PROG_TYPE_SCHED_CLS and BPF_PROG_TYPE_XDP should be allowed
> for CAP_NET_ADMIN capability. Nearly everything one can do with
> these program types can be done some other way with CAP_NET_ADMIN
> capability (e.g. NFQUEUE), but only slower.
>
> This change is similar in behaviour to the /proc/sys/net
> CAP_NET_ADMIN exemption.
>
> Overall chances are of increased security as network related
> applications do no longer require to keep CAP_SYS_ADMIN
> admin capability for network related eBPF operations.
>
> It may well be that other program types than BPF_PROG_TYPE_XDP
> and BPF_PROG_TYPE_SCHED_CLS do need the same exemption, though
> I do not have sufficient knowledge of other program types
> to be able to decide this.
>
> Preloading BPF programs is not possible in case of application
> modified or generated BPF programs, so this is no alternative.
> The verifier does prevent the BPF program from doing harmful
> things anyway.
>
> Signed-off-by: Andreas Steinmetz <ast@domdv.de>
>
> --- a/kernel/bpf/syscall.c      2019-05-28 18:00:40.472841432 +0200
> +++ b/kernel/bpf/syscall.c      2019-05-28 18:17:50.162811510 +0200
> @@ -1561,8 +1561,13 @@ static int bpf_prog_load(union bpf_attr
>                 return -E2BIG;
>         if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
>             type != BPF_PROG_TYPE_CGROUP_SKB &&

You should extend this if () statement instead of adding another
if () below.

Thanks,
Song

> -           !capable(CAP_SYS_ADMIN))
> -               return -EPERM;
> +           !capable(CAP_SYS_ADMIN)) {
> +               if (type != BPF_PROG_TYPE_SCHED_CLS &&
> +                   type != BPF_PROG_TYPE_XDP)
> +                       return -EPERM;
> +               if(!capable(CAP_NET_ADMIN))
> +                       return -EPERM;
> +       }
>
>         bpf_prog_load_fixup_attach_type(attr);
>         if (bpf_prog_load_check_attach_type(type, attr->expected_attach_type))
>

Re: [RFC][PATCH kernel_bpf] honor CAP_NET_ADMIN for BPF_PROG_LOAD

[Nicolas Dichtel]

Le 28/05/2019 à 18:53, Andreas Steinmetz a écrit :
> [sorry for crossposting but this affects both lists]
> 
> BPF_PROG_TYPE_SCHED_CLS and BPF_PROG_TYPE_XDP should be allowed
> for CAP_NET_ADMIN capability. Nearly everything one can do with
> these program types can be done some other way with CAP_NET_ADMIN
> capability (e.g. NFQUEUE), but only slower.
> 
> This change is similar in behaviour to the /proc/sys/net
> CAP_NET_ADMIN exemption.
> 
> Overall chances are of increased security as network related
> applications do no longer require to keep CAP_SYS_ADMIN
> admin capability for network related eBPF operations.
> 
> It may well be that other program types than BPF_PROG_TYPE_XDP
> and BPF_PROG_TYPE_SCHED_CLS do need the same exemption, though
> I do not have sufficient knowledge of other program types
> to be able to decide this.
> 
> Preloading BPF programs is not possible in case of application
> modified or generated BPF programs, so this is no alternative.
> The verifier does prevent the BPF program from doing harmful
> things anyway.
> 
> Signed-off-by: Andreas Steinmetz <ast@domdv.de>
It makes sense to me.
Do you plan to submit it formally?

Looking a bit more at this topic, I see that most part of the bpf code uses
capable(CAP_NET_ADMIN). I don't see why we cannot use ns_capable(CAP_NET_ADMIN).


Regards,
Nicolas

> 
> --- a/kernel/bpf/syscall.c	2019-05-28 18:00:40.472841432 +0200
> +++ b/kernel/bpf/syscall.c	2019-05-28 18:17:50.162811510 +0200
> @@ -1561,8 +1561,13 @@ static int bpf_prog_load(union bpf_attr
>  		return -E2BIG;
>  	if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
>  	    type != BPF_PROG_TYPE_CGROUP_SKB &&
> -	    !capable(CAP_SYS_ADMIN))
> -		return -EPERM;
> +	    !capable(CAP_SYS_ADMIN)) {
> +		if (type != BPF_PROG_TYPE_SCHED_CLS &&
> +		    type != BPF_PROG_TYPE_XDP)
> +			return -EPERM;
> +		if(!capable(CAP_NET_ADMIN))
> +			return -EPERM;
> +	}
>  
>  	bpf_prog_load_fixup_attach_type(attr);
>  	if (bpf_prog_load_check_attach_type(type, attr->expected_attach_type))
> 

Re: [RFC][PATCH kernel_bpf] honor CAP_NET_ADMIN for BPF_PROG_LOAD

[Andreas Steinmetz]

On Tue, 2019-05-28 at 14:04 -0700, Song Liu wrote:
> >          if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
> >              type != BPF_PROG_TYPE_CGROUP_SKB &&
> 
> You should extend this if () statement instead of adding another
> if () below.

Reworking the if-statement is possible but the result is something like:

        if ((type != BPF_PROG_TYPE_SOCKET_FILTER &&
             type != BPF_PROG_TYPE_CGROUP_SKB &&
             !capable(CAP_SYS_ADMIN)) &&
            !((type == BPF_PROG_TYPE_SCHED_CLS ||
               type == BPF_PROG_TYPE_XDP) &&
              capable(CAP_NET_ADMIN)))
                return -EPERM;

This is not really readable and I do prefer an easy to verify code
when it comes to security, so how about the following version:

Signed-off-by: Andreas Steinmetz <ast@domdv.de>

--- a/kernel/bpf/syscall.c	2019-05-28 18:00:40.472841432 +0200
+++ b/kernel/bpf/syscall.c	2019-06-05 12:34:48.197107612 +0200
@@ -1559,10 +1559,18 @@ static int bpf_prog_load(union bpf_attr
 
 	if (attr->insn_cnt == 0 || attr->insn_cnt > BPF_MAXINSNS)
 		return -E2BIG;
-	if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
-	    type != BPF_PROG_TYPE_CGROUP_SKB &&
-	    !capable(CAP_SYS_ADMIN))
-		return -EPERM;
+	switch (type) {
+	case BPF_PROG_TYPE_SOCKET_FILTER:
+	case BPF_PROG_TYPE_CGROUP_SKB:
+		break;
+	case BPF_PROG_TYPE_SCHED_CLS:
+	case BPF_PROG_TYPE_XDP:
+		if (capable(CAP_NET_ADMIN))
+			break;
+	default:
+		if (!capable(CAP_SYS_ADMIN))
+			return -EPERM;
+	}
 
 	bpf_prog_load_fixup_attach_type(attr);
 	if (bpf_prog_load_check_attach_type(type, attr->expected_attach_type))

Re: [RFC][PATCH kernel_bpf] honor CAP_NET_ADMIN for BPF_PROG_LOAD

[Andreas Steinmetz]

On Mon, 2019-06-03 at 19:12 +0200, Nicolas Dichtel wrote:
> It makes sense to me.
> Do you plan to submit it formally?
> 
> Looking a bit more at this topic, I see that most part of the bpf
> code uses
> capable(CAP_NET_ADMIN). I don't see why we cannot use
> ns_capable(CAP_NET_ADMIN).

If there is a change for this to get accepted, sure, I'm willing to
submit this formally (need some advice, though).

As for capable vs. ns_capable, this is a bit above my knowledge of
kernel internals.

Regards,
Andreas

Re: [RFC][PATCH kernel_bpf] honor CAP_NET_ADMIN for BPF_PROG_LOAD

[Nicolas Dichtel]

Le 05/06/2019 à 12:59, Andreas Steinmetz a écrit :
[snip]
> If there is a change for this to get accepted, sure, I'm willing to
> submit this formally (need some advice, though).
At least, you need to submit it without the RFC tag. RFC patches are not aimed
to be merged.


Regards,
Nicolas