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

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)

> +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?

> +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 ','?

> + 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?

> + > + 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?

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

> + 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?

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