[libvirt] [PATCHv2 0/2] lxc/dac: avoid getgrouplist between fork/exec
v1 was here: https://www.redhat.com/archives/libvir-list/2013-July/msg00853.html Changes since then: split into two patches, and delay supplemental group computation until just before forking Eric Blake (2): security: framework for driver PreFork handler security_dac: compute supplemental groups before fork src/qemu/qemu_process.c | 3 +- src/security/security_dac.c | 63 ++++++++++++++++++++++++++++------------- src/security/security_driver.h | 4 +++ src/security/security_manager.c | 16 +++++++++-- src/security/security_manager.h | 2 +- src/security/security_stack.c | 23 +++++++++++++++ 6 files changed, 88 insertions(+), 23 deletions(-) -- 1.8.3.1
A future patch wants the DAC security manager to be able to safely get the supplemental group list for a given uid, but at the time of a fork rather than during initialization so as to pick up on live changes to the system's group database. This patch adds the framework, including the possibility of a pre-fork callback failing. For now, any driver that implements a prefork callback must be robust against the possibility of being part of a security stack where a later element in the chain fails prefork. This means that drivers cannot do any action that requires a call to postfork for proper cleanup (no grabbing a mutex, for example). If this is too prohibitive in the future, we would have to switch to a transactioning sequence, where each driver has (up to) 3 callbacks: PreForkPrepare, PreForkCommit, and PreForkAbort, to either clean up or commit changes made during prepare. * src/security/security_driver.h (virSecurityDriverPreFork): New callback. * src/security/security_manager.h (virSecurityManagerPreFork): Change signature. * src/security/security_manager.c (virSecurityManagerPreFork): Optionally call into driver, and allow returning failure. * src/security/security_stack.c (virSecurityDriverStack): Wrap the handler for the stack driver. * src/qemu/qemu_process.c (qemuProcessStart): Adjust caller. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_process.c | 3 ++- src/security/security_driver.h | 4 ++++ src/security/security_manager.c | 16 ++++++++++++++-- src/security/security_manager.h | 2 +- src/security/security_stack.c | 23 +++++++++++++++++++++++ 5 files changed, 44 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3d5e8f6..a594cc6 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3730,7 +3730,8 @@ int qemuProcessStart(virConnectPtr conn, virCommandDaemonize(cmd); virCommandRequireHandshake(cmd); - virSecurityManagerPreFork(driver->securityManager); + if (virSecurityManagerPreFork(driver->securityManager) < 0) + goto cleanup; ret = virCommandRun(cmd, NULL); virSecurityManagerPostFork(driver->securityManager); diff --git a/src/security/security_driver.h b/src/security/security_driver.h index cc401e1..8735558 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -47,6 +47,8 @@ typedef int (*virSecurityDriverClose) (virSecurityManagerPtr mgr); typedef const char *(*virSecurityDriverGetModel) (virSecurityManagerPtr mgr); typedef const char *(*virSecurityDriverGetDOI) (virSecurityManagerPtr mgr); +typedef int (*virSecurityDriverPreFork) (virSecurityManagerPtr mgr); + typedef int (*virSecurityDomainRestoreImageLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainDiskDefPtr disk); @@ -119,6 +121,8 @@ struct _virSecurityDriver { virSecurityDriverGetModel getModel; virSecurityDriverGetDOI getDOI; + virSecurityDriverPreFork preFork; + virSecurityDomainSecurityVerify domainSecurityVerify; virSecurityDomainSetImageLabel domainSetSecurityImageLabel; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 411a909..92fb504 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -193,11 +193,23 @@ virSecurityManagerPtr virSecurityManagerNew(const char *name, /* * Must be called before fork()'ing to ensure mutex state - * is sane for the child to use + * is sane for the child to use. A negative return means the + * child must not be forked; a successful return must be + * followed by a call to virSecurityManagerPostFork() in both + * parent and child. */ -void virSecurityManagerPreFork(virSecurityManagerPtr mgr) +int virSecurityManagerPreFork(virSecurityManagerPtr mgr) { + int ret = 0; + virObjectLock(mgr); + if (mgr->drv->preFork) { + ret = mgr->drv->preFork(mgr); + if (ret < 0) + virObjectUnlock(mgr); + } + + return ret; } diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 711b354..9252830 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -47,7 +47,7 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, bool requireConfined, bool dynamicOwnership); -void virSecurityManagerPreFork(virSecurityManagerPtr mgr); +int virSecurityManagerPreFork(virSecurityManagerPtr mgr); void virSecurityManagerPostFork(virSecurityManagerPtr mgr); void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr); diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 1e229d7..ed69b9c 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -112,6 +112,27 @@ virSecurityStackGetDOI(virSecurityManagerPtr mgr) } static int +virSecurityStackPreFork(virSecurityManagerPtr mgr) +{ + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityStackItemPtr item = priv->itemsHead; + int rc = 0; + + /* XXX For now, we rely on no driver having any state that requires + * rollback if a later driver in the stack fails; if this changes, + * we'd need to split this into transaction semantics by dividing + * the work into prepare/commit/abort. */ + for (; item; item = item->next) { + if (virSecurityManagerPreFork(item->securityManager) < 0) { + rc = -1; + break; + } + } + + return rc; +} + +static int virSecurityStackVerify(virSecurityManagerPtr mgr, virDomainDefPtr def) { @@ -539,6 +560,8 @@ virSecurityDriver virSecurityDriverStack = { .getModel = virSecurityStackGetModel, .getDOI = virSecurityStackGetDOI, + .preFork = virSecurityStackPreFork, + .domainSecurityVerify = virSecurityStackVerify, .domainSetSecurityImageLabel = virSecurityStackSetSecurityImageLabel, -- 1.8.3.1
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 (_virSecurityDACData): Track groups in private data. (virSecurityDACPreFork): New function, to set them. (virSecurityDACClose): Clean up new fields. (virSecurityDACGetIds): Alter signature. (virSecurityDACSetSecurityHostdevLabelHelper) (virSecurityDACSetChardevLabel, virSecurityDACSetProcessLabel) (virSecurityDACSetChildProcessLabel): Update callers. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/security/security_dac.c | 63 +++++++++++++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 19 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 9b5eaa8..00d04bf 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -43,6 +43,8 @@ typedef virSecurityDACData *virSecurityDACDataPtr; struct _virSecurityDACData { uid_t user; gid_t group; + gid_t *groups; + int ngroups; bool dynamicOwnership; }; @@ -146,7 +148,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 +160,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 +179,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 +262,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; } @@ -269,6 +283,21 @@ virSecurityDACGetDOI(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) } static int +virSecurityDACPreFork(virSecurityManagerPtr mgr) +{ + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + int ngroups; + + VIR_FREE(priv->groups); + priv->ngroups = 0; + if ((ngroups = virGetGroupList(priv->user, priv->group, + &priv->groups)) < 0) + return -1; + priv->ngroups = ngroups; + return 0; +} + +static int virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid) { VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", @@ -448,7 +477,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 +731,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 +1025,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 +1051,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", @@ -1206,6 +1229,8 @@ virSecurityDriver virSecurityDriverDAC = { .getModel = virSecurityDACGetModel, .getDOI = virSecurityDACGetDOI, + .preFork = virSecurityDACPreFork, + .domainSecurityVerify = virSecurityDACVerify, .domainSetSecurityImageLabel = virSecurityDACSetSecurityImageLabel, -- 1.8.3.1
On 18.07.2013 01:08, Eric Blake wrote:
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 (_virSecurityDACData): Track groups in private data. (virSecurityDACPreFork): New function, to set them. (virSecurityDACClose): Clean up new fields. (virSecurityDACGetIds): Alter signature. (virSecurityDACSetSecurityHostdevLabelHelper) (virSecurityDACSetChardevLabel, virSecurityDACSetProcessLabel) (virSecurityDACSetChildProcessLabel): Update callers.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/security/security_dac.c | 63 +++++++++++++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 19 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 9b5eaa8..00d04bf 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -43,6 +43,8 @@ typedef virSecurityDACData *virSecurityDACDataPtr; struct _virSecurityDACData { uid_t user; gid_t group; + gid_t *groups; + int ngroups; bool dynamicOwnership; };
@@ -146,7 +148,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 +160,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;
I believe you wanted *ngroups = 0; in here.
return ret; + }
if (!priv) { virReportError(VIR_ERR_INTERNAL_ERROR,
Michal
On 07/18/2013 06:46 AM, Michal Privoznik wrote:
On 18.07.2013 01:08, Eric Blake wrote:
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.
- if ((ret = virSecurityDACParseIds(def, uidPtr, gidPtr)) <= 0) + if ((ret = virSecurityDACParseIds(def, uidPtr, gidPtr)) <= 0) { + if (groups) + *groups = NULL; + if (ngroups) + ngroups = 0;
I believe you wanted *ngroups = 0; in here.
Indeed. I blame C for treating 0 and NULL interchangeably.
ACK series, but see the issue I'm raising in 2/2.
Thanks; I'll push after fixing that typo. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 18.07.2013 15:16, Eric Blake wrote:
On 07/18/2013 06:46 AM, Michal Privoznik wrote:
On 18.07.2013 01:08, Eric Blake wrote:
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.
- if ((ret = virSecurityDACParseIds(def, uidPtr, gidPtr)) <= 0) + if ((ret = virSecurityDACParseIds(def, uidPtr, gidPtr)) <= 0) { + if (groups) + *groups = NULL; + if (ngroups) + ngroups = 0;
I believe you wanted *ngroups = 0; in here.
Indeed. I blame C for treating 0 and NULL interchangeably.
ACK series, but see the issue I'm raising in 2/2.
Thanks; I'll push after fixing that typo.
In fact, gcc warned me about possibly unused variable: security/security_dac.c: In function 'virSecurityDACSetProcessLabel': security/security_dac.c:1038:21: error: 'ngroups' may be used uninitialized in this function [-Werror=maybe-uninitialized] if (virSetUIDGID(user, group, groups, ngroups) < 0) ^ security/security_dac.c: At top level: cc1: error: unrecognized command line option "-Wno-unused-command-line-argument" [-Werror] cc1: all warnings being treated as errors Truth is, I'm using the most recent gcc (4.8.1). Which is both advantage and disadvantage. One hand, I can catch errors like these, on the other hand, the gcc bug in static analysis has been fixed. The bug, where local variable 'shadows' global function. This is NOT what I call shadowing. Michal
On 18.07.2013 01:08, Eric Blake wrote:
v1 was here: https://www.redhat.com/archives/libvir-list/2013-July/msg00853.html
Changes since then: split into two patches, and delay supplemental group computation until just before forking
Eric Blake (2): security: framework for driver PreFork handler security_dac: compute supplemental groups before fork
src/qemu/qemu_process.c | 3 +- src/security/security_dac.c | 63 ++++++++++++++++++++++++++++------------- src/security/security_driver.h | 4 +++ src/security/security_manager.c | 16 +++++++++-- src/security/security_manager.h | 2 +- src/security/security_stack.c | 23 +++++++++++++++ 6 files changed, 88 insertions(+), 23 deletions(-)
ACK series, but see the issue I'm raising in 2/2. Michal
participants (2)
-
Eric Blake -
Michal Privoznik