From e6e8ccf983c15591a86805a8f9d63d7529a87639 Mon Sep 17 00:00:00 2001
Message-Id: <e6e8ccf983c15591a86805a8f9d63d7529a87639.1379193140.git.jdenemar@redhat.com>
From: Eric Blake <eblake@redhat.com>
Date: Thu, 12 Sep 2013 11:37:32 -0600
Subject: [PATCH] security: provide supplemental groups even when parsing label
 (CVE-2013-4291)

https://bugzilla.redhat.com/show_bug.cgi?id=1006513

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

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@redhat.com>
(cherry picked from commit 745aa55fbf3e076c4288d5ec3239f5a5d43508a6)
---
 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;
 }
-- 
1.8.3.2