[libvirt] [PATCH] security_dac: compute supplemental groups before fork
Eric Blake
eblake at redhat.com
Fri Jul 12 21:13:48 UTC 2013
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 at 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
More information about the libvir-list
mailing list