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

Re: [libvirt] [PATCH 8/8] Add test case for SELinux label generation



On Fri, Aug 10, 2012 at 03:50:25PM -0600, Eric Blake wrote:
> On 08/10/2012 07:48 AM, Daniel P. Berrange wrote:
> > +++ b/tests/securityselinuxhelper.c
> > @@ -0,0 +1,65 @@
> > +/*
> > + * Copyright (C) 2011-2012 Red Hat, Inc.
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, write to the Free Software
> > + * License along with this library;  If not, see
> > + * <http://www.gnu.org/licenses/>.
> > + *
> > + */
> > +
> > +#include <config.h>
> > +
> > +#include <selinux/selinux.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <unistd.h>
> > +#include <errno.h>
> > +/*
> > + * The kernel policy will not allow us to arbitrarily change
> > + * test process context. This helper is used as an LD_PRELOAD
> > + * so that the libvirt code /thinks/ it is changing/reading
> > + * the process context, where as in fact we're faking it all
> > + */
> > +
> > +int getcon(security_context_t *context)
> > +{
> > +    if (getenv("FAKE_CONTEXT") == NULL) {
> > +        *context = NULL;
> > +        errno = EINVAL;
> > +        return -1;
> > +    }
> > +    *context = strdup(getenv("FAKE_CONTEXT"));
> 
> No check for allocation failure?  (not that it's likely, but it never
> hurts to be thorough)

Ok, adding these


> > +static virDomainDefPtr
> > +testBuildDomainDef(bool dynamic,
> > +                   const char *label,
> > +                   const char *baselabel)
> > +{
> > +    virDomainDefPtr def;
> > +
> > +    if (VIR_ALLOC(def) < 0)
> > +        goto no_memory;
> > +
> > +    def->seclabel.type = dynamic ? VIR_DOMAIN_SECLABEL_DYNAMIC : VIR_DOMAIN_SECLABEL_STATIC;
> > +//    if (!(def->seclabel.model = strdup("selinux")))
> > +//        goto no_memory;
> 
> Why the comments?

Obsolete code, removing it.

> 
> > +static bool
> > +testSELinuxCheckCon(context_t con,
> > +                    const char *user,
> > +                    const char *role,
> > +                    const char *type,
> > +                    int sensMin,
> > +                    int sensMax,
> > +                    int catMin,
> > +                    int catMax)
> > +{
> 
> > +    tmp++;
> > +    if (virStrToLong_i(tmp, &tmp, 10, &gotCatOne) < 0) {
> > +        fprintf(stderr, "Malformed range %s, cannot parse category one\n",
> > +                tmp);
> > +        return false;
> > +    }
> > +    if (tmp && *tmp == ',')
> 
> Did you mean '.' instead of ','?

No. Subtle distinction in semantics. 'cN.cM' is used to denote a
category range, while 'cM,cM' is used to denate a category pair

> 
> > +        tmp++;
> > +    if (tmp && *tmp == 'c') {
> > +        tmp++;
> > +        if (virStrToLong_i(tmp, &tmp, 10, &gotCatTwo) < 0) {
> > +            fprintf(stderr, "Malformed range %s, cannot parse category two\n",
> > +                    tmp);
> > +            return false;
> > +        }
> > +    } else {
> > +        gotCatTwo = gotCatOne;
> > +    }
> 
> Where do you check that theres no junk after the last category?

Added a check for this.

> 
> > +
> > +    if (gotSens < sensMin ||
> > +        gotSens > sensMax) {
> > +        fprintf(stderr, "Sensitivity %d is out of range %d-%d\n",
> > +                gotSens, sensMin, sensMax);
> > +        return false;
> > +    }
> 
> Are you meaning to do a range check here (where input of s2-s3 could let
> libvirt choose s3), or actually enforcing that libvirt always picks the
> lowest end of the range?

Good point, I'm changing this to enforce gotSens == sensMin

> > +
> > +    /* We can only run these tests if we're unconfined */
> 
> Does the test gracefully skip when run in a different context?

This is an obsolete comment now - it predates my use of the
LD_PRELOAD hack

> > +    DO_TEST_GEN_LABEL("dynamic unconfined, s0, c0.c1023",
> > +                      "unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023",
> > +                      true, NULL, NULL,
> > +                      "unconfined_u", "unconfined_r", "svirt_t", "svirt_image_t",
> > +                      0, 0, 0, 1023);
> > +    DO_TEST_GEN_LABEL("dynamic virtd, s0, c0.c1023",
> > +                      "system_u:system_r:virtd_t:s0-s0:c0.c1023",
> > +                      true, NULL, NULL,
> > +                      "system_u", "system_r", "svirt_t", "svirt_image_t",
> > +                      0, 0, 0, 1023);
> > +    DO_TEST_GEN_LABEL("dynamic virtd, s0, c0.c10",
> > +                      "system_u:system_r:virtd_t:s0-s0:c0.c10",
> > +                      true, NULL, NULL,
> > +                      "system_u", "system_r", "svirt_t", "svirt_image_t",
> > +                      0, 0, 0, 10);
> > +    DO_TEST_GEN_LABEL("dynamic virtd, s2-s3, c0.c1023",
> > +                      "system_u:system_r:virtd_t:s2-s3:c0.c1023",
> > +                      true, NULL, NULL,
> > +                      "system_u", "system_r", "svirt_t", "svirt_image_t",
> > +                      2, 3, 0, 1023);
> 
> No test of a degenerate single-range category?

I'll change one of the 's0-s0' case to just 's0'

> > index f372c23..16414d9 100644
> > --- a/tests/testutils.h
> > +++ b/tests/testutils.h
> > @@ -70,4 +70,13 @@ int virtTestMain(int argc,
> >          return virtTestMain(argc, argv, func);  \
> >      }
> >  
> > +# define VIRT_TEST_MAIN_PRELOAD(func, lib)          \
> > +    int main(int argc, char **argv) {               \
> > +        if (getenv("LD_PRELOAD") == NULL) {         \
> > +            setenv("LD_PRELOAD", lib, 1);           \
> > +            execv(argv[0], argv);                   \
> 
> This is insufficient - if LD_PRELOAD is already set for other reasons
> (such as valgrind), then you still want to modify it and re-exec.  I
> think you need to strstr() the existing contents for the new lib before
> deciding whether to re-exec.

Ah yes, good point



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 :|


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