Re: [PATCH] virtfs-proxy-helper: switch from libcap to libcap-ng

[Greg Kurz <groug@kaod.org>]

On Fri, 29 Nov 2019 12:16:32 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> virtfs-proxy-helper is the only user of libcap; everyone else is using
> the simpler libcap-ng API.  Switch and remove the configure code to
> detect libcap.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

Nice. :)

Reviewed-by: Greg Kurz <groug@kaod.org>

Paolo,

I can take this through my 9p tree if you want. Otherwise,

Acked-by: Greg Kurz <groug@kaod.org>



Also, this calls for some extra cleanup in travis.yml and gitlab-ci.yml
which were recently amended by Thomas to install libcap-dev.

commit c269447f78b7cfb0e85d14bc7bb8cb0d25d19781
Author: Thomas Huth <thuth@redhat.com>
Date:   Thu Sep 5 13:33:46 2019 +0200

    travis.yml: Install libcap-dev for testing virito-9p

and

commit e7dc804ef0d7cac9ac8b4a1324ab7dbfafb55704
Author: Thomas Huth <thuth@redhat.com>
Date:   Thu Sep 5 12:36:50 2019 +0200

    gitlab-ci.yml: Install libattr-devel and libcap-devel to test virtio-9p



>  configure                   |  18 +------
>  fsdev/virtfs-proxy-helper.c | 100 ++++++++++++++++--------------------
>  2 files changed, 46 insertions(+), 72 deletions(-)
> 
> diff --git a/configure b/configure
> index afe9393f04..2216662bf6 100755
> --- a/configure
> +++ b/configure
> @@ -3863,22 +3863,6 @@ else
>    mpathpersist=no
>  fi
>  
> -##########################################
> -# libcap probe
> -
> -if test "$cap" != "no" ; then
> -  cat > $TMPC <<EOF
> -#include <stdio.h>
> -#include <sys/capability.h>
> -int main(void) { cap_t caps; caps = cap_init(); return caps != NULL; }
> -EOF
> -  if compile_prog "" "-lcap" ; then
> -    cap=yes
> -  else
> -    cap=no
> -  fi
> -fi
> -
>  ##########################################
>  # pthread probe
>  PTHREADLIBS_LIST="-pthread -lpthread -lpthreadGC2"
> @@ -6204,7 +6188,7 @@ if test "$want_tools" = "yes" ; then
>  fi
>  if test "$softmmu" = yes ; then
>    if test "$linux" = yes; then
> -    if test "$virtfs" != no && test "$cap" = yes && test "$attr" = yes ; then
> +    if test "$virtfs" != no && test "$cap_ng" = yes && test "$attr" = yes ; then
>        virtfs=yes
>        tools="$tools fsdev/virtfs-proxy-helper\$(EXESUF)"
>      else
> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
> index 6f132c5ff1..0d4de49dcf 100644
> --- a/fsdev/virtfs-proxy-helper.c
> +++ b/fsdev/virtfs-proxy-helper.c
> @@ -13,7 +13,6 @@
>  #include <sys/resource.h>
>  #include <getopt.h>
>  #include <syslog.h>
> -#include <sys/capability.h>
>  #include <sys/fsuid.h>
>  #include <sys/vfs.h>
>  #include <sys/ioctl.h>
> @@ -21,6 +20,7 @@
>  #ifdef CONFIG_LINUX_MAGIC_H
>  #include <linux/magic.h>
>  #endif
> +#include <cap-ng.h>
>  #include "qemu-common.h"
>  #include "qemu/sockets.h"
>  #include "qemu/xattr.h"
> @@ -79,49 +79,10 @@ static void do_perror(const char *string)
>      }
>  }
>  
> -static int do_cap_set(cap_value_t *cap_value, int size, int reset)
> -{
> -    cap_t caps;
> -    if (reset) {
> -        /*
> -         * Start with an empty set and set permitted and effective
> -         */
> -        caps = cap_init();
> -        if (caps == NULL) {
> -            do_perror("cap_init");
> -            return -1;
> -        }
> -        if (cap_set_flag(caps, CAP_PERMITTED, size, cap_value, CAP_SET) < 0) {
> -            do_perror("cap_set_flag");
> -            goto error;
> -        }
> -    } else {
> -        caps = cap_get_proc();
> -        if (!caps) {
> -            do_perror("cap_get_proc");
> -            return -1;
> -        }
> -    }
> -    if (cap_set_flag(caps, CAP_EFFECTIVE, size, cap_value, CAP_SET) < 0) {
> -        do_perror("cap_set_flag");
> -        goto error;
> -    }
> -    if (cap_set_proc(caps) < 0) {
> -        do_perror("cap_set_proc");
> -        goto error;
> -    }
> -    cap_free(caps);
> -    return 0;
> -
> -error:
> -    cap_free(caps);
> -    return -1;
> -}
> -
>  static int init_capabilities(void)
>  {
>      /* helper needs following capabilities only */
> -    cap_value_t cap_list[] = {
> +    int cap_list[] = {
>          CAP_CHOWN,
>          CAP_DAC_OVERRIDE,
>          CAP_FOWNER,
> @@ -130,7 +91,34 @@ static int init_capabilities(void)
>          CAP_MKNOD,
>          CAP_SETUID,
>      };
> -    return do_cap_set(cap_list, ARRAY_SIZE(cap_list), 1);
> +    int i;
> +
> +    capng_clear(CAPNG_SELECT_BOTH);
> +    for (i = 0; i < ARRAY_SIZE(cap_list); i++) {
> +        if (capng_update(CAPNG_ADD, CAPNG_EFFECTIVE | CAPNG_PERMITTED,
> +                         cap_list[i]) < 0) {
> +            do_perror("capng_update");
> +            return -1;
> +        }
> +    }
> +    if (capng_apply(CAPNG_SELECT_BOTH) < 0) {
> +        do_perror("capng_apply");
> +        return -1;
> +    }
> +
> +    /* Prepare effective set for setugid.  */
> +    for (i = 0; i < ARRAY_SIZE(cap_list); i++) {
> +        if (cap_list[i] == CAP_DAC_OVERRIDE) {
> +            continue;
> +        }
> +
> +        if (capng_update(CAPNG_DROP, CAPNG_EFFECTIVE,
> +                         cap_list[i]) < 0) {
> +            do_perror("capng_update");
> +            return -1;
> +        }
> +    }
> +    return 0;
>  }
>  
>  static int socket_read(int sockfd, void *buff, ssize_t size)
> @@ -295,14 +283,6 @@ static int setugid(int uid, int gid, int *suid, int *sgid)
>  {
>      int retval;
>  
> -    /*
> -     * We still need DAC_OVERRIDE because we don't change
> -     * supplementary group ids, and hence may be subjected DAC rules
> -     */
> -    cap_value_t cap_list[] = {
> -        CAP_DAC_OVERRIDE,
> -    };
> -
>      *suid = geteuid();
>      *sgid = getegid();
>  
> @@ -316,11 +296,21 @@ static int setugid(int uid, int gid, int *suid, int *sgid)
>          goto err_sgid;
>      }
>  
> -    if (uid != 0 || gid != 0) {
> -        if (do_cap_set(cap_list, ARRAY_SIZE(cap_list), 0) < 0) {
> -            retval = -errno;
> -            goto err_suid;
> -        }
> +    if (uid == 0 && gid == 0) {
> +        /* Linux has already copied the permitted set to the effective set.  */
> +        return 0;
> +    }
> +
> +    /*
> +     * All capabilities have been cleared from the effective set.  However
> +     * we still need DAC_OVERRIDE because we don't change supplementary
> +     * group ids, and hence may be subject to DAC rules.  init_capabilities
> +     * left the set of capabilities that we want in libcap-ng's state.
> +     */
> +    if (capng_apply(CAPNG_SELECT_CAPS) < 0) {
> +        retval = -errno;
> +        do_perror("capng_apply");
> +        goto err_suid;
>      }
>      return 0;
>