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 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.

-- 
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
>>>>
>>>>
>>>>
>>>