[libvirt] [PATCH 0/3] Fix bugs in DAC security driver preventing guest start
When using the DAC security driver with security_default_confined = 0 the daemon segfaulted. After fixing that problem guests could not be started because the driver had a logic bug that used uninitialized values while setting hypervisor uid and gid. This is probably a regression and I would like to hear your opinion if it's worth to push before the release tomorrow. Peter Krempa (3): util: Fix error message when getpwuid_r fails to find the user security_dac: Avoid segfault when no label is requested security_dac: Don't return uninitialised value when parsing seclabels src/security/security_dac.c | 5 +++-- src/util/util.c | 22 ++++++++++++++++------ 2 files changed, 19 insertions(+), 8 deletions(-) -- 1.7.12
getpwuid_r returns success but sets the return structure to NULL when it fails to deliver data about the requested uid. In our helper code this created following strange error messages: " ... cannot getpwuid_r(1234): Success" This patch creates a more helpful message: " ... getpwuid_r failed to retrieve data for uid '1234'" --- This patch is not strictly necessary to fix the bug. --- src/util/util.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 9068e0f..91eab72 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2629,6 +2629,7 @@ int virSetUIDGID(uid_t uid, gid_t gid) { int err; + char *buf = NULL; if (gid > 0) { if (setregid(gid, gid) < 0) { @@ -2642,7 +2643,6 @@ virSetUIDGID(uid_t uid, gid_t gid) if (uid > 0) { # ifdef HAVE_INITGROUPS struct passwd pwd, *pwd_result; - char *buf = NULL; size_t bufsize; int rc; @@ -2659,25 +2659,32 @@ virSetUIDGID(uid_t uid, gid_t gid) &pwd_result)) == ERANGE) { if (VIR_RESIZE_N(buf, bufsize, bufsize, bufsize) < 0) { virReportOOMError(); - VIR_FREE(buf); err = ENOMEM; goto error; } } - if (rc || !pwd_result) { + + if (rc) { virReportSystemError(err = rc, _("cannot getpwuid_r(%d)"), (unsigned int) uid); - VIR_FREE(buf); goto error; } + + if (!pwd_result) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("getpwuid_r failed to retrieve data " + "for uid '%d'"), + (unsigned int) uid); + err = EINVAL; + goto error; + } + if (initgroups(pwd.pw_name, pwd.pw_gid) < 0) { virReportSystemError(err = errno, _("cannot initgroups(\"%s\", %d)"), pwd.pw_name, (unsigned int) pwd.pw_gid); - VIR_FREE(buf); goto error; } - VIR_FREE(buf); # endif if (setreuid(uid, uid) < 0) { virReportSystemError(err = errno, @@ -2686,9 +2693,12 @@ virSetUIDGID(uid_t uid, gid_t gid) goto error; } } + + VIR_FREE(buf); return 0; error: + VIR_FREE(buf); errno = err; return -1; } -- 1.7.12
When no DAC "label" was requested for a domain the DAC manager tried to strdup a NULL string causing a segfault. --- src/security/security_dac.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 925498f..4162e26 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -890,6 +890,7 @@ virSecurityDACGenLabel(virSecurityManagerPtr mgr, break; case VIR_DOMAIN_SECLABEL_NONE: /* no op */ + return 0; break; default: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -899,7 +900,7 @@ virSecurityDACGenLabel(virSecurityManagerPtr mgr, } if (!seclabel->norelabel) { - if (seclabel->imagelabel == NULL) { + if (seclabel->imagelabel == NULL && seclabel->label != NULL) { seclabel->imagelabel = strdup(seclabel->label); if (seclabel->imagelabel == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, -- 1.7.12
On 08/28/2012 09:51 AM, Peter Krempa wrote:
When no DAC "label" was requested for a domain the DAC manager tried to strdup a NULL string causing a segfault. --- src/security/security_dac.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 925498f..4162e26 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -890,6 +890,7 @@ virSecurityDACGenLabel(virSecurityManagerPtr mgr, break; case VIR_DOMAIN_SECLABEL_NONE: /* no op */ + return 0; break;
The 'break' is now dead code, and can be deleted. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
When starting a machine the DAC security driver tries to set the UID and GID of the newly spawned process. This worked as desired if the desired label was set. When the label was missing a logical bug in virSecurityDACGenLabel() caused that uninitialised values were used as uid and gid for the new process. With this patch, default values (from qemu driver configuration) are used if the label is not found. --- src/security/security_dac.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4162e26..2527759 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -101,7 +101,7 @@ int virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr) return -1; seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - if (seclabel == NULL) { + if (seclabel == NULL || seclabel->label == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, _("security label for DAC not found in domain %s"), def->name); -- 1.7.12
On 08/28/2012 09:51 AM, Peter Krempa wrote:
When using the DAC security driver with security_default_confined = 0 the daemon segfaulted. After fixing that problem guests could not be started because the driver had a logic bug that used uninitialized values while setting hypervisor uid and gid.
This is probably a regression and I would like to hear your opinion if it's worth to push before the release tomorrow.
Peter Krempa (3): util: Fix error message when getpwuid_r fails to find the user security_dac: Avoid segfault when no label is requested security_dac: Don't return uninitialised value when parsing seclabels
ACK series, with a nit in 2/3. Definitely worth including all 3 before 0.10.0. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/28/12 19:24, Eric Blake wrote:
On 08/28/2012 09:51 AM, Peter Krempa wrote:
When using the DAC security driver with security_default_confined = 0 the daemon segfaulted. After fixing that problem guests could not be started because the driver had a logic bug that used uninitialized values while setting hypervisor uid and gid.
This is probably a regression and I would like to hear your opinion if it's worth to push before the release tomorrow.
Peter Krempa (3): util: Fix error message when getpwuid_r fails to find the user security_dac: Avoid segfault when no label is requested security_dac: Don't return uninitialised value when parsing seclabels
ACK series, with a nit in 2/3. Definitely worth including all 3 before 0.10.0.
I fixed 2/3 and pushed the series. Thanks for the review. Peter
As in the previous commit also images are chowned to uninitialised values if the label is not present. --- As this fix is identical to already ACKed 3/3, I'm pushing this as trivial. --- src/security/security_dac.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 2527759..5de7391 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -152,7 +152,7 @@ int virSecurityDACParseImageIds(virDomainDefPtr def, return -1; seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - if (seclabel == NULL) { + if (seclabel == NULL || seclabel->imagelabel == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, _("security label for DAC not found in domain %s"), def->name); -- 1.7.12
participants (2)
-
Eric Blake -
Peter Krempa