Re: [Qemu-devel] [PATCH 3/4] Add cap reduction support to enable use as SUID

[Anthony Liguori <aliguori@us.ibm.com>]

On 10/06/2011 11:34 AM, Daniel P. Berrange wrote:
> On Thu, Oct 06, 2011 at 11:38:27AM -0400, Richa Marwaha wrote:
>> The ideal way to use qemu-bridge-helper is to give it an fscap of using:
>>
>>   setcap cap_net_admin=ep qemu-bridge-helper
>>
>> Unfortunately, most distros still do not have a mechanism to package files
>> with fscaps applied.  This means they'll have to SUID the qemu-bridge-helper
>> binary.
>>
>> To improve security, use libcap to reduce our capability set to just
>> cap_net_admin, then reduce privileges down to the calling user.  This is
>> hopefully close to equivalent to fscap support from a security perspective.
>> +#ifdef CONFIG_LIBCAP
>> +static int drop_privileges(void)
>> +{
>> +    cap_t cap;
>> +    cap_value_t new_caps[] = {CAP_NET_ADMIN};
>> +
>> +    cap = cap_init();
>
> Check for NULL ?
>
>> +
>> +    /* set capabilities to be permitted and inheritable.  we don't need the
>> +     * caps to be effective right now as they'll get reset when we seteuid
>> +     * anyway */
>> +    cap_set_flag(cap, CAP_PERMITTED, 1, new_caps, CAP_SET);
>> +    cap_set_flag(cap, CAP_INHERITABLE, 1, new_caps, CAP_SET);
>
> Check for failure ?
>
>> +
>> +    if (cap_set_proc(cap) == -1) {
>> +        return -1;
>> +    }
>> +
>> +    cap_free(cap);
>
> Check for failure ?
>
>> +
>> +    /* reduce our privileges to a normal user */
>> +    setegid(getgid());
>> +    seteuid(getuid());
>
> Check for failure ?
>
>> +    cap = cap_init();
>
> Check for NULL ?
>
>> +
>> +    /* enable the our capabilities.  we marked them as inheritable earlier
>> +     * which is what allows this to work. */
>> +    cap_set_flag(cap, CAP_EFFECTIVE, 1, new_caps, CAP_SET);
>> +    cap_set_flag(cap, CAP_PERMITTED, 1, new_caps, CAP_SET);
>
> Check for failure ?
>
>> +
>> +    if (cap_set_proc(cap) == -1) {
>> +        return -1;
>> +    }
>> +
>> +    cap_free(cap);
>
> Check for failure ?
>
>> +
>> +    return 0;
>> +}
>> +#endif
>
> It may seem like checking for failure on cap_free/cap_set_flag is
> not required because they can only return EINVAL for invalid
> args, but since this is missing the check for NULL on cap_init
> you can actually see errors from those latter functions in an
> OOM cenario.
>
> I think I'd suggest not using libcap, instead try libcap-ng [1] whose
> APIs are designed with safety in mind&  result in much simpler and
> clearer code:
>
> eg, that entire function above can be expressed using capng with
> something approximating:
>
>       capng_clear(CAPNG_SELECT_BOTH);
>       if (capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_NET_ADMIN)<  0)
>           error(...);
>       if (capng_change_id(getuid(), getgid(), CAPNG_DROP_SUPP_GRP | CAPNG_CLEAR_BOUNDING))
>           error(...);

Ah, libcap-ng didn't exist when the code was initially written but I agree, it 
looks like a nice library.

Regards,

Anthony Liguori

>
>
> Regards,
> Daniel
>
> [1] http://people.redhat.com/sgrubb/libcap-ng/
>