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@xxxxxxxxxx> > --- > 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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list