Improve security label error reporting & verification (Dan Walsh)

diff --git a/ChangeLog b/ChangeLog
index 4bcaf13eb109ab3b445e0c23cc32de2ffeb68e1e..3bf68fc4c93f443404f8d2fe548c850d9940a6ff 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,18 @@
+Thu Apr  3 11:55:00 BST 2009 Daniel P. Berrange <berrange@redhat.com>
+
+       Improve error reporting/ verification of security labels
+       (Dan Walsh)
+       * src/domain_conf.c: Improve error reporting for parsing of
+       seclabel XML
+       * src/libvirt_private.syms: Export virSecurityDriverVerify
+       * src/qemu_driver.c: Verify seclabel when creating or
+       defining a new domain
+       * src/security.c, src/security.h, src/security_linux.c: Add
+       functions for verifying security labels
+       * tests/.gitignore: Ignore seclabeltest
+       * tests/Makefile.am, tests/seclabeltest.c: Add test for
+       security driver
+
 Thu Apr  2 19:41:00 BST 2009 Daniel P. Berrange <berrange@redhat.com>
 
        Mingw portability fixes

diff --git a/src/Makefile.am b/src/Makefile.am
index 60b2d466fc96cb8dafea1627d754180da3d94862..f176b46d6ae1c401ec2b970bcd5f32ccfedeccdc 100644 (file)
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -419,7 +419,8 @@ EXTRA_DIST +=                                                       \
                $(STORAGE_DRIVER_DISK_SOURCES)                  \
                $(NODE_DEVICE_DRIVER_SOURCES)                   \
                $(NODE_DEVICE_DRIVER_HAL_SOURCES)               \
-               $(NODE_DEVICE_DRIVER_DEVKIT_SOURCES)
+               $(NODE_DEVICE_DRIVER_DEVKIT_SOURCES)            \
+               $(SECURITY_DRIVER_SELINUX_SOURCES)
 
 #
 # Build our version script.  This is composed of three parts:

diff --git a/src/domain_conf.c b/src/domain_conf.c
index 3d734148d4fecba573db975213910b802f5b99d8..0efa50feeceb4909686291e9032a0b5f39d87bd7 100644 (file)
--- 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;
     }
 

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 78f093c1965465635840c9dde02ab857fc5de5d9..350a931eb3329ab5552306371998b28b1db6d8c8 100644 (file)
--- 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 fb857ee839dfd78023fe27e7fda7a6f920203de8..8e0231b97697763b9f769eb18a782fa17c19809a 100644 (file)
--- 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 e2bd20aeda1348b6e1ec05dba801b35410c8d693..573895e8d43c74afd3fb71dae8d0daf53c53be3e 100644 (file)
--- a/src/security.c
+++ b/src/security.c
@@ -27,6 +27,25 @@ static virSecurityDriverPtr security_drivers[] = {
     NULL
 };
 
+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);
+        }
+    }
+    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 8cc2c17fe9b964159485667581a6320d891934de..05bf88c5df6775c6c611890cf84d3c118a65ea42 100644 (file)
--- 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 091fe6ee5a583b2c5f164f6f8a916ed54294b955..73d6aeaba07f547906b9f69a07d4e9fafe5547c4 100644 (file)
--- 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.
  */
@@ -356,6 +357,20 @@ SELinuxRestoreSecurityLabel(virConnectPtr conn,
     return rc;
 }
 
+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;
+}
+
 static int
 SELinuxSetSecurityLabel(virConnectPtr conn,
                         virSecurityDriverPtr drv,
@@ -402,6 +417,7 @@ virSecurityDriver virSELinuxSecurityDriver = {
     .name                       = SECURITY_SELINUX_NAME,
     .probe                      = SELinuxSecurityDriverProbe,
     .open                       = SELinuxSecurityDriverOpen,
+    .domainSecurityVerify       = SELinuxSecurityVerify,
     .domainSetSecurityImageLabel = SELinuxSetSecurityImageLabel,
     .domainRestoreSecurityImageLabel = SELinuxRestoreSecurityImageLabel,
     .domainGenSecurityLabel     = SELinuxGenSecurityLabel,

diff --git a/tests/.cvsignore b/tests/.cvsignore
index 9d809c958b2213158a48d6e3b4ddb024d0358aa3..4f33d0bd81afc7a8e1b56268adb9194dafbf1f86 100644 (file)
--- a/tests/.cvsignore
+++ b/tests/.cvsignore
@@ -15,6 +15,7 @@ nodedevxml2xmltest
 nodeinfotest
 statstest
 qparamtest
+seclabeltest
 *.gcda
 *.gcno
 *.exe

diff --git a/tests/.gitignore b/tests/.gitignore
index 9d809c958b2213158a48d6e3b4ddb024d0358aa3..4f33d0bd81afc7a8e1b56268adb9194dafbf1f86 100644 (file)
--- 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 28b273702ceebd72d7b500029357abc5c42808c0..52d5c398e442adb20068028092678549c8c43a3e 100644 (file)
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -64,6 +64,10 @@ if WITH_QEMU
 noinst_PROGRAMS += qemuxml2argvtest qemuxml2xmltest
 endif
 
+if WITH_SECDRIVER_SELINUX
+noinst_PROGRAMS += seclabeltest
+endif
+
 noinst_PROGRAMS += nodedevxml2xmltest
 
 test_scripts = \
@@ -114,6 +118,10 @@ if WITH_QEMU
 TESTS += qemuxml2argvtest qemuxml2xmltest
 endif
 
+if WITH_SECDRIVER_SELINUX
+TESTS += seclabeltest
+endif
+
 TESTS += nodedevxml2xmltest
 
 path_add = $$abs_top_builddir/src$(PATH_SEPARATOR)$$abs_top_builddir/qemud
@@ -203,6 +211,14 @@ statstest_SOURCES = \
        statstest.c testutils.h testutils.c
 statstest_LDADD = $(LDADDS)
 
+if WITH_SECDRIVER_SELINUX
+seclabeltest_SOURCES = \
+       seclabeltest.c
+seclabeltest_LDADD = ../src/libvirt_driver_security.la $(LDADDS)
+else
+EXTRA_DIST += seclabeltest.c
+endif
+
 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 (file)
index 0000000..fe83db8
--- /dev/null
+++ b/tests/seclabeltest.c
@@ -0,0 +1,45 @@
+#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);
+    }
+
+    return 0;
+}