From 9d03e9adf1776104290e7c7d3368e58998f27987 Mon Sep 17 00:00:00 2001 From: Michal Privoznik Date: Wed, 11 Sep 2019 11:19:06 +0200 Subject: [PATCH] security_stack: Perform rollback if one of stacked drivers fails 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. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1740024 Signed-off-by: Michal Privoznik Reviewed-by: Cole Robinson --- 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; } -- GitLab