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

[Blue Swirl]

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.

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