[libvirt] [PATCH 0/2] fix chardev security labels
Pavel Hrdina (2): security: introduce virSecurityManager(Set|Restore)ChardevLabel qemu: fix security labeling for attach/detach of char devices src/libvirt_private.syms | 2 ++ src/qemu/qemu_hotplug.c | 10 +++++++ src/qemu/qemu_security.c | 60 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_security.h | 8 ++++++ src/security/security_dac.c | 3 +++ src/security/security_driver.h | 11 ++++++++ src/security/security_manager.c | 40 +++++++++++++++++++++++++++ src/security/security_manager.h | 10 +++++++ src/security/security_nop.c | 20 ++++++++++++++ src/security/security_selinux.c | 3 +++ src/security/security_stack.c | 43 +++++++++++++++++++++++++++++ 11 files changed, 210 insertions(+) -- 2.14.3
SELinux and DAC drivers already have both functions but they were not exported as public API of security manager. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt_private.syms | 2 ++ src/security/security_dac.c | 3 +++ src/security/security_driver.h | 11 +++++++++++ src/security/security_manager.c | 40 ++++++++++++++++++++++++++++++++++++++ src/security/security_manager.h | 10 ++++++++++ src/security/security_nop.c | 20 +++++++++++++++++++ src/security/security_selinux.c | 3 +++ src/security/security_stack.c | 43 +++++++++++++++++++++++++++++++++++++++++ 8 files changed, 132 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 53057aa82e..de4ec4d442 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1276,6 +1276,7 @@ virSecurityManagerPreFork; virSecurityManagerReleaseLabel; virSecurityManagerReserveLabel; virSecurityManagerRestoreAllLabel; +virSecurityManagerRestoreChardevLabel; virSecurityManagerRestoreDiskLabel; virSecurityManagerRestoreHostdevLabel; virSecurityManagerRestoreImageLabel; @@ -1283,6 +1284,7 @@ virSecurityManagerRestoreInputLabel; virSecurityManagerRestoreMemoryLabel; virSecurityManagerRestoreSavedStateLabel; virSecurityManagerSetAllLabel; +virSecurityManagerSetChardevLabel; virSecurityManagerSetChildProcessLabel; virSecurityManagerSetDaemonSocketLabel; virSecurityManagerSetDiskLabel; diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 52ca07a10f..609d2595b2 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -2155,4 +2155,7 @@ virSecurityDriver virSecurityDriverDAC = { .getBaseLabel = virSecurityDACGetBaseLabel, .domainSetPathLabel = virSecurityDACDomainSetPathLabel, + + .domainSetSecurityChardevLabel = virSecurityDACSetChardevLabel, + .domainRestoreSecurityChardevLabel = virSecurityDACRestoreChardevLabel, }; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 1b3070d06d..47dad8ba20 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -140,6 +140,14 @@ typedef int (*virSecurityDomainRestoreInputLabel) (virSecurityManagerPtr mgr, typedef int (*virSecurityDomainSetPathLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, const char *path); +typedef int (*virSecurityDomainSetChardevLabel) (virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainChrSourceDefPtr dev_source, + bool chardevStdioLogd); +typedef int (*virSecurityDomainRestoreChardevLabel) (virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainChrSourceDefPtr dev_source, + bool chardevStdioLogd); struct _virSecurityDriver { @@ -201,6 +209,9 @@ struct _virSecurityDriver { virSecurityDriverGetBaseLabel getBaseLabel; virSecurityDomainSetPathLabel domainSetPathLabel; + + virSecurityDomainSetChardevLabel domainSetSecurityChardevLabel; + virSecurityDomainRestoreChardevLabel domainRestoreSecurityChardevLabel; }; virSecurityDriverPtr virSecurityDriverLookup(const char *name, diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 3cf12188a0..9249aba1fa 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -1152,3 +1152,43 @@ virSecurityManagerRestoreInputLabel(virSecurityManagerPtr mgr, virReportUnsupportedError(); return -1; } + + +int +virSecurityManagerSetChardevLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainChrSourceDefPtr dev_source, + bool chardevStdioLogd) +{ + if (mgr->drv->domainSetSecurityChardevLabel) { + int ret; + virObjectLock(mgr); + ret = mgr->drv->domainSetSecurityChardevLabel(mgr, def, dev_source, + chardevStdioLogd); + virObjectUnlock(mgr); + return ret; + } + + virReportUnsupportedError(); + return -1; +} + + +int +virSecurityManagerRestoreChardevLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainChrSourceDefPtr dev_source, + bool chardevStdioLogd) +{ + if (mgr->drv->domainRestoreSecurityChardevLabel) { + int ret; + virObjectLock(mgr); + ret = mgr->drv->domainRestoreSecurityChardevLabel(mgr, def, dev_source, + chardevStdioLogd); + virObjectUnlock(mgr); + return ret; + } + + virReportUnsupportedError(); + return -1; +} diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 834c7f1593..013e3b9b18 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -184,4 +184,14 @@ int virSecurityManagerDomainSetPathLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, const char *path); +int virSecurityManagerSetChardevLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainChrSourceDefPtr dev_source, + bool chardevStdioLogd); + +int virSecurityManagerRestoreChardevLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainChrSourceDefPtr dev_source, + bool chardevStdioLogd); + #endif /* VIR_SECURITY_MANAGER_H__ */ diff --git a/src/security/security_nop.c b/src/security/security_nop.c index cfb032c686..ff739f8199 100644 --- a/src/security/security_nop.c +++ b/src/security/security_nop.c @@ -262,6 +262,23 @@ virSecurityDomainInputLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return 0; } +static int +virSecurityDomainSetChardevLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainChrSourceDefPtr dev_source ATTRIBUTE_UNUSED, + bool chardevStdioLogd ATTRIBUTE_UNUSED) +{ + return 0; +} + +static int +virSecurityDomainRestoreChardevLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainChrSourceDefPtr dev_source ATTRIBUTE_UNUSED, + bool chardevStdioLogd ATTRIBUTE_UNUSED) +{ + return 0; +} virSecurityDriver virSecurityDriverNop = { .privateDataLen = 0, @@ -314,4 +331,7 @@ virSecurityDriver virSecurityDriverNop = { .domainGetSecurityMountOptions = virSecurityDomainGetMountOptionsNop, .getBaseLabel = virSecurityGetBaseLabel, + + .domainSetSecurityChardevLabel = virSecurityDomainSetChardevLabelNop, + .domainRestoreSecurityChardevLabel = virSecurityDomainRestoreChardevLabelNop, }; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index b677fbcda7..0815a02d18 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -3095,4 +3095,7 @@ virSecurityDriver virSecurityDriverSELinux = { .getBaseLabel = virSecuritySELinuxGetBaseLabel, .domainSetPathLabel = virSecuritySELinuxDomainSetPathLabel, + + .domainSetSecurityChardevLabel = virSecuritySELinuxSetChardevLabel, + .domainRestoreSecurityChardevLabel = virSecuritySELinuxRestoreChardevLabel, }; diff --git a/src/security/security_stack.c b/src/security/security_stack.c index cd916382b2..0375e7d89d 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -719,6 +719,46 @@ virSecurityStackDomainSetPathLabel(virSecurityManagerPtr mgr, return rc; } +static int +virSecurityStackDomainSetChardevLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainChrSourceDefPtr dev_source, + bool chardevStdioLogd) +{ + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityStackItemPtr item = priv->itemsHead; + int rc = 0; + + for (; item; item = item->next) { + if (virSecurityManagerSetChardevLabel(item->securityManager, + def, dev_source, + chardevStdioLogd) < 0) + rc = -1; + } + + return rc; +} + +static int +virSecurityStackDomainRestoreChardevLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainChrSourceDefPtr dev_source, + bool chardevStdioLogd) +{ + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityStackItemPtr item = priv->itemsHead; + int rc = 0; + + for (; item; item = item->next) { + if (virSecurityManagerRestoreChardevLabel(item->securityManager, + def, dev_source, + chardevStdioLogd) < 0) + rc = -1; + } + + return rc; +} + virSecurityDriver virSecurityDriverStack = { .privateDataLen = sizeof(virSecurityStackData), .name = "stack", @@ -778,4 +818,7 @@ virSecurityDriver virSecurityDriverStack = { .getBaseLabel = virSecurityStackGetBaseLabel, .domainSetPathLabel = virSecurityStackDomainSetPathLabel, + + .domainSetSecurityChardevLabel = virSecurityStackDomainSetChardevLabel, + .domainRestoreSecurityChardevLabel = virSecurityStackDomainRestoreChardevLabel, }; -- 2.14.3
Commit e93d844b90 was not enough to fix the permission denied issue. We need to apply security labels as well. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1465833 Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_hotplug.c | 10 ++++++++ src/qemu/qemu_security.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_security.h | 8 +++++++ 3 files changed, 78 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index cbc7af59b7..6ef28bf051 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1815,6 +1815,7 @@ int qemuDomainAttachChrDevice(virConnectPtr conn, bool chardevAttached = false; bool teardowncgroup = false; bool teardowndevice = false; + bool teardownlabel = false; char *tlsAlias = NULL; char *secAlias = NULL; bool need_release = false; @@ -1835,6 +1836,10 @@ int qemuDomainAttachChrDevice(virConnectPtr conn, goto cleanup; teardowndevice = true; + if (qemuSecuritySetChardevLabel(driver, vm, chr) < 0) + goto cleanup; + teardownlabel = true; + if (qemuSetupChardevCgroup(vm, chr) < 0) goto cleanup; teardowncgroup = true; @@ -1877,6 +1882,8 @@ int qemuDomainAttachChrDevice(virConnectPtr conn, qemuDomainReleaseDeviceAddress(vm, &chr->info, NULL); if (teardowncgroup && qemuTeardownChardevCgroup(vm, chr) < 0) VIR_WARN("Unable to remove chr device cgroup ACL on hotplug fail"); + if (teardownlabel && qemuSecurityRestoreChardevLabel(driver, vm, chr) < 0) + VIR_WARN("Unable to restore security label on char device"); if (teardowndevice && qemuDomainNamespaceTeardownChardev(driver, vm, chr) < 0) VIR_WARN("Unable to remove chr device from /dev"); } @@ -4154,6 +4161,9 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, if (qemuTeardownChardevCgroup(vm, chr) < 0) VIR_WARN("Failed to remove chr device cgroup ACL"); + if (qemuSecurityRestoreChardevLabel(driver, vm, chr) < 0) + VIR_WARN("Unable to restore security label on char device"); + if (qemuDomainNamespaceTeardownChardev(driver, vm, chr) < 0) VIR_WARN("Unable to remove chr device from /dev"); diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index e7d2bbd5a3..2aced22d2d 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -364,3 +364,63 @@ qemuSecurityRestoreInputLabel(virDomainObjPtr vm, virSecurityManagerTransactionAbort(driver->securityManager); return ret; } + + +int +qemuSecuritySetChardevLabel(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainChrDefPtr chr) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && + virSecurityManagerTransactionStart(driver->securityManager) < 0) + goto cleanup; + + if (virSecurityManagerSetChardevLabel(driver->securityManager, + vm->def, + chr->source, + priv->chardevStdioLogd) < 0) + goto cleanup; + + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && + virSecurityManagerTransactionCommit(driver->securityManager, + vm->pid) < 0) + goto cleanup; + + ret = 0; + cleanup: + virSecurityManagerTransactionAbort(driver->securityManager); + return ret; +} + + +int +qemuSecurityRestoreChardevLabel(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainChrDefPtr chr) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && + virSecurityManagerTransactionStart(driver->securityManager) < 0) + goto cleanup; + + if (virSecurityManagerRestoreChardevLabel(driver->securityManager, + vm->def, + chr->source, + priv->chardevStdioLogd) < 0) + goto cleanup; + + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && + virSecurityManagerTransactionCommit(driver->securityManager, + vm->pid) < 0) + goto cleanup; + + ret = 0; + cleanup: + virSecurityManagerTransactionAbort(driver->securityManager); + return ret; +} diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index 76d63f06ec..d54ce6fead 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -76,6 +76,14 @@ int qemuSecuritySetInputLabel(virDomainObjPtr vm, int qemuSecurityRestoreInputLabel(virDomainObjPtr vm, virDomainInputDefPtr input); +int qemuSecuritySetChardevLabel(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainChrDefPtr chr); + +int qemuSecurityRestoreChardevLabel(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainChrDefPtr chr); + /* Please note that for these APIs there is no wrapper yet. Do NOT blindly add * new APIs here. If an API can touch a /dev file add a proper wrapper instead. */ -- 2.14.3
On Fri, Dec 01, 2017 at 01:54:30PM +0100, Pavel Hrdina wrote:
Pavel Hrdina (2): security: introduce virSecurityManager(Set|Restore)ChardevLabel qemu: fix security labeling for attach/detach of char devices
src/libvirt_private.syms | 2 ++ src/qemu/qemu_hotplug.c | 10 +++++++ src/qemu/qemu_security.c | 60 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_security.h | 8 ++++++ src/security/security_dac.c | 3 +++ src/security/security_driver.h | 11 ++++++++ src/security/security_manager.c | 40 +++++++++++++++++++++++++++ src/security/security_manager.h | 10 +++++++ src/security/security_nop.c | 20 ++++++++++++++ src/security/security_selinux.c | 3 +++ src/security/security_stack.c | 43 +++++++++++++++++++++++++++++ 11 files changed, 210 insertions(+)
ACK series I will refrain from commenting on whether these are bug fixes or style cleanups. Jan
participants (2)
-
Ján Tomko -
Pavel Hrdina