From: Michal Privoznik <mprivozn@redhat.com> When starting a domain with a disk, or when hotplugging one, the seclabel on corresponding file is set (the original seclabel is recorded in XATTRs) and then on hotunplug or VM shutdown the original seclabel is restored. This works so well, that even if QEMU rejects hotplug seclabel of the file is the same before and after hotplug attempt. Except for read only and shared disks. For these restore is explicitly skipped. Now imagine a user, trying to hotplug a disk with the source as say /root and QEMU denying the hotplug request (this is in fact fairly easy to achieve - hotplug a cdrom with a directory as its source). We'd set seclabels on the directory (/root), tell QEMU to hotplug it (or change media in a cdrom, doesn't really matter) which fails. But there's no seclabel restore leaving the directory with incorrect seclabels. Check for this specific case in our DAC and SELinux secdrivers. AppArmor is not necessary because it doesn't actually touch the directory. NB, one might expect qemusecuritytest to fail after this commit, but in fact, our qemusecuritymock re-implements stat() and pretends everything is a file. This is fixed in next commits. Resolves: https://redhat.atlassian.net/browse/RHEL-262704 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 37 +++++++++++++++++++++++++++++++++ src/security/security_selinux.c | 37 +++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index e7cffa8e59..32e3d20b6e 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -947,6 +947,43 @@ virSecurityDACSetImageLabelInternal(virSecurityManager *mgr, if (src->seclabelSkipRemember) remember = false; + /* If we don't remember the label, do an extra check if the + * storage source is actually of expected type. Ideally, if + * hypervisor rejects incorrect type then we'd just restore + * the remembered label, BUT if we do not remember it in the + * first place, then there's no restore. */ + if (!remember && src->path) { + struct stat sb; + + switch (src->type) { + case VIR_STORAGE_TYPE_FILE: + case VIR_STORAGE_TYPE_BLOCK: + case VIR_STORAGE_TYPE_DIR: + case VIR_STORAGE_TYPE_CTL: + if (stat(src->path, &sb) < 0) { + virReportSystemError(errno, _("Failed to stat %1$s"), src->path); + return -1; + } + + if ((src->type == VIR_STORAGE_TYPE_DIR) != !!S_ISDIR(sb.st_mode)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid file type for disk source %1$s"), + src->path); + return -1; + } + break; + + case VIR_STORAGE_TYPE_NONE: + case VIR_STORAGE_TYPE_NETWORK: + case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NVME: + case VIR_STORAGE_TYPE_VHOST_USER: + case VIR_STORAGE_TYPE_VHOST_VDPA: + case VIR_STORAGE_TYPE_LAST: + break; + } + } + return virSecurityDACSetOwnership(mgr, src, NULL, user, group, remember); } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 423dcd0bd9..9dc6242cfa 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2025,6 +2025,43 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManager *mgr, if (src->seclabelSkipRemember) remember = false; + /* If we don't remember the label, do an extra check if the + * storage source is actually of expected type. Ideally, if + * hypervisor rejects incorrect type then we'd just restore + * the remembered label, BUT if we do not remember it in the + * first place, then there's no restore. */ + if (!remember) { + struct stat sb; + + switch (src->type) { + case VIR_STORAGE_TYPE_FILE: + case VIR_STORAGE_TYPE_BLOCK: + case VIR_STORAGE_TYPE_DIR: + case VIR_STORAGE_TYPE_CTL: + if (stat(path, &sb) < 0) { + virReportSystemError(errno, _("Failed to stat %1$s"), path); + return -1; + } + + if ((src->type == VIR_STORAGE_TYPE_DIR) != !!S_ISDIR(sb.st_mode)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid file type for disk source %1$s"), + path); + return -1; + } + break; + + case VIR_STORAGE_TYPE_NONE: + case VIR_STORAGE_TYPE_NETWORK: + case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NVME: + case VIR_STORAGE_TYPE_VHOST_USER: + case VIR_STORAGE_TYPE_VHOST_VDPA: + case VIR_STORAGE_TYPE_LAST: + break; + } + } + ret = virSecuritySELinuxSetFilecon(mgr, path, use_label, remember); } -- 2.56.0

From: Michal Privoznik <mprivozn@redhat.com> Our qemusecuritymock reimplements stat() (for the purpose of testing our DAC secdriver) and so far, for every path S_IFREG mode is set (i.e. pretending every path thrown into stat() is a regular file). Extend the mock so that a path can be directory (S_IFDIR). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemusecuritymock.c | 7 ++++++- tests/qemusecuritytest.h | 3 +++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/qemusecuritymock.c b/tests/qemusecuritymock.c index cbd928ab38..51ba584853 100644 --- a/tests/qemusecuritymock.c +++ b/tests/qemusecuritymock.c @@ -232,6 +232,7 @@ int virFileRemoveXAttr(const char *path, #define VIR_MOCK_STAT_HOOK \ do { \ if (g_getenv(ENVVAR)) { \ + const char *mimicDir = g_getenv(PATH_MIMIC_DIR); \ uint32_t *val; \ \ virMutexLock(&m); \ @@ -239,7 +240,11 @@ int virFileRemoveXAttr(const char *path, \ memset(sb, 0, sizeof(*sb)); \ \ - sb->st_mode = S_IFREG | 0666; \ + if (STREQ_NULLABLE(path, mimicDir)) { \ + sb->st_mode = S_IFDIR | 0666; \ + } else { \ + sb->st_mode = S_IFREG | 0666; \ + } \ sb->st_size = 123456; \ sb->st_ino = 1; \ \ diff --git a/tests/qemusecuritytest.h b/tests/qemusecuritytest.h index 696cfb4b63..68b37352f3 100644 --- a/tests/qemusecuritytest.h +++ b/tests/qemusecuritytest.h @@ -19,6 +19,9 @@ #pragma once #define ENVVAR "LIBVIRT_QEMU_SECURITY_TEST" +/* If PATH_MIMIC_DIR is set then stat() mocked in qemusecuritymock.c pretends + * the value is a directory instead of a file. */ +#define PATH_MIMIC_DIR "LIBVIRT_QEMU_SECURITY_TEST_MIMIC_DIR" #include "internal.h" -- 2.56.0