4:20 p.m.
On 08/10/2012 07:48 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org