[libvirt] [PATCH] security: provide supplemental groups even when parsing label (CVE-2013-4291)

[Daniel P. Berrange]

On Thu, Aug 29, 2013 at 08:47:11AM -0600, Eric Blake wrote:
> Commit 29fe5d7 (released in 1.1.1) introduced a latent problem
> for any caller of virSecurityManagerSetProcessLabel and where
> the domain already had a uid:gid label to be parsed.  Such a
> setup would collect the list of supplementary groups during
> virSecurityManagerPreFork, but then ignores that information,
> and thus fails to call setgroups() to adjust the supplementary
> groups of the process.
> 
> Upstream does not use virSecurityManagerSetProcessLabel for
> qemu (it uses virSecurityManagerSetChildProcessLabel instead),
> so this problem remained latent until backporting the initial
> commit into v0.10.2-maint (commit c061ff5, released in 0.10.2.7),
> where virSecurityManagerSetChildProcessLabel has not been
> backported.  As a result of using a different code path in the
> backport, attempts to start a qemu domain that runs as qemu:qemu
> will end up with supplementary groups unchanged from the libvirtd
> parent process, rather than the desired supplementary groups of
> the qemu user.  This can lead to failure to start a domain
> (typical Fedora setup assigns user 107 'qemu' to both group 107
> 'qemu' and group 36 'kvm', so a disk image that is only readable
> under kvm group rights is locked out).  Worse, it is a security
> hole (the qemu process will inherit supplemental group rights
> from the parent libvirtd process, which means it has access
> rights to files owned by group 0 even when such files should
> not normally be visible to user qemu).
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=964359 first demonstrated
> the problem of broken permissions in qemu when using a build based
> on 0.10.2.
> 
> LXC does not use the DAC security driver, so it is not vulnerable
> at this time.  Still, it is better to plug the latent hole on
> the master branch first, before cherry-picking it to the only
> vulnerable branch v0.10.2-maint.
> 
> * src/security/security_dac.c (virSecurityDACGetIds): Always populate
> groups and ngroups, rather than only when no label is parsed.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/security/security_dac.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 8cbb083..6876bd5 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -115,13 +115,13 @@ virSecurityDACGetIds(virDomainDefPtr def, virSecurityDACDataPtr priv,
>          return -1;
>      }
> 
> -    if ((ret = virSecurityDACParseIds(def, uidPtr, gidPtr)) <= 0) {
> -        if (groups)
> -            *groups = NULL;
> -        if (ngroups)
> -            *ngroups = 0;
> +    if (groups)
> +        *groups = priv ? priv->groups : NULL;
> +    if (ngroups)
> +        *ngroups = priv ? priv->ngroups : 0;
> +
> +    if ((ret = virSecurityDACParseIds(def, uidPtr, gidPtr)) <= 0)
>          return ret;
> -    }
> 
>      if (!priv) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -134,10 +134,6 @@ virSecurityDACGetIds(virDomainDefPtr def, virSecurityDACDataPtr priv,
>          *uidPtr = priv->user;
>      if (gidPtr)
>          *gidPtr = priv->group;
> -    if (groups)
> -        *groups = priv->groups;
> -    if (ngroups)
> -        *ngroups = priv->ngroups;
> 
>      return 0;
>  }

ACK


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|