Re: [libvirt] [PATCH v2 4/4] security: don't relabel chardev source if virtlogd is used as stdio handler

In the case that virtlogd is used as stdio handler we pass to QEMU only FD to a PIPE connected to virtlogd instead of the file itself. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1430988 Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- Notes: new in v2 src/lxc/lxc_process.c | 6 ++--- src/qemu/qemu_security.c | 9 +++++-- src/security/security_apparmor.c | 7 ++++-- src/security/security_dac.c | 54 +++++++++++++++++++++++++++++++--------- src/security/security_driver.h | 6 +++-- src/security/security_manager.c | 12 ++++++--- src/security/security_manager.h | 6 +++-- src/security/security_nop.c | 6 +++-- src/security/security_selinux.c | 53 ++++++++++++++++++++++++++++++--------- src/security/security_stack.c | 12 ++++++--- tests/securityselinuxlabeltest.c | 2 +- 11 files changed, 127 insertions(+), 46 deletions(-)

diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index d8727c3b43..2658ea61f8 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -852,7 +852,7 @@ int virLXCProcessStop(virLXCDriverPtr driver, } virSecurityManagerRestoreAllLabel(driver->securityManager, - vm->def, false); + vm->def, false, false); virSecurityManagerReleaseLabel(driver->securityManager, vm->def); /* Clear out dynamically assigned labels */ if (vm->def->nseclabels && @@ -1349,7 +1349,7 @@ int virLXCProcessStart(virConnectPtr conn, VIR_DEBUG("Setting domain security labels"); if (virSecurityManagerSetAllLabel(driver->securityManager, - vm->def, NULL) < 0) + vm->def, NULL, false) < 0) goto cleanup; VIR_DEBUG("Setting up consoles"); @@ -1578,7 +1578,7 @@ int virLXCProcessStart(virConnectPtr conn, virLXCProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED); } else { virSecurityManagerRestoreAllLabel(driver->securityManager, - vm->def, false); + vm->def, false, false); virSecurityManagerReleaseLabel(driver->securityManager, vm->def); /* Clear out dynamically assigned labels */ if (vm->def->nseclabels && diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index 61934f9905..6fc3b0bb6e 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -38,6 +38,7 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver, const char *stdin_path) { int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && virSecurityManagerTransactionStart(driver->securityManager) < 0) @@ -45,7 +46,8 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver, if (virSecurityManagerSetAllLabel(driver->securityManager, vm->def, - stdin_path) < 0) + stdin_path, + priv->chardevStdioLogd) < 0) goto cleanup; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && @@ -65,6 +67,8 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, bool migrated) { + qemuDomainObjPrivatePtr priv = vm->privateData; + /* In contrast to qemuSecuritySetAllLabel, do not use * secdriver transactions here. This function is called from * qemuProcessStop() which is meant to do cleanup after qemu @@ -73,7 +77,8 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver, * in entering the namespace then. */ virSecurityManagerRestoreAllLabel(driver->securityManager, vm->def, - migrated); + migrated, + priv->chardevStdioLogd); } diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 62672b0af0..5afe0c5c85 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -489,7 +489,9 @@ AppArmorGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, static int AppArmorSetSecurityAllLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, const char *stdin_path) + virDomainDefPtr def, + const char *stdin_path, + bool chardevStdioLogd ATTRIBUTE_UNUSED) { virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME); @@ -567,7 +569,8 @@ AppArmorReleaseSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, static int AppArmorRestoreSecurityAllLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr def, - bool migrated ATTRIBUTE_UNUSED) + bool migrated ATTRIBUTE_UNUSED, + bool chardevStdioLogd ATTRIBUTE_UNUSED) { int rc = 0; virSecurityLabelDefPtr secdef = diff --git a/src/security/security_dac.c b/src/security/security_dac.c index fd4d8f5047..79941f480a 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1159,7 +1159,8 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr mgr, static int virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, - virDomainChrSourceDefPtr dev_source) + virDomainChrSourceDefPtr dev_source, + bool chardevStdioLogd) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); @@ -1178,6 +1179,9 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, if (chr_seclabel && !chr_seclabel->relabel) return 0; + if (!chr_seclabel && chardevStdioLogd) + return 0; + if (chr_seclabel && chr_seclabel->label) { if (virParseOwnershipIds(chr_seclabel->label, &user, &group) < 0) return -1; @@ -1243,7 +1247,8 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, static int virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr, virDomainDefPtr def ATTRIBUTE_UNUSED, - virDomainChrSourceDefPtr dev_source) + virDomainChrSourceDefPtr dev_source, + bool chardevStdioLogd) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityDeviceLabelDefPtr chr_seclabel = NULL; @@ -1256,6 +1261,9 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr, if (chr_seclabel && !chr_seclabel->relabel) return 0; + if (!chr_seclabel && chardevStdioLogd) + return 0; + switch ((virDomainChrType) dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: @@ -1298,14 +1306,21 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr, } +struct _virSecuritySELinuxChardevCallbackData { + virSecurityManagerPtr mgr; + bool chardevStdioLogd; +}; + + static int virSecurityDACRestoreChardevCallback(virDomainDefPtr def, virDomainChrDefPtr dev ATTRIBUTE_UNUSED, void *opaque) { - virSecurityManagerPtr mgr = opaque; + struct _virSecuritySELinuxChardevCallbackData *data = opaque; - return virSecurityDACRestoreChardevLabel(mgr, def, dev->source); + return virSecurityDACRestoreChardevLabel(data->mgr, def, dev->source, + data->chardevStdioLogd); } @@ -1319,7 +1334,8 @@ virSecurityDACSetTPMFileLabel(virSecurityManagerPtr mgr, switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: ret = virSecurityDACSetChardevLabel(mgr, def, - &tpm->data.passthrough.source); + &tpm->data.passthrough.source, + false); break; case VIR_DOMAIN_TPM_TYPE_LAST: break; @@ -1339,7 +1355,8 @@ virSecurityDACRestoreTPMFileLabel(virSecurityManagerPtr mgr, switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: ret = virSecurityDACRestoreChardevLabel(mgr, def, - &tpm->data.passthrough.source); + &tpm->data.passthrough.source, + false); break; case VIR_DOMAIN_TPM_TYPE_LAST: break; @@ -1436,7 +1453,8 @@ virSecurityDACRestoreMemoryLabel(virSecurityManagerPtr mgr, static int virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, - bool migrated) + bool migrated, + bool chardevStdioLogd) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityLabelDefPtr secdef; @@ -1479,10 +1497,15 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr, rc = -1; } + struct _virSecuritySELinuxChardevCallbackData chardevData = { + .mgr = mgr, + .chardevStdioLogd = chardevStdioLogd, + }; + if (virDomainChrDefForeach(def, false, virSecurityDACRestoreChardevCallback, - mgr) < 0) + &chardevData) < 0) rc = -1; if (def->tpm) { @@ -1505,9 +1528,10 @@ virSecurityDACSetChardevCallback(virDomainDefPtr def, virDomainChrDefPtr dev ATTRIBUTE_UNUSED, void *opaque) { - virSecurityManagerPtr mgr = opaque; + struct _virSecuritySELinuxChardevCallbackData *data = opaque; - return virSecurityDACSetChardevLabel(mgr, def, dev->source); + return virSecurityDACSetChardevLabel(data->mgr, def, dev->source, + data->chardevStdioLogd); } @@ -1549,7 +1573,8 @@ virSecurityDACSetMemoryLabel(virSecurityManagerPtr mgr, static int virSecurityDACSetAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, - const char *stdin_path ATTRIBUTE_UNUSED) + const char *stdin_path ATTRIBUTE_UNUSED, + bool chardevStdioLogd) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityLabelDefPtr secdef; @@ -1592,10 +1617,15 @@ virSecurityDACSetAllLabel(virSecurityManagerPtr mgr, return -1; } + struct _virSecuritySELinuxChardevCallbackData chardevData = { + .mgr = mgr, + .chardevStdioLogd = chardevStdioLogd, + }; + if (virDomainChrDefForeach(def, true, virSecurityDACSetChardevCallback, - mgr) < 0) + &chardevData) < 0) return -1; if (def->tpm) { diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 0f5cce5f8d..0b3b452486 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -91,10 +91,12 @@ typedef int (*virSecurityDomainReleaseLabel) (virSecurityManagerPtr mgr, virDomainDefPtr sec); typedef int (*virSecurityDomainSetAllLabel) (virSecurityManagerPtr mgr, virDomainDefPtr sec, - const char *stdin_path); + const char *stdin_path, + bool chardevStdioLogd); typedef int (*virSecurityDomainRestoreAllLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, - bool migrated); + bool migrated, + bool chardevStdioLogd); typedef int (*virSecurityDomainGetProcessLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, pid_t pid, diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 90d491c1bc..013bbc37ef 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -856,12 +856,14 @@ int virSecurityManagerCheckAllLabel(virSecurityManagerPtr mgr, int virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, - const char *stdin_path) + const char *stdin_path, + bool chardevStdioLogd) { if (mgr->drv->domainSetSecurityAllLabel) { int ret; virObjectLock(mgr); - ret = mgr->drv->domainSetSecurityAllLabel(mgr, vm, stdin_path); + ret = mgr->drv->domainSetSecurityAllLabel(mgr, vm, stdin_path, + chardevStdioLogd); virObjectUnlock(mgr); return ret; } @@ -874,12 +876,14 @@ virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr, int virSecurityManagerRestoreAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, - bool migrated) + bool migrated, + bool chardevStdioLogd) { if (mgr->drv->domainRestoreSecurityAllLabel) { int ret; virObjectLock(mgr); - ret = mgr->drv->domainRestoreSecurityAllLabel(mgr, vm, migrated); + ret = mgr->drv->domainRestoreSecurityAllLabel(mgr, vm, migrated, + chardevStdioLogd); virObjectUnlock(mgr); return ret; } diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 238e66cd0b..01296d339e 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -130,10 +130,12 @@ int virSecurityManagerCheckAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr sec); int virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr sec, - const char *stdin_path); + const char *stdin_path, + bool chardevStdioLogd); int virSecurityManagerRestoreAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, - bool migrated); + bool migrated, + bool chardevStdioLogd); int virSecurityManagerGetProcessLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, pid_t pid, diff --git a/src/security/security_nop.c b/src/security/security_nop.c index 0a9b515288..527be11e5a 100644 --- a/src/security/security_nop.c +++ b/src/security/security_nop.c @@ -151,7 +151,8 @@ virSecurityDomainReleaseLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, static int virSecurityDomainSetAllLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr sec ATTRIBUTE_UNUSED, - const char *stdin_path ATTRIBUTE_UNUSED) + const char *stdin_path ATTRIBUTE_UNUSED, + bool chardevStdioLogd ATTRIBUTE_UNUSED) { return 0; } @@ -159,7 +160,8 @@ virSecurityDomainSetAllLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, static int virSecurityDomainRestoreAllLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr vm ATTRIBUTE_UNUSED, - bool migrated ATTRIBUTE_UNUSED) + bool migrated ATTRIBUTE_UNUSED, + bool chardevStdioLogd ATTRIBUTE_UNUSED) { return 0; } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 75f387b3fa..26137f6d8d 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2179,7 +2179,8 @@ virSecuritySELinuxRestoreHostdevLabel(virSecurityManagerPtr mgr, static int virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, - virDomainChrSourceDefPtr dev_source) + virDomainChrSourceDefPtr dev_source, + bool chardevStdioLogd) { virSecurityLabelDefPtr seclabel; @@ -2198,6 +2199,9 @@ virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr, if (chr_seclabel && !chr_seclabel->relabel) return 0; + if (!chr_seclabel && chardevStdioLogd) + return 0; + if (chr_seclabel) imagelabel = chr_seclabel->label; if (!imagelabel) @@ -2252,7 +2256,8 @@ virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr, static int virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, - virDomainChrSourceDefPtr dev_source) + virDomainChrSourceDefPtr dev_source, + bool chardevStdioLogd) { virSecurityLabelDefPtr seclabel; @@ -2269,6 +2274,9 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr, if (chr_seclabel && !chr_seclabel->relabel) return 0; + if (!chr_seclabel && chardevStdioLogd) + return 0; + switch (dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: @@ -2312,14 +2320,21 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr, } +struct _virSecuritySELinuxChardevCallbackData { + virSecurityManagerPtr mgr; + bool chardevStdioLogd; +}; + + static int virSecuritySELinuxRestoreSecurityChardevCallback(virDomainDefPtr def, virDomainChrDefPtr dev ATTRIBUTE_UNUSED, void *opaque) { - virSecurityManagerPtr mgr = opaque; + struct _virSecuritySELinuxChardevCallbackData *data = opaque; - return virSecuritySELinuxRestoreChardevLabel(mgr, def, dev->source); + return virSecuritySELinuxRestoreChardevLabel(data->mgr, def, dev->source, + data->chardevStdioLogd); } @@ -2342,7 +2357,8 @@ virSecuritySELinuxRestoreSecuritySmartcardCallback(virDomainDefPtr def, return virSecuritySELinuxRestoreFileLabel(mgr, database); case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH: - return virSecuritySELinuxRestoreChardevLabel(mgr, def, dev->data.passthru); + return virSecuritySELinuxRestoreChardevLabel(mgr, def, + dev->data.passthru, false); default: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2369,7 +2385,8 @@ virSecuritySELinuxGetBaseLabel(virSecurityManagerPtr mgr, int virtType) static int virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, - bool migrated) + bool migrated, + bool chardevStdioLogd) { virSecurityLabelDefPtr secdef; virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); @@ -2414,10 +2431,15 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr mgr, rc = -1; } + struct _virSecuritySELinuxChardevCallbackData chardevData = { + .mgr = mgr, + .chardevStdioLogd = chardevStdioLogd + }; + if (virDomainChrDefForeach(def, false, virSecuritySELinuxRestoreSecurityChardevCallback, - mgr) < 0) + &chardevData) < 0) rc = -1; if (virDomainSmartcardDefForeach(def, @@ -2706,9 +2728,10 @@ virSecuritySELinuxSetSecurityChardevCallback(virDomainDefPtr def, virDomainChrDefPtr dev ATTRIBUTE_UNUSED, void *opaque) { - virSecurityManagerPtr mgr = opaque; + struct _virSecuritySELinuxChardevCallbackData *data = opaque; - return virSecuritySELinuxSetChardevLabel(mgr, def, dev->source); + return virSecuritySELinuxSetChardevLabel(data->mgr, def, dev->source, + data->chardevStdioLogd); } @@ -2733,7 +2756,7 @@ virSecuritySELinuxSetSecuritySmartcardCallback(virDomainDefPtr def, case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH: return virSecuritySELinuxSetChardevLabel(mgr, def, - dev->data.passthru); + dev->data.passthru, false); default: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2749,7 +2772,8 @@ virSecuritySELinuxSetSecuritySmartcardCallback(virDomainDefPtr def, static int virSecuritySELinuxSetAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, - const char *stdin_path) + const char *stdin_path, + bool chardevStdioLogd) { size_t i; virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); @@ -2797,10 +2821,15 @@ virSecuritySELinuxSetAllLabel(virSecurityManagerPtr mgr, return -1; } + struct _virSecuritySELinuxChardevCallbackData chardevData = { + .mgr = mgr, + .chardevStdioLogd = chardevStdioLogd + }; + if (virDomainChrDefForeach(def, true, virSecuritySELinuxSetSecurityChardevCallback, - mgr) < 0) + &chardevData) < 0) return -1; if (virDomainSmartcardDefForeach(def, diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 9a1a7b30c5..53eee1692f 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -350,14 +350,16 @@ virSecurityStackRestoreHostdevLabel(virSecurityManagerPtr mgr, static int virSecurityStackSetAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, - const char *stdin_path) + const char *stdin_path, + bool chardevStdioLogd) { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; int rc = 0; for (; item; item = item->next) { - if (virSecurityManagerSetAllLabel(item->securityManager, vm, stdin_path) < 0) + if (virSecurityManagerSetAllLabel(item->securityManager, vm, + stdin_path, chardevStdioLogd) < 0) rc = -1; } @@ -368,14 +370,16 @@ virSecurityStackSetAllLabel(virSecurityManagerPtr mgr, static int virSecurityStackRestoreAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, - bool migrated) + bool migrated, + bool chardevStdioLogd) { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; int rc = 0; for (; item; item = item->next) { - if (virSecurityManagerRestoreAllLabel(item->securityManager, vm, migrated) < 0) + if (virSecurityManagerRestoreAllLabel(item->securityManager, vm, + migrated, chardevStdioLogd) < 0) rc = -1; } diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c index 3e134991f2..ddcc954429 100644 --- a/tests/securityselinuxlabeltest.c +++ b/tests/securityselinuxlabeltest.c @@ -313,7 +313,7 @@ testSELinuxLabeling(const void *opaque) if (!(def = testSELinuxLoadDef(testname))) goto cleanup; - if (virSecurityManagerSetAllLabel(mgr, def, NULL) < 0) + if (virSecurityManagerSetAllLabel(mgr, def, NULL, false) < 0) goto cleanup; if (testSELinuxCheckLabels(files, nfiles) < 0)