From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:53243) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RBsN8-0005I2-KN for qemu-devel@nongnu.org; Thu, 06 Oct 2011 14:08:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RBsN7-0000vS-2y for qemu-devel@nongnu.org; Thu, 06 Oct 2011 14:08:42 -0400 Received: from e36.co.us.ibm.com ([32.97.110.154]:43385) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RBsN6-0000vN-RT for qemu-devel@nongnu.org; Thu, 06 Oct 2011 14:08:41 -0400 Received: from d03relay03.boulder.ibm.com (d03relay03.boulder.ibm.com [9.17.195.228]) by e36.co.us.ibm.com (8.14.4/8.13.1) with ESMTP id p96I1mi9006667 for ; Thu, 6 Oct 2011 12:01:48 -0600 Received: from d03av03.boulder.ibm.com (d03av03.boulder.ibm.com [9.17.195.169]) by d03relay03.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p96I8Ue2111682 for ; Thu, 6 Oct 2011 12:08:34 -0600 Received: from d03av03.boulder.ibm.com (loopback [127.0.0.1]) by d03av03.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p96I8TsV011630 for ; Thu, 6 Oct 2011 12:08:29 -0600 Message-ID: <4E8DEE98.9040209@linux.vnet.ibm.com> Date: Thu, 06 Oct 2011 14:08:24 -0400 From: Corey Bryant 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> <4E8DE869.4050407@us.ibm.com> In-Reply-To: <4E8DE869.4050407@us.ibm.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: Anthony Liguori , Richa Marwaha , qemu-devel@nongnu.org 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/ >> > >