Message-ID: <CAEf4BzaD3UW8AL7ZEiqMzpSP_u_RT-p=VK5oTVjMHyd7Wpckyg@mail.gmail.com>
Date:   Mon, 18 May 2020 21:39:50 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Quentin Monnet <quentin@...valent.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next] tools: bpftool: make capability check account
 for new BPF caps

On Mon, May 18, 2020 at 6:03 PM Quentin Monnet <quentin@...valent.com> wrote:
>
> 2020-05-18 17:07 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@...il.com>
> > On Fri, May 15, 2020 at 5:52 PM Quentin Monnet <quentin@...valent.com> wrote:
> >>
> >> Following the introduction of CAP_BPF, and the switch from CAP_SYS_ADMIN
> >> to other capabilities for various BPF features, update the capability
> >> checks (and potentially, drops) in bpftool for feature probes. Because
> >> bpftool and/or the system might not know of CAP_BPF yet, some caution is
> >> necessary:
> >>
> >> - If compiled and run on a system with CAP_BPF, check CAP_BPF,
> >>   CAP_SYS_ADMIN, CAP_PERFMON, CAP_NET_ADMIN.
> >>
> >> - Guard against CAP_BPF being undefined, to allow compiling bpftool from
> >>   latest sources on older systems. If the system where feature probes
> >>   are run does not know of CAP_BPF, stop checking after CAP_SYS_ADMIN,
> >>   as this should be the only capability required for all the BPF
> >>   probing.
> >>
> >> - If compiled from latest sources on a system without CAP_BPF, but later
> >>   executed on a newer system with CAP_BPF knowledge, then we only test
> >>   CAP_SYS_ADMIN. Some probes may fail if the bpftool process has
> >>   CAP_SYS_ADMIN but misses the other capabilities. The alternative would
> >>   be to redefine the value for CAP_BPF in bpftool, but this does not
> >>   look clean, and the case sounds relatively rare anyway.
> >>
> >> Note that libcap offers a cap_to_name() function to retrieve the name of
> >> a given capability (e.g. "cap_sys_admin"). We do not use it because
> >> deriving the names from the macros looks simpler than using
> >> cap_to_name() (doing a strdup() on the string) + cap_free() + handling
> >> the case of failed allocations, when we just want to use the name of the
> >> capability in an error message.
> >>
> >> The checks when compiling without libcap (i.e. root versus non-root) are
> >> unchanged.
> >>
> >> Signed-off-by: Quentin Monnet <quentin@...valent.com>
> >> ---
> >>  tools/bpf/bpftool/feature.c | 85 +++++++++++++++++++++++++++++--------
> >>  1 file changed, 67 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c
> >> index 1b73e63274b5..3c3d779986c7 100644
> >> --- a/tools/bpf/bpftool/feature.c
> >> +++ b/tools/bpf/bpftool/feature.c
> >> @@ -758,12 +758,32 @@ static void section_misc(const char *define_prefix, __u32 ifindex)
> >>         print_end_section();
> >>  }
> >>
> >> +#ifdef USE_LIBCAP
> >> +#define capability(c) { c, #c }
> >> +#endif
> >> +
> >>  static int handle_perms(void)
> >>  {
> >>  #ifdef USE_LIBCAP
> >> -       cap_value_t cap_list[1] = { CAP_SYS_ADMIN };
> >> -       bool has_sys_admin_cap = false;
> >> +       struct {
> >> +               cap_value_t cap;
> >> +               char name[14];  /* strlen("CAP_SYS_ADMIN") */
> >> +       } required_caps[] = {
> >> +               capability(CAP_SYS_ADMIN),
> >> +#ifdef CAP_BPF
> >> +               /* Leave CAP_BPF in second position here: We will stop checking
> >> +                * if the system does not know about it, since it probably just
> >> +                * needs CAP_SYS_ADMIN to run all the probes in that case.
> >> +                */
> >> +               capability(CAP_BPF),
> >> +               capability(CAP_NET_ADMIN),
> >> +               capability(CAP_PERFMON),
> >> +#endif
> >> +       };
> >> +       bool has_admin_caps = true;
> >> +       cap_value_t *cap_list;
> >>         cap_flag_value_t val;
> >> +       unsigned int i;
> >>         int res = -1;
> >>         cap_t caps;
> >>
> >> @@ -774,41 +794,70 @@ static int handle_perms(void)
> >>                 return -1;
> >>         }
> >>
> >> -       if (cap_get_flag(caps, CAP_SYS_ADMIN, CAP_EFFECTIVE, &val)) {
> >> -               p_err("bug: failed to retrieve CAP_SYS_ADMIN status");
> >> +       cap_list = malloc(sizeof(cap_value_t) * ARRAY_SIZE(required_caps));
> >
> > I fail to see why you need to dynamically allocate cap_list?
> > cap_value_t cap_list[ARRAY_SIZE(required_caps)] wouldn't work?
>
> Oh I should have thought about that, thanks! I'll fix it.
>
> >> +       if (!cap_list) {
> >> +               p_err("failed to allocate cap_list: %s", strerror(errno));
> >>                 goto exit_free;
> >>         }
> >> -       if (val == CAP_SET)
> >> -               has_sys_admin_cap = true;
> >>
> >> -       if (!run_as_unprivileged && !has_sys_admin_cap) {
> >> -               p_err("full feature probing requires CAP_SYS_ADMIN, run as root or use 'unprivileged'");
> >> -               goto exit_free;
> >> +       for (i = 0; i < ARRAY_SIZE(required_caps); i++) {
> >> +               const char *cap_name = required_caps[i].name;
> >> +               cap_value_t cap = required_caps[i].cap;
> >> +
> >> +#ifdef CAP_BPF
> >> +               if (cap == CAP_BPF && !CAP_IS_SUPPORTED(cap))
> >> +                       /* System does not know about CAP_BPF, meaning
> >> +                        * that CAP_SYS_ADMIN is the only capability
> >> +                        * required. We already checked it, break.
> >> +                        */
> >> +                       break;
> >> +#endif
> >
> > Seems more reliable to check all 4 capabilities independently (so
> > don't stop if !CAP_IS_SUPPORTED(cap)), and drop those that you have
> > set. Or there are some downsides to that?
>
> If CAP_BPF is not supported, there is simply no point in going on
> checking the other capabilities, since CAP_SYS_ADMIN is the only one we
> need to do the feature probes. So in that case I see little point in
> checking the others.
>
> But if I understand your concern, you're right in the sense that the
> current code would consider a user as "unprivileged" if they do not have
> all four capabilities (in the case where CAP_BPF is supported); but they
> may still have a subset of them and not be completely unprivileged, and
> in that case we would have has_admin_caps at false and skip capabilities
> drop.
>
> I will fix that in next version. I am not sure about the advantage of
> keeping track of the capabilities and building a list just for dropping
> only the ones we have, but I can do that if you prefer.
>

Honestly, I don't use bpftool feature at all, so I'm not very
qualified to decide. I just like tools not making too many
assumptions, where not necessary. So see for yourself :)

> Thanks a lot for the review!
> Quentin

Powered by blists - more mailing lists