On 08/10/2012 07:48 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> +++ 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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list