Re: [libvirt] [PATCH v4 18/25] 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(a)redhat.com&gt; --- src/security/security_dac.c | 25 ++++++++++++++++++++++++- src/security/security_selinux.c | 27 +++++++++++++++++++++------ tests/qemusecuritytest.c | 24 +++++++++++++++++++++++- 3 files changed, 68 insertions(+), 8 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index a39dae5226..56416e6f6a 100644 --- 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 3ac3b83e45..cb46004896 100644 --- 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 65e08b4503..2d88979168 100644 --- 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"); -- 2.21.0 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list