Refactor setup & cleanup of security labels in security driver

The current security driver architecture has the following
split of logic

 * domainGenSecurityLabel

    Allocate the unique label for the domain about to be started

 * domainGetSecurityLabel

    Retrieve the current live security label for a process

 * domainSetSecurityLabel

    Apply the previously allocated label to the current process
    Setup all disk image / device labelling

 * domainRestoreSecurityLabel

    Restore the original disk image / device labelling.
    Release the unique label for the domain

The 'domainSetSecurityLabel' method is special because it runs
in the context of the child process between the fork + exec.

This is require in order to set the process label. It is not
required in order to label disks/devices though. Having the
disk labelling code run in the child process limits what it
can do.

In particularly libvirtd would like to remember the current
disk image label, and only change shared image labels for the
first VM to start. This requires use & update of global state
in the libvirtd daemon, and thus cannot run in the child
process context.

The solution is to split domainSetSecurityLabel into two parts,
one applies process label, and the other handles disk image
labelling. At the same time domainRestoreSecurityLabel is
similarly split, just so that it matches the style. Thus the
previous 4 methods are replaced by the following 6 new methods

 * domainGenSecurityLabel

    Allocate the unique label for the domain about to be started
    No actual change here.

 * domainReleaseSecurityLabel

   Release the unique label for the domain

 * domainGetSecurityProcessLabel

   Retrieve the current live security label for a process
   Merely renamed for clarity.

 * domainSetSecurityProcessLabel

   Apply the previously allocated label to the current process

 * domainRestoreSecurityAllLabel

    Restore the original disk image / device labelling.

 * domainSetSecurityAllLabel

    Setup all disk image / device labelling

The SELinux and AppArmour drivers are then updated to comply with
this new spec. Notice that the AppArmour driver was actually a
little different. It was creating its profile for the disk image
and device labels in the 'domainGenSecurityLabel' method, where as
the SELinux driver did it in 'domainSetSecurityLabel'. With the
new method split, we can have consistency, with both drivers doing
that in the domainSetSecurityAllLabel method.

NB, the AppArmour changes here haven't been compiled so may not
build.

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 15bda52fffdb6bcc003ec5baae538c0533be3e8b..ad2737a5a23b4769cc8b09b67e4c2da3e348afe3 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2450,8 +2450,14 @@ static int qemudDomainSetSecurityLabel(virConnectPtr conn, struct qemud_driver *
     int rc = 0;
 
     if (driver->securityDriver &&
-        driver->securityDriver->domainSetSecurityLabel &&
-        driver->securityDriver->domainSetSecurityLabel(conn, driver->securityDriver, vm) < 0)
+        driver->securityDriver->domainSetSecurityAllLabel &&
+        driver->securityDriver->domainSetSecurityAllLabel(conn, vm) < 0)
+        rc = -1;
+
+    if (rc == 0 &&
+        driver->securityDriver &&
+        driver->securityDriver->domainSetSecurityProcessLabel &&
+        driver->securityDriver->domainSetSecurityProcessLabel(conn, driver->securityDriver, vm) < 0)
         rc = -1;
 
     return rc;
@@ -3063,8 +3069,11 @@ static void qemudShutdownVMDaemon(virConnectPtr conn,
 
     /* Reset Security Labels */
     if (driver->securityDriver &&
-        driver->securityDriver->domainRestoreSecurityLabel)
-        driver->securityDriver->domainRestoreSecurityLabel(conn, vm);
+        driver->securityDriver->domainRestoreSecurityAllLabel)
+        driver->securityDriver->domainRestoreSecurityAllLabel(conn, vm);
+    if (driver->securityDriver &&
+        driver->securityDriver->domainReleaseSecurityLabel)
+        driver->securityDriver->domainReleaseSecurityLabel(conn, vm);
 
     /* Clear out dynamically assigned labels */
     if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
