6:08 p.m.
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
6:08 p.m.
New subject: [libvirt] [PATCHv2 1/2] security: framework for driver PreFork handler
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(a)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
6:08 p.m.
New subject: [libvirt] [PATCHv2 2/2] 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 (_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(a)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
7:46 a.m.
New subject: [libvirt] [PATCHv2 2/2] security_dac: compute supplemental groups before fork
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(a)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
8:16 a.m.
New subject: [libvirt] [PATCHv2 2/2] security_dac: compute supplemental groups before fork
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
8:29 a.m.
New subject: [libvirt] [PATCHv2 2/2] security_dac: compute supplemental groups before fork
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
7:46 a.m.
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
4145
days inactive
4146
days old
6 comments
2 participants
participants (2)
-
Eric Blake -
Michal Privoznik