On Thu, Apr 25, 2019 at 10:19:54AM +0200, Michal Privoznik wrote:
> 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).
How do we get into this situation ? Is this the case when we
have a guest which was running before libvirt was upgraded, and
then a new guest is launched ?
> 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@xxxxxxxxxx>
> ---
> 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(¬Restored, src->path) < 0)
> + return -1;
> +
> + for (n = src->backingStore; virStorageSourceIsBacking(n); n = n->backingStore) {
> + if (virStorageSourceIsLocalStorage(n) && n->path &&
> + virStringListAdd(¬Restored, 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
