From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:44302) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RBs3X-0000rM-LA for qemu-devel@nongnu.org; Thu, 06 Oct 2011 13:48:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RBs3W-0005bd-A5 for qemu-devel@nongnu.org; Thu, 06 Oct 2011 13:48:27 -0400 Received: from e1.ny.us.ibm.com ([32.97.182.141]:53381) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RBs3W-0005bZ-7J for qemu-devel@nongnu.org; Thu, 06 Oct 2011 13:48:26 -0400 Received: from /spool/local by us.ibm.com with XMail ESMTP for from ; Thu, 6 Oct 2011 13:42:41 -0400 Received: from d01av01.pok.ibm.com (d01av01.pok.ibm.com [9.56.224.215]) by d01relay03.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p96Hg2ff241602 for ; Thu, 6 Oct 2011 13:42:02 -0400 Received: from d01av01.pok.ibm.com (loopback [127.0.0.1]) by d01av01.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p96Hg2X6025258 for ; Thu, 6 Oct 2011 13:42:02 -0400 Message-ID: <4E8DE869.4050407@us.ibm.com> Date: Thu, 06 Oct 2011 12:42:01 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1317915508-15491-1-git-send-email-rmarwah@linux.vnet.ibm.com> <1317915508-15491-4-git-send-email-rmarwah@linux.vnet.ibm.com> <20111006163455.GE2450@redhat.com> In-Reply-To: <20111006163455.GE2450@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/4] Add cap reduction support to enable use as SUID List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: Richa Marwaha , coreyb@linux.vnet.ibm.com, qemu-devel@nongnu.org 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/ >