Re: [libvirt] [PATCH 7/8] Honour current sensitivity and category ranges in SELinux label generation
On Fri, Aug 10, 2012 at 03:20:58PM -0600, Eric Blake wrote:
On 08/10/2012 07:48 AM, Daniel P. Berrange wrote:
> +
> + 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).
Having a single sensitivity (s0) is valid, but having just a
single category (c0) is not valid, since then we have no range
of categories from which to allocate VM categories. I'd added
code which explicitly raises an error in the case of a 'c0'
category.
> + 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] != '.') {
I'm actually adding
/* We *must* have a pair of categories otherwise
* there's no range to allocate VM categories from */
if (!tmp[0]) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("No category range available"));
goto cleanup;
}
> + }
> + 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);
And here I'm adding
if (catRange < 8) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Category range c%d-c%d too small"),
catMin, catMax);
goto cleanup;
}
since we really need a few categories in the pool to play
with.
>
> 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.
Yep.
>
> 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.)
The check on catRange will address this
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|