From: "Daniel P. Berrange" <berrange@redhat.com> The body of the loop in virSecuritySELinuxMCSFind would directly 'return NULL' on OOM, instead of jumping to the cleanup label. This caused a leak of several local vars. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/security/security_selinux.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index a042b26..f1399af 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -214,7 +214,7 @@ virSecuritySELinuxMCSFind(virSecurityManagerPtr mgr) if (c1 == c2) { if (virAsprintf(&mcs, "%s:c%d", sens, catMin + c1) < 0) { virReportOOMError(); - return NULL; + goto cleanup; } } else { if (c1 > c2) { @@ -224,7 +224,7 @@ virSecuritySELinuxMCSFind(virSecurityManagerPtr mgr) } if (virAsprintf(&mcs, "%s:c%d,c%d", sens, catMin + c1, catMin + c2) < 0) { virReportOOMError(); - return NULL; + goto cleanup; } } -- 1.8.1.4

From: "Daniel P. Berrange" <berrange@redhat.com> Pull the code which parses the current process MCS range out of virSecuritySELinuxMCSFind and into a new method virSecuritySELinuxMCSGetProcessRange. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/security/security_selinux.c | 135 ++++++++++++++++++++++++---------------- 1 file changed, 80 insertions(+), 55 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index f1399af..bfbb006 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -105,16 +105,69 @@ virSecuritySELinuxMCSRemove(virSecurityManagerPtr mgr, static char * -virSecuritySELinuxMCSFind(virSecurityManagerPtr mgr) +virSecuritySELinuxMCSFind(virSecurityManagerPtr mgr, + const char *sens, + int catMin, + int catMax) { virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); - int c1 = 0; - int c2 = 0; + int catRange; char *mcs = NULL; + + /* +1 since virRandomInt range is exclusive of the upper bound */ + catRange = (catMax - catMin) + 1; + + if (catRange < 8) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Category range c%d-c%d too small"), + catMin, catMax); + return NULL; + } + + VIR_DEBUG("Using sensitivity level '%s' cat min %d max %d range %d", + sens, catMin, catMax, catRange); + + for (;;) { + int c1 = virRandomInt(catRange); + int c2 = virRandomInt(catRange); + + VIR_DEBUG("Try cat %s:c%d,c%d", sens, c1+catMin, c2+catMin); + + if (c1 == c2) { + if (virAsprintf(&mcs, "%s:c%d", sens, catMin + c1) < 0) { + virReportOOMError(); + return NULL; + } + } else { + if (c1 > c2) { + int t = c1; + c1 = c2; + c2 = t; + } + if (virAsprintf(&mcs, "%s:c%d,c%d", sens, catMin + c1, catMin + c2) < 0) { + virReportOOMError(); + return NULL; + } + } + + if (virHashLookup(data->mcs, mcs) == NULL) + break; + + VIR_FREE(mcs); + } + + return mcs; +} + +static int +virSecuritySELinuxMCSGetProcessRange(char **sens, + int *catMin, + int *catMax) +{ security_context_t ourSecContext = NULL; context_t ourContext = NULL; - char *sens = NULL, *cat, *tmp; - int catMin, catMax, catRange; + char *cat, *tmp; + int ret = -1; if (getcon_raw(&ourSecContext) < 0) { virReportSystemError(errno, "%s", @@ -128,22 +181,22 @@ virSecuritySELinuxMCSFind(virSecurityManagerPtr mgr) goto cleanup; } - if (!(sens = strdup(context_range_get(ourContext)))) { + if (!(*sens = strdup(context_range_get(ourContext)))) { virReportOOMError(); goto cleanup; } /* Find and blank out the category part */ - if (!(tmp = strchr(sens, ':'))) { + if (!(tmp = strchr(*sens, ':'))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot parse sensitivity level in %s"), - sens); + *sens); goto cleanup; } *tmp = '\0'; cat = tmp + 1; /* Find and blank out the sensitivity upper bound */ - if ((tmp = strchr(sens, '-'))) + if ((tmp = strchr(*sens, '-'))) *tmp = '\0'; /* sens now just contains the sensitivity lower bound */ @@ -156,7 +209,7 @@ virSecuritySELinuxMCSFind(virSecurityManagerPtr mgr) goto cleanup; } tmp++; - if (virStrToLong_i(tmp, &tmp, 10, &catMin) < 0) { + if (virStrToLong_i(tmp, &tmp, 10, catMin) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot parse category in %s"), cat); @@ -186,60 +239,21 @@ virSecuritySELinuxMCSFind(virSecurityManagerPtr mgr) goto cleanup; } tmp++; - if (virStrToLong_i(tmp, &tmp, 10, &catMax) < 0) { + if (virStrToLong_i(tmp, &tmp, 10, catMax) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot parse category in %s"), cat); goto cleanup; } - /* +1 since virRandomInt range is exclusive of the upper bound */ - catRange = (catMax - catMin) + 1; - - if (catRange < 8) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Category range c%d-c%d too small"), - catMin, catMax); - goto cleanup; - } - - VIR_DEBUG("Using sensitivity level '%s' cat min %d max %d range %d", - sens, catMin, catMax, catRange); - - for (;;) { - c1 = virRandomInt(catRange); - c2 = virRandomInt(catRange); - VIR_DEBUG("Try cat %s:c%d,c%d", sens, c1+catMin, c2+catMin); - - if (c1 == c2) { - if (virAsprintf(&mcs, "%s:c%d", sens, catMin + c1) < 0) { - virReportOOMError(); - goto cleanup; - } - } else { - if (c1 > c2) { - int t = c1; - c1 = c2; - c2 = t; - } - if (virAsprintf(&mcs, "%s:c%d,c%d", sens, catMin + c1, catMin + c2) < 0) { - virReportOOMError(); - goto cleanup; - } - } - - if (virHashLookup(data->mcs, mcs) == NULL) - goto cleanup; - - VIR_FREE(mcs); - } + ret = 0; cleanup: - VIR_DEBUG("Found context '%s'", NULLSTR(mcs)); - VIR_FREE(sens); + if (ret < 0) + VIR_FREE(*sens); freecon(ourSecContext); context_free(ourContext); - return mcs; + return ret; } static char * @@ -559,6 +573,8 @@ virSecuritySELinuxGenSecurityLabel(virSecurityManagerPtr mgr, virSecurityLabelDefPtr seclabel; virSecuritySELinuxDataPtr data; const char *baselabel; + char *sens = NULL; + int catMin, catMax; seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (seclabel == NULL) { @@ -609,7 +625,15 @@ virSecuritySELinuxGenSecurityLabel(virSecurityManagerPtr mgr, break; case VIR_DOMAIN_SECLABEL_DYNAMIC: - if (!(mcs = virSecuritySELinuxMCSFind(mgr))) + if (virSecuritySELinuxMCSGetProcessRange(&sens, + &catMin, + &catMax) < 0) + goto cleanup; + + if (!(mcs = virSecuritySELinuxMCSFind(mgr, + sens, + catMin, + catMax))) goto cleanup; if (virSecuritySELinuxMCSAdd(mgr, mcs) < 0) @@ -688,6 +712,7 @@ cleanup: context_free(ctx); VIR_FREE(scontext); VIR_FREE(mcs); + VIR_FREE(sens); VIR_DEBUG("model=%s label=%s imagelabel=%s baselabel=%s", NULLSTR(seclabel->model), -- 1.8.1.4

