[libvirt] [PATCH 0/5] security_stack: Perform rollback if one of stacked drivers fails
See 5/5 for explanation. Michal Prívozník (5): security: Pass @migrated to virSecurityManagerSetAllLabel security: Rename virSecurityManagerGetDriver() to virSecurityManagerGetVirtDriver() security: Introduce virSecurityManagerGetDriver() security_stack: Turn list of nested drivers into a doubly linked list security_stack: Perform rollback if one of stacked drivers fails src/lxc/lxc_process.c | 2 +- src/qemu/qemu_process.c | 3 +- src/qemu/qemu_security.c | 6 +- src/qemu/qemu_security.h | 3 +- src/security/security_apparmor.c | 3 +- src/security/security_dac.c | 3 +- src/security/security_driver.h | 3 +- src/security/security_manager.c | 17 ++- src/security/security_manager.h | 4 +- src/security/security_nop.c | 3 +- src/security/security_selinux.c | 9 +- src/security/security_stack.c | 220 +++++++++++++++++++++++++------ tests/qemusecuritytest.c | 2 +- tests/securityselinuxlabeltest.c | 2 +- 14 files changed, 222 insertions(+), 58 deletions(-) -- 2.21.0
In upcoming commits, virSecurityManagerSetAllLabel() will perform rollback in case of failure by calling virSecurityManagerRestoreAllLabel(). But in order to do that, the former needs to have @migrated argument so that it can be passed to the latter. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/lxc/lxc_process.c | 2 +- src/qemu/qemu_process.c | 3 ++- src/qemu/qemu_security.c | 6 ++++-- src/qemu/qemu_security.h | 3 ++- src/security/security_apparmor.c | 3 ++- src/security/security_dac.c | 3 ++- src/security/security_driver.h | 3 ++- src/security/security_manager.c | 6 ++++-- src/security/security_manager.h | 3 ++- src/security/security_nop.c | 3 ++- src/security/security_selinux.c | 3 ++- src/security/security_stack.c | 6 ++++-- tests/qemusecuritytest.c | 2 +- tests/securityselinuxlabeltest.c | 2 +- 14 files changed, 31 insertions(+), 17 deletions(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index cbdc7b1268..65775424cb 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1346,7 +1346,7 @@ int virLXCProcessStart(virConnectPtr conn, VIR_DEBUG("Setting domain security labels"); if (virSecurityManagerSetAllLabel(driver->securityManager, - vm->def, NULL, false) < 0) + vm->def, NULL, false, false) < 0) goto cleanup; VIR_DEBUG("Setting up consoles"); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 955ba4de4c..4348a6dd36 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6937,7 +6937,8 @@ qemuProcessLaunch(virConnectPtr conn, VIR_DEBUG("Setting domain security labels"); if (qemuSecuritySetAllLabel(driver, vm, - incoming ? incoming->path : NULL) < 0) + incoming ? incoming->path : NULL, + incoming != NULL) < 0) goto cleanup; /* Security manager labeled all devices, therefore diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index 91dd34f0e7..f4e815e966 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -32,7 +32,8 @@ VIR_LOG_INIT("qemu.qemu_process"); int qemuSecuritySetAllLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, - const char *stdin_path) + const char *stdin_path, + bool migrated) { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -47,7 +48,8 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver, if (virSecurityManagerSetAllLabel(driver->securityManager, vm->def, stdin_path, - priv->chardevStdioLogd) < 0) + priv->chardevStdioLogd, + migrated) < 0) goto cleanup; if (virSecurityManagerTransactionCommit(driver->securityManager, diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index 224a4d61c9..29908141ba 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -26,7 +26,8 @@ int qemuSecuritySetAllLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, - const char *stdin_path); + const char *stdin_path, + bool migrated); void qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 77eee9410c..699590ee00 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -488,7 +488,8 @@ static int AppArmorSetSecurityAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, const char *stdin_path, - bool chardevStdioLogd ATTRIBUTE_UNUSED) + bool chardevStdioLogd ATTRIBUTE_UNUSED, + bool migrated ATTRIBUTE_UNUSED) { virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME); diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4b4afef18a..9e71513f14 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1983,7 +1983,8 @@ static int virSecurityDACSetAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, const char *stdin_path ATTRIBUTE_UNUSED, - bool chardevStdioLogd) + bool chardevStdioLogd, + bool migrated ATTRIBUTE_UNUSED) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityLabelDefPtr secdef; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index b4ffed29ec..3353955813 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -83,7 +83,8 @@ typedef int (*virSecurityDomainReleaseLabel) (virSecurityManagerPtr mgr, typedef int (*virSecurityDomainSetAllLabel) (virSecurityManagerPtr mgr, virDomainDefPtr sec, const char *stdin_path, - bool chardevStdioLogd); + bool chardevStdioLogd, + bool migrated); typedef int (*virSecurityDomainRestoreAllLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, bool migrated, diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 7c905f0785..a04d2d848d 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -852,13 +852,15 @@ int virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, const char *stdin_path, - bool chardevStdioLogd) + bool chardevStdioLogd, + bool migrated) { if (mgr->drv->domainSetSecurityAllLabel) { int ret; virObjectLock(mgr); ret = mgr->drv->domainSetSecurityAllLabel(mgr, vm, stdin_path, - chardevStdioLogd); + chardevStdioLogd, + migrated); virObjectUnlock(mgr); return ret; } diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 0d2375b263..1d4928fae3 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -121,7 +121,8 @@ int virSecurityManagerCheckAllLabel(virSecurityManagerPtr mgr, int virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr sec, const char *stdin_path, - bool chardevStdioLogd); + bool chardevStdioLogd, + bool migrated); int virSecurityManagerRestoreAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, bool migrated, diff --git a/src/security/security_nop.c b/src/security/security_nop.c index 966b9d41a1..96cdac03d8 100644 --- a/src/security/security_nop.c +++ b/src/security/security_nop.c @@ -136,7 +136,8 @@ static int virSecurityDomainSetAllLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr sec ATTRIBUTE_UNUSED, const char *stdin_path ATTRIBUTE_UNUSED, - bool chardevStdioLogd ATTRIBUTE_UNUSED) + bool chardevStdioLogd ATTRIBUTE_UNUSED, + bool migrated ATTRIBUTE_UNUSED) { return 0; } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index e879fa39ab..df0523abeb 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -3104,7 +3104,8 @@ static int virSecuritySELinuxSetAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, const char *stdin_path, - bool chardevStdioLogd) + bool chardevStdioLogd, + bool migrated ATTRIBUTE_UNUSED) { size_t i; virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); diff --git a/src/security/security_stack.c b/src/security/security_stack.c index d445c0773e..dd055075cb 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -316,7 +316,8 @@ static int virSecurityStackSetAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, const char *stdin_path, - bool chardevStdioLogd) + bool chardevStdioLogd, + bool migrated) { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; @@ -324,7 +325,8 @@ virSecurityStackSetAllLabel(virSecurityManagerPtr mgr, for (; item; item = item->next) { if (virSecurityManagerSetAllLabel(item->securityManager, vm, - stdin_path, chardevStdioLogd) < 0) + stdin_path, chardevStdioLogd, + migrated) < 0) rc = -1; } diff --git a/tests/qemusecuritytest.c b/tests/qemusecuritytest.c index 2d88979168..9efc15c105 100644 --- a/tests/qemusecuritytest.c +++ b/tests/qemusecuritytest.c @@ -116,7 +116,7 @@ testDomain(const void *opaque) if (setenv(ENVVAR, "1", 0) < 0) return -1; - if (qemuSecuritySetAllLabel(data->driver, vm, NULL) < 0) + if (qemuSecuritySetAllLabel(data->driver, vm, NULL, false) < 0) goto cleanup; qemuSecurityRestoreAllLabel(data->driver, vm, false); diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c index 8c3cb29c41..6f9b5c0e70 100644 --- a/tests/securityselinuxlabeltest.c +++ b/tests/securityselinuxlabeltest.c @@ -310,7 +310,7 @@ testSELinuxLabeling(const void *opaque) if (!(def = testSELinuxLoadDef(testname))) goto cleanup; - if (virSecurityManagerSetAllLabel(mgr, def, NULL, false) < 0) + if (virSecurityManagerSetAllLabel(mgr, def, NULL, false, false) < 0) goto cleanup; if (testSELinuxCheckLabels(files, nfiles) < 0) -- 2.21.0
This function is in fact returning the name of the virtualization driver that registered the security manager/driver. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_manager.c | 4 ++-- src/security/security_manager.h | 2 +- src/security/security_selinux.c | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index a04d2d848d..c513740a13 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -114,7 +114,7 @@ virSecurityManagerNewStack(virSecurityManagerPtr primary) { virSecurityManagerPtr mgr = virSecurityManagerNewDriver(&virSecurityDriverStack, - virSecurityManagerGetDriver(primary), + virSecurityManagerGetVirtDriver(primary), primary->flags); if (!mgr) @@ -326,7 +326,7 @@ virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr) const char * -virSecurityManagerGetDriver(virSecurityManagerPtr mgr) +virSecurityManagerGetVirtDriver(virSecurityManagerPtr mgr) { return mgr->virtDriver; } diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 1d4928fae3..306de92978 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -80,7 +80,7 @@ void virSecurityManagerTransactionAbort(virSecurityManagerPtr mgr); void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr); -const char *virSecurityManagerGetDriver(virSecurityManagerPtr mgr); +const char *virSecurityManagerGetVirtDriver(virSecurityManagerPtr mgr); const char *virSecurityManagerGetDOI(virSecurityManagerPtr mgr); const char *virSecurityManagerGetModel(virSecurityManagerPtr mgr); const char *virSecurityManagerGetBaseLabel(virSecurityManagerPtr mgr, int virtType); diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index df0523abeb..7263af50b8 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -791,7 +791,7 @@ virSecuritySELinuxQEMUInitialize(virSecurityManagerPtr mgr) static int virSecuritySELinuxInitialize(virSecurityManagerPtr mgr) { - VIR_DEBUG("SELinuxInitialize %s", virSecurityManagerGetDriver(mgr)); + VIR_DEBUG("SELinuxInitialize %s", virSecurityManagerGetVirtDriver(mgr)); if (virThreadLocalInit(&contextList, virSecuritySELinuxContextListFree) < 0) { @@ -800,7 +800,7 @@ virSecuritySELinuxInitialize(virSecurityManagerPtr mgr) return -1; } - if (STREQ(virSecurityManagerGetDriver(mgr), "LXC")) { + if (STREQ(virSecurityManagerGetVirtDriver(mgr), "LXC")) { return virSecuritySELinuxLXCInitialize(mgr); } else { return virSecuritySELinuxQEMUInitialize(mgr); @@ -829,7 +829,7 @@ virSecuritySELinuxGenLabel(virSecurityManagerPtr mgr, data = virSecurityManagerGetPrivateData(mgr); - VIR_DEBUG("label=%s", virSecurityManagerGetDriver(mgr)); + VIR_DEBUG("label=%s", virSecurityManagerGetVirtDriver(mgr)); if (seclabel->type == VIR_DOMAIN_SECLABEL_DYNAMIC && seclabel->label) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", -- 2.21.0
This function returns the name of the secdriver. Since the name is invariant we don't really need to lock the manager - it won't change. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_manager.c | 7 +++++++ src/security/security_manager.h | 1 + 2 files changed, 8 insertions(+) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index c513740a13..99281821d9 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -332,6 +332,13 @@ virSecurityManagerGetVirtDriver(virSecurityManagerPtr mgr) } +const char * +virSecurityManagerGetDriver(virSecurityManagerPtr mgr) +{ + return mgr->drv->name; +} + + const char * virSecurityManagerGetDOI(virSecurityManagerPtr mgr) { diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 306de92978..f835356b7e 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -80,6 +80,7 @@ void virSecurityManagerTransactionAbort(virSecurityManagerPtr mgr); void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr); +const char *virSecurityManagerGetDriver(virSecurityManagerPtr mgr); const char *virSecurityManagerGetVirtDriver(virSecurityManagerPtr mgr); const char *virSecurityManagerGetDOI(virSecurityManagerPtr mgr); const char *virSecurityManagerGetModel(virSecurityManagerPtr mgr); -- 2.21.0
In near future we will need to walk through the list of internal drivers in reversed order. The simplest solution is to turn singly linked list into a doubly linked list. We will not need to start from the end really, so there's no tail pointer kept. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_stack.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/security/security_stack.c b/src/security/security_stack.c index dd055075cb..51a8b76748 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -35,6 +35,7 @@ typedef virSecurityStackItem *virSecurityStackItemPtr; struct _virSecurityStackItem { virSecurityManagerPtr securityManager; virSecurityStackItemPtr next; + virSecurityStackItemPtr prev; }; struct _virSecurityStackData { @@ -56,6 +57,7 @@ virSecurityStackAddNested(virSecurityManagerPtr mgr, if (VIR_ALLOC(item) < 0) return -1; item->securityManager = nested; + item->prev = tmp; if (tmp) tmp->next = item; else -- 2.21.0
In order to have multiple security drivers hidden under one virSecurity* call, we have virSecurityStack driver which holds a list of registered security drivers and for every virSecurity* call it iterates over the list and calls corresponding callback in real security drivers. For instance, for virSecurityManagerSetAllLabel() it calls domainSetSecurityAllLabel callback sequentially in NOP, DAC and (possibly) SELinux or AppArmor drivers. This works just fine if the callback from every driver returns success. Problem arises when one of the drivers fails. For instance, aforementioned SetAllLabel() succeeds for DAC but fails in SELinux in which case all files that DAC relabelled are now owned by qemu:qemu (or whomever runs qemu) and thus permissions are leaked. This is even more visible with XATTRs which remain set for DAC. The solution is to perform a rollback on failure, i.e. call opposite action on drivers that succeeded. I'm providing rollback only for set calls and intentionally omitting restore calls for two reasons: 1) restore calls are less likely to fail (they merely remove XATTRs and chown()/setfilecon() file - all of these operations succeeded in set call), 2) we are not really interested in restore failures - in a very few places we check for retval of a restore function we do so only to print a warning. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_stack.c | 212 ++++++++++++++++++++++++++++------ 1 file changed, 176 insertions(+), 36 deletions(-) diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 51a8b76748..f27b09265c 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -24,8 +24,10 @@ #include "virerror.h" #include "viralloc.h" +#include "virlog.h" #define VIR_FROM_THIS VIR_FROM_SECURITY +VIR_LOG_INIT("security.security_stack"); typedef struct _virSecurityStackData virSecurityStackData; typedef virSecurityStackData *virSecurityStackDataPtr; @@ -145,14 +147,18 @@ virSecurityStackTransactionStart(virSecurityManagerPtr mgr) { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerTransactionStart(item->securityManager) < 0) - rc = -1; + goto rollback; } - return rc; + return 0; + + rollback: + for (item = item->prev; item; item = item->prev) + virSecurityManagerTransactionAbort(item->securityManager); + return -1; } @@ -163,14 +169,18 @@ virSecurityStackTransactionCommit(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerTransactionCommit(item->securityManager, pid, lock) < 0) - rc = -1; + goto rollback; } - return rc; + return 0; + + rollback: + for (item = item->prev; item; item = item->prev) + virSecurityManagerTransactionAbort(item->securityManager); + return -1; } @@ -278,17 +288,31 @@ virSecurityStackSetHostdevLabel(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerSetHostdevLabel(item->securityManager, vm, dev, vroot) < 0) - rc = -1; + goto rollback; } - return rc; + return 0; + + rollback: + for (item = item->prev; item; item = item->prev) { + if (virSecurityManagerRestoreHostdevLabel(item->securityManager, + vm, + dev, + vroot) < 0) { + VIR_WARN("Unable to restore hostdev label after failed set label " + "call virDriver=%s driver=%s domain=%s hostdev=%p", + virSecurityManagerGetVirtDriver(mgr), + virSecurityManagerGetDriver(item->securityManager), + vm->name, dev); + } + } + return -1; } @@ -323,16 +347,30 @@ virSecurityStackSetAllLabel(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerSetAllLabel(item->securityManager, vm, stdin_path, chardevStdioLogd, migrated) < 0) - rc = -1; + goto rollback; } - return rc; + return 0; + + rollback: + for (item = item->prev; item; item = item->prev) { + if (virSecurityManagerRestoreAllLabel(item->securityManager, + vm, + migrated, + chardevStdioLogd) < 0) { + VIR_WARN("Unable to restore all labels after failed set label call " + "virDriver=%s driver=%s domain=%s migrated=%d", + virSecurityManagerGetVirtDriver(mgr), + virSecurityManagerGetDriver(item->securityManager), + vm->name, migrated); + } + } + return -1; } @@ -363,14 +401,27 @@ virSecurityStackSetSavedStateLabel(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerSetSavedStateLabel(item->securityManager, vm, savefile) < 0) - rc = -1; + goto rollback; } - return rc; + return 0; + + rollback: + for (item = item->prev; item; item = item->prev) { + if (virSecurityManagerRestoreSavedStateLabel(item->securityManager, + vm, + savefile) < 0) { + VIR_WARN("Unable to restore saved state label after failed set " + "label call virDriver=%s driver=%s savefile=%s", + virSecurityManagerGetVirtDriver(mgr), + virSecurityManagerGetDriver(item->securityManager), + savefile); + } + } + return -1; } @@ -451,14 +502,25 @@ virSecurityStackSetDaemonSocketLabel(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerSetDaemonSocketLabel(item->securityManager, vm) < 0) - rc = -1; + goto rollback; } - return rc; + return 0; + rollback: + for (item = item->prev; item; item = item->prev) { + if (virSecurityManagerClearSocketLabel(item->securityManager, + vm) < 0) { + VIR_WARN("Unable to clear new daemon socket label after failed " + "set label call virDriver=%s driver=%s domain=%s", + virSecurityManagerGetVirtDriver(mgr), + virSecurityManagerGetDriver(item->securityManager), + vm->name); + } + } + return -1; } @@ -468,14 +530,25 @@ virSecurityStackSetSocketLabel(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerSetSocketLabel(item->securityManager, vm) < 0) - rc = -1; + goto rollback; } - return rc; + return 0; + rollback: + for (item = item->prev; item; item = item->prev) { + if (virSecurityManagerClearSocketLabel(item->securityManager, + vm) < 0) { + VIR_WARN("Unable to clear new socket label after failed " + "set label call virDriver=%s driver=%s domain=%s", + virSecurityManagerGetVirtDriver(mgr), + virSecurityManagerGetDriver(item->securityManager), + vm->name); + } + } + return -1; } @@ -573,15 +646,30 @@ virSecurityStackSetImageLabel(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerSetImageLabel(item->securityManager, vm, src, flags) < 0) - rc = -1; + goto rollback; } - return rc; + return 0; + + rollback: + for (item = item->prev; item; item = item->prev) { + if (virSecurityManagerRestoreImageLabel(item->securityManager, + vm, + src, + flags) < 0) { + VIR_WARN("Unable to restore image label after failed set label " + "call virDriver=%s driver=%s domain=%s src=%p (path=%s) " + "flags=0x%x", + virSecurityManagerGetVirtDriver(mgr), + virSecurityManagerGetDriver(item->securityManager), + vm->name, src, NULLSTR(src->path), flags); + } + } + return -1; } static int @@ -629,14 +717,27 @@ virSecurityStackSetMemoryLabel(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerSetMemoryLabel(item->securityManager, vm, mem) < 0) - rc = -1; + goto rollback; } - return rc; + return 0; + + rollback: + for (item = item->prev; item; item = item->prev) { + if (virSecurityManagerRestoreMemoryLabel(item->securityManager, + vm, + mem) < 0) { + VIR_WARN("Unable to restore memory label after failed set label " + "call virDriver=%s driver=%s domain=%s mem=%p", + virSecurityManagerGetVirtDriver(mgr), + virSecurityManagerGetDriver(item->securityManager), + vm->name, mem); + } + } + return -1; } static int @@ -664,14 +765,27 @@ virSecurityStackSetInputLabel(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerSetInputLabel(item->securityManager, vm, input) < 0) - rc = -1; + goto rollback; } - return rc; + return 0; + + rollback: + for (item = item->prev; item; item = item->prev) { + if (virSecurityManagerRestoreInputLabel(item->securityManager, + vm, + input) < 0) { + VIR_WARN("Unable to restore input label after failed set label " + "call virDriver=%s driver=%s domain=%s input=%p", + virSecurityManagerGetVirtDriver(mgr), + virSecurityManagerGetDriver(item->securityManager), + vm->name, input); + } + } + return -1; } static int @@ -719,16 +833,30 @@ virSecurityStackDomainSetChardevLabel(virSecurityManagerPtr mgr, { 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; + goto rollback; } - return rc; + return 0; + + rollback: + for (item = item->prev; item; item = item->prev) { + if (virSecurityManagerRestoreChardevLabel(item->securityManager, + def, + dev_source, + chardevStdioLogd) < 0) { + VIR_WARN("Unable to restore chardev label after failed set label " + "call virDriver=%s driver=%s domain=%s dev_source=%p", + virSecurityManagerGetVirtDriver(mgr), + virSecurityManagerGetDriver(item->securityManager), + def->name, dev_source); + } + } + return -1; } static int @@ -758,15 +886,27 @@ virSecurityStackSetTPMLabels(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerSetTPMLabels(item->securityManager, vm) < 0) - rc = -1; + goto rollback; } - return rc; + return 0; + + rollback: + for (item = item->prev; item; item = item->prev) { + if (virSecurityManagerRestoreTPMLabels(item->securityManager, + vm) < 0) { + VIR_WARN("Unable to restore TPM label after failed set label " + "call virDriver=%s driver=%s domain=%s", + virSecurityManagerGetVirtDriver(mgr), + virSecurityManagerGetDriver(item->securityManager), + vm->name); + } + } + return -1; } -- 2.21.0
On 9/16/19 5:12 AM, Michal Privoznik wrote:
See 5/5 for explanation.
Michal Prívozník (5): security: Pass @migrated to virSecurityManagerSetAllLabel security: Rename virSecurityManagerGetDriver() to virSecurityManagerGetVirtDriver() security: Introduce virSecurityManagerGetDriver() security_stack: Turn list of nested drivers into a doubly linked list security_stack: Perform rollback if one of stacked drivers fails
src/lxc/lxc_process.c | 2 +- src/qemu/qemu_process.c | 3 +- src/qemu/qemu_security.c | 6 +- src/qemu/qemu_security.h | 3 +- src/security/security_apparmor.c | 3 +- src/security/security_dac.c | 3 +- src/security/security_driver.h | 3 +- src/security/security_manager.c | 17 ++- src/security/security_manager.h | 4 +- src/security/security_nop.c | 3 +- src/security/security_selinux.c | 9 +- src/security/security_stack.c | 220 +++++++++++++++++++++++++------ tests/qemusecuritytest.c | 2 +- tests/securityselinuxlabeltest.c | 2 +- 14 files changed, 222 insertions(+), 58 deletions(-)
I gotta admit I'm seriously wondering if supporting this label remembering stuff is worth it. I know you've put a heroic amount of work into it over a long period of time, but I think it's worth taking another look at this whole thing end to end to decide whether it's worth the complexity for what we are actually getting The old RHEL bug that was tracking this is here: https://bugzilla.redhat.com/show_bug.cgi?id=547546 It's closed because it was against RHEL7 and these patches aren't going to hit RHEL7. Is there still a major product or project issue that this is solving? In that bug, I see that rjones (cc'd) said that libvirt not remembering labels/uid causes issues for libguestfs that requires workarounds. Rich, do you have links to threads or bug reports where this is described in more detail? From the end user distro perspective, the main place I have historically heard people complain about this is basically: * download $ISO to home, owned by uid=crobinso * point virt-manager at it, which uses qemu:///system * VM starts, $ISO chown'd to uid=qemu * VM stops, $ISO chown'd to uid=root * Now there's a root owned image in your homedir. Worse, if you have a /media directory somewhere shared over http or some other service, owned as a non-root user, then changing to root owner can disrupt that access. This issue definitely annoys users. Unfortunately remember_owner doesn't help here because it's limited to RW media, which generally is less often shared than things like ISOs. I'm interested in hearing other concrete usecases that are solved by remember_owner (or at one time we thought would be solved by this) (in the mean time I will review your patches tomorrow) Thanks, Cole
On Wed, Oct 09, 2019 at 07:49:29PM -0400, Cole Robinson wrote:
In that bug, I see that rjones (cc'd) said that libvirt not remembering labels/uid causes issues for libguestfs that requires workarounds. Rich, do you have links to threads or bug reports where this is described in more detail?
I think there are two problems (which I often confuse) and they are possibly related. This one where libvirt doesn't restore permissions afterwards, and the other one where qemu:///session cannot be used as root which implies that when you run libguestfs as root it doesn't have access to things that root would normally have access to (bug 890291 / 1045069). In answer to your question this is the only one I could find which is definitely related to this bug: https://www.redhat.com/archives/libguestfs/2013-May/msg00115.html Here's another one, but I think this is related to the other bug: https://bugs.launchpad.net/nova/+bug/1241659/comments/6 I suspect there are cases where openstack sets LIBGUESTFS_BACKEND=direct to workaround one of these two bugs. Is fixing the qemu:///session as root problem going to also solve this? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On Thu, Oct 10, 2019 at 11:29:17AM +0100, Richard W.M. Jones wrote:
On Wed, Oct 09, 2019 at 07:49:29PM -0400, Cole Robinson wrote:
In that bug, I see that rjones (cc'd) said that libvirt not remembering labels/uid causes issues for libguestfs that requires workarounds. Rich, do you have links to threads or bug reports where this is described in more detail?
I think there are two problems (which I often confuse) and they are possibly related. This one where libvirt doesn't restore permissions afterwards, and the other one where qemu:///session cannot be used as root which implies that when you run libguestfs as root it doesn't have access to things that root would normally have access to (bug 890291 / 1045069).
In answer to your question this is the only one I could find which is definitely related to this bug:
https://www.redhat.com/archives/libguestfs/2013-May/msg00115.html
Anything related to device nodes & permissions/ownership shouldn't be an issue any more. We switched to create a private mount namespace for each QEMU and setup a custom /dev populated with only the devices QEMU is allowed. Thus we should no longer be touching permisisons/owners in the real /dev
Here's another one, but I think this is related to the other bug:
https://bugs.launchpad.net/nova/+bug/1241659/comments/6
I suspect there are cases where openstack sets LIBGUESTFS_BACKEND=direct to workaround one of these two bugs.
Is fixing the qemu:///session as root problem going to also solve this?
If we had a real qemu:///session mode running QEMU itself as root, then we would never change permissions/ownership. We would still need to be changing SELinux labels & so the label restore logic is needd there. We should be able to use qemu:///system & the DAC driver to run QEMU as root though. There was previously a problem wrt monitor sockets that you hit when trying this with libguestfs, but I believe that should now be fixed: https://bugzilla.redhat.com/show_bug.cgi?id=890291#c30 If using the DAC driver to request running as root, the only remaining difference in terms of permissions is that we clear CAP_DAC_OVERRIDE, so the root user will only be able to access files which explicitly grant root access. We could fix this limitation in the DAC driver I believe to allow capabilities to be retained. 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 :|
On 9/16/19 5:12 AM, Michal Privoznik wrote:
See 5/5 for explanation.
Michal Prívozník (5): security: Pass @migrated to virSecurityManagerSetAllLabel security: Rename virSecurityManagerGetDriver() to virSecurityManagerGetVirtDriver() security: Introduce virSecurityManagerGetDriver() security_stack: Turn list of nested drivers into a doubly linked list security_stack: Perform rollback if one of stacked drivers fails
src/lxc/lxc_process.c | 2 +- src/qemu/qemu_process.c | 3 +- src/qemu/qemu_security.c | 6 +- src/qemu/qemu_security.h | 3 +- src/security/security_apparmor.c | 3 +- src/security/security_dac.c | 3 +- src/security/security_driver.h | 3 +- src/security/security_manager.c | 17 ++- src/security/security_manager.h | 4 +- src/security/security_nop.c | 3 +- src/security/security_selinux.c | 9 +- src/security/security_stack.c | 220 +++++++++++++++++++++++++------ tests/qemusecuritytest.c | 2 +- tests/securityselinuxlabeltest.c | 2 +- 14 files changed, 222 insertions(+), 58 deletions(-)
For the series: Reviewed-by: Cole Robinson <crobinso@redhat.com> - Cole
participants (4)
-
Cole Robinson -
Daniel P. Berrangé -
Michal Privoznik -
Richard W.M. Jones