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

[Blue Swirl]

On Mon, Oct 24, 2011 at 18:38, Corey Bryant <address@hidden> wrote:
>
>
> On 10/24/2011 01:10 PM, Blue Swirl wrote:
>>
>> On Mon, Oct 24, 2011 at 14:13, Corey Bryant<address@hidden>
>>  wrote:
>>>
>>>
>>> On 10/23/2011 09:22 AM, Blue Swirl wrote:
>>>>
>>>> On Fri, Oct 21, 2011 at 15:07, Corey Bryant<address@hidden>
>>>>  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<address@hidden>
>>>>> Signed-off-by: Richa Marwaha<address@hidden>
>>>>> Signed-off-by: Corey Bryant<address@hidden>
>>>>> ---
>>>>>  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?

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