11:51 a.m.
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
11:51 a.m.
New subject: [libvirt] [PATCH 1/3] util: Fix error message when getpwuid_r fails to find the user
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
11:51 a.m.
New subject: [libvirt] [PATCH 2/3] security_dac: Avoid segfault when no label is requested
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
12:23 p.m.
New subject: [libvirt] [PATCH 2/3] security_dac: Avoid segfault when no label is requested
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:51 a.m.
New subject: [libvirt] [PATCH 3/3] security_dac: Don't return uninitialised value when parsing seclabels
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
12:24 p.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
5:44 p.m.
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
6:30 p.m.
New subject: [libvirt] [PATCH 4/3] security_dac: Don't return uninitialised uid and gid for image labels
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
4469
days inactive
4469
days old
7 comments
2 participants
participants (2)
-
Eric Blake -
Peter Krempa