security: Don't remember owner for shared resources

This effectively reverts d7420430ce6 and adds new code.

Here is the problem: Imagine a file X that is to be shared
between two domains as a disk. Let the first domain (vm1) have
seclabel remembering turned on and the other (vm2) has it turned
off. Assume that both domains will run under the same user, but
the original owner of X is different (i.e. trying to access X
without relabelling leads to EPERM).

Let's start vm1 first. This will cause X to be relabelled and to
gain new attributes:

  trusted.libvirt.security.ref_dac="1"
  trusted.libvirt.security.dac="$originalOwner"

When vm2 is started, X will again be relabelled, but since the
new label is the same as X already has (because of vm1) nothing
changes and vm1 and vm2 can access X just fine. Note that no
XATTR is changed (especially the refcounter keeps its value of 1)
because the vm2 domain has the feature turned off.

Now, vm1 is shut off and vm2 continues running. In seclabel
restore process we would get to X and since its refcounter is 1
we would restore the $originalOwner on it. But this is unsafe to
do because vm2 is still using X (remember the assumption that
$originalOwner and vm2's seclabel are distinct?).

The problem is that refcounter stored in XATTRs doesn't reflect
the actual times a resource is in use. Since I don't see any easy
way around it let's just not store original owner on shared
resources. Shared resource in world of domain disks is:

  - whole backing chain but the top layer,
  - read only disk (we don't require CDROM to be explicitly
    marked as shareable),
  - disk marked as shareable.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index a39dae52269e6bb15ee93c86d6e999fdc865f6f4..56416e6f6a6366c2f9eaf2ee7ee0d8a74e064e11 100644 (file)
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -878,6 +878,7 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr,
     virSecurityDeviceLabelDefPtr disk_seclabel;
     virSecurityDeviceLabelDefPtr parent_seclabel = NULL;
     virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+    bool remember;
     uid_t user;
     gid_t group;
 
@@ -911,7 +912,21 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr,
             return -1;
     }
 
-    return virSecurityDACSetOwnership(mgr, src, NULL, user, group, true);
+    /* We can't do restore on shared resources safely. Not even
+     * with refcounting implemented in XATTRs because if there
+     * was a domain running with the feature turned off the
+     * refcounter in XATTRs would not reflect the actual number
+     * of times the resource is in use and thus the last restore
+     * on the resource (which actually restores the original
+     * owner) might cut off access to the domain with the feature
+     * disabled.
+     * For disks, a shared resource is the whole backing chain
+     * but the top layer, or read only image, or disk explicitly
+     * marked as shared.
+     */
+    remember = src == parent && !src->readonly && !src->shared;
+
+    return virSecurityDACSetOwnership(mgr, src, NULL, user, group, remember);
 }
 
 
@@ -948,6 +963,14 @@ virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr,
     if (!priv->dynamicOwnership)
         return 0;
 
+    /* Don't restore labels on readoly/shared disks, because other VMs may
+     * still be accessing these. Alternatively we could iterate over all
+     * running domains and try to figure out if it is in use, but this would
+     * not work for clustered filesystems, since we can't see running VMs using
+     * the file on other nodes. Safest bet is thus to skip the restore step. */
+    if (src->readonly || src->shared)
+        return 0;
+
     secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
     if (secdef && !secdef->relabel)
         return 0;

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 3ac3b83e45ba7999d9e23aaa90dd99fc8defffa7..cb460048965184aa50c06834415239996f7773c3 100644 (file)
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1819,6 +1819,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr,
     virSecurityLabelDefPtr secdef;
     virSecurityDeviceLabelDefPtr disk_seclabel;
     virSecurityDeviceLabelDefPtr parent_seclabel = NULL;
+    bool remember;
     int ret;
 
     if (!src->path || !virStorageSourceIsLocalStorage(src))
@@ -1828,6 +1829,20 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr,
     if (!secdef || !secdef->relabel)
         return 0;
 
+    /* We can't do restore on shared resources safely. Not even
+     * with refcounting implemented in XATTRs because if there
+     * was a domain running with the feature turned off the
+     * refcounter in XATTRs would not reflect the actual number
+     * of times the resource is in use and thus the last restore
+     * on the resource (which actually restores the original
+     * owner) might cut off access to the domain with the feature
+     * disabled.
+     * For disks, a shared resource is the whole backing chain
+     * but the top layer, or read only image, or disk explicitly
+     * marked as shared.
+     */
+    remember = src == parent && !src->readonly && !src->shared;
+
     disk_seclabel = virStorageSourceGetSecurityLabelDef(src,
                                                         SECURITY_SELINUX_NAME);
     if (parent)
