On 08/10/2012 07:48 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Currently the dynamic label generation code will create labels with a sensitivity of s0, and a category pair in the range 0-1023. This is fine when running a standard MCS policy because libvirtd will run with a label
system_u:system_r:virtd_t:s0-s0:c0.c1023
With custom policies though, it is possible for libvirtd to have a different sensitivity, or category range. For example
system_u:system_r:virtd_t:s2-s3:c512.c1023
In this case we must assign the VM a sensitivity matching the current lower sensitivity value, and categories in the range 512-1023
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
+++ b/src/security/security_selinux.c @@ -106,13 +106,90 @@ virSecuritySELinuxMCSFind(virSecurityManagerPtr mgr) int c1 = 0; int c2 = 0; char *mcs = NULL; + security_context_t curseccontext = NULL;
More of that fun naming. Underscoresreallydomakeiteasiertoread.
+ context_t curcontext = NULL; + char *sens, *cat, *tmp; + int catMin, catMax, catRange;
OK, so camel case would also make things easier to read. I don't know if we have a coding guideline which says which to use, but consistency argues for the same type of separation for all local variables in a single function.
+ + if (getcon(&curseccontext) < 0) { + virReportSystemError(errno, "%s", + _("Unable to get current process SELinux context")); + goto cleanup; + } + if (!(curcontext = context_new(curseccontext))) { + virReportSystemError(errno, + _("Unable to parse current SELinux context '%s'"), + curseccontext); + goto cleanup; + } + + if (!(sens = strdup(context_range_get(curcontext)))) {
Just to make sure I'm following here, I'll assume two different strings for sens at this point; one from your commit message (s2-s3:c512.c1023) and one from a degenerate s0:c0 (as the previous patch showed that libvirt will create a single context instead of a range if two random numbers match).
+ virReportOOMError(); + 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; + } + *tmp = '\0'; + cat = tmp + 1;
so here, with my examples, we would sens with s2-s3 (or s0), and cat with c512.c1023 (or c0)
+ /* Find and blank out the sensitivity upper bound */ + if ((tmp = strchr(sens, '-'))) + *tmp = '\0';
and here, sens is now shortened to s2 (or still s0)
+ + /* sens now just contains the sensitivity lower bound */ + tmp = cat;
The spacing for this comment was awkward; it made me look for sens in the next block of code, even though it is an assertion about the results after the prior block of code.
+ if (tmp[0] != 'c') { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse category in %s"), + cat); + goto cleanup; + } + tmp++; + if (virStrToLong_i(tmp, &tmp, 10, &catMin) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse category in %s"), + cat); + goto cleanup; + }
here, we've parsed out either 512 from c512.c1023, or 0 from c0
+ if (tmp[0] != '.') { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse category in %s"), + cat); + goto cleanup;
and here, we fall flat on our face if we were in the degenerate case of a single category. Oops. You need to start: if (!tmp[0]) { catMax = catMin; } else if (tmp[0] != '.') {
+ } + tmp++; + if (tmp[0] != 'c') { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse category in %s"), + cat); + goto cleanup; + } + tmp++; + if (virStrToLong_i(tmp, &tmp, 10, &catMax) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse category in %s"), + cat); + goto cleanup; + }
and this parsed out 1023 on the c512.c1023 example.
+ + /* max is inclusive, hence the + 1 */ + catRange = (catMax - catMin) + 1; + + VIR_DEBUG("Using sensitivity level '%s' cat min %d max %d range %d", + sens, catMin, catMax, catRange);
for (;;) { - c1 = virRandomBits(10); - c2 = virRandomBits(10); + c1 = virRandom(catRange); + c2 = virRandom(catRange); + VIR_DEBUG("Try cat %s:c%d,c%d", sens, c1, c2);
This debug message would make more sense as c1+catMin, c2+catMin.
if (c1 == c2) { - if (virAsprintf(&mcs, "s0:c%d", c1) < 0) { + if (virAsprintf(&mcs, "%s:c%d", sens, catMin + c1) < 0) {
If you fix the parsing of the degenerate case, then for the c0 input case, you would always reach here, with catMin == 0 and c1 == 0 (since virRandom(1) == 0 - actually, you need to make sure that we don't trigger undefined behavior when calling virRandom(1), with whatever algorithm we finally end up with for that function; but I do agree that virRandom(0) should trigger an assertion failure.)
virReportOOMError(); return NULL; } @@ -122,7 +199,7 @@ virSecuritySELinuxMCSFind(virSecurityManagerPtr mgr) c2 ^= c1; c1 ^= c2; } - if (virAsprintf(&mcs, "s0:c%d,c%d", c1, c2) < 0) { + if (virAsprintf(&mcs, "%s:c%d,c%d", sens, catMin + c1, catMin + c2) < 0) {
and most of the time for the c512.c1023 case, you would get here with two random numbers more or less evenly distributed.
virReportOOMError(); return NULL; } @@ -136,6 +213,9 @@ virSecuritySELinuxMCSFind(virSecurityManagerPtr mgr)
cleanup: VIR_DEBUG("Found context '%s'", NULLSTR(mcs)); + VIR_FREE(sens); + freecon(curseccontext); + context_free(curcontext); return mcs; }
Looks mostly sane. ACK if you clean up the degenerate context case. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org