[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 7/8] Honour current sensitivity and category ranges in SELinux label generation



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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]