@@ -4632,8 +4641,8 @@ static int qemudDomainGetSecurityLabel(virDomainPtr dom, virSecurityLabelPtr sec
      *   QEMU monitor hasn't seen SIGHUP/ERR on poll().
      */
     if (virDomainObjIsActive(vm)) {
-        if (driver->securityDriver && driver->securityDriver->domainGetSecurityLabel) {
-            if (driver->securityDriver->domainGetSecurityLabel(dom->conn, vm, seclabel) == -1) {
+        if (driver->securityDriver && driver->securityDriver->domainGetSecurityProcessLabel) {
+            if (driver->securityDriver->domainGetSecurityProcessLabel(dom->conn, vm, seclabel) == -1) {
                 qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
                                  "%s", _("Failed to get security label"));
                 goto cleanup;

diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 0f9ff95e6aa93b82a98693ab4fd3c79a81f5823d..f2886455a9ba1b21cd8f1384ba72b1cfd13e37b5 100644 (file)
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -341,16 +341,6 @@ AppArmorGenSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
     if ((profile_name = get_profile_name(conn, vm)) == NULL)
         return rc;
 
-    /* if the profile is not already loaded, then load one */
-    if (profile_loaded(profile_name) < 0) {
-        if (load_profile(conn, profile_name, vm, NULL) < 0) {
-            virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
-                                   _("cannot generate AppArmor profile "
-                                   "\'%s\'"), profile_name);
-            goto clean;
-        }
-    }
-
     vm->def->seclabel.label = strndup(profile_name, strlen(profile_name));
     if (!vm->def->seclabel.label) {
         virReportOOMError(NULL);
@@ -375,7 +365,6 @@ AppArmorGenSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
     goto clean;
 
   err:
-    remove_profile(profile_name);
     VIR_FREE(vm->def->seclabel.label);
     VIR_FREE(vm->def->seclabel.imagelabel);
     VIR_FREE(vm->def->seclabel.model);
@@ -386,12 +375,33 @@ AppArmorGenSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
     return rc;
 }
 
+static int
+AppArmorSetSecurityAllLabel(virConnectPtr conn, virDomainObjPtr vm)
+{
+    int rc = -1;
+
+    if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC)
+        return 0;
+
+    /* if the profile is not already loaded, then load one */
+    if (profile_loaded(vm->def->seclabel.label) < 0) {
+        if (load_profile(conn, vm->def->seclabel.label, vm, NULL) < 0) {
+            virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                                   _("cannot generate AppArmor profile "
+                                   "\'%s\'"), vm->def->seclabel.label);
+            return -1;
+        }
+    }
+
+    return 0;
+}
+
 /* Seen with 'virsh dominfo <vm>'. This function only called if the VM is
  * running.
  */
 static int
-AppArmorGetSecurityLabel(virConnectPtr conn,
-                         virDomainObjPtr vm, virSecurityLabelPtr sec)
+AppArmorGetSecurityProcessLabel(virConnectPtr conn,
+                                virDomainObjPtr vm, virSecurityLabelPtr sec)
 {
     int rc = -1;
     char *profile_name = NULL;
@@ -423,7 +433,20 @@ AppArmorGetSecurityLabel(virConnectPtr conn,
  * more details. Currently called via qemudShutdownVMDaemon.
  */
 static int
-AppArmorRestoreSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
+AppArmorReleaseSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
+{
+    const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+
+    VIR_FREE(secdef->model);
+    VIR_FREE(secdef->label);
+    VIR_FREE(secdef->imagelabel);
+
+    return 0;
+}
+
+
+static int
+AppArmorRestoreSecurityAllLabel(virConnectPtr conn, virDomainObjPtr vm)
 {
     const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
     int rc = 0;
@@ -434,9 +457,6 @@ AppArmorRestoreSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
                                    _("could not remove profile for \'%s\'"),
                                    secdef->label);
         }
-        VIR_FREE(secdef->model);
-        VIR_FREE(secdef->label);
-        VIR_FREE(secdef->imagelabel);
     }
     return rc;
 }
@@ -445,8 +465,8 @@ AppArmorRestoreSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
  * LOCAL_STATE_DIR/log/libvirt/qemu/<vm name>.log
  */
 static int
-AppArmorSetSecurityLabel(virConnectPtr conn,
-                         virSecurityDriverPtr drv, virDomainObjPtr vm)
+AppArmorSetSecurityProcessLabel(virConnectPtr conn,
+                                virSecurityDriverPtr drv, virDomainObjPtr vm)
 {
     const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
     int rc = -1;
@@ -620,9 +640,11 @@ virSecurityDriver virAppArmorSecurityDriver = {
     .domainRestoreSecurityImageLabel = AppArmorRestoreSecurityImageLabel,
     .domainGenSecurityLabel = AppArmorGenSecurityLabel,
     .domainReserveSecurityLabel = AppArmorReserveSecurityLabel,
-    .domainGetSecurityLabel = AppArmorGetSecurityLabel,
-    .domainRestoreSecurityLabel = AppArmorRestoreSecurityLabel,
-    .domainSetSecurityLabel = AppArmorSetSecurityLabel,
+    .domainReleaseSecurityLabel = AppArmorReleaseSecurityLabel,
+    .domainGetSecurityProcessLabel = AppArmorGetSecurityProcessLabel,
+    .domainSetSecurityProcessLabel = AppArmorSetSecurityProcessLabel,
+    .domainRestoreSecurityAllLabel = AppArmorRestoreSecurityAllLabel,
+    .domainSetSecurityAllLabel = AppArmorSetSecurityAllLabel,
     .domainSetSecurityHostdevLabel = AppArmorSetSecurityHostdevLabel,
     .domainRestoreSecurityHostdevLabel = AppArmorRestoreSecurityHostdevLabel,
 };

diff --git a/src/security/security_driver.h b/src/security/security_driver.h
index 97c900202ca2eace7871f14ae40d51bbb7b2db55..5d2446d246ea02d2ccdf3056bfc0474de98c6432 100644 (file)
--- a/src/security/security_driver.h
+++ b/src/security/security_driver.h
@@ -52,15 +52,19 @@ typedef int (*virSecurityDomainRestoreSavedStateLabel) (virConnectPtr conn,
 typedef int (*virSecurityDomainGenLabel) (virConnectPtr conn,
                                           virDomainObjPtr sec);
 typedef int (*virSecurityDomainReserveLabel) (virConnectPtr conn,
-                                           virDomainObjPtr sec);
-typedef int (*virSecurityDomainGetLabel) (virConnectPtr conn,
-                                          virDomainObjPtr vm,
-                                          virSecurityLabelPtr sec);
-typedef int (*virSecurityDomainRestoreLabel) (virConnectPtr conn,
-                                              virDomainObjPtr vm);
-typedef int (*virSecurityDomainSetLabel) (virConnectPtr conn,
-                                          virSecurityDriverPtr drv,
-                                          virDomainObjPtr vm);
+                                              virDomainObjPtr sec);
+typedef int (*virSecurityDomainReleaseLabel) (virConnectPtr conn,
+                                              virDomainObjPtr sec);
+typedef int (*virSecurityDomainSetAllLabel) (virConnectPtr conn,
+                                             virDomainObjPtr sec);
+typedef int (*virSecurityDomainRestoreAllLabel) (virConnectPtr conn,
+                                                 virDomainObjPtr vm);
+typedef int (*virSecurityDomainGetProcessLabel) (virConnectPtr conn,
+                                                 virDomainObjPtr vm,
+                                                 virSecurityLabelPtr sec);
+typedef int (*virSecurityDomainSetProcessLabel) (virConnectPtr conn,
+                                                 virSecurityDriverPtr drv,
+                                                 virDomainObjPtr vm);
 typedef int (*virSecurityDomainSecurityVerify) (virConnectPtr conn,
                                                 virDomainDefPtr def);
 
@@ -73,9 +77,11 @@ struct _virSecurityDriver {
     virSecurityDomainSetImageLabel domainSetSecurityImageLabel;
     virSecurityDomainGenLabel domainGenSecurityLabel;
     virSecurityDomainReserveLabel domainReserveSecurityLabel;
-    virSecurityDomainGetLabel domainGetSecurityLabel;
-    virSecurityDomainSetLabel domainSetSecurityLabel;
-    virSecurityDomainRestoreLabel domainRestoreSecurityLabel;
+    virSecurityDomainReleaseLabel domainReleaseSecurityLabel;
+    virSecurityDomainGetProcessLabel domainGetSecurityProcessLabel;
+    virSecurityDomainSetProcessLabel domainSetSecurityProcessLabel;
+    virSecurityDomainSetAllLabel domainSetSecurityAllLabel;
+    virSecurityDomainRestoreAllLabel domainRestoreSecurityAllLabel;
     virSecurityDomainRestoreHostdevLabel domainRestoreSecurityHostdevLabel;
     virSecurityDomainSetHostdevLabel domainSetSecurityHostdevLabel;
     virSecurityDomainSetSavedStateLabel domainSetSavedStateLabel;

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index c48f4c8af53872b5afb116ce34e737d41e1cbc89..9f1a644695b91a0962e5a890a69bbd747ddae028 100644 (file)
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -277,9 +277,9 @@ SELinuxSecurityDriverOpen(virConnectPtr conn, virSecurityDriverPtr drv)
 }
 
 static int
-SELinuxGetSecurityLabel(virConnectPtr conn,
-                        virDomainObjPtr vm,
-                        virSecurityLabelPtr sec)
+SELinuxGetSecurityProcessLabel(virConnectPtr conn,
+                               virDomainObjPtr vm,
+                               virSecurityLabelPtr sec)
 {
     security_context_t ctx;
 
@@ -615,8 +615,8 @@ done:
 }
 
 static int
-SELinuxRestoreSecurityLabel(virConnectPtr conn,
-                            virDomainObjPtr vm)
+SELinuxRestoreSecurityAllLabel(virConnectPtr conn,
+                               virDomainObjPtr vm)
 {
     const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
     int i;
@@ -636,6 +636,19 @@ SELinuxRestoreSecurityLabel(virConnectPtr conn,
                                              vm->def->disks[i]) < 0)
             rc = -1;
     }
+
+    return rc;
+}
+
+static int
+SELinuxReleaseSecurityLabel(virConnectPtr conn ATTRIBUTE_UNUSED,
+                            virDomainObjPtr vm)
+{
+    const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+
+    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+        return 0;
+
     context_t con = context_new(secdef->label);
     if (con) {
         mcsRemove(context_range_get(con));
@@ -646,7 +659,7 @@ SELinuxRestoreSecurityLabel(virConnectPtr conn,
     VIR_FREE(secdef->label);
     VIR_FREE(secdef->imagelabel);
 
-    return rc;
+    return 0;
 }
 
 
@@ -693,13 +706,12 @@ SELinuxSecurityVerify(virConnectPtr conn, virDomainDefPtr def)
 }
 
 static int
-SELinuxSetSecurityLabel(virConnectPtr conn,
-                        virSecurityDriverPtr drv,
-                        virDomainObjPtr vm)
+SELinuxSetSecurityProcessLabel(virConnectPtr conn,
+                               virSecurityDriverPtr drv,
+                               virDomainObjPtr vm)
 {
     /* TODO: verify DOI */
     const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
-    int i;
 
     if (vm->def->seclabel.label == NULL)
         return 0;
@@ -722,18 +734,32 @@ SELinuxSetSecurityLabel(virConnectPtr conn,
             return -1;
     }
 
-    if (secdef->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
-        for (i = 0 ; i < vm->def->ndisks ; i++) {
-            /* XXX fixme - we need to recursively label the entriy tree :-( */
-            if (vm->def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_DIR)
-                continue;
-            if (SELinuxSetSecurityImageLabel(conn, vm, vm->def->disks[i]) < 0)
-                return -1;
-        }
-        for (i = 0 ; i < vm->def->nhostdevs ; i++) {
-            if (SELinuxSetSecurityHostdevLabel(conn, vm, vm->def->hostdevs[i]) < 0)
-                return -1;
+    return 0;
+}
+
+static int
+SELinuxSetSecurityAllLabel(virConnectPtr conn,
+                           virDomainObjPtr vm)
+{
+    const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+    int i;
+
+    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+        return 0;
+
+    for (i = 0 ; i < vm->def->ndisks ; i++) {
+        /* XXX fixme - we need to recursively label the entire tree :-( */
+        if (vm->def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_DIR) {
+            VIR_WARN("Unable to relabel directory tree %s for disk %s",
+                     vm->def->disks[i]->src, vm->def->disks[i]->dst);
+            continue;
         }
+        if (SELinuxSetSecurityImageLabel(conn, vm, vm->def->disks[i]) < 0)
+            return -1;
+    }
+    for (i = 0 ; i < vm->def->nhostdevs ; i++) {
+        if (SELinuxSetSecurityHostdevLabel(conn, vm, vm->def->hostdevs[i]) < 0)
+            return -1;
     }
 
     return 0;
@@ -748,9 +774,11 @@ virSecurityDriver virSELinuxSecurityDriver = {
     .domainRestoreSecurityImageLabel = SELinuxRestoreSecurityImageLabel,
     .domainGenSecurityLabel     = SELinuxGenSecurityLabel,
     .domainReserveSecurityLabel     = SELinuxReserveSecurityLabel,
-    .domainGetSecurityLabel     = SELinuxGetSecurityLabel,
-    .domainRestoreSecurityLabel = SELinuxRestoreSecurityLabel,
-    .domainSetSecurityLabel     = SELinuxSetSecurityLabel,
+    .domainReleaseSecurityLabel     = SELinuxReleaseSecurityLabel,
+    .domainGetSecurityProcessLabel     = SELinuxGetSecurityProcessLabel,
+    .domainSetSecurityProcessLabel     = SELinuxSetSecurityProcessLabel,
+    .domainRestoreSecurityAllLabel = SELinuxRestoreSecurityAllLabel,
+    .domainSetSecurityAllLabel     = SELinuxSetSecurityAllLabel,
     .domainSetSecurityHostdevLabel = SELinuxSetSecurityHostdevLabel,
     .domainRestoreSecurityHostdevLabel = SELinuxRestoreSecurityHostdevLabel,
     .domainSetSavedStateLabel = SELinuxSetSavedStateLabel,