[libvirt] [PATCH v3 00/18] Implement original label remembering
v3 of: https://www.redhat.com/archives/libvir-list/2018-November/msg01070.html diff to v2: - dropped 01/18 from v2 - Introduced a test - Couple of minor adjustments as suggested in review of v2 Michal Prívozník (18): util: Introduce xattr getter/setter/remover security: Include security_util security_dac: Restore label on failed chown() attempt virSecurityDACTransactionRun: Implement rollback virSecurityDACRestoreAllLabel: Reorder device relabeling virSecurityDACRestoreAllLabel: Restore more labels security_dac: Allow callers to enable/disable label remembering/recall security_dac: Remember old labels virSecurityDACRestoreImageLabelInt: Restore even shared/RO disks security_selinux: Track if transaction is restore security_selinux: Remember old labels security_selinux: Restore label on failed setfilecon() attempt virSecuritySELinuxTransactionRun: Implement rollback virSecuritySELinuxRestoreAllLabel: Reorder device relabeling virSecuritySELinuxRestoreAllLabel: Restore more labels tests: Introduce qemusecuritytest tools: Provide a script to recover fubar'ed XATTRs setup qemu.conf: Allow users to enable/disable label remembering cfg.mk | 4 +- src/libvirt_private.syms | 3 + src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 4 + src/qemu/qemu_conf.c | 4 + src/qemu/test_libvirtd_qemu.aug.in | 1 + src/security/Makefile.inc.am | 2 + src/security/security_dac.c | 227 ++++++++++---- src/security/security_selinux.c | 272 ++++++++++++---- src/security/security_util.c | 256 +++++++++++++++ src/security/security_util.h | 32 ++ src/util/virfile.c | 121 ++++++++ src/util/virfile.h | 20 +- tests/Makefile.am | 10 + tests/qemusecuritymock.c | 480 +++++++++++++++++++++++++++++ tests/qemusecuritytest.c | 173 +++++++++++ tests/qemusecuritytest.h | 28 ++ tools/Makefile.am | 1 + tools/libvirt_recover_xattrs.sh | 96 ++++++ 19 files changed, 1600 insertions(+), 135 deletions(-) create mode 100644 src/security/security_util.c create mode 100644 src/security/security_util.h create mode 100644 tests/qemusecuritymock.c create mode 100644 tests/qemusecuritytest.c create mode 100644 tests/qemusecuritytest.h create mode 100755 tools/libvirt_recover_xattrs.sh -- 2.19.2
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 3 + src/util/virfile.c | 121 +++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 11 ++++ 3 files changed, 135 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fd63c9ca61..9835e9a56c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1830,6 +1830,7 @@ virFileGetACLs; virFileGetHugepageSize; virFileGetMountReverseSubtree; virFileGetMountSubtree; +virFileGetXAttr; virFileHasSuffix; virFileInData; virFileIsAbsPath; @@ -1869,6 +1870,7 @@ virFileReadValueUint; virFileRelLinkPointsTo; virFileRemove; virFileRemoveLastComponent; +virFileRemoveXAttr; virFileResolveAllLinks; virFileResolveLink; virFileRewrite; @@ -1876,6 +1878,7 @@ virFileRewriteStr; virFileSanitizePath; virFileSetACLs; virFileSetupDev; +virFileSetXAttr; virFileSkipRoot; virFileStripSuffix; virFileTouch; diff --git a/src/util/virfile.c b/src/util/virfile.c index f6f9e4ceda..263c92667c 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -64,6 +64,10 @@ # include <linux/cdrom.h> #endif +#if HAVE_LIBATTR +# include <sys/xattr.h> +#endif + #include "configmake.h" #include "intprops.h" #include "vircommand.h" @@ -4354,3 +4358,120 @@ virFileWaitForExists(const char *path, return 0; } + + +#if HAVE_LIBATTR +/** + * virFileGetXAttr; + * @path: a filename + * @name: name of xattr + * @value: read value + * + * Reads xattr with @name for given @path and stores it into + * @value. Caller is responsible for freeing @value. + * + * Returns: 0 on success, + * -1 otherwise (with errno set). + */ +int +virFileGetXAttr(const char *path, + const char *name, + char **value) +{ + char *buf = NULL; + int ret = -1; + + /* We might be racing with somebody who sets the same attribute. */ + while (1) { + ssize_t need; + ssize_t got; + + /* The first call determines how many bytes we need to allocate. */ + if ((need = getxattr(path, name, NULL, 0)) < 0) + goto cleanup; + + if (VIR_REALLOC_N_QUIET(buf, need + 1) < 0) + goto cleanup; + + if ((got = getxattr(path, name, buf, need)) < 0) { + if (errno == ERANGE) + continue; + goto cleanup; + } + + buf[got] = '\0'; + break; + } + + VIR_STEAL_PTR(*value, buf); + ret = 0; + cleanup: + VIR_FREE(buf); + return ret; +} + +/** + * virFileSetXAttr: + * @path: a filename + * @name: name of xattr + * @value: value to set + * + * Sets xattr of @name and @value on @path. + * + * Returns: 0 on success, + * -1 otherwise (with errno set). + */ +int +virFileSetXAttr(const char *path, + const char *name, + const char *value) +{ + return setxattr(path, name, value, strlen(value), 0); +} + +/** + * virFileRemoveXAttr: + * @path: a filename + * @name: name of xattr + * + * Remove xattr of @name on @path. + * + * Returns: 0 on success, + * -1 otherwise (with errno set). + */ +int +virFileRemoveXAttr(const char *path, + const char *name) +{ + return removexattr(path, name); +} + +#else /* !HAVE_LIBATTR */ + +int +virFileGetXAttr(const char *path ATTRIBUTE_UNUSED, + const char *name ATTRIBUTE_UNUSED, + char **value ATTRIBUTE_UNUSED) +{ + errno = ENOSYS; + return -1; +} + +int +virFileSetXAttr(const char *path ATTRIBUTE_UNUSED, + const char *name ATTRIBUTE_UNUSED, + const char *value ATTRIBUTE_UNUSED) +{ + errno = ENOSYS; + return -1; +} + +int +virFileRemoveXAttr(const char *path ATTRIBUTE_UNUSED, + const char *name ATTRIBUTE_UNUSED) +{ + errno = ENOSYS; + return -1; +} + +#endif /* HAVE_LIBATTR */ diff --git a/src/util/virfile.h b/src/util/virfile.h index 0f7dece958..aa4c29dc40 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -383,4 +383,15 @@ int virFileInData(int fd, VIR_DEFINE_AUTOPTR_FUNC(virFileWrapperFd, virFileWrapperFdFree) +int virFileGetXAttr(const char *path, + const char *name, + char **value); + +int virFileSetXAttr(const char *path, + const char *name, + const char *value); + +int virFileRemoveXAttr(const char *path, + const char *name); + #endif /* __VIR_FILE_H */ -- 2.19.2
This file implements wrappers over XATTR getter/setter. It ensures the proper XATTR namespace is used. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/Makefile.inc.am | 2 + src/security/security_util.c | 256 +++++++++++++++++++++++++++++++++++ src/security/security_util.h | 32 +++++ 3 files changed, 290 insertions(+) create mode 100644 src/security/security_util.c create mode 100644 src/security/security_util.h diff --git a/src/security/Makefile.inc.am b/src/security/Makefile.inc.am index f88b82df7b..0ade97d355 100644 --- a/src/security/Makefile.inc.am +++ b/src/security/Makefile.inc.am @@ -14,6 +14,8 @@ SECURITY_DRIVER_SOURCES = \ security/security_dac.c \ security/security_manager.h \ security/security_manager.c \ + security/security_util.h \ + security/security_util.c \ $(NULL) SECURITY_DRIVER_SELINUX_SOURCES = \ diff --git a/src/security/security_util.c b/src/security/security_util.c new file mode 100644 index 0000000000..194343c407 --- /dev/null +++ b/src/security/security_util.c @@ -0,0 +1,256 @@ +/* + * Copyright (C) 2018 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include "viralloc.h" +#include "virfile.h" +#include "virstring.h" +#include "virerror.h" + +#include "security_util.h" + +#define VIR_FROM_THIS VIR_FROM_SECURITY + +/* There are four namespaces available on Linux (xattr(7)): + * + * user - can be modified by anybody, + * system - used by ACLs + * security - used by SELinux + * trusted - accessibly by CAP_SYS_ADMIN processes only + * + * Looks like the last one is way to go. + * Unfortunately, FreeBSD only supports: + * + * user - can be modified by anybody, + * system - accessible by CAP_SYS_ADMIN processes only + * + * Note that 'system' on FreeBSD corresponds to 'trusted' on + * Linux. So far the only point where FreeBSD and Linux can meet + * is NFS which still doesn't support XATTRs. Therefore we can + * use different namespace on each system. If NFS gains support + * for XATTRs then we have to find a way to deal with the + * different namespaces. But that is a problem for future me. + */ +#if defined(__linux__) +# define XATTR_NAMESPACE "trusted" +#elif defined(__FreeBSD__) +# define XATTR_NAMESPACE "system" +#endif + +static char * +virSecurityGetAttrName(const char *name ATTRIBUTE_UNUSED) +{ + char *ret = NULL; +#ifdef XATTR_NAMESPACE + ignore_value(virAsprintf(&ret, XATTR_NAMESPACE".libvirt.security.%s", name)); +#else + errno = ENOSYS; + virReportSystemError(errno, "%s", + _("Extended attributes are not supported on this system")); +#endif + return ret; +} + + +static char * +virSecurityGetRefCountAttrName(const char *name ATTRIBUTE_UNUSED) +{ + char *ret = NULL; +#ifdef XATTR_NAMESPACE + ignore_value(virAsprintf(&ret, XATTR_NAMESPACE".libvirt.security.ref_%s", name)); +#else + errno = ENOSYS; + virReportSystemError(errno, "%s", + _("Extended attributes are not supported on this system")); +#endif + return ret; +} + + +/** + * virSecurityGetRememberedLabel: + * @name: security driver name + * @path: file name + * @label: label + * + * For given @path and security driver (@name) fetch remembered + * @label. The caller must not restore label if an error is + * indicated or if @label is NULL upon return. + * + * The idea is that the first time + * virSecuritySetRememberedLabel() is called over @path the + * @label is recorded and refcounter is set to 1. Each subsequent + * call to virSecuritySetRememberedLabel() increases the counter. + * Counterpart to this is virSecurityGetRememberedLabel() which + * decreases the counter and reads the @label only if the counter + * reached value of zero. For any other call (i.e. when the + * counter is not zero), virSecurityGetRememberedLabel() set + * @label to NULL (to notify the caller that the refcount is not + * zero) and returns zero. + * + * Returns: 0 on success, + * -1 otherwise (with error reported) + */ +int +virSecurityGetRememberedLabel(const char *name, + const char *path, + char **label) +{ + char *ref_name = NULL; + char *attr_name = NULL; + char *value = NULL; + unsigned int refcount = 0; + int ret = -1; + + *label = NULL; + + if (!(ref_name = virSecurityGetRefCountAttrName(name))) + goto cleanup; + + if (virFileGetXAttr(path, ref_name, &value) < 0) { + if (errno == ENOSYS || errno == ENODATA || errno == ENOTSUP) { + ret = 0; + } else { + virReportSystemError(errno, + _("Unable to get XATTR %s on %s"), + ref_name, + path); + } + goto cleanup; + } + + if (virStrToLong_ui(value, NULL, 10, &refcount) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("malformed refcount %s on %s"), + value, path); + goto cleanup; + } + + VIR_FREE(value); + + refcount--; + + if (refcount > 0) { + if (virAsprintf(&value, "%u", refcount) < 0) + goto cleanup; + + if (virFileSetXAttr(path, ref_name, value) < 0) + goto cleanup; + } else { + if (virFileRemoveXAttr(path, ref_name) < 0) + goto cleanup; + + if (!(attr_name = virSecurityGetAttrName(name))) + goto cleanup; + + if (virFileGetXAttr(path, attr_name, label) < 0) + goto cleanup; + + if (virFileRemoveXAttr(path, attr_name) < 0) + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(value); + VIR_FREE(attr_name); + VIR_FREE(ref_name); + return ret; +} + + +/** + * virSecuritySetRememberedLabel: + * @name: security driver name + * @path: file name + * @label: label + * + * For given @path and security driver (@name), if called the + * first time over @path, set the @label to remember (i.e. the + * original owner of the @path). Any subsequent call over @path + * will increment refcounter. It is strongly recommended that the + * caller checks for the return value and if it is greater than 1 + * (meaning that some domain is already using @path) the current + * label is required instead of setting a new one. + * + * See also virSecurityGetRememberedLabel. + * + * Returns: the new refcount value on success, + * -1 otherwise (with error reported) + */ +int +virSecuritySetRememberedLabel(const char *name, + const char *path, + const char *label) +{ + char *ref_name = NULL; + char *attr_name = NULL; + char *value = NULL; + unsigned int refcount = 0; + int ret = -1; + + if (!(ref_name = virSecurityGetRefCountAttrName(name))) + goto cleanup; + + if (virFileGetXAttr(path, ref_name, &value) < 0) { + if (errno == ENOSYS || errno == ENOTSUP) { + ret = 0; + goto cleanup; + } else if (errno != ENODATA) { + virReportSystemError(errno, + _("Unable to get XATTR %s on %s"), + ref_name, + path); + goto cleanup; + } + } + + if (value && + virStrToLong_ui(value, NULL, 10, &refcount) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("malformed refcount %s on %s"), + value, path); + goto cleanup; + } + + VIR_FREE(value); + + refcount++; + + if (refcount == 1) { + if (!(attr_name = virSecurityGetAttrName(name))) + goto cleanup; + + if (virFileSetXAttr(path, attr_name, label) < 0) + goto cleanup; + } + + if (virAsprintf(&value, "%u", refcount) < 0) + goto cleanup; + + if (virFileSetXAttr(path, ref_name, value) < 0) + goto cleanup; + + ret = refcount; + cleanup: + VIR_FREE(value); + VIR_FREE(attr_name); + VIR_FREE(ref_name); + return ret; +} diff --git a/src/security/security_util.h b/src/security/security_util.h new file mode 100644 index 0000000000..a6e67f4390 --- /dev/null +++ b/src/security/security_util.h @@ -0,0 +1,32 @@ +/* + * Copyright (C) 2018 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#ifndef __SECURITY_UTIL_H__ +# define __SECURITY_UTIL_H__ + +int +virSecurityGetRememberedLabel(const char *name, + const char *path, + char **label); + +int +virSecuritySetRememberedLabel(const char *name, + const char *path, + const char *label); + +#endif /* __SECURITY_UTIL_H__ */ -- 2.19.2
On Wed, Dec 12, 2018 at 01:40:46PM +0100, Michal Privoznik wrote:
This file implements wrappers over XATTR getter/setter. It ensures the proper XATTR namespace is used.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/Makefile.inc.am | 2 + src/security/security_util.c | 256 +++++++++++++++++++++++++++++++++++ src/security/security_util.h | 32 +++++ 3 files changed, 290 insertions(+) create mode 100644 src/security/security_util.c create mode 100644 src/security/security_util.h
+/** + * virSecurityGetRememberedLabel: + * @name: security driver name + * @path: file name + * @label: label + * + * For given @path and security driver (@name) fetch remembered + * @label. The caller must not restore label if an error is + * indicated or if @label is NULL upon return. + * + * The idea is that the first time + * virSecuritySetRememberedLabel() is called over @path the + * @label is recorded and refcounter is set to 1. Each subsequent + * call to virSecuritySetRememberedLabel() increases the counter. + * Counterpart to this is virSecurityGetRememberedLabel() which + * decreases the counter and reads the @label only if the counter + * reached value of zero. For any other call (i.e. when the + * counter is not zero), virSecurityGetRememberedLabel() set
s/set/sets/
+ * @label to NULL (to notify the caller that the refcount is not + * zero) and returns zero. + * + * Returns: 0 on success, + * -1 otherwise (with error reported) + */
and of course: sed -i s/__SECURITY_UTIL_H__/LIBVIRT_SECURITY_UTIL_H/g src/security/security_util.h Jano
It's important to keep XATTRs untouched (well, in the same state they were in when entering the function). Otherwise our refcounting would be messed up. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- src/security/security_dac.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4bdc6ed213..f01d0b4732 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -718,7 +718,25 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", NULLSTR(src ? src->path : path), (long)uid, (long)gid); - return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid); + if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0) { + virErrorPtr origerr; + + virErrorPreserveLast(&origerr); + /* Try to restore the label. This is done so that XATTRs + * are left in the same state as when the control entered + * this function. However, if our attempt fails, there's + * not much we can do. XATTRs refcounting is fubar'ed and + * the only option we have is warn users. */ + if (virSecurityDACRestoreFileLabelInternal(mgr, src, path) < 0) + VIR_WARN("Unable to restore label on '%s'. " + "XATTRs might have been left in inconsistent state.", + NULLSTR(src ? src->path : path)); + + virErrorRestore(&origerr); + return -1; + } + + return 0; } -- 2.19.2
When iterating over list of paths/disk sources to relabel it may happen that the process fails at some point. In that case, for the sake of keeping seclabel refcount (stored in XATTRs) in sync with reality we have to perform rollback. However, if that fails too the only thing we can do is warn user. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- src/security/security_dac.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index f01d0b4732..7be555903d 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -229,7 +229,6 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, for (i = 0; i < list->nItems; i++) { virSecurityDACChownItemPtr item = list->items[i]; - /* TODO Implement rollback */ if (!item->restore) { rv = virSecurityDACSetOwnership(list->manager, item->src, @@ -246,6 +245,19 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, break; } + for (; rv < 0 && i > 0; i--) { + virSecurityDACChownItemPtr item = list->items[i - 1]; + + if (!item->restore) { + virSecurityDACRestoreFileLabelInternal(list->manager, + item->src, + item->path); + } else { + VIR_WARN("Ignoring failed restore attempt on %s", + NULLSTR(item->src ? item->src->path : item->path)); + } + } + if (list->lock) virSecurityManagerMetadataUnlock(list->manager, &state); -- 2.19.2
It helps whe trying to match calls with virSecurityDACSetAllLabel if the order in which devices are set/restored is the same in both functions. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- src/security/security_dac.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 7be555903d..4935c962b9 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1664,24 +1664,6 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr, VIR_DEBUG("Restoring security label on %s migrated=%d", def->name, migrated); - for (i = 0; i < def->nhostdevs; i++) { - if (virSecurityDACRestoreHostdevLabel(mgr, - def, - def->hostdevs[i], - NULL) < 0) - rc = -1; - } - - for (i = 0; i < def->ngraphics; i++) { - if (virSecurityDACRestoreGraphicsLabel(mgr, def, def->graphics[i]) < 0) - return -1; - } - - for (i = 0; i < def->ninputs; i++) { - if (virSecurityDACRestoreInputLabel(mgr, def, def->inputs[i]) < 0) - rc = -1; - } - for (i = 0; i < def->ndisks; i++) { if (virSecurityDACRestoreImageLabelInt(mgr, def, @@ -1690,6 +1672,24 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr, rc = -1; } + for (i = 0; i < def->ngraphics; i++) { + if (virSecurityDACRestoreGraphicsLabel(mgr, def, def->graphics[i]) < 0) + return -1; + } + + for (i = 0; i < def->ninputs; i++) { + if (virSecurityDACRestoreInputLabel(mgr, def, def->inputs[i]) < 0) + rc = -1; + } + + for (i = 0; i < def->nhostdevs; i++) { + if (virSecurityDACRestoreHostdevLabel(mgr, + def, + def->hostdevs[i], + NULL) < 0) + rc = -1; + } + for (i = 0; i < def->nmems; i++) { if (virSecurityDACRestoreMemoryLabel(mgr, def, -- 2.19.2
We are setting label on kernel, initrd, dtb and slic_table files. But we never restored it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- src/security/security_dac.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4935c962b9..dcd0bb558a 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1719,6 +1719,22 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr, virSecurityDACRestoreFileLabel(mgr, def->os.loader->nvram) < 0) rc = -1; + if (def->os.kernel && + virSecurityDACRestoreFileLabel(mgr, def->os.kernel) < 0) + rc = -1; + + if (def->os.initrd && + virSecurityDACRestoreFileLabel(mgr, def->os.initrd) < 0) + rc = -1; + + if (def->os.dtb && + virSecurityDACRestoreFileLabel(mgr, def->os.dtb) < 0) + rc = -1; + + if (def->os.slic_table && + virSecurityDACRestoreFileLabel(mgr, def->os.slic_table) < 0) + rc = -1; + return rc; } -- 2.19.2
Because the implementation that will be used for label remembering/recall is not atomic we have to give callers a chance to enable or disable it. That is, enable it if and only if metadata locking is enabled. Otherwise the feature MUST be turned off. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- src/security/security_dac.c | 74 ++++++++++++++++++++++--------------- 1 file changed, 45 insertions(+), 29 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index dcd0bb558a..e5899c1746 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -182,11 +182,13 @@ static int virSecurityDACSetOwnership(virSecurityManagerPtr mgr, const virStorageSource *src, const char *path, uid_t uid, - gid_t gid); + gid_t gid, + bool remember); static int virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr, const virStorageSource *src, - const char *path); + const char *path, + bool recall); /** * virSecurityDACTransactionRun: * @pid: process pid @@ -234,11 +236,13 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, item->src, item->path, item->uid, - item->gid); + item->gid, + list->lock); } else { rv = virSecurityDACRestoreFileLabelInternal(list->manager, item->src, - item->path); + item->path, + list->lock); } if (rv < 0) @@ -251,7 +255,8 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, if (!item->restore) { virSecurityDACRestoreFileLabelInternal(list->manager, item->src, - item->path); + item->path, + list->lock); } else { VIR_WARN("Ignoring failed restore attempt on %s", NULLSTR(item->src ? item->src->path : item->path)); @@ -699,7 +704,8 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, const virStorageSource *src, const char *path, uid_t uid, - gid_t gid) + gid_t gid, + bool remember) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); struct stat sb; @@ -717,7 +723,7 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, else if (rc > 0) return 0; - if (path) { + if (remember && path) { if (stat(path, &sb) < 0) { virReportSystemError(errno, _("unable to stat: %s"), path); return -1; @@ -739,7 +745,7 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, * this function. However, if our attempt fails, there's * not much we can do. XATTRs refcounting is fubar'ed and * the only option we have is warn users. */ - if (virSecurityDACRestoreFileLabelInternal(mgr, src, path) < 0) + if (virSecurityDACRestoreFileLabelInternal(mgr, src, path, remember) < 0) VIR_WARN("Unable to restore label on '%s'. " "XATTRs might have been left in inconsistent state.", NULLSTR(src ? src->path : path)); @@ -755,7 +761,8 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, static int virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr, const virStorageSource *src, - const char *path) + const char *path, + bool recall) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); int rv; @@ -774,7 +781,7 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr, else if (rv > 0) return 0; - if (path) { + if (recall && path) { rv = virSecurityDACRecallLabel(priv, path, &uid, &gid); if (rv < 0) return -1; @@ -793,7 +800,7 @@ static int virSecurityDACRestoreFileLabel(virSecurityManagerPtr mgr, const char *path) { - return virSecurityDACRestoreFileLabelInternal(mgr, NULL, path); + return virSecurityDACRestoreFileLabelInternal(mgr, NULL, path, false); } @@ -840,7 +847,7 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr, return -1; } - return virSecurityDACSetOwnership(mgr, src, NULL, user, group); + return virSecurityDACSetOwnership(mgr, src, NULL, user, group, false); } @@ -920,7 +927,7 @@ virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr, } } - return virSecurityDACRestoreFileLabelInternal(mgr, src, NULL); + return virSecurityDACRestoreFileLabelInternal(mgr, src, NULL, false); } @@ -956,7 +963,7 @@ virSecurityDACSetHostdevLabelHelper(const char *file, if (virSecurityDACGetIds(secdef, priv, &user, &group, NULL, NULL) < 0) return -1; - return virSecurityDACSetOwnership(mgr, NULL, file, user, group); + return virSecurityDACSetOwnership(mgr, NULL, file, user, group, false); } @@ -1332,7 +1339,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_CHR_TYPE_FILE: ret = virSecurityDACSetOwnership(mgr, NULL, dev_source->data.file.path, - user, group); + user, group, false); break; case VIR_DOMAIN_CHR_TYPE_PIPE: @@ -1340,12 +1347,12 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, virAsprintf(&out, "%s.out", dev_source->data.file.path) < 0) goto done; if (virFileExists(in) && virFileExists(out)) { - if (virSecurityDACSetOwnership(mgr, NULL, in, user, group) < 0 || - virSecurityDACSetOwnership(mgr, NULL, out, user, group) < 0) + if (virSecurityDACSetOwnership(mgr, NULL, in, user, group, false) < 0 || + virSecurityDACSetOwnership(mgr, NULL, out, user, group, false) < 0) goto done; } else if (virSecurityDACSetOwnership(mgr, NULL, dev_source->data.file.path, - user, group) < 0) { + user, group, false) < 0) { goto done; } ret = 0; @@ -1360,7 +1367,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, * and passed via FD */ if (virSecurityDACSetOwnership(mgr, NULL, dev_source->data.nix.path, - user, group) < 0) + user, group, false) < 0) goto done; } ret = 0; @@ -1543,7 +1550,7 @@ virSecurityDACSetGraphicsLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0) return -1; - if (virSecurityDACSetOwnership(mgr, NULL, rendernode, user, group) < 0) + if (virSecurityDACSetOwnership(mgr, NULL, rendernode, user, group, false) < 0) return -1; return 0; @@ -1584,7 +1591,9 @@ virSecurityDACSetInputLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0) return -1; - ret = virSecurityDACSetOwnership(mgr, NULL, input->source.evdev, user, group); + ret = virSecurityDACSetOwnership(mgr, NULL, + input->source.evdev, + user, group, false); break; case VIR_DOMAIN_INPUT_TYPE_MOUSE: @@ -1772,7 +1781,9 @@ virSecurityDACSetMemoryLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0) return -1; - ret = virSecurityDACSetOwnership(mgr, NULL, mem->nvdimmPath, user, group); + ret = virSecurityDACSetOwnership(mgr, NULL, + mem->nvdimmPath, + user, group, false); break; case VIR_DOMAIN_MEMORY_MODEL_DIMM: @@ -1861,27 +1872,32 @@ virSecurityDACSetAllLabel(virSecurityManagerPtr mgr, if (def->os.loader && def->os.loader->nvram && virSecurityDACSetOwnership(mgr, NULL, - def->os.loader->nvram, user, group) < 0) + def->os.loader->nvram, + user, group, false) < 0) return -1; if (def->os.kernel && virSecurityDACSetOwnership(mgr, NULL, - def->os.kernel, user, group) < 0) + def->os.kernel, + user, group, false) < 0) return -1; if (def->os.initrd && virSecurityDACSetOwnership(mgr, NULL, - def->os.initrd, user, group) < 0) + def->os.initrd, + user, group, false) < 0) return -1; if (def->os.dtb && virSecurityDACSetOwnership(mgr, NULL, - def->os.dtb, user, group) < 0) + def->os.dtb, + user, group, false) < 0) return -1; if (def->os.slic_table && virSecurityDACSetOwnership(mgr, NULL, - def->os.slic_table, user, group) < 0) + def->os.slic_table, + user, group, false) < 0) return -1; return 0; @@ -1903,7 +1919,7 @@ virSecurityDACSetSavedStateLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetImageIds(secdef, priv, &user, &group) < 0) return -1; - return virSecurityDACSetOwnership(mgr, NULL, savefile, user, group); + return virSecurityDACSetOwnership(mgr, NULL, savefile, user, group, false); } @@ -2223,7 +2239,7 @@ virSecurityDACDomainSetPathLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0) return -1; - return virSecurityDACSetOwnership(mgr, NULL, path, user, group); + return virSecurityDACSetOwnership(mgr, NULL, path, user, group, false); } virSecurityDriver virSecurityDriverDAC = { -- 2.19.2
This also requires the same DAC label to be used for shared paths. If a path is already in use by a domain (or domains) then and the domain we are starting now wants to access the path it has to have the same DAC label. This might look too restrictive as the new label can still guarantee access to already running domains but in reality it is very unlikely and usually an admin mistake. This requirement also simplifies seclabel remembering, because we can store only one seclabel and have a refcounter for how many times the path is in use. If we were to allow different labels and store them in some sort of array the algorithm to match labels to domains would be needlessly complicated. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 63 +++++++++++++++++++++++++++++++------ 1 file changed, 53 insertions(+), 10 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index e5899c1746..3264a2967c 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -29,6 +29,7 @@ #endif #include "security_dac.h" +#include "security_util.h" #include "virerror.h" #include "virfile.h" #include "viralloc.h" @@ -411,15 +412,26 @@ virSecurityDACGetImageIds(virSecurityLabelDefPtr seclabel, * * Remember the owner of @path (represented by @uid:@gid). * - * Returns: 0 on success, -1 on failure + * Returns: the @path refcount, or + * -1 on failure */ static int virSecurityDACRememberLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED, - const char *path ATTRIBUTE_UNUSED, - uid_t uid ATTRIBUTE_UNUSED, - gid_t gid ATTRIBUTE_UNUSED) + const char *path, + uid_t uid, + gid_t gid) { - return 0; + char *label = NULL; + int ret = -1; + + if (virAsprintf(&label, "+%u:+%u", + (unsigned int)uid, + (unsigned int)gid) < 0) + return -1; + + ret = virSecuritySetRememberedLabel(SECURITY_DAC_NAME, path, label); + VIR_FREE(label); + return ret; } /** @@ -439,11 +451,27 @@ virSecurityDACRememberLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED, */ static int virSecurityDACRecallLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED, - const char *path ATTRIBUTE_UNUSED, - uid_t *uid ATTRIBUTE_UNUSED, - gid_t *gid ATTRIBUTE_UNUSED) + const char *path, + uid_t *uid, + gid_t *gid) { - return 0; + char *label; + int ret = -1; + + if (virSecurityGetRememberedLabel(SECURITY_DAC_NAME, + path, &label) < 0) + goto cleanup; + + if (!label) + return 1; + + if (virParseOwnershipIds(label, uid, gid) < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(label); + return ret; } static virSecurityDriverStatus @@ -709,6 +737,7 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); struct stat sb; + int refcount; int rc; if (!path && src && src->path && @@ -729,8 +758,22 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, return -1; } - if (virSecurityDACRememberLabel(priv, path, sb.st_uid, sb.st_gid) < 0) + refcount = virSecurityDACRememberLabel(priv, path, sb.st_uid, sb.st_gid); + if (refcount < 0) { return -1; + } else if (refcount > 1) { + /* Refcount is greater than 1 which means that there + * is @refcount domains using the @path. Do not + * change the label (as it would almost certainly + * cause the other domains to lose access to the + * @path). */ + if (sb.st_uid != uid || sb.st_gid != gid) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Setting different DAC user or group on %s " + "which is already in use"), path); + return -1; + } + } } VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", -- 2.19.2
Now that we have seclabel remembering we can safely restore labels for shared and RO disks. In fact we need to do that to keep seclabel refcount stored in XATTRs in sync with reality. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- src/security/security_dac.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 3264a2967c..533d990de1 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -932,14 +932,6 @@ virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr, if (!priv->dynamicOwnership) return 0; - /* Don't restore labels on readoly/shared disks, because other VMs may - * still be accessing these. Alternatively we could iterate over all - * running domains and try to figure out if it is in use, but this would - * not work for clustered filesystems, since we can't see running VMs using - * the file on other nodes. Safest bet is thus to skip the restore step. */ - if (src->readonly || src->shared) - return 0; - secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); if (secdef && !secdef->relabel) return 0; -- 2.19.2
It is going to be important to know if the current transaction we are running is a restore operation or set label operation so that we know whether to call virSecurityGetRememberedLabel() or virSecuritySetRememberedLabel(). That is, whether we are in a restore and therefore have to fetch the remembered label, or we are in set operation and therefore have to store the original label. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- src/security/security_selinux.c | 36 +++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 95e9a1b0c7..715d9a428b 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -85,6 +85,7 @@ struct _virSecuritySELinuxContextItem { char *path; char *tcon; bool optional; + bool restore; }; typedef struct _virSecuritySELinuxContextList virSecuritySELinuxContextList; @@ -123,7 +124,8 @@ static int virSecuritySELinuxContextListAppend(virSecuritySELinuxContextListPtr list, const char *path, const char *tcon, - bool optional) + bool optional, + bool restore) { int ret = -1; virSecuritySELinuxContextItemPtr item = NULL; @@ -135,6 +137,7 @@ virSecuritySELinuxContextListAppend(virSecuritySELinuxContextListPtr list, goto cleanup; item->optional = optional; + item->restore = restore; if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) goto cleanup; @@ -178,7 +181,8 @@ virSecuritySELinuxContextListFree(void *opaque) static int virSecuritySELinuxTransactionAppend(const char *path, const char *tcon, - bool optional) + bool optional, + bool restore) { virSecuritySELinuxContextListPtr list; @@ -186,7 +190,7 @@ virSecuritySELinuxTransactionAppend(const char *path, if (!list) return 0; - if (virSecuritySELinuxContextListAppend(list, path, tcon, optional) < 0) + if (virSecuritySELinuxContextListAppend(list, path, tcon, optional, restore) < 0) return -1; return 1; @@ -198,6 +202,11 @@ static int virSecuritySELinuxSetFileconHelper(const char *path, bool optional, bool privileged); + +static int virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, + const char *path); + + /** * virSecuritySELinuxTransactionRun: * @pid: process pid @@ -242,13 +251,18 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, virSecuritySELinuxContextItemPtr item = list->items[i]; /* TODO Implement rollback */ - if (virSecuritySELinuxSetFileconHelper(item->path, - item->tcon, - item->optional, - privileged) < 0) { - rv = -1; - break; + if (!item->restore) { + rv = virSecuritySELinuxSetFileconHelper(item->path, + item->tcon, + item->optional, + privileged); + } else { + rv = virSecuritySELinuxRestoreFileLabel(list->manager, + item->path); } + + if (rv < 0) + break; } if (list->lock) @@ -1265,7 +1279,7 @@ virSecuritySELinuxSetFileconHelper(const char *path, const char *tcon, { int rc; - if ((rc = virSecuritySELinuxTransactionAppend(path, tcon, optional)) < 0) + if ((rc = virSecuritySELinuxTransactionAppend(path, tcon, optional, false)) < 0) return -1; else if (rc > 0) return 0; @@ -1387,7 +1401,7 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, goto cleanup; } - if ((rc = virSecuritySELinuxTransactionAppend(path, fcon, false)) < 0) + if ((rc = virSecuritySELinuxTransactionAppend(path, fcon, false, true)) < 0) return -1; else if (rc > 0) return 0; -- 2.19.2
Similarly to what I did in DAC driver, this also requires the same SELinux label to be used for shared paths. If a path is already in use by a domain (or domains) then and the domain we are starting now wants to access the path it has to have the same SELinux label. This might look too restrictive as the new label can still guarantee access to already running domains but in reality it is very unlikely and usually an admin mistake. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_selinux.c | 177 +++++++++++++++++++++++--------- 1 file changed, 130 insertions(+), 47 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 715d9a428b..43d2ef32a5 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -33,6 +33,7 @@ #include "security_driver.h" #include "security_selinux.h" +#include "security_util.h" #include "virerror.h" #include "viralloc.h" #include "virlog.h" @@ -197,14 +198,40 @@ virSecuritySELinuxTransactionAppend(const char *path, } +static int +virSecuritySELinuxRememberLabel(const char *path, + const security_context_t con) +{ + return virSecuritySetRememberedLabel(SECURITY_SELINUX_NAME, + path, con); +} + + +static int +virSecuritySELinuxRecallLabel(const char *path, + security_context_t *con) +{ + if (virSecurityGetRememberedLabel(SECURITY_SELINUX_NAME, + path, con) < 0) + return -1; + + if (!con) + return 1; + + return 0; +} + + static int virSecuritySELinuxSetFileconHelper(const char *path, const char *tcon, bool optional, - bool privileged); + bool privileged, + bool remember); static int virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, - const char *path); + const char *path, + bool recall); /** @@ -255,10 +282,12 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, rv = virSecuritySELinuxSetFileconHelper(item->path, item->tcon, item->optional, - privileged); + privileged, + list->lock); } else { rv = virSecuritySELinuxRestoreFileLabel(list->manager, - item->path); + item->path, + list->lock); } if (rv < 0) @@ -1275,16 +1304,54 @@ virSecuritySELinuxSetFileconImpl(const char *path, const char *tcon, static int virSecuritySELinuxSetFileconHelper(const char *path, const char *tcon, - bool optional, bool privileged) + bool optional, bool privileged, bool remember) { + security_context_t econ = NULL; + int refcount; int rc; + int ret = -1; if ((rc = virSecuritySELinuxTransactionAppend(path, tcon, optional, false)) < 0) return -1; else if (rc > 0) return 0; - return virSecuritySELinuxSetFileconImpl(path, tcon, optional, privileged); + if (remember) { + if (getfilecon_raw(path, &econ) < 0 && + errno != ENOTSUP && errno != ENODATA) { + virReportSystemError(errno, + _("unable to get SELinux context of %s"), + path); + goto cleanup; + } + + if (econ) { + refcount = virSecuritySELinuxRememberLabel(path, econ); + if (refcount < 0) { + goto cleanup; + } else if (refcount > 1) { + /* Refcount is greater than 1 which means that there + * is @refcount domains using the @path. Do not + * change the label (as it would almost certainly + * cause the other domains to lose access to the + * @path). */ + if (STRNEQ(econ, tcon)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Setting different SELinux label on %s " + "which is already in use"), path); + goto cleanup; + } + } + } + } + + if (virSecuritySELinuxSetFileconImpl(path, tcon, optional, privileged) < 0) + goto cleanup; + + ret = 0; + cleanup: + freecon(econ); + return ret; } @@ -1293,7 +1360,7 @@ virSecuritySELinuxSetFileconOptional(virSecurityManagerPtr mgr, const char *path, const char *tcon) { bool privileged = virSecurityManagerGetPrivileged(mgr); - return virSecuritySELinuxSetFileconHelper(path, tcon, true, privileged); + return virSecuritySELinuxSetFileconHelper(path, tcon, true, privileged, false); } static int @@ -1301,7 +1368,7 @@ virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr, const char *path, const char *tcon) { bool privileged = virSecurityManagerGetPrivileged(mgr); - return virSecuritySELinuxSetFileconHelper(path, tcon, false, privileged); + return virSecuritySELinuxSetFileconHelper(path, tcon, false, privileged, false); } static int @@ -1362,7 +1429,8 @@ getContext(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, * errors that the caller(s) are already dealing with */ static int virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, - const char *path) + const char *path, + bool recall) { bool privileged = virSecurityManagerGetPrivileged(mgr); struct stat buf; @@ -1386,26 +1454,35 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, goto cleanup; } - if (stat(newpath, &buf) != 0) { - VIR_WARN("cannot stat %s: %s", newpath, - virStrerror(errno, ebuf, sizeof(ebuf))); - goto cleanup; - } - - if (getContext(mgr, newpath, buf.st_mode, &fcon) < 0) { - /* Any user created path likely does not have a default label, - * which makes this an expected non error - */ - VIR_WARN("cannot lookup default selinux label for %s", newpath); - ret = 0; - goto cleanup; - } - - if ((rc = virSecuritySELinuxTransactionAppend(path, fcon, false, true)) < 0) + if ((rc = virSecuritySELinuxTransactionAppend(path, NULL, false, true)) < 0) return -1; else if (rc > 0) return 0; + if (recall) { + if ((rc = virSecuritySELinuxRecallLabel(newpath, &fcon)) < 0) { + goto cleanup; + } else if (rc > 0) { + ret = 0; + goto cleanup; + } + } else { + if (stat(newpath, &buf) != 0) { + VIR_WARN("cannot stat %s: %s", newpath, + virStrerror(errno, ebuf, sizeof(ebuf))); + goto cleanup; + } + + if (getContext(mgr, newpath, buf.st_mode, &fcon) < 0) { + /* Any user created path likely does not have a default label, + * which makes this an expected non error + */ + VIR_WARN("cannot lookup default selinux label for %s", newpath); + ret = 0; + goto cleanup; + } + } + if (virSecuritySELinuxSetFileconImpl(newpath, fcon, false, privileged) < 0) goto cleanup; @@ -1460,7 +1537,7 @@ virSecuritySELinuxRestoreInputLabel(virSecurityManagerPtr mgr, switch ((virDomainInputType)input->type) { case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH: - rc = virSecuritySELinuxRestoreFileLabel(mgr, input->source.evdev); + rc = virSecuritySELinuxRestoreFileLabel(mgr, input->source.evdev, false); break; case VIR_DOMAIN_INPUT_TYPE_MOUSE: @@ -1516,7 +1593,7 @@ virSecuritySELinuxRestoreMemoryLabel(virSecurityManagerPtr mgr, if (!seclabel || !seclabel->relabel) return 0; - ret = virSecuritySELinuxRestoreFileLabel(mgr, mem->nvdimmPath); + ret = virSecuritySELinuxRestoreFileLabel(mgr, mem->nvdimmPath, false); break; case VIR_DOMAIN_MEMORY_MODEL_DIMM: @@ -1595,10 +1672,10 @@ virSecuritySELinuxRestoreTPMFileLabelInt(virSecurityManagerPtr mgr, switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: tpmdev = tpm->data.passthrough.source.data.file.path; - rc = virSecuritySELinuxRestoreFileLabel(mgr, tpmdev); + rc = virSecuritySELinuxRestoreFileLabel(mgr, tpmdev, false); if ((cancel_path = virTPMCreateCancelPath(tpmdev)) != NULL) { - if (virSecuritySELinuxRestoreFileLabel(mgr, cancel_path) < 0) + if (virSecuritySELinuxRestoreFileLabel(mgr, cancel_path, false) < 0) rc = -1; VIR_FREE(cancel_path); } @@ -1665,7 +1742,7 @@ virSecuritySELinuxRestoreImageLabelInt(virSecurityManagerPtr mgr, } } - return virSecuritySELinuxRestoreFileLabel(mgr, src->path); + return virSecuritySELinuxRestoreFileLabel(mgr, src->path, false); } @@ -2053,7 +2130,7 @@ virSecuritySELinuxRestorePCILabel(virPCIDevicePtr dev ATTRIBUTE_UNUSED, { virSecurityManagerPtr mgr = opaque; - return virSecuritySELinuxRestoreFileLabel(mgr, file); + return virSecuritySELinuxRestoreFileLabel(mgr, file, false); } static int @@ -2063,7 +2140,7 @@ virSecuritySELinuxRestoreUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED, { virSecurityManagerPtr mgr = opaque; - return virSecuritySELinuxRestoreFileLabel(mgr, file); + return virSecuritySELinuxRestoreFileLabel(mgr, file, false); } @@ -2080,7 +2157,7 @@ virSecuritySELinuxRestoreSCSILabel(virSCSIDevicePtr dev, if (virSCSIDeviceGetShareable(dev) || virSCSIDeviceGetReadonly(dev)) return 0; - return virSecuritySELinuxRestoreFileLabel(mgr, file); + return virSecuritySELinuxRestoreFileLabel(mgr, file, false); } static int @@ -2090,7 +2167,7 @@ virSecuritySELinuxRestoreHostLabel(virSCSIVHostDevicePtr dev ATTRIBUTE_UNUSED, { virSecurityManagerPtr mgr = opaque; - return virSecuritySELinuxRestoreFileLabel(mgr, file); + return virSecuritySELinuxRestoreFileLabel(mgr, file, false); } @@ -2194,7 +2271,7 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr, if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdevsrc->uuidstr))) goto done; - ret = virSecuritySELinuxRestoreFileLabel(mgr, vfiodev); + ret = virSecuritySELinuxRestoreFileLabel(mgr, vfiodev, false); VIR_FREE(vfiodev); break; @@ -2228,7 +2305,7 @@ virSecuritySELinuxRestoreHostdevCapsLabel(virSecurityManagerPtr mgr, if (VIR_STRDUP(path, dev->source.caps.u.storage.block) < 0) return -1; } - ret = virSecuritySELinuxRestoreFileLabel(mgr, path); + ret = virSecuritySELinuxRestoreFileLabel(mgr, path, false); VIR_FREE(path); break; } @@ -2242,7 +2319,7 @@ virSecuritySELinuxRestoreHostdevCapsLabel(virSecurityManagerPtr mgr, if (VIR_STRDUP(path, dev->source.caps.u.misc.chardev) < 0) return -1; } - ret = virSecuritySELinuxRestoreFileLabel(mgr, path); + ret = virSecuritySELinuxRestoreFileLabel(mgr, path, false); VIR_FREE(path); break; } @@ -2390,14 +2467,18 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr, switch (dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - if (virSecuritySELinuxRestoreFileLabel(mgr, dev_source->data.file.path) < 0) + if (virSecuritySELinuxRestoreFileLabel(mgr, + dev_source->data.file.path, + false) < 0) goto done; ret = 0; break; case VIR_DOMAIN_CHR_TYPE_UNIX: if (!dev_source->data.nix.listen) { - if (virSecuritySELinuxRestoreFileLabel(mgr, dev_source->data.file.path) < 0) + if (virSecuritySELinuxRestoreFileLabel(mgr, + dev_source->data.file.path, + false) < 0) goto done; } ret = 0; @@ -2408,11 +2489,13 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr, (virAsprintf(&in, "%s.in", dev_source->data.file.path) < 0)) goto done; if (virFileExists(in) && virFileExists(out)) { - if ((virSecuritySELinuxRestoreFileLabel(mgr, out) < 0) || - (virSecuritySELinuxRestoreFileLabel(mgr, in) < 0)) { + if ((virSecuritySELinuxRestoreFileLabel(mgr, out, false) < 0) || + (virSecuritySELinuxRestoreFileLabel(mgr, in, false) < 0)) { goto done; } - } else if (virSecuritySELinuxRestoreFileLabel(mgr, dev_source->data.file.path) < 0) { + } else if (virSecuritySELinuxRestoreFileLabel(mgr, + dev_source->data.file.path, + false) < 0) { goto done; } ret = 0; @@ -2464,7 +2547,7 @@ virSecuritySELinuxRestoreSecuritySmartcardCallback(virDomainDefPtr def, database = dev->data.cert.database; if (!database) database = VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE; - return virSecuritySELinuxRestoreFileLabel(mgr, database); + return virSecuritySELinuxRestoreFileLabel(mgr, database, false); case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH: return virSecuritySELinuxRestoreChardevLabel(mgr, def, @@ -2559,7 +2642,7 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr mgr, rc = -1; if (def->os.loader && def->os.loader->nvram && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram) < 0) + virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram, false) < 0) rc = -1; return rc; @@ -2619,7 +2702,7 @@ virSecuritySELinuxRestoreSavedStateLabel(virSecurityManagerPtr mgr, if (!secdef || !secdef->relabel) return 0; - return virSecuritySELinuxRestoreFileLabel(mgr, savefile); + return virSecuritySELinuxRestoreFileLabel(mgr, savefile, false); } @@ -3214,7 +3297,7 @@ virSecuritySELinuxRestoreFileLabels(virSecurityManagerPtr mgr, char *filename = NULL; DIR *dir; - if ((ret = virSecuritySELinuxRestoreFileLabel(mgr, path))) + if ((ret = virSecuritySELinuxRestoreFileLabel(mgr, path, false))) return ret; if (!virFileIsDir(path)) @@ -3231,7 +3314,7 @@ virSecuritySELinuxRestoreFileLabels(virSecurityManagerPtr mgr, ret = -1; break; } - ret = virSecuritySELinuxRestoreFileLabel(mgr, filename); + ret = virSecuritySELinuxRestoreFileLabel(mgr, filename, false); VIR_FREE(filename); if (ret < 0) break; -- 2.19.2
On 12/12/18 7:40 AM, Michal Privoznik wrote:
Similarly to what I did in DAC driver, this also requires the same SELinux label to be used for shared paths. If a path is already in use by a domain (or domains) then and the domain we are starting now wants to access the path it has to have the same SELinux label. This might look too restrictive as the new label can still guarantee access to already running domains but in reality it is very unlikely and usually an admin mistake.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_selinux.c | 177 +++++++++++++++++++++++--------- 1 file changed, 130 insertions(+), 47 deletions(-)
[...]
+ +static int +virSecuritySELinuxRecallLabel(const char *path, + security_context_t *con) +{ + if (virSecurityGetRememberedLabel(SECURITY_SELINUX_NAME, + path, con) < 0) + return -1; + + if (!con) + return 1;
This ordering of the !con check has caused a Coverity concern that we use @con in the first call... When compared to the *_dac.c code which passes &label, I assume this should be passing &con, right? I'd usually send a patch, but wanted to make sure it was the right option... John
+ + return 0; +} + +
[...]
On 12/20/18 12:39 AM, John Ferlan wrote:
On 12/12/18 7:40 AM, Michal Privoznik wrote:
Similarly to what I did in DAC driver, this also requires the same SELinux label to be used for shared paths. If a path is already in use by a domain (or domains) then and the domain we are starting now wants to access the path it has to have the same SELinux label. This might look too restrictive as the new label can still guarantee access to already running domains but in reality it is very unlikely and usually an admin mistake.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_selinux.c | 177 +++++++++++++++++++++++--------- 1 file changed, 130 insertions(+), 47 deletions(-)
[...]
+ +static int +virSecuritySELinuxRecallLabel(const char *path, + security_context_t *con) +{ + if (virSecurityGetRememberedLabel(SECURITY_SELINUX_NAME, + path, con) < 0) + return -1; + + if (!con) + return 1;
This ordering of the !con check has caused a Coverity concern that we use @con in the first call... When compared to the *_dac.c code which passes &label, I assume this should be passing &con, right?
Ooops, this hould have been if (!*con) return 1;. security_context_t is actually char *; therefore here con is type of char ** (just look at virSecurityGetRememberedLabel). I wonder if this will fix the issue Marc reported (unfortunately I don't have much time to dig into it right now). Michal
On 12/12/18 7:40 AM, Michal Privoznik wrote:
Similarly to what I did in DAC driver, this also requires the same SELinux label to be used for shared paths. If a path is already in use by a domain (or domains) then and the domain we are starting now wants to access the path it has to have the same SELinux label. This might look too restrictive as the new label can still guarantee access to already running domains but in reality it is very unlikely and usually an admin mistake.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_selinux.c | 177 +++++++++++++++++++++++--------- 1 file changed, 130 insertions(+), 47 deletions(-)
[...]
static int @@ -1362,7 +1429,8 @@ getContext(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, * errors that the caller(s) are already dealing with */ static int virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, - const char *path) + const char *path, + bool recall) { bool privileged = virSecurityManagerGetPrivileged(mgr); struct stat buf; @@ -1386,26 +1454,35 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, goto cleanup; }
- if (stat(newpath, &buf) != 0) { - VIR_WARN("cannot stat %s: %s", newpath, - virStrerror(errno, ebuf, sizeof(ebuf))); - goto cleanup; - } - - if (getContext(mgr, newpath, buf.st_mode, &fcon) < 0) { - /* Any user created path likely does not have a default label, - * which makes this an expected non error - */ - VIR_WARN("cannot lookup default selinux label for %s", newpath); - ret = 0; - goto cleanup; - } - - if ((rc = virSecuritySELinuxTransactionAppend(path, fcon, false, true)) < 0) + if ((rc = virSecuritySELinuxTransactionAppend(path, NULL, false, true)) < 0) return -1; else if (rc > 0) return 0;
Since you've touched the code, Coverity looks again and determines that @newpath can be leaked above John
+ if (recall) { + if ((rc = virSecuritySELinuxRecallLabel(newpath, &fcon)) < 0) { + goto cleanup; + } else if (rc > 0) { + ret = 0; + goto cleanup; + } + } else { + if (stat(newpath, &buf) != 0) { + VIR_WARN("cannot stat %s: %s", newpath, + virStrerror(errno, ebuf, sizeof(ebuf))); + goto cleanup; + } + + if (getContext(mgr, newpath, buf.st_mode, &fcon) < 0) { + /* Any user created path likely does not have a default label, + * which makes this an expected non error + */ + VIR_WARN("cannot lookup default selinux label for %s", newpath); + ret = 0; + goto cleanup; + } + } +
[...]
On 12/20/18 12:39 AM, John Ferlan wrote:
On 12/12/18 7:40 AM, Michal Privoznik wrote:
Similarly to what I did in DAC driver, this also requires the same SELinux label to be used for shared paths. If a path is already in use by a domain (or domains) then and the domain we are starting now wants to access the path it has to have the same SELinux label. This might look too restrictive as the new label can still guarantee access to already running domains but in reality it is very unlikely and usually an admin mistake.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_selinux.c | 177 +++++++++++++++++++++++--------- 1 file changed, 130 insertions(+), 47 deletions(-)
[...]
static int @@ -1362,7 +1429,8 @@ getContext(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, * errors that the caller(s) are already dealing with */ static int virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, - const char *path) + const char *path, + bool recall) { bool privileged = virSecurityManagerGetPrivileged(mgr); struct stat buf; @@ -1386,26 +1454,35 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, goto cleanup; }
- if (stat(newpath, &buf) != 0) { - VIR_WARN("cannot stat %s: %s", newpath, - virStrerror(errno, ebuf, sizeof(ebuf))); - goto cleanup; - } - - if (getContext(mgr, newpath, buf.st_mode, &fcon) < 0) { - /* Any user created path likely does not have a default label, - * which makes this an expected non error - */ - VIR_WARN("cannot lookup default selinux label for %s", newpath); - ret = 0; - goto cleanup; - } - - if ((rc = virSecuritySELinuxTransactionAppend(path, fcon, false, true)) < 0) + if ((rc = virSecuritySELinuxTransactionAppend(path, NULL, false, true)) < 0) return -1; else if (rc > 0) return 0;
Since you've touched the code, Coverity looks again and determines that @newpath can be leaked above
Ah, right. this should have been "goto cleanup" instead of "return -1" and "{ret = 0; goto cleanup}" instead of "return 0". Michal
It's important to keep XATTRs untouched (well, in the same state they were in when entering the function). Otherwise our refcounting would be messed up. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- src/security/security_selinux.c | 40 +++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 43d2ef32a5..f52c88259d 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -222,10 +222,10 @@ virSecuritySELinuxRecallLabel(const char *path, } -static int virSecuritySELinuxSetFileconHelper(const char *path, +static int virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, + const char *path, const char *tcon, bool optional, - bool privileged, bool remember); @@ -252,7 +252,6 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, { virSecuritySELinuxContextListPtr list = opaque; virSecurityManagerMetadataLockStatePtr state; - bool privileged = virSecurityManagerGetPrivileged(list->manager); const char **paths = NULL; size_t npaths = 0; size_t i; @@ -279,10 +278,10 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, /* TODO Implement rollback */ if (!item->restore) { - rv = virSecuritySELinuxSetFileconHelper(item->path, + rv = virSecuritySELinuxSetFileconHelper(list->manager, + item->path, item->tcon, item->optional, - privileged, list->lock); } else { rv = virSecuritySELinuxRestoreFileLabel(list->manager, @@ -1303,9 +1302,13 @@ virSecuritySELinuxSetFileconImpl(const char *path, const char *tcon, static int -virSecuritySELinuxSetFileconHelper(const char *path, const char *tcon, - bool optional, bool privileged, bool remember) +virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, + const char *path, + const char *tcon, + bool optional, + bool remember) { + bool privileged = virSecurityManagerGetPrivileged(mgr); security_context_t econ = NULL; int refcount; int rc; @@ -1345,8 +1348,23 @@ virSecuritySELinuxSetFileconHelper(const char *path, const char *tcon, } } - if (virSecuritySELinuxSetFileconImpl(path, tcon, optional, privileged) < 0) + if (virSecuritySELinuxSetFileconImpl(path, tcon, optional, privileged) < 0) { + virErrorPtr origerr; + + virErrorPreserveLast(&origerr); + /* Try to restore the label. This is done so that XATTRs + * are left in the same state as when the control entered + * this function. However, if our attempt fails, there's + * not much we can do. XATTRs refcounting is fubar'ed and + * the only option we have is warn users. */ + if (virSecuritySELinuxRestoreFileLabel(mgr, path, remember) < 0) + VIR_WARN("Unable to restore label on '%s'. " + "XATTRs might have been left in inconsistent state.", + path); + + virErrorRestore(&origerr); goto cleanup; + } ret = 0; cleanup: @@ -1359,16 +1377,14 @@ static int virSecuritySELinuxSetFileconOptional(virSecurityManagerPtr mgr, const char *path, const char *tcon) { - bool privileged = virSecurityManagerGetPrivileged(mgr); - return virSecuritySELinuxSetFileconHelper(path, tcon, true, privileged, false); + return virSecuritySELinuxSetFileconHelper(mgr, path, tcon, true, false); } static int virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr, const char *path, const char *tcon) { - bool privileged = virSecurityManagerGetPrivileged(mgr); - return virSecuritySELinuxSetFileconHelper(path, tcon, false, privileged, false); + return virSecuritySELinuxSetFileconHelper(mgr, path, tcon, false, false); } static int -- 2.19.2
When iterating over list of paths/disk sources to relabel it may happen that the process fails at some point. In that case, for the sake of keeping seclabel refcount (stored in XATTRs) in sync with reality we have to perform rollback. However, if that fails too the only thing we can do is warn user. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- src/security/security_selinux.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index f52c88259d..4b68eb2717 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -276,7 +276,6 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, for (i = 0; i < list->nItems; i++) { virSecuritySELinuxContextItemPtr item = list->items[i]; - /* TODO Implement rollback */ if (!item->restore) { rv = virSecuritySELinuxSetFileconHelper(list->manager, item->path, @@ -293,6 +292,18 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, break; } + for (; rv < 0 && i > 0; i--) { + virSecuritySELinuxContextItemPtr item = list->items[i - 1]; + + if (!item->restore) { + virSecuritySELinuxRestoreFileLabel(list->manager, + item->path, + list->lock); + } else { + VIR_WARN("Ignoring failed restore attempt on %s", item->path); + } + } + if (list->lock) virSecurityManagerMetadataUnlock(list->manager, &state); -- 2.19.2
It helps whe trying to match calls with virSecuritySELinuxSetAllLabel if the order in which devices are set/restored is the same in both functions. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- src/security/security_selinux.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 4b68eb2717..4e30523e2c 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2620,8 +2620,11 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr mgr, if (!secdef || !secdef->relabel || data->skipAllLabel) return 0; - if (def->tpm) { - if (virSecuritySELinuxRestoreTPMFileLabelInt(mgr, def, def->tpm) < 0) + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = def->disks[i]; + + if (virSecuritySELinuxRestoreImageLabelInt(mgr, def, disk->src, + migrated) < 0) rc = -1; } @@ -2643,11 +2646,8 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr mgr, return -1; } - for (i = 0; i < def->ndisks; i++) { - virDomainDiskDefPtr disk = def->disks[i]; - - if (virSecuritySELinuxRestoreImageLabelInt(mgr, def, disk->src, - migrated) < 0) + if (def->tpm) { + if (virSecuritySELinuxRestoreTPMFileLabelInt(mgr, def, def->tpm) < 0) rc = -1; } -- 2.19.2
We are setting label on kernel, initrd, dtb and slic_table files. But we never restored it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- src/security/security_selinux.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 4e30523e2c..2d32e65f13 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2672,6 +2672,22 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr mgr, virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram, false) < 0) rc = -1; + if (def->os.kernel && + virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel, false) < 0) + rc = -1; + + if (def->os.initrd && + virSecuritySELinuxRestoreFileLabel(mgr, def->os.initrd, false) < 0) + rc = -1; + + if (def->os.dtb && + virSecuritySELinuxRestoreFileLabel(mgr, def->os.dtb, false) < 0) + rc = -1; + + if (def->os.slic_table && + virSecuritySELinuxRestoreFileLabel(mgr, def->os.slic_table, false) < 0) + rc = -1; + return rc; } -- 2.19.2
This test checks if security label remembering works correctly. It uses qemuSecurity* APIs to do that. And some mocking (even though it's not real mocking as we are used to from other tests like virpcitest). So far, only DAC driver is tested. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 4 +- src/util/virfile.h | 15 +- tests/Makefile.am | 10 + tests/qemusecuritymock.c | 480 +++++++++++++++++++++++++++++++++++++++ tests/qemusecuritytest.c | 173 ++++++++++++++ tests/qemusecuritytest.h | 28 +++ 6 files changed, 703 insertions(+), 7 deletions(-) create mode 100644 tests/qemusecuritymock.c create mode 100644 tests/qemusecuritytest.c create mode 100644 tests/qemusecuritytest.h diff --git a/cfg.mk b/cfg.mk index c468d153eb..bd67cfd940 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1178,7 +1178,7 @@ exclude_file_name_regexp--sc_copyright_usage = \ ^COPYING(|\.LESSER)$$ exclude_file_name_regexp--sc_flags_usage = \ - ^(cfg\.mk|docs/|src/util/virnetdevtap\.c$$|tests/((vir(cgroup|pci|test|usb)|nss|qemuxml2argv)mock|virfilewrapper)\.c$$) + ^(cfg\.mk|docs/|src/util/virnetdevtap\.c$$|tests/((vir(cgroup|pci|test|usb)|nss|qemuxml2argv|qemusecurity)mock|virfilewrapper)\.c$$) exclude_file_name_regexp--sc_libvirt_unmarked_diagnostics = \ ^(src/rpc/gendispatch\.pl$$|tests/) @@ -1201,7 +1201,7 @@ exclude_file_name_regexp--sc_prohibit_strdup = \ ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c|tests/commandhelper\.c$$) exclude_file_name_regexp--sc_prohibit_close = \ - (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir.+mock\.c|tests/commandhelper\.c)$$) + (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/(vir.+mock\.c|commandhelper\.c|qemusecuritymock\.c))$$) exclude_file_name_regexp--sc_prohibit_empty_lines_at_EOF = \ (^tests/(virhostcpu|virpcitest)data/|docs/js/.*\.js|docs/fonts/.*\.woff|\.diff|tests/virconfdata/no-newline\.conf$$) diff --git a/src/util/virfile.h b/src/util/virfile.h index aa4c29dc40..9304d69349 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -115,8 +115,10 @@ int virFileWrapperFdClose(virFileWrapperFdPtr dfd); void virFileWrapperFdFree(virFileWrapperFdPtr dfd); -int virFileLock(int fd, bool shared, off_t start, off_t len, bool waitForLock); -int virFileUnlock(int fd, off_t start, off_t len); +int virFileLock(int fd, bool shared, off_t start, off_t len, bool waitForLock) + ATTRIBUTE_NOINLINE; +int virFileUnlock(int fd, off_t start, off_t len) + ATTRIBUTE_NOINLINE; int virFileFlock(int fd, bool lock, bool shared); @@ -385,13 +387,16 @@ VIR_DEFINE_AUTOPTR_FUNC(virFileWrapperFd, virFileWrapperFdFree) int virFileGetXAttr(const char *path, const char *name, - char **value); + char **value) + ATTRIBUTE_NOINLINE; int virFileSetXAttr(const char *path, const char *name, - const char *value); + const char *value) + ATTRIBUTE_NOINLINE; int virFileRemoveXAttr(const char *path, - const char *name); + const char *name) + ATTRIBUTE_NOINLINE; #endif /* __VIR_FILE_H */ diff --git a/tests/Makefile.am b/tests/Makefile.am index d7ec7e3a6f..fbecf4179d 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -289,6 +289,7 @@ test_programs += qemuxml2argvtest qemuxml2xmltest \ qemucommandutiltest \ qemublocktest \ qemumigparamstest \ + qemusecuritytest \ $(NULL) test_helpers += qemucapsprobe test_libraries += libqemumonitortestutils.la \ @@ -683,6 +684,13 @@ qemumigparamstest_SOURCES = \ qemumigparamstest_LDADD = libqemumonitortestutils.la \ $(qemu_LDADDS) $(LDADDS) +qemusecuritytest_SOURCES = \ + qemusecuritytest.c qemusecuritytest.h \ + qemusecuritymock.c \ + testutils.h testutils.c \ + testutilsqemu.h testutilsqemu.c +qemusecuritytest_LDADD = $(qemu_LDADDS) $(LDADDS) + else ! WITH_QEMU EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c qemuargv2xmltest.c \ domainsnapshotxml2xmltest.c \ @@ -694,6 +702,8 @@ EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c qemuargv2xmltest.c \ qemumemlocktest.c qemucpumock.c testutilshostcpus.h \ qemublocktest.c \ qemumigparamstest.c \ + qemusecuritytest.c qemusecuritytest.h \ + qemusecuritymock.c \ $(QEMUMONITORTESTUTILS_SOURCES) endif ! WITH_QEMU diff --git a/tests/qemusecuritymock.c b/tests/qemusecuritymock.c new file mode 100644 index 0000000000..6148d02cb0 --- /dev/null +++ b/tests/qemusecuritymock.c @@ -0,0 +1,480 @@ +/* + * Copyright (C) 2018 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Michal Privoznik <mprivozn@redhat.com> + */ + +#include <config.h> + +#include <string.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <unistd.h> +#include <fcntl.h> + +#include "virmock.h" +#include "virfile.h" +#include "virthread.h" +#include "virhash.h" +#include "virstring.h" +#include "qemusecuritytest.h" +#include "security/security_manager.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +/* Okay, here's the deal. The qemusecuritytest calls several + * virSecurityManager public APIs in order to check if XATTRs + * work as expected. Therefore there is a lot we have to mock + * (chown, stat, XATTR APIs, etc.). Since the test won't run as + * root chown() would fail, therefore we have to keep everything + * in memory. By default, all files are owned by 1:2. + * By the way, since there are some cases where real stat needs + * to be called, the mocked functions are effective only if + * $ENVVAR is set. + */ + +#define DEFAULT_UID 1 +#define DEFAULT_GID 2 + + +static int (*real_chown)(const char *path, uid_t uid, gid_t gid); +static int (*real_lstat)(const char *path, struct stat *sb); +static int (*real___lxstat)(int ver, const char *path, struct stat *sb); +static int (*real_stat)(const char *path, struct stat *sb); +static int (*real___xstat)(int ver, const char *path, struct stat *sb); +static int (*real_open)(const char *path, int flags, ...); +static int (*real_close)(int fd); + + +/* Global mutex to avoid races */ +virMutex m = VIR_MUTEX_INITIALIZER; + +/* Hash table to store XATTRs for paths. For simplicity, key is + * "$path:$name" and value is just XATTR "$value". We don't need + * to list XATTRs a path has, therefore we don't need something + * more clever. */ +virHashTablePtr xattr_paths = NULL; + + +/* The UID:GID is stored in a hash table. Again, for simplicity, + * the path is the key and the value is an uint32_t , where + * the lower half is UID and the higher is GID. */ +virHashTablePtr chown_paths = NULL; + + +static void +init_hash(void) +{ + /* The reason the init is split is that virHash calls + * virRandomBits() which in turn calls a gnutls function. + * However, when gnutls is initializing itself it calls + * stat() so we would call a gnutls function before it is + * initialized which will lead to a crash. + */ + + if (xattr_paths) + return; + + if (!(xattr_paths = virHashCreate(10, virHashValueFree))) { + fprintf(stderr, "Unable to create hash table for XATTR paths\n"); + abort(); + } + + if (!(chown_paths = virHashCreate(10, virHashValueFree))) { + fprintf(stderr, "Unable to create hash table for chowned paths\n"); + abort(); + } +} + + +static void +init_syms(void) +{ + if (real_chown) + return; + + VIR_MOCK_REAL_INIT(chown); + VIR_MOCK_REAL_INIT_ALT(lstat, __lxstat); + VIR_MOCK_REAL_INIT_ALT(stat, __xstat); + VIR_MOCK_REAL_INIT(open); + VIR_MOCK_REAL_INIT(close); + + /* Intentionally not calling init_hash() here */ +} + + +static char * +get_key(const char *path, + const char *name) +{ + char *ret; + + if (virAsprintf(&ret, "%s:%s", path, name) < 0) { + fprintf(stderr, "Unable to create hash table key\n"); + abort(); + } + + return ret; +} + + +int +virFileGetXAttr(const char *path, + const char *name, + char **value) +{ + int ret = -1; + char *key; + char *val; + + key = get_key(path, name); + + virMutexLock(&m); + init_syms(); + init_hash(); + + if (!(val = virHashLookup(xattr_paths, key))) { + errno = ENODATA; + goto cleanup; + } + + if (VIR_STRDUP(*value, val) < 0) + goto cleanup; + + ret = 0; + cleanup: + virMutexUnlock(&m); + VIR_FREE(key); + return ret; +} + + +int virFileSetXAttr(const char *path, + const char *name, + const char *value) +{ + int ret = -1; + char *key; + char *val; + + key = get_key(path, name); + if (VIR_STRDUP(val, value) < 0) + return -1; + + virMutexLock(&m); + init_syms(); + init_hash(); + + if (virHashUpdateEntry(xattr_paths, key, val) < 0) + goto cleanup; + val = NULL; + + ret = 0; + cleanup: + virMutexUnlock(&m); + VIR_FREE(val); + VIR_FREE(key); + return ret; +} + + +int virFileRemoveXAttr(const char *path, + const char *name) +{ + int ret = -1; + char *key; + + key = get_key(path, name); + + virMutexLock(&m); + init_syms(); + init_hash(); + + if ((ret = virHashRemoveEntry(xattr_paths, key)) < 0) + errno = ENODATA; + + virMutexUnlock(&m); + VIR_FREE(key); + return ret; +} + + +static int +mock_stat(const char *path, + struct stat *sb) +{ + uint32_t *val; + + virMutexLock(&m); + init_hash(); + + memset(sb, 0, sizeof(*sb)); + + sb->st_mode = S_IFREG | 0666; + sb->st_size = 123456; + sb->st_ino = 1; + + if (!(val = virHashLookup(chown_paths, path))) { + /* New path. Set the defaults */ + sb->st_uid = DEFAULT_UID; + sb->st_gid = DEFAULT_GID; + } else { + /* Known path. Set values passed to chown() earlier */ + sb->st_uid = *val % 16; + sb->st_gid = *val >> 16; + } + + virMutexUnlock(&m); + + return 0; +} + + +static int +mock_chown(const char *path, + uid_t uid, + gid_t gid) +{ + uint32_t *val = NULL; + int ret = -1; + + if (gid >> 16 || uid >> 16) { + fprintf(stderr, "Attempt to set too high UID or GID: %lld %lld", + (unsigned long long) uid, (unsigned long long) gid); + abort(); + } + + if (VIR_ALLOC(val) < 0) + return -1; + + *val = (gid << 16) + uid; + + virMutexLock(&m); + init_hash(); + + if (virHashUpdateEntry(chown_paths, path, val) < 0) + goto cleanup; + val = NULL; + + ret = 0; + cleanup: + virMutexUnlock(&m); + VIR_FREE(val); + return ret; +} + + +#ifdef HAVE___LXSTAT +int +__lxstat(int ver, const char *path, struct stat *sb) +{ + int ret; + + init_syms(); + + if (getenv(ENVVAR)) + ret = mock_stat(path, sb); + else + ret = real___lxstat(ver, path, sb); + + return ret; +} +#endif /* HAVE___LXSTAT */ + +int +lstat(const char *path, struct stat *sb) +{ + int ret; + + init_syms(); + + if (getenv(ENVVAR)) + ret = mock_stat(path, sb); + else + ret = real_lstat(path, sb); + + return ret; +} + +#ifdef HAVE___XSTAT +int +__xstat(int ver, const char *path, struct stat *sb) +{ + int ret; + + init_syms(); + + if (getenv(ENVVAR)) + ret = mock_stat(path, sb); + else + ret = real___xstat(ver, path, sb); + + return ret; +} +#endif /* HAVE___XSTAT */ + +int +stat(const char *path, struct stat *sb) +{ + int ret; + + init_syms(); + + if (getenv(ENVVAR)) + ret = mock_stat(path, sb); + else + ret = real_stat(path, sb); + + return ret; +} + + +int +chown(const char *path, uid_t uid, gid_t gid) +{ + int ret; + + init_syms(); + + if (getenv(ENVVAR)) + ret = mock_chown(path, uid, gid); + else + ret = real_chown(path, uid, gid); + + return ret; +} + + +int +open(const char *path, int flags, ...) +{ + int ret; + + init_syms(); + + if (getenv(ENVVAR)) { + ret = 42; /* Some dummy FD */ + } else if (flags & O_CREAT) { + va_list ap; + mode_t mode; + va_start(ap, flags); + mode = (mode_t) va_arg(ap, int); + va_end(ap); + ret = real_open(path, flags, mode); + } else { + ret = real_open(path, flags); + } + + return ret; +} + + +int +close(int fd) +{ + int ret; + + if (fd == 42 && getenv(ENVVAR)) + ret = 0; + else + ret = real_close(fd); + + return ret; +} + + +int virFileLock(int fd ATTRIBUTE_UNUSED, + bool shared ATTRIBUTE_UNUSED, + off_t start ATTRIBUTE_UNUSED, + off_t len ATTRIBUTE_UNUSED, + bool waitForLock ATTRIBUTE_UNUSED) +{ + return 0; +} + + +int virFileUnlock(int fd ATTRIBUTE_UNUSED, + off_t start ATTRIBUTE_UNUSED, + off_t len ATTRIBUTE_UNUSED) +{ + return 0; +} + + +static int +checkOwner(void *payload, + const void *name, + void *data) +{ + bool *chown_fail = data; + uint32_t owner = *((uint32_t*) payload); + + if (owner % 16 != DEFAULT_UID || + owner >> 16 != DEFAULT_GID) { + fprintf(stderr, + "Path %s wasn't restored back to its original owner\n", + (const char *) name); + *chown_fail = false; + } + + return 0; +} + + +static int +printXATTR(void *payload, + const void *name, + void *data) +{ + bool *xattr_fail = data; + + /* The fact that we are in this function means that there are + * some XATTRs left behind. This is enough to claim an error. */ + *xattr_fail = false; + + /* Hash table key consists of "$path:$xattr_name", xattr + * value is then the value stored in the hash table. */ + printf("key=%s val=%s\n", (const char *) name, (const char *) payload); + return 0; +} + + +int checkPaths(void) +{ + int ret = -1; + bool chown_fail = false; + bool xattr_fail = false; + + virMutexLock(&m); + init_hash(); + + if ((virHashForEach(chown_paths, checkOwner, &chown_fail)) < 0) + goto cleanup; + + if ((virHashForEach(xattr_paths, printXATTR, &xattr_fail)) < 0) + goto cleanup; + + if (chown_fail || xattr_fail) + goto cleanup; + + ret = 0; + cleanup: + virHashRemoveAll(chown_paths); + virHashRemoveAll(xattr_paths); + virMutexUnlock(&m); + return ret; +} diff --git a/tests/qemusecuritytest.c b/tests/qemusecuritytest.c new file mode 100644 index 0000000000..4d835f83e1 --- /dev/null +++ b/tests/qemusecuritytest.c @@ -0,0 +1,173 @@ +/* + * Copyright (C) 2018 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Michal Privoznik <mprivozn@redhat.com> + */ + +#include <config.h> + +#include "qemusecuritytest.h" +#include "testutils.h" +#include "testutilsqemu.h" +#include "security/security_manager.h" +#include "conf/domain_conf.h" +#include "qemu/qemu_domain.h" +#include "qemu/qemu_security.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +struct testData { + virQEMUDriverPtr driver; + const char *file; /* file name to load VM def XML from; qemuxml2argvdata/ */ +}; + + +static int +prepareObjects(virQEMUDriverPtr driver, + const char *xmlname, + virDomainObjPtr *vm) +{ + qemuDomainObjPrivatePtr priv; + char *filename = NULL; + char *domxml = NULL; + int ret = -1; + + if (virAsprintf(&filename, "%s/qemuxml2argvdata/%s.xml", abs_srcdir, xmlname) < 0) + return -1; + + if (virTestLoadFile(filename, &domxml) < 0) + goto cleanup; + + if (!(*vm = virDomainObjNew(driver->xmlopt))) + goto cleanup; + + (*vm)->pid = -1; + priv = (*vm)->privateData; + priv->chardevStdioLogd = false; + priv->rememberOwner = true; + + if (!(priv->qemuCaps = virQEMUCapsNew())) + goto cleanup; + + virQEMUCapsSetList(priv->qemuCaps, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_VIRTIO_MMIO, + QEMU_CAPS_DEVICE_VIRTIO_RNG, + QEMU_CAPS_OBJECT_GPEX, + QEMU_CAPS_OBJECT_RNG_RANDOM, + QEMU_CAPS_VIRTIO_SCSI, + QEMU_CAPS_LAST); + + if (qemuTestCapsCacheInsert(driver->qemuCapsCache, priv->qemuCaps) < 0) + goto cleanup; + + if (!((*vm)->def = virDomainDefParseString(domxml, + driver->caps, + driver->xmlopt, + NULL, + 0))) + goto cleanup; + + ret = 0; + cleanup: + if (ret < 0) { + virObjectUnref(*vm); + *vm = NULL; + } + VIR_FREE(domxml); + VIR_FREE(filename); + return ret; +} + + +static int +testDomain(const void *opaque) +{ + const struct testData *data = opaque; + virSecurityManagerPtr securityManager = NULL; + virDomainObjPtr vm = NULL; + int ret = -1; + + if (prepareObjects(data->driver, data->file, &vm) < 0) + return -1; + + /* Mocking is enabled only when this env variable is set. + * See mock code for explanation. */ + if (setenv(ENVVAR, "1", 0) < 0) + goto cleanup; + + if (qemuSecuritySetAllLabel(data->driver, vm, NULL) < 0) + goto cleanup; + + qemuSecurityRestoreAllLabel(data->driver, vm, false); + + if (checkPaths() < 0) + goto cleanup; + + ret = 0; + cleanup: + unsetenv(ENVVAR); + virObjectUnref(vm); + virObjectUnref(securityManager); + return ret; +} + + +static int +mymain(void) +{ + virQEMUDriver driver; + int ret = 0; + + if (virInitialize() < 0 || + qemuTestDriverInit(&driver) < 0) + return -1; + + /* Now fix the secdriver */ + virObjectUnref(driver.securityManager); + if (!(driver.securityManager = virSecurityManagerNewDAC("test", 1000, 1000, + VIR_SECURITY_MANAGER_PRIVILEGED | + VIR_SECURITY_MANAGER_DYNAMIC_OWNERSHIP, + NULL))) { + virFilePrintf(stderr, "Cannot initialize DAC security driver"); + ret = -1; + goto cleanup; + } + +#define DO_TEST_DOMAIN(f) \ + do { \ + struct testData data = {.driver = &driver, .file = f}; \ + if (virTestRun(f, testDomain, &data) < 0) \ + ret = -1; \ + } while (0) + + DO_TEST_DOMAIN("disk-virtio"); + DO_TEST_DOMAIN("pci-bridge-many-disks"); + DO_TEST_DOMAIN("arm-virt-virtio"); + DO_TEST_DOMAIN("aarch64-virtio-pci-manual-addresses"); + DO_TEST_DOMAIN("acpi-table"); + + cleanup: + qemuTestDriverFree(&driver); + return ret; +} + +VIR_TEST_MAIN(mymain) diff --git a/tests/qemusecuritytest.h b/tests/qemusecuritytest.h new file mode 100644 index 0000000000..235e05fec7 --- /dev/null +++ b/tests/qemusecuritytest.h @@ -0,0 +1,28 @@ +/* + * Copyright (C) 2018 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Michal Privoznik <mprivozn@redhat.com> + */ + +#ifndef __QEMU_SECURITY_TEST_H__ +# define __QEMU_SECURITY_TEST_H__ + +# define ENVVAR "LIBVIRT_QEMU_SECURITY_TEST" + +extern int checkPaths(void); + +#endif /* __QEMU_SECURITY_TEST_H__ */ -- 2.19.2
On Wed, Dec 12, 2018 at 01:41:00PM +0100, Michal Privoznik wrote:
This test checks if security label remembering works correctly. It uses qemuSecurity* APIs to do that. And some mocking (even though it's not real mocking as we are used to from other tests like virpcitest). So far, only DAC driver is tested.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 4 +- src/util/virfile.h | 15 +- tests/Makefile.am | 10 + tests/qemusecuritymock.c | 480 +++++++++++++++++++++++++++++++++++++++ tests/qemusecuritytest.c | 173 ++++++++++++++ tests/qemusecuritytest.h | 28 +++ 6 files changed, 703 insertions(+), 7 deletions(-) create mode 100644 tests/qemusecuritymock.c create mode 100644 tests/qemusecuritytest.c create mode 100644 tests/qemusecuritytest.h
sed -i 's/__QEMU_SECURITY_TEST_H__/LIBVIRT_QEMUSECURITYTEST_H/g' tests/qemusecuritytest.h perl -0777 -ni -pe 's/ \*\n \* Author: [^\n]*\n//s' tests/qemusecurity* Jano
Our code is not bug free. The refcounting I introduced will almost certainly not work in some use cases. Provide a script that will remove all the XATTRs set by libvirt so that it can start cleanly. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/Makefile.am | 1 + tools/libvirt_recover_xattrs.sh | 96 +++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+) create mode 100755 tools/libvirt_recover_xattrs.sh diff --git a/tools/Makefile.am b/tools/Makefile.am index f069167acc..1dc009c4fb 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -75,6 +75,7 @@ EXTRA_DIST = \ virt-login-shell.conf \ virsh-edit.c \ bash-completion/vsh \ + libvirt_recover_xattrs.sh \ $(PODFILES) \ $(MANINFILES) \ $(NULL) diff --git a/tools/libvirt_recover_xattrs.sh b/tools/libvirt_recover_xattrs.sh new file mode 100755 index 0000000000..69dfca0160 --- /dev/null +++ b/tools/libvirt_recover_xattrs.sh @@ -0,0 +1,96 @@ +#!/bin/bash + +function die { + echo $@ >&2 + exit 1 +} + +function show_help { + cat << EOF +Usage: ${0##*/} -[hqn] [PATH] + +Clear out any XATTRs set by libvirt on all files that have them. +The idea is to reset refcounting, should it break. + + -h display this help and exit + -q quiet (don't print which files are being fixed) + -n dry run; don't remove any XATTR just report the file name + +PATH can be specified to refine search to only to given path +instead of whole root ('/'), which is the default. +EOF +} + +QUIET=0 +DRY_RUN=0 +P="/" + +# So far only qemu and lxc drivers use security driver. +URI=("qemu:///system" + "qemu:///session" + "lxc:///system") + +LIBVIRT_XATTR_PREFIX="trusted.libvirt.security" + +if [ `whoami` != "root" ]; then + die "Must be run as root" +fi + +while getopts hqn opt; do + case $opt in + h) + show_help + exit 0 + ;; + q) + QUIET=1 + ;; + n) + DRY_RUN=1 + ;; + *) + show_help >&2 + exit 1 + ;; + esac +done + +shift $((OPTIND - 1)) +if [ $# -gt 0 ]; then + P=$1 +fi + +if [ ${DRY_RUN} -eq 0 ]; then + for u in ${URI[*]} ; do + if [ -n "`virsh -q -c $u list 2>/dev/null`" ]; then + die "There are still some domains running for $u" + fi + done +fi + + +# On Linux we use 'trusted' namespace, on FreeBSD we use 'system' +# as there is no 'trusted'. +XATTRS=("trusted.libvirt.security.dac" + "trusted.libvirt.security.ref_dac" + "trusted.libvirt.security.selinux" + "trusted.libvirt.security.ref_selinux", + "system.libvirt.security.dac" + "system.libvirt.security.ref_dac" + "system.libvirt.security.selinux" + "system.libvirt.security.ref_selinux") + +for i in $(getfattr -R -d -m ${LIBVIRT_XATTR_PREFIX} --absolute-names ${P} 2>/dev/null | grep "^# file:" | cut -d':' -f 2); do + if [ ${DRY_RUN} -ne 0 ]; then + echo $i + getfattr -d -m ${LIBVIRT_XATTR_PREFIX} $i + continue + fi + + if [ ${QUIET} -eq 0 ]; then + echo "Fixing $i"; + fi + for x in ${XATTRS[*]}; do + setfattr -x $x $i + done +done -- 2.19.2
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 4 ++++ src/qemu/qemu_conf.c | 4 ++++ src/qemu/test_libvirtd_qemu.aug.in | 1 + 4 files changed, 10 insertions(+) diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index ddc4bbfd1d..8a5b39e568 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -71,6 +71,7 @@ module Libvirtd_qemu = | str_entry "user" | str_entry "group" | bool_entry "dynamic_ownership" + | bool_entry "remember_owner" | str_array_entry "cgroup_controllers" | str_array_entry "cgroup_device_acl" | int_entry "seccomp_sandbox" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 8391332cb4..29093f6329 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -450,6 +450,10 @@ # Set to 0 to disable file ownership changes. #dynamic_ownership = 1 +# Whether libvirt should remember and restore the original +# ownership over files it is relabeling. Defaults to 1, set +# to 0 to disable the feature. +#remember_owner = 1 # What cgroup controllers to make use of with QEMU guests # diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index a946b05d5d..89491a37b7 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -147,6 +147,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->group = (gid_t)-1; } cfg->dynamicOwnership = privileged; + cfg->rememberOwner = true; cfg->cgroupControllers = -1; /* -1 == auto-detect */ @@ -730,6 +731,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (virConfGetValueBool(conf, "dynamic_ownership", &cfg->dynamicOwnership) < 0) goto cleanup; + if (virConfGetValueBool(conf, "remember_owner", &cfg->rememberOwner) < 0) + goto cleanup; + if (virConfGetValueStringList(conf, "cgroup_controllers", false, &controllers) < 0) goto cleanup; diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index f1e8806ad2..92a8ae1192 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -43,6 +43,7 @@ module Test_libvirtd_qemu = { "user" = "root" } { "group" = "root" } { "dynamic_ownership" = "1" } +{ "remember_owner" = "1" } { "cgroup_controllers" { "1" = "cpu" } { "2" = "devices" } -- 2.19.2
On Wed, Dec 12, 2018 at 01:40:44PM +0100, Michal Privoznik wrote:
v3 of:
https://www.redhat.com/archives/libvir-list/2018-November/msg01070.html
diff to v2: - dropped 01/18 from v2 - Introduced a test - Couple of minor adjustments as suggested in review of v2
Michal Prívozník (18): util: Introduce xattr getter/setter/remover security: Include security_util security_dac: Restore label on failed chown() attempt virSecurityDACTransactionRun: Implement rollback virSecurityDACRestoreAllLabel: Reorder device relabeling virSecurityDACRestoreAllLabel: Restore more labels security_dac: Allow callers to enable/disable label remembering/recall security_dac: Remember old labels virSecurityDACRestoreImageLabelInt: Restore even shared/RO disks security_selinux: Track if transaction is restore security_selinux: Remember old labels security_selinux: Restore label on failed setfilecon() attempt virSecuritySELinuxTransactionRun: Implement rollback virSecuritySELinuxRestoreAllLabel: Reorder device relabeling virSecuritySELinuxRestoreAllLabel: Restore more labels tests: Introduce qemusecuritytest tools: Provide a script to recover fubar'ed XATTRs setup qemu.conf: Allow users to enable/disable label remembering
cfg.mk | 4 +- src/libvirt_private.syms | 3 + src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 4 + src/qemu/qemu_conf.c | 4 + src/qemu/test_libvirtd_qemu.aug.in | 1 + src/security/Makefile.inc.am | 2 + src/security/security_dac.c | 227 ++++++++++---- src/security/security_selinux.c | 272 ++++++++++++---- src/security/security_util.c | 256 +++++++++++++++ src/security/security_util.h | 32 ++ src/util/virfile.c | 121 ++++++++ src/util/virfile.h | 20 +- tests/Makefile.am | 10 + tests/qemusecuritymock.c | 480 +++++++++++++++++++++++++++++ tests/qemusecuritytest.c | 173 +++++++++++ tests/qemusecuritytest.h | 28 ++ tools/Makefile.am | 1 + tools/libvirt_recover_xattrs.sh | 96 ++++++ 19 files changed, 1600 insertions(+), 135 deletions(-) create mode 100644 src/security/security_util.c create mode 100644 src/security/security_util.h create mode 100644 tests/qemusecuritymock.c create mode 100644 tests/qemusecuritytest.c create mode 100644 tests/qemusecuritytest.h create mode 100755 tools/libvirt_recover_xattrs.sh
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Wed, Dec 19, 2018 at 03:37 PM +0100, Michal Privoznik <mprivozn@redhat.com> wrote:
On 12/19/18 2:54 PM, Ján Tomko wrote:
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Thanks to you and Dan. I've pushed these.
I tried out the current master (e05d8e570b) and I got the following error message regularly: 2018-12-20 11:37:37.056+0000: 30026: error : virProcessWait:274 : internal error: Child process (31926) unexpected fatal signal 11 2018-12-20 11:37:37.060+0000: 30026: warning : qemuSecurityRestoreAllLabel:89 : Unable to run security manager transaction Did you try it with SELinux?
Michal
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Matthias Hartmann Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 12/20/18 12:48 PM, Marc Hartmayer wrote:
On Wed, Dec 19, 2018 at 03:37 PM +0100, Michal Privoznik <mprivozn@redhat.com> wrote:
On 12/19/18 2:54 PM, Ján Tomko wrote:
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Thanks to you and Dan. I've pushed these.
I tried out the current master (e05d8e570b) and I got the following error message regularly:
2018-12-20 11:37:37.056+0000: 30026: error : virProcessWait:274 : internal error: Child process (31926) unexpected fatal signal 11 2018-12-20 11:37:37.060+0000: 30026: warning : qemuSecurityRestoreAllLabel:89 : Unable to run security manager transaction
Looks like there is some crash. Can you try to get stack trace please? Michal
On Thu, Dec 20, 2018 at 09:15 PM +0100, Michal Prívozník <mprivozn@redhat.com> wrote:
On 12/20/18 12:48 PM, Marc Hartmayer wrote:
On Wed, Dec 19, 2018 at 03:37 PM +0100, Michal Privoznik <mprivozn@redhat.com> wrote:
On 12/19/18 2:54 PM, Ján Tomko wrote:
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Thanks to you and Dan. I've pushed these.
I tried out the current master (e05d8e570b) and I got the following error message regularly:
2018-12-20 11:37:37.056+0000: 30026: error : virProcessWait:274 : internal error: Child process (31926) unexpected fatal signal 11 2018-12-20 11:37:37.060+0000: 30026: warning : qemuSecurityRestoreAllLabel:89 : Unable to run security manager transaction
Looks like there is some crash. Can you try to get stack trace please?
Hmm with the newest master (9d42d51eef793d7c) I get no error message. I’ll try to revalidate the behavior/error messages with the previous version.
Michal
-- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Matthias Hartmann Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 12/21/18 10:32 AM, Marc Hartmayer wrote:
On Thu, Dec 20, 2018 at 09:15 PM +0100, Michal Prívozník <mprivozn@redhat.com> wrote:
On 12/20/18 12:48 PM, Marc Hartmayer wrote:
On Wed, Dec 19, 2018 at 03:37 PM +0100, Michal Privoznik <mprivozn@redhat.com> wrote:
On 12/19/18 2:54 PM, Ján Tomko wrote:
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Thanks to you and Dan. I've pushed these.
I tried out the current master (e05d8e570b) and I got the following error message regularly:
2018-12-20 11:37:37.056+0000: 30026: error : virProcessWait:274 : internal error: Child process (31926) unexpected fatal signal 11 2018-12-20 11:37:37.060+0000: 30026: warning : qemuSecurityRestoreAllLabel:89 : Unable to run security manager transaction
Looks like there is some crash. Can you try to get stack trace please?
Hmm with the newest master (9d42d51eef793d7c) I get no error message. I’ll try to revalidate the behavior/error messages with the previous version.
I pushed a patch that probably is a fix for what you saw... See commit 9d42d51eef - essentially avoids passing contents of a empty @con into virSecuritySELinuxSetFileconImpl which I assume is where you had a fairly spectacular failure. John
participants (5)
-
John Ferlan -
Ján Tomko -
Marc Hartmayer -
Michal Privoznik -
Michal Prívozník