Re: [PATCH] btf: Expose kernel BTF only to tasks with CAP_PERFMON

Re: [PATCH] btf: Expose kernel BTF only to tasks with CAP_PERFMON

[Andrii Nakryiko]

+ bpf@vger.kernel.org

On Wed, Oct 28, 2020 at 1:40 PM Dan Aloni <dan@kernelim.com> wrote:
>
> Commit 341dfcf8d78e ("btf: expose BTF info through sysfs") added a sysfs
> file that exposes to userspace kernel BTF information which allows
> userspace to deduce the structure layout of all kernel internal
> structures.
>
> This file is currently accessible to unprivileged users, without
> requiring any special capability. Given that knowledge on kernel
> structure layout is useful for dynamically building local privilege
> escalation exploit in userspace, access to this file should be
> restricted.

So is /proc/config.gz, which is also very helpful in understanding
what exactly is there in the kernel. So seems to be
/boot/vmlinux-$(uname -r), which has exactly the same BTF data and
more.

Guarding /sys/kernel/bpf/vmlinux behind CAP_PERFMON would break a lot
of users relying on BTF availability to build their BPF applications.
We shouldn't expect developers to build their applications under root.
But that's what this patch is trying to do.

>
> Fixes: 341dfcf8d78e ("btf: expose BTF info through sysfs")
> Co-developed-by: Liran Alon <liran@amazon.com>
> Signed-off-by: Liran Alon <liran@amazon.com>
> Signed-off-by: Dan Aloni <dan@kernelim.com>
> ---
>  kernel/bpf/sysfs_btf.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/kernel/bpf/sysfs_btf.c b/kernel/bpf/sysfs_btf.c
> index 11b3380887fa..c985d42dfa49 100644
> --- a/kernel/bpf/sysfs_btf.c
> +++ b/kernel/bpf/sysfs_btf.c
> @@ -7,6 +7,7 @@
>  #include <linux/kobject.h>
>  #include <linux/init.h>
>  #include <linux/sysfs.h>
> +#include <linux/capability.h>
>
>  /* See scripts/link-vmlinux.sh, gen_btf() func for details */
>  extern char __weak __start_BTF[];
> @@ -17,6 +18,9 @@ btf_vmlinux_read(struct file *file, struct kobject *kobj,
>                  struct bin_attribute *bin_attr,
>                  char *buf, loff_t off, size_t len)
>  {
> +       if (!perfmon_capable())
> +               return -EACCES;
> +
>         memcpy(buf, __start_BTF + off, len);
>         return len;
>  }
> --
> 2.26.2
>

Re: [PATCH] btf: Expose kernel BTF only to tasks with CAP_PERFMON

[Alon, Liran]

On 28/10/2020 23:56, Andrii Nakryiko wrote:
> + bpf@vger.kernel.org
You shouldn't Cc public email lists for a patch submitted to 
security@kernel.org.
>
> On Wed, Oct 28, 2020 at 1:40 PM Dan Aloni <dan@kernelim.com> wrote:
>> Commit 341dfcf8d78e ("btf: expose BTF info through sysfs") added a sysfs
>> file that exposes to userspace kernel BTF information which allows
>> userspace to deduce the structure layout of all kernel internal
>> structures.
>>
>> This file is currently accessible to unprivileged users, without
>> requiring any special capability. Given that knowledge on kernel
>> structure layout is useful for dynamically building local privilege
>> escalation exploit in userspace, access to this file should be
>> restricted.
> So is /proc/config.gz, which is also very helpful in understanding
> what exactly is there in the kernel.
Viewing kernel build config is more like querying supported kernel features.
I don't consider it as a meaningful information disclosure, as I see 
disclosing
the kernel internal struct layout.
> So seems to be
> /boot/vmlinux-$(uname -r), which has exactly the same BTF data and
> more.
I agree. True. Good enough argument for dropping this patch.
>
> Guarding /sys/kernel/bpf/vmlinux behind CAP_PERFMON would break a lot
> of users relying on BTF availability to build their BPF applications.
True. If this patch is applied, would need to at least be behind an 
optin knob. Similar to dmesg_restrict.

Re: [PATCH] btf: Expose kernel BTF only to tasks with CAP_PERFMON

[Alexei Starovoitov]

On Thu, Oct 29, 2020 at 12:30:49AM +0200, Alon, Liran wrote:
> > Guarding /sys/kernel/bpf/vmlinux behind CAP_PERFMON would break a lot
> > of users relying on BTF availability to build their BPF applications.
> True. If this patch is applied, would need to at least be behind an optin
> knob. Similar to dmesg_restrict.

It's not going to be applied. If a file shouldn't be read by a user
it should have appropriate file permissions instead of 444.
Checking capable() in read() is very non-unix way to deal with permissions.

Re: [PATCH] btf: Expose kernel BTF only to tasks with CAP_PERFMON

[Willy Tarreau]

On Wed, Oct 28, 2020 at 04:06:02PM -0700, Alexei Starovoitov wrote:
> On Thu, Oct 29, 2020 at 12:30:49AM +0200, Alon, Liran wrote:
> > > Guarding /sys/kernel/bpf/vmlinux behind CAP_PERFMON would break a lot
> > > of users relying on BTF availability to build their BPF applications.
> > True. If this patch is applied, would need to at least be behind an optin
> > knob. Similar to dmesg_restrict.
> 
> It's not going to be applied. If a file shouldn't be read by a user
> it should have appropriate file permissions instead of 444.
> Checking capable() in read() is very non-unix way to deal with permissions.

Not only it's a non-unix way, both don't achieve the same goals at all!

One checks for permissions at open() time and may for example allow a
process to drop its uid after opening, while the other one allows to
filter who can really read it, particularly in case the FD is inherited
between processes. With this said, I don't see why there would be a
special case for this one, it should definitely stick to file permissions
only.

Willy

Re: [PATCH] btf: Expose kernel BTF only to tasks with CAP_PERFMON

[Michael Ellerman]

Willy Tarreau <w@1wt.eu> writes:
> On Wed, Oct 28, 2020 at 04:06:02PM -0700, Alexei Starovoitov wrote:
>> On Thu, Oct 29, 2020 at 12:30:49AM +0200, Alon, Liran wrote:
>> > > Guarding /sys/kernel/bpf/vmlinux behind CAP_PERFMON would break a lot
>> > > of users relying on BTF availability to build their BPF applications.
>> > True. If this patch is applied, would need to at least be behind an optin
>> > knob. Similar to dmesg_restrict.
>> 
>> It's not going to be applied. If a file shouldn't be read by a user
>> it should have appropriate file permissions instead of 444.
>> Checking capable() in read() is very non-unix way to deal with permissions.
>
> Not only it's a non-unix way, both don't achieve the same goals at all!
>
> One checks for permissions at open() time and may for example allow a
> process to drop its uid after opening, while the other one allows to
> filter who can really read it, particularly in case the FD is inherited
> between processes. With this said, I don't see why there would be a
> special case for this one, it should definitely stick to file permissions
> only.

From include/linux/bpf.h:

static inline bool bpf_allow_ptr_leaks(void)
{
	return perfmon_capable();
}

static inline bool bpf_allow_ptr_to_map_access(void)
{
	return perfmon_capable();
}

static inline bool bpf_bypass_spec_v1(void)
{
	return perfmon_capable();
}

static inline bool bpf_bypass_spec_v4(void)
{
	return perfmon_capable();
}


There's also several cases in bpf_base_func_proto().

So it seems entirely reasonable to suggest that perfmon_capable() is the
right check here.

cheers