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

[Daniel P. Berrange]

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(...);


Regards,
Daniel

[1] http://people.redhat.com/sgrubb/libcap-ng/

-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|