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

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

On 10/24/2011 02:58 PM, Blue Swirl wrote:
> On Mon, Oct 24, 2011 at 18:38, Corey Bryant<coreyb@linux.vnet.ibm.com>  wrote:
>>
>>
>> On 10/24/2011 01:10 PM, Blue Swirl wrote:
>>>
>>> On Mon, Oct 24, 2011 at 14:13, Corey Bryant<coreyb@linux.vnet.ibm.com>
>>>   wrote:
>>>>
>>>>
>>>> On 10/23/2011 09:22 AM, Blue Swirl wrote:
>>>>>
>>>>> On Fri, Oct 21, 2011 at 15:07, Corey Bryant<coreyb@linux.vnet.ibm.com>
>>>>>   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.
>>>>>>
>>>>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>>>>> Signed-off-by: Richa Marwaha<rmarwah@linux.vnet.ibm.com>
>>>>>> Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com>
>>>>>> ---
>>>>>>   configure            |   34 ++++++++++++++++++++++++++++++++++
>>>>>>   qemu-bridge-helper.c |   39 +++++++++++++++++++++++++++++++++++++++
>>>>>>   2 files changed, 73 insertions(+), 0 deletions(-)
>>>>>>
>>>>>> diff --git a/configure b/configure
>>>>>> index 6c8b659..fed66b0 100755
>>>>>> --- a/configure
>>>>>> +++ b/configure
>>>>>> @@ -128,6 +128,7 @@ vnc_thread="no"
>>>>>>   xen=""
>>>>>>   xen_ctrl_version=""
>>>>>>   linux_aio=""
>>>>>> +cap=""
>>>>>>   attr=""
>>>>>>   xfs=""
>>>>>>
>>>>>> @@ -653,6 +654,10 @@ for opt do
>>>>>>    ;;
>>>>>>    --enable-kvm) kvm="yes"
>>>>>>    ;;
>>>>>> +  --disable-cap)  cap="no"
>>>>>> +  ;;
>>>>>> +  --enable-cap) cap="yes"
>>>>>> +  ;;
>>>>>>    --disable-spice) spice="no"
>>>>>>    ;;
>>>>>>    --enable-spice) spice="yes"
>>>>>> @@ -1032,6 +1037,8 @@ echo "  --disable-vde            disable support
>>>>>> for vde network"
>>>>>>   echo "  --enable-vde             enable support for vde network"
>>>>>>   echo "  --disable-linux-aio      disable Linux AIO support"
>>>>>>   echo "  --enable-linux-aio       enable Linux AIO support"
>>>>>> +echo "  --disable-cap            disable libcap-ng support"
>>>>>> +echo "  --enable-cap             enable libcap-ng support"
>>>>>>   echo "  --disable-attr           disables attr and xattr support"
>>>>>>   echo "  --enable-attr            enable attr and xattr support"
>>>>>>   echo "  --disable-blobs          disable installing provided firmware
>>>>>> blobs"
>>>>>> @@ -1638,6 +1645,29 @@ EOF
>>>>>>   fi
>>>>>>
>>>>>>   ##########################################
>>>>>> +# libcap-ng library probe
>>>>>> +if test "$cap" != "no" ; then
>>>>>> +  cap_libs="-lcap-ng"
>>>>>> +  cat>      $TMPC<<      EOF
>>>>>> +#include<cap-ng.h>
>>>>>> +int main(void)
>>>>>> +{
>>>>>> +    capng_capability_to_name(CAPNG_EFFECTIVE);
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +EOF
>>>>>> +  if compile_prog "" "$cap_libs" ; then
>>>>>> +    cap=yes
>>>>>> +    libs_tools="$cap_libs $libs_tools"
>>>>>> +  else
>>>>>> +    if test "$cap" = "yes" ; then
>>>>>> +      feature_not_found "cap"
>>>>>> +    fi
>>>>>> +    cap=no
>>>>>> +  fi
>>>>>> +fi
>>>>>> +
>>>>>> +##########################################
>>>>>>   # Sound support libraries probe
>>>>>>
>>>>>>   audio_drv_probe()
>>>>>> @@ -2735,6 +2765,7 @@ echo "fdatasync         $fdatasync"
>>>>>>   echo "madvise           $madvise"
>>>>>>   echo "posix_madvise     $posix_madvise"
>>>>>>   echo "uuid support      $uuid"
>>>>>> +echo "libcap-ng support $cap"
>>>>>>   echo "vhost-net support $vhost_net"
>>>>>>   echo "Trace backend     $trace_backend"
>>>>>>   echo "Trace output file $trace_file-<pid>"
>>>>>> @@ -2846,6 +2877,9 @@ fi
>>>>>>   if test "$vde" = "yes" ; then
>>>>>>    echo "CONFIG_VDE=y">>      $config_host_mak
>>>>>>   fi
>>>>>> +if test "$cap" = "yes" ; then
>>>>>> +  echo "CONFIG_LIBCAP=y">>      $config_host_mak
>>>>>> +fi
>>>>>>   for card in $audio_card_list; do
>>>>>>      def=CONFIG_`echo $card | tr '[:lower:]' '[:upper:]'`
>>>>>>      echo "$def=y">>      $config_host_mak
>>>>>> diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
>>>>>> index db257d5..b1562eb 100644
>>>>>> --- a/qemu-bridge-helper.c
>>>>>> +++ b/qemu-bridge-helper.c
>>>>>> @@ -33,6 +33,10 @@
>>>>>>
>>>>>>   #include "net/tap-linux.h"
>>>>>>
>>>>>> +#ifdef CONFIG_LIBCAP
>>>>>> +#include<cap-ng.h>
>>>>>> +#endif
>>>>>> +
>>>>>>   #define MAX_ACLS (128)
>>>>>>   #define DEFAULT_ACL_FILE CONFIG_QEMU_CONFDIR "/bridge.conf"
>>>>>>
>>>>>> @@ -185,6 +189,27 @@ static int send_fd(int c, int fd)
>>>>>>      return sendmsg(c,&msg, 0);
>>>>>>   }
>>>>>>
>>>>>> +#ifdef CONFIG_LIBCAP
>>>>>> +static int drop_privileges(void)
>>>>>> +{
>>>>>> +    /* clear all capabilities */
>>>>>> +    capng_clear(CAPNG_SELECT_BOTH);
>>>>>> +
>>>>>> +    if (capng_update(CAPNG_ADD, CAPNG_EFFECTIVE | CAPNG_PERMITTED,
>>>>>> +                     CAP_NET_ADMIN)<      0) {
>>>>>> +        return -1;
>>>>>> +    }
>>>>>> +
>>>>>> +    /* change to calling user's real uid and gid, retaining
>>>>>> supplemental
>>>>>> +     * groups and CAP_NET_ADMIN */
>>>>>> +    if (capng_change_id(getuid(), getgid(), CAPNG_CLEAR_BOUNDING)) {
>>>>>> +        return -1;
>>>>>> +    }
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +#endif
>>>>>> +
>>>>>>   int main(int argc, char **argv)
>>>>>>   {
>>>>>>      struct ifreq ifr;
>>>>>> @@ -198,6 +223,20 @@ int main(int argc, char **argv)
>>>>>>      int acl_count = 0;
>>>>>>      int i, access_allowed, access_denied;
>>>>>>
>>>>>> +    /* if we're run from an suid binary, immediately drop privileges
>>>>>> preserving
>>>>>> +     * cap_net_admin -- exit immediately if libcap not configured */
>>>>>> +    if (geteuid() == 0&&      getuid() != geteuid()) {
>>>>>> +#ifdef CONFIG_LIBCAP
>>>>>> +        if (drop_privileges() == -1) {
>>>>>> +            fprintf(stderr, "failed to drop privileges\n");
>>>>>> +            return 1;
>>>>>> +        }
>>>>>> +#else
>>>>>> +        fprintf(stderr, "failed to drop privileges\n");
>>>>>
>>>>> This makes the tool useless without CONFIG_LIBCAP. Wouldn't it be
>>>>> possible to use setfsuid() instead for Linux?
>>>>>
>>>>> Some fork+setuid helper could be used for other Unix and for the lame
>>>>> OSes without any file system DAC capabilities, a different syntax that
>>>>> does not rely on underlying FS may need to be introduced. Again, I
>>>>> don't know if the tool is even interesting for non-Linux.
>>>>>
>>>>
>>>> I just want to make sure that there is no chance that the helper is run
>>>> as
>>>> root beyond this point.  Are you saying to seteuid(getuid) and
>>>> setfsuid(root)?  I'm not sure that would drop the privileges enough.
>>>
>>> Without capabilities, we can't drop root privileges because bridge
>>> setup would fail otherwise, but we could use setfsuid(getuid()) and
>>> setfsgid(getgid()) during file access so permission checks work.
>>> Perhaps non-Linux could use seteuid() etc. instead.
>>>
>>
>> This would reduce file system access from effective UID/GID (root/root) to
>> real UID/GID (non-root/non-root).  Other than file system access, the helper
>> would still run under root/root, right?  I don't think we want that from a
>> security aspect.
>
> Right, it's not desirable, but isn't that the best we can do without
> libcap or FS capabilities?
>

I think the best we can do is not let it run in those cases. :)  I'd 
like see if others in the community have an opinion on this though.

-- 
Regards,
Corey

>> --
>> Regards,
>> Corey
>>
>>>>>> +        return 1;
>>>>>> +#endif
>>>>>> +    }
>>>>>> +
>>>>>>      /* parse arguments */
>>>>>>      if (argc<      3 || argc>      4) {
>>>>>>          fprintf(stderr, "Usage: %s [--use-vnet] BRIDGE FD\n", argv[0]);
>>>>>> --
>>>>>> 1.7.3.4
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>
>>
>