[libvirt] [PATCH] security: provide supplemental groups even when parsing label (CVE-2013-4291)
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@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; } -- 1.8.3.1
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@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 :|
On 08/29/2013 08:53 AM, Daniel P. Berrange wrote:
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.
ACK
Thanks; pushed to master and v0.10.2-maint. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel P. Berrange -
Eric Blake