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

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

On 10/06/2011 01:42 PM, Anthony Liguori wrote:
> 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
>

This looks a lot simpler.  We'll definitely look into implementing this 
in v2.

-- 
Regards,
Corey

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