From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:33812) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RBrlg-0004wA-QJ for qemu-devel@nongnu.org; Thu, 06 Oct 2011 13:30:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RBrle-0001mZ-KI for qemu-devel@nongnu.org; Thu, 06 Oct 2011 13:30:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34229) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RBrle-0001mP-C3 for qemu-devel@nongnu.org; Thu, 06 Oct 2011 13:29:58 -0400 Date: Thu, 6 Oct 2011 17:34:55 +0100 From: "Daniel P. Berrange" Message-ID: <20111006163455.GE2450@redhat.com> References: <1317915508-15491-1-git-send-email-rmarwah@linux.vnet.ibm.com> <1317915508-15491-4-git-send-email-rmarwah@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1317915508-15491-4-git-send-email-rmarwah@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH 3/4] Add cap reduction support to enable use as SUID Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richa Marwaha Cc: aliguori@us.ibm.com, coreyb@linux.vnet.ibm.com, qemu-devel@nongnu.org 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 :|