On 04/01/2009 12:50 PM, Daniel P. Berrange wrote:
On Wed, Apr 01, 2009 at 10:33:56AM -0400, Daniel J Walsh wrote:
Main goal of this patch it to verify data being written to xml and inform the user when he makes a mistake.
Only able to verify static/dynamic in domain_conf.
Added verify code to qemu_driver.c for model and potentially label.
diff --git a/src/Makefile.am b/src/Makefile.am index d5aac11..8d53740 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -170,9 +170,9 @@ STORAGE_HELPER_DISK_SOURCES = \ SECURITY_DRIVER_SOURCES = \ security.h security.c
-SECURITY_DRIVER_SELINUX_SOURCES = \ - security_selinux.h security_selinux.c - +if WITH_SECDRIVER_SELINUX +SECURITY_DRIVER_SOURCES += security_selinux.h security_selinux.c +endif
NODE_DEVICE_DRIVER_SOURCES = \ node_device.c node_device.h @@ -390,9 +390,6 @@ endif libvirt_driver_security_la_SOURCES = $(SECURITY_DRIVER_SOURCES) noinst_LTLIBRARIES += libvirt_driver_security.la libvirt_la_LIBADD += libvirt_driver_security.la -if WITH_SECDRIVER_SELINUX -libvirt_driver_security_la_SOURCES += $(SECURITY_DRIVER_SELINUX_SOURCES) -endif
What's the purpose of this change ?
The reason for keeping an explicit variable name SECURITY_DRIVER_SELINUX_SOURCES was that those sounds need to be added to EXTRA_DIST, even when the compilation of the selinux bits is turned off - so that 'make dist' always gets all files.
diff --git a/src/domain_conf.c b/src/domain_conf.c index 2696449..c9259db 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -1859,29 +1859,44 @@ virSecurityLabelDefParseXML(virConnectPtr conn, if (virXPathNode(conn, "./seclabel", ctxt) == NULL) return 0;
+ p = virXPathStringLimit(conn, "string(./seclabel/@model)", + VIR_SECURITY_MODEL_BUFLEN-1, ctxt); + if (p == NULL) { + virDomainReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("missing security model")); + goto error; + } + def->seclabel.model = p; + p = virXPathStringLimit(conn, "string(./seclabel/@type)", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p == NULL) - goto error; - if ((def->seclabel.type = virDomainSeclabelTypeFromString(p))< 0) + if (p == NULL) { + virDomainReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("missing security type")); goto error; + } + def->seclabel.type = virDomainSeclabelTypeFromString(p); VIR_FREE(p); + if (def->seclabel.type< 0) { + virDomainReportError(conn, VIR_ERR_XML_ERROR, + _("invalid security type")); + goto error; + }
/* Only parse details, if using static labels, or * if the 'live' VM XML is requested */ if (def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC || !(flags& VIR_DOMAIN_XML_INACTIVE)) { - p = virXPathStringLimit(conn, "string(./seclabel/@model)", - VIR_SECURITY_MODEL_BUFLEN-1, ctxt); - if (p == NULL) - goto error; - def->seclabel.model = p;
p = virXPathStringLimit(conn, "string(./seclabel/label[1])", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p == NULL) + if (p == NULL) { + virDomainReportError(conn, VIR_ERR_XML_ERROR, + _("security label is missing")); goto error; + } + def->seclabel.label = p; }
ACK, this looks fine
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a5f9f92..5c88009 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -248,6 +248,7 @@ free_qparam_set;
# security.h +virSecurityDriverVerify; virSecurityDriverStartup; virSecurityDriverInit; virSecurityDriverSetDOI; diff --git a/src/qemu_driver.c b/src/qemu_driver.c index b2974bb..cd48193 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -2115,6 +2115,9 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, VIR_DOMAIN_XML_INACTIVE))) goto cleanup;
+ if (virSecurityDriverVerify(conn, def)< 0) + goto cleanup; + vm = virDomainFindByName(&driver->domains, def->name); if (vm) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, @@ -3398,6 +3401,9 @@ static virDomainPtr qemudDomainDefine(virConnectPtr conn, const char *xml) { VIR_DOMAIN_XML_INACTIVE))) goto cleanup;
+ if (virSecurityDriverVerify(conn, def)< 0) + goto cleanup; + vm = virDomainFindByName(&driver->domains, def->name); if (vm) { virDomainObjUnlock(vm); diff --git a/src/security.c b/src/security.c index e2bd20a..72aadf4 100644 --- a/src/security.c +++ b/src/security.c @@ -28,6 +28,25 @@ static virSecurityDriverPtr security_drivers[] = { };
int +virSecurityDriverVerify(virConnectPtr conn, virDomainDefPtr def) +{ + unsigned int i; + const virSecurityLabelDefPtr secdef =&def->seclabel; + + if (STREQ(secdef->model, "none")) + return 0; + + for (i = 0; security_drivers[i] != NULL ; i++) { + if (STREQ(security_drivers[i]->name, secdef->model)) { + return security_drivers[i]->domainSecurityVerify(conn, def); + } + }
A few whitespace problems here - HACKING file has some helpful emacs rules to avoid this
+ virSecurityReportError(conn, VIR_ERR_XML_ERROR, + _("invalid security model")); + return -1; +} + +int virSecurityDriverStartup(virSecurityDriverPtr *drv, const char *name) { diff --git a/src/security.h b/src/security.h index 8cc2c17..396f47f 100644 --- a/src/security.h +++ b/src/security.h @@ -46,11 +46,14 @@ typedef int (*virSecurityDomainRestoreLabel) (virConnectPtr conn, typedef int (*virSecurityDomainSetLabel) (virConnectPtr conn, virSecurityDriverPtr drv, virDomainObjPtr vm); +typedef int (*virSecurityDomainSecurityVerify) (virConnectPtr conn, + virDomainDefPtr def);
struct _virSecurityDriver { const char *name; virSecurityDriverProbe probe; virSecurityDriverOpen open; + virSecurityDomainSecurityVerify domainSecurityVerify; virSecurityDomainRestoreImageLabel domainRestoreSecurityImageLabel; virSecurityDomainSetImageLabel domainSetSecurityImageLabel; virSecurityDomainGenLabel domainGenSecurityLabel; @@ -71,6 +74,9 @@ struct _virSecurityDriver { int virSecurityDriverStartup(virSecurityDriverPtr *drv, const char *name);
+int +virSecurityDriverVerify(virConnectPtr conn, virDomainDefPtr def); + void virSecurityReportError(virConnectPtr conn, int code, const char *fmt, ...) ATTRIBUTE_FORMAT(printf, 3, 4); diff --git a/src/security_selinux.c b/src/security_selinux.c index 1708d55..7b8e107 100644 --- a/src/security_selinux.c +++ b/src/security_selinux.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008 Red Hat, Inc. + * Copyright (C) 2008,2009 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 @@ -8,6 +8,7 @@ * * Authors: * James Morris<jmorris@namei.org> + * Dan Walsh<dwalsh@redhat.com> * * SELinux security driver. */ @@ -361,6 +364,20 @@ SELinuxRestoreSecurityLabel(virConnectPtr conn, }
static int +SELinuxSecurityVerify(virConnectPtr conn, virDomainDefPtr def) +{ + const virSecurityLabelDefPtr secdef =&def->seclabel; + if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC) { + if (security_check_context(secdef->label) != 0) { + virSecurityReportError(conn, VIR_ERR_XML_ERROR, + _("Invalid security label %s"), secdef->label); + return -1; + } + } + return 0; +}
Some whitespace problem there too
+ +static int SELinuxSetSecurityLabel(virConnectPtr conn, virSecurityDriverPtr drv, virDomainObjPtr vm) @@ -406,6 +423,7 @@ virSecurityDriver virSELinuxSecurityDriver = { .name = SECURITY_SELINUX_NAME, .probe = SELinuxSecurityDriverProbe, .open = SELinuxSecurityDriverOpen, + .domainSecurityVerify = SELinuxSecurityVerify, .domainSetSecurityImageLabel = SELinuxSetSecurityImageLabel, .domainRestoreSecurityImageLabel = SELinuxRestoreSecurityImageLabel, .domainGenSecurityLabel = SELinuxGenSecurityLabel, diff --git a/tests/.gitignore b/tests/.gitignore index 9d809c9..4f33d0b 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -15,6 +15,7 @@ nodedevxml2xmltest nodeinfotest statstest qparamtest +seclabeltest *.gcda *.gcno *.exe diff --git a/tests/Makefile.am b/tests/Makefile.am index 28b2737..48db913 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -54,7 +54,7 @@ EXTRA_DIST = \ nodedevschemadata
noinst_PROGRAMS = virshtest conftest \ - nodeinfotest statstest qparamtest + nodeinfotest statstest qparamtest seclabeltest
if WITH_XEN noinst_PROGRAMS += xml2sexprtest sexpr2xmltest \ @@ -97,6 +97,7 @@ EXTRA_DIST += $(test_scripts) TESTS = virshtest \ nodeinfotest \ statstest \ + seclabeltest \ qparamtest \ $(test_scripts)
@@ -203,6 +204,10 @@ statstest_SOURCES = \ statstest.c testutils.h testutils.c statstest_LDADD = $(LDADDS)
+seclabeltest_SOURCES = \ + seclabeltest.c +seclabeltest_LDADD = ../src/libvirt_driver_security.la $(LDADDS) + qparamtest_SOURCES = \ qparamtest.c testutils.h testutils.c qparamtest_LDADD = $(LDADDS) diff --git a/tests/seclabeltest.c b/tests/seclabeltest.c new file mode 100644 index 0000000..f068a00 --- /dev/null +++ b/tests/seclabeltest.c @@ -0,0 +1,60 @@ +#include<config.h> + +#include<unistd.h> +#include<stdlib.h> +#include<stdio.h> +#include<string.h> +#include<errno.h> +#include "security.h" + +int +main (int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED) +{ + int ret; + + const char *doi, *model; + virSecurityDriverPtr security_drv; + + ret = virSecurityDriverStartup (&security_drv, "selinux"); + if (ret == -1) + { + fprintf (stderr, "Failed to start security driver"); + exit (-1); + } + /* No security driver wanted to be enabled: just return */ + if (ret == -2) + return 0; + + model = virSecurityDriverGetModel (security_drv); + if (!model) + { + fprintf (stderr, "Failed to copy secModel model: %s", + strerror (errno)); + exit (-1); + } + + doi = virSecurityDriverGetDOI (security_drv); + if (!doi) + { + fprintf (stderr, "Failed to copy secModel DOI: %s", + strerror (errno)); + exit (-1); + } + + virConnectPtr conn; + conn = virConnectOpen (NULL); + if (conn == NULL) + { + fprintf (stderr, "First virConnectOpen() failed\n"); + exit (1); + } + virSecurityDriverSetDOI (conn, security_drv, "1"); + doi = virSecurityDriverGetDOI (security_drv); + if (strcmp (doi, "1") != 0)
Can you switch that one to STREQ
Basically looks good aside from the whitespace bug/makefile change
Regards, Daniel Try again...
Sorry forgot to do the make syntax-check, Fixed Whitespace. Removed Makefile patch. Use STREQ in seclabeltest.