From: "Daniel P. Berrange" <berrange@redhat.com> Normally libvirtd should run with a SELinux label system_u:system_r:virtd_t:s0-s0:c0.c1023 If a user manually runs libvirtd though, it is sometimes possible to get into a situation where it is running system_u:system_r:init_t:s0 The SELinux security driver isn't expecting this and can't parse the security label since it lacks the ':c0.c1023' part causing it to complain internal error Cannot parse sensitivity level in s0 This updates the parser to cope with this, so if no category is present, libvirtd will hardcode the equivalent of c0.c1023. Now this won't work if SELinux is in Enforcing mode, but that's not an issue, because the user can only get into this problem if in Permissive mode. This means they can now start VMs in Permissive mode without hitting that parsing error Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/security/security_selinux.c | 38 +++++++++++++++++++++++++++++--------- tests/securityselinuxtest.c | 12 ++++++++++++ 2 files changed, 41 insertions(+), 9 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index bfbb006..ced2b12 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -159,6 +159,20 @@ virSecuritySELinuxMCSFind(virSecurityManagerPtr mgr, return mcs; } + +/* + * This needs to cope with several styles of range + * + * system_u:system_r:virtd_t:s0 + * system_u:system_r:virtd_t:s0-s0 + * system_u:system_r:virtd_t:s0-s0:c0.c1023 + * + * In the first two cases, we'll assume c0.c1023 for + * the category part, since that's what we're really + * interested in. This won't work in Enforcing mode, + * but will prevent libvirtd breaking in Permissive + * mode when run with a wierd procss label. + */ static int virSecuritySELinuxMCSGetProcessRange(char **sens, int *catMin, @@ -166,7 +180,8 @@ virSecuritySELinuxMCSGetProcessRange(char **sens, { security_context_t ourSecContext = NULL; context_t ourContext = NULL; - char *cat, *tmp; + char *cat = NULL; + char *tmp; int ret = -1; if (getcon_raw(&ourSecContext) < 0) { @@ -186,20 +201,25 @@ virSecuritySELinuxMCSGetProcessRange(char **sens, goto cleanup; } - /* Find and blank out the category part */ - if (!(tmp = strchr(*sens, ':'))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot parse sensitivity level in %s"), - *sens); - goto cleanup; + /* Find and blank out the category part (if any) */ + tmp = strchr(*sens, ':'); + if (tmp) { + *tmp = '\0'; + cat = tmp + 1; } - *tmp = '\0'; - cat = tmp + 1; /* Find and blank out the sensitivity upper bound */ if ((tmp = strchr(*sens, '-'))) *tmp = '\0'; /* sens now just contains the sensitivity lower bound */ + /* If there was no category part, just assume c0.c1024 */ + if (!cat) { + *catMin = 0; + *catMax = 1024; + ret = 0; + goto cleanup; + } + /* Find & extract category min */ tmp = cat; if (tmp[0] != 'c') { diff --git a/tests/securityselinuxtest.c b/tests/securityselinuxtest.c index ba00010..da8a12f 100644 --- a/tests/securityselinuxtest.c +++ b/tests/securityselinuxtest.c @@ -297,6 +297,18 @@ mymain(void) } while (0) DO_TEST_GEN_LABEL("dynamic unconfined, s0, c0.c1023", + "unconfined_u:unconfined_r:unconfined_t:s0", + true, NULL, NULL, + "unconfined_u", "unconfined_r", "object_r", + "svirt_t", "svirt_image_t", + 0, 0, 0, 1023); + DO_TEST_GEN_LABEL("dynamic unconfined, s0, c0.c1023", + "unconfined_u:unconfined_r:unconfined_t:s0-s0", + true, NULL, NULL, + "unconfined_u", "unconfined_r", "object_r", + "svirt_t", "svirt_image_t", + 0, 0, 0, 1023); + DO_TEST_GEN_LABEL("dynamic unconfined, s0, c0.c1023", "unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023", true, NULL, NULL, "unconfined_u", "unconfined_r", "object_r", -- 1.8.1.4