Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- 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

We are setting label on kernel, initrd, dtb and slic_table files. But we never restored it. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- 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

On 12/12/18 7:40 AM, Michal Privoznik wrote: [...] Since you've touched the code, Coverity looks again and determines that @newpath can be leaked above John [...]

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(a)redhat.com&gt; Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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(a)redhat.com&gt; + */ + +#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(a)redhat.com&gt; + */ + +#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(a)redhat.com&gt; + */ + +#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

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(a)redhat.com&gt; --- 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