@@ -1839,29 +1854,29 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr,
             return 0;
 
         ret = virSecuritySELinuxSetFilecon(mgr, src->path,
-                                           disk_seclabel->label, true);
+                                           disk_seclabel->label, remember);
     } else if (parent_seclabel && (!parent_seclabel->relabel || parent_seclabel->label)) {
         if (!parent_seclabel->relabel)
             return 0;
 
         ret = virSecuritySELinuxSetFilecon(mgr, src->path,
-                                           parent_seclabel->label, true);
+                                           parent_seclabel->label, remember);
     } else if (!parent || parent == src) {
         if (src->shared) {
             ret = virSecuritySELinuxSetFileconOptional(mgr,
                                                        src->path,
                                                        data->file_context,
-                                                       true);
+                                                       remember);
         } else if (src->readonly) {
             ret = virSecuritySELinuxSetFileconOptional(mgr,
                                                        src->path,
                                                        data->content_context,
-                                                       true);
+                                                       remember);
         } else if (secdef->imagelabel) {
             ret = virSecuritySELinuxSetFileconOptional(mgr,
                                                        src->path,
                                                        secdef->imagelabel,
-                                                       true);
+                                                       remember);
         } else {
             ret = 0;
         }
@@ -1869,7 +1884,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr,
         ret = virSecuritySELinuxSetFileconOptional(mgr,
                                                    src->path,
                                                    data->content_context,
-                                                   true);
+                                                   remember);
     }
 
     if (ret == 1 && !disk_seclabel) {

diff --git a/tests/qemusecuritytest.c b/tests/qemusecuritytest.c
index 65e08b45033b6227acb1e5dd47b3b766ab1db5df..2d88979168d158ca1f415c2acb81ba0e106e67a5 100644 (file)
--- a/tests/qemusecuritytest.c
+++ b/tests/qemusecuritytest.c
@@ -85,11 +85,32 @@ testDomain(const void *opaque)
 {
     const struct testData *data = opaque;
     VIR_AUTOUNREF(virDomainObjPtr) vm = NULL;
+    VIR_AUTOSTRINGLIST notRestored = NULL;
+    size_t i;
     int ret = -1;
 
     if (prepareObjects(data->driver, data->file, &vm) < 0)
         return -1;
 
+    for (i = 0; i < vm->def->ndisks; i++) {
+        virStorageSourcePtr src = vm->def->disks[i]->src;
+        virStorageSourcePtr n;
+
+        if (!src)
+            continue;
+
+        if (virStorageSourceIsLocalStorage(src) && src->path &&
+            (src->shared || src->readonly) &&
+            virStringListAdd(&notRestored, src->path) < 0)
+            return -1;
+
+        for (n = src->backingStore; virStorageSourceIsBacking(n); n = n->backingStore) {
+            if (virStorageSourceIsLocalStorage(n) && n->path &&
+                virStringListAdd(&notRestored, n->path) < 0)
+                return -1;
+        }
+    }
+
     /* Mocking is enabled only when this env variable is set.
      * See mock code for explanation. */
     if (setenv(ENVVAR, "1", 0) < 0)
@@ -100,7 +121,7 @@ testDomain(const void *opaque)
 
     qemuSecurityRestoreAllLabel(data->driver, vm, false);
 
-    if (checkPaths(NULL) < 0)
+    if (checkPaths((const char **) notRestored) < 0)
         goto cleanup;
 
     ret = 0;
@@ -144,6 +165,7 @@ mymain(void)
     DO_TEST_DOMAIN("console-virtio-unix");
     DO_TEST_DOMAIN("controller-virtio-scsi");
     DO_TEST_DOMAIN("disk-aio");
+    DO_TEST_DOMAIN("disk-backing-chains-noindex");
     DO_TEST_DOMAIN("disk-cache");
     DO_TEST_DOMAIN("disk-cdrom");
     DO_TEST_DOMAIN("disk-cdrom-bus-other");