[libvirt] [PATCH] security_dac: compute supplemental groups before fork
Commit 75c1256 states that virGetGroupList must not be called between fork and exec, then commit ee777e99 promptly violated that for lxc's use of virSecurityManagerSetProcessLabel. Hoist the supplemental group detection to the time that the security manager is created. Qemu is safe, as it uses virSecurityManagerSetChildProcessLabel which in turn uses virCommand to determine supplemental groups. This does not fix the fact that virSecurityManagerSetProcessLabel calls virSecurityDACParseIds calls parseIds which eventually calls getpwnam_r, which also violates fork/exec async-signal-safe safety rules, but so far no one has complained of hitting deadlock in that case. * src/security/security_dac.c (virSecurityDACSetUser) (virSecurityDACSetGroup): Merge... (virSecurityDACSetUIDGID): ...into new function. (_virSecurityDACData): Track groups in private data. (virSecurityDACClose): Clean up new fields. (virSecurityDACGetIds): Alter signature. (virSecurityDACSetSecurityHostdevLabelHelper) (virSecurityDACSetChardevLabel, virSecurityDACSetProcessLabel) (virSecurityDACSetChildProcessLabel): Update callers. * src/security/security_dac.h: Update prototypes to match. * src/security/security_manager.c (virSecurityManagerNewDAC): Adjust caller. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/security/security_dac.c | 59 ++++++++++++++++++++++------------------- src/security/security_dac.h | 9 +++---- src/security/security_manager.c | 11 ++++++-- 3 files changed, 44 insertions(+), 35 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 9b5eaa8..124f270 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -43,23 +43,20 @@ typedef virSecurityDACData *virSecurityDACDataPtr; struct _virSecurityDACData { uid_t user; gid_t group; + gid_t *groups; + int ngroups; bool dynamicOwnership; }; void -virSecurityDACSetUser(virSecurityManagerPtr mgr, - uid_t user) +virSecurityDACSetUIDGID(virSecurityManagerPtr mgr, + uid_t user, gid_t group, gid_t *groups, int ngroups) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); priv->user = user; -} - -void -virSecurityDACSetGroup(virSecurityManagerPtr mgr, - gid_t group) -{ - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); priv->group = group; + priv->groups = groups; + priv->ngroups = ngroups; } void @@ -146,7 +143,8 @@ virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr) static int virSecurityDACGetIds(virDomainDefPtr def, virSecurityDACDataPtr priv, - uid_t *uidPtr, gid_t *gidPtr) + uid_t *uidPtr, gid_t *gidPtr, + gid_t **groups, int *ngroups) { int ret; @@ -157,8 +155,13 @@ virSecurityDACGetIds(virDomainDefPtr def, virSecurityDACDataPtr priv, return -1; } - if ((ret = virSecurityDACParseIds(def, uidPtr, gidPtr)) <= 0) + if ((ret = virSecurityDACParseIds(def, uidPtr, gidPtr)) <= 0) { + if (groups) + *groups = NULL; + if (ngroups) + ngroups = 0; return ret; + } if (!priv) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -171,6 +174,10 @@ 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; } @@ -250,8 +257,10 @@ virSecurityDACOpen(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) } static int -virSecurityDACClose(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) +virSecurityDACClose(virSecurityManagerPtr mgr) { + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + VIR_FREE(priv->groups); return 0; } @@ -448,7 +457,7 @@ virSecurityDACSetSecurityHostdevLabelHelper(const char *file, uid_t user; gid_t group; - if (virSecurityDACGetIds(def, priv, &user, &group)) + if (virSecurityDACGetIds(def, priv, &user, &group, NULL, NULL)) return -1; return virSecurityDACSetOwnership(file, user, group); @@ -702,7 +711,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, uid_t user; gid_t group; - if (virSecurityDACGetIds(def, priv, &user, &group)) + if (virSecurityDACGetIds(def, priv, &user, &group, NULL, NULL)) return -1; switch (dev->type) { @@ -996,26 +1005,20 @@ virSecurityDACSetProcessLabel(virSecurityManagerPtr mgr, { uid_t user; gid_t group; - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); gid_t *groups; int ngroups; - int ret = -1; + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); - if (virSecurityDACGetIds(def, priv, &user, &group)) - return -1; - ngroups = virGetGroupList(user, group, &groups); - if (ngroups < 0) + if (virSecurityDACGetIds(def, priv, &user, &group, &groups, &ngroups)) return -1; - VIR_DEBUG("Dropping privileges of DEF to %u:%u", - (unsigned int) user, (unsigned int) group); + VIR_DEBUG("Dropping privileges of DEF to %u:%u, %d supplemental groups", + (unsigned int) user, (unsigned int) group, ngroups); if (virSetUIDGID(user, group, groups, ngroups) < 0) - goto cleanup; - ret = 0; -cleanup: - VIR_FREE(groups); - return ret; + return -1; + + return 0; } @@ -1028,7 +1031,7 @@ virSecurityDACSetChildProcessLabel(virSecurityManagerPtr mgr, gid_t group; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); - if (virSecurityDACGetIds(def, priv, &user, &group)) + if (virSecurityDACGetIds(def, priv, &user, &group, NULL, NULL)) return -1; VIR_DEBUG("Setting child to drop privileges of DEF to %u:%u", diff --git a/src/security/security_dac.h b/src/security/security_dac.h index 02432a5..23fcf2c 100644 --- a/src/security/security_dac.h +++ b/src/security/security_dac.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2010-2011 Red Hat, Inc. + * Copyright (C) 2010-2011, 2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -25,10 +25,9 @@ extern virSecurityDriver virSecurityDriverDAC; -void virSecurityDACSetUser(virSecurityManagerPtr mgr, - uid_t user); -void virSecurityDACSetGroup(virSecurityManagerPtr mgr, - gid_t group); +void virSecurityDACSetUIDGID(virSecurityManagerPtr mgr, + uid_t user, gid_t group, + gid_t *groups, int ngroups); void virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr, bool dynamic); diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 6946637..92ff264 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -136,6 +136,9 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, bool requireConfined, bool dynamicOwnership) { + gid_t *groups; + int ngroups; + virSecurityManagerPtr mgr = virSecurityManagerNewDriver(&virSecurityDriverDAC, virtDriver, @@ -146,8 +149,12 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, if (!mgr) return NULL; - virSecurityDACSetUser(mgr, user); - virSecurityDACSetGroup(mgr, group); + if ((ngroups = virGetGroupList(user, group, &groups)) < 0) { + virObjectUnref(mgr); + return NULL; + } + + virSecurityDACSetUIDGID(mgr, user, group, groups, ngroups); virSecurityDACSetDynamicOwnership(mgr, dynamicOwnership); return mgr; -- 1.8.1.4
On Fri, Jul 12, 2013 at 03:13:48PM -0600, Eric Blake wrote:
@@ -146,8 +149,12 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, if (!mgr) return NULL;
- virSecurityDACSetUser(mgr, user); - virSecurityDACSetGroup(mgr, group); + if ((ngroups = virGetGroupList(user, group, &groups)) < 0) { + virObjectUnref(mgr); + return NULL; + } + + virSecurityDACSetUIDGID(mgr, user, group, groups, ngroups);
Hmm, the virSecurityManagerNewDAC method is run once at libvirtd startup. Previously if you edited /etc/group to change a user's groups it would take effect the very next time a VM is started. With this change, it will only take effect if libvirtd is restarted, which is a regression in behaviour. 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 07/15/2013 06:14 AM, Daniel P. Berrange wrote:
On Fri, Jul 12, 2013 at 03:13:48PM -0600, Eric Blake wrote:
@@ -146,8 +149,12 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, if (!mgr) return NULL;
- virSecurityDACSetUser(mgr, user); - virSecurityDACSetGroup(mgr, group); + if ((ngroups = virGetGroupList(user, group, &groups)) < 0) { + virObjectUnref(mgr); + return NULL; + } + + virSecurityDACSetUIDGID(mgr, user, group, groups, ngroups);
Hmm, the virSecurityManagerNewDAC method is run once at libvirtd startup. Previously if you edited /etc/group to change a user's groups it would take effect the very next time a VM is started. With this change, it will only take effect if libvirtd is restarted, which is a regression in behaviour.
But we still want to avoid the deadlock possible when probing the group database between fork and exec. I guess that means I need to change virSecurityManagerPreFork() to be the entry point where we probe supplemental groups, so that the probe is done as late as possible before the fork (and thus picks up the latest /etc changes). I'll try that for a v2. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel P. Berrange -
Eric Blake