[libvirt] [PATCH 5/5] qemu: caps: Use CAP_DAC_OVERRIDE for probing to avoid permission issues

[Daniel P. Berrangé]

On Thu, Jan 31, 2019 at 04:26:18PM +0100, Erik Skultety wrote:
> This is mainly about /dev/sev and its default permissions 0600. Of
> course, rule of 'tinfoil' would be that we can't trust anything, but the
> probing code in QEMU is considered safe from security's perspective + we
> can't create an udev rule for this at the moment, because ioctls and
> filesystem permisions are cross checked in kernel and therefore a user
> with read permisions could issue a 'privileged' operation on SEV which
> is currently only limited to root.
> 
> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> ---
>  src/qemu/qemu_capabilities.c | 11 +++++++++++
>  src/util/virutil.c           | 31 +++++++++++++++++++++++++++++--
>  2 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 5cf4b617c6..2e84c965e8 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -53,6 +53,10 @@
>  #include <stdarg.h>
>  #include <sys/utsname.h>
>  
> +#if WITH_CAPNG
> +# include <cap-ng.h>
> +#endif
> +
>  #define VIR_FROM_THIS VIR_FROM_QEMU
>  
>  VIR_LOG_INIT("qemu.qemu_capabilities");
> @@ -4515,6 +4519,13 @@ virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd,
>                                      NULL);
>      virCommandAddEnvPassCommon(cmd->cmd);
>      virCommandClearCaps(cmd->cmd);
> +
> +#if WITH_CAPNG
> +    /* QEMU might run into permission issues, e.g. /dev/sev (0600), override
> +     * them just for the purpose of probing */
> +    virCommandAllowCap(cmd->cmd, CAP_DAC_OVERRIDE);
> +#endif
> +
>      virCommandSetGID(cmd->cmd, cmd->runGid);
>      virCommandSetUID(cmd->cmd, cmd->runUid);
>  
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index 5251b66454..02de92061c 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -1502,8 +1502,10 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups,
>  {
>      size_t i;
>      int capng_ret, ret = -1;
> -    bool need_setgid = false, need_setuid = false;
> +    bool need_setgid = false;
> +    bool need_setuid = false;
>      bool need_setpcap = false;
> +    const char *capstr = NULL;
>  
>      /* First drop all caps (unless the requested uid is "unchanged" or
>       * root and clearExistingCaps wasn't requested), then add back
> @@ -1512,14 +1514,18 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups,
>       */
>  
>      if (clearExistingCaps || (uid != (uid_t)-1 && uid != 0))
> -       capng_clear(CAPNG_SELECT_BOTH);
> +        capng_clear(CAPNG_SELECT_BOTH);
>  
>      for (i = 0; i <= CAP_LAST_CAP; i++) {
> +        capstr = capng_capability_to_name(i);
> +
>          if (capBits & (1ULL << i)) {
>              capng_update(CAPNG_ADD,
>                           CAPNG_EFFECTIVE|CAPNG_INHERITABLE|
>                           CAPNG_PERMITTED|CAPNG_BOUNDING_SET,
>                           i);
> +
> +            VIR_DEBUG("Added '%s' to child capabilities' set", capstr);
>          }
>      }
>  
> @@ -1579,6 +1585,27 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups,
>          goto cleanup;
>      }
>  
> +# ifdef PR_CAP_AMBIENT
> +    /* we couldn't do this in the loop earlier above, because the capabilities
> +     * were not applied yet, since in order to add a capability into the AMBIENT
> +     * set, it has to be present in both the PERMITTED and INHERITABLE sets
> +     * (capabilities(7))
> +     */
> +    for (i = 0; i <= CAP_LAST_CAP; i++) {
> +        capstr = capng_capability_to_name(i);
> +
> +        if (capBits & (1ULL << i)) {
> +            if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, i, 0, 0) < 0) {
> +                virReportSystemError(errno,
> +                                     _("prctl failed to enable '%s' in the "
> +                                       "AMBIENT set"),
> +                                     capstr);
> +                goto cleanup;
> +            }
> +        }
> +    }
> +# endif

This is set a bit earlier than I set it in my PoC patch, but I'll assume
it still works given the comment you added.

Reviewed-by: Daniel P. Berrangé <berrange at redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|