2020-05-18 17:07 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>
> On Fri, May 15, 2020 at 5:52 PM Quentin Monnet <quentin@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>> ---
>> 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.
Thanks a lot for the review!
Quentin
