[libvirt] [PATCH 0/5] security: Deal with stale XATTRs
There are some ways users can end up in stale XATTRs. One is sudden power loss, the other is stopping libvirt whilst some domains are running and then rebooting the host. And I believe users will find other creative ways to shut down domains without qemuProcessStop() being called. When that happens our XATTRs will be left behind and not reflect the real state of things (e.g. refcounter). To resolve this, record a timestamp within XATTRs too so that host reboots can be detected. Michal Prívozník (5): virUUIDFormat: s/VIR_UUID_RAW_LEN/VIR_UUID_BUFLEN/ in comment security_util: Use more VIR_AUTOFREE() security_util: Document virSecurityMoveRememberedLabel util: Introduce virhostuptime security_util: Remove stale XATTRs src/libvirt_private.syms | 4 + src/security/security_util.c | 293 +++++++++++++++++++++++++++++------ src/util/Makefile.inc.am | 2 + src/util/virhostuptime.c | 61 ++++++++ src/util/virhostuptime.h | 27 ++++ src/util/viruuid.c | 2 +- tests/qemusecuritymock.c | 12 ++ 7 files changed, 353 insertions(+), 48 deletions(-) create mode 100644 src/util/virhostuptime.c create mode 100644 src/util/virhostuptime.h -- 2.21.0
The function takes raw UUID and formats it into string representation. However, the comment mistakenly states that the expected size of raw UUID buffer is VIR_UUID_RAW_LEN bytes. We don't have such constant since v0.3.2~24. It should have been VIR_UUID_BUFLEN. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/viruuid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/viruuid.c b/src/util/viruuid.c index 0c12ddcc3e..8930a0e199 100644 --- a/src/util/viruuid.c +++ b/src/util/viruuid.c @@ -141,7 +141,7 @@ virUUIDParse(const char *uuidstr, unsigned char *uuid) /** * virUUIDFormat: - * @uuid: array of VIR_UUID_RAW_LEN bytes to store the raw UUID + * @uuid: array of VIR_UUID_BUFLEN bytes to store the raw UUID * @uuidstr: array of VIR_UUID_STRING_BUFLEN bytes to store the * string representation of the UUID in. The resulting string * will be NULL terminated. -- 2.21.0
On Wed, Aug 14, 2019 at 16:33:19 +0200, Michal Privoznik wrote:
The function takes raw UUID and formats it into string representation. However, the comment mistakenly states that the expected size of raw UUID buffer is VIR_UUID_RAW_LEN bytes. We don't have such constant since v0.3.2~24. It should have been VIR_UUID_BUFLEN.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/viruuid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/viruuid.c b/src/util/viruuid.c index 0c12ddcc3e..8930a0e199 100644 --- a/src/util/viruuid.c +++ b/src/util/viruuid.c @@ -141,7 +141,7 @@ virUUIDParse(const char *uuidstr, unsigned char *uuid)
/** * virUUIDFormat: - * @uuid: array of VIR_UUID_RAW_LEN bytes to store the raw UUID + * @uuid: array of VIR_UUID_BUFLEN bytes to store the raw UUID * @uuidstr: array of VIR_UUID_STRING_BUFLEN bytes to store the * string representation of the UUID in. The resulting string * will be NULL terminated.
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_util.c | 78 +++++++++++++++--------------------- 1 file changed, 32 insertions(+), 46 deletions(-) diff --git a/src/security/security_util.c b/src/security/security_util.c index 9d3f483f6b..04347f51e5 100644 --- a/src/security/security_util.c +++ b/src/security/security_util.c @@ -113,34 +113,32 @@ virSecurityGetRememberedLabel(const char *name, const char *path, char **label) { - char *ref_name = NULL; - char *attr_name = NULL; - char *value = NULL; + VIR_AUTOFREE(char *) ref_name = NULL; + VIR_AUTOFREE(char *) attr_name = NULL; + VIR_AUTOFREE(char *) value = NULL; unsigned int refcount = 0; - int ret = -1; *label = NULL; if (!(ref_name = virSecurityGetRefCountAttrName(name))) - goto cleanup; + return -1; if (virFileGetXAttrQuiet(path, ref_name, &value) < 0) { - if (errno == ENOSYS || errno == ENODATA || errno == ENOTSUP) { - ret = -2; - } else { - virReportSystemError(errno, - _("Unable to get XATTR %s on %s"), - ref_name, - path); - } - goto cleanup; + if (errno == ENOSYS || errno == ENODATA || errno == ENOTSUP) + return -2; + + virReportSystemError(errno, + _("Unable to get XATTR %s on %s"), + ref_name, + path); + return -1; } if (virStrToLong_ui(value, NULL, 10, &refcount) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("malformed refcount %s on %s"), value, path); - goto cleanup; + return -1; } VIR_FREE(value); @@ -149,30 +147,25 @@ virSecurityGetRememberedLabel(const char *name, if (refcount > 0) { if (virAsprintf(&value, "%u", refcount) < 0) - goto cleanup; + return -1; if (virFileSetXAttr(path, ref_name, value) < 0) - goto cleanup; + return -1; } else { if (virFileRemoveXAttr(path, ref_name) < 0) - goto cleanup; + return -1; if (!(attr_name = virSecurityGetAttrName(name))) - goto cleanup; + return -1; if (virFileGetXAttr(path, attr_name, label) < 0) - goto cleanup; + return -1; if (virFileRemoveXAttr(path, attr_name) < 0) - goto cleanup; + return -1; } - ret = 0; - cleanup: - VIR_FREE(value); - VIR_FREE(attr_name); - VIR_FREE(ref_name); - return ret; + return 0; } @@ -201,25 +194,23 @@ virSecuritySetRememberedLabel(const char *name, const char *path, const char *label) { - char *ref_name = NULL; - char *attr_name = NULL; - char *value = NULL; + VIR_AUTOFREE(char *) ref_name = NULL; + VIR_AUTOFREE(char *) attr_name = NULL; + VIR_AUTOFREE(char *) value = NULL; unsigned int refcount = 0; - int ret = -1; if (!(ref_name = virSecurityGetRefCountAttrName(name))) - goto cleanup; + return -1; if (virFileGetXAttrQuiet(path, ref_name, &value) < 0) { if (errno == ENOSYS || errno == ENOTSUP) { - ret = -2; - goto cleanup; + return -2; } else if (errno != ENODATA) { virReportSystemError(errno, _("Unable to get XATTR %s on %s"), ref_name, path); - goto cleanup; + return -1; } } @@ -228,7 +219,7 @@ virSecuritySetRememberedLabel(const char *name, virReportError(VIR_ERR_INTERNAL_ERROR, _("malformed refcount %s on %s"), value, path); - goto cleanup; + return -1; } VIR_FREE(value); @@ -237,24 +228,19 @@ virSecuritySetRememberedLabel(const char *name, if (refcount == 1) { if (!(attr_name = virSecurityGetAttrName(name))) - goto cleanup; + return -1; if (virFileSetXAttr(path, attr_name, label) < 0) - goto cleanup; + return -1; } if (virAsprintf(&value, "%u", refcount) < 0) - goto cleanup; + return -1; if (virFileSetXAttr(path, ref_name, value) < 0) - goto cleanup; + return -1; - ret = refcount; - cleanup: - VIR_FREE(value); - VIR_FREE(attr_name); - VIR_FREE(ref_name); - return ret; + return refcount; } -- 2.21.0
On Wed, Aug 14, 2019 at 16:33:20 +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_util.c | 78 +++++++++++++++--------------------- 1 file changed, 32 insertions(+), 46 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_util.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/security/security_util.c b/src/security/security_util.c index 04347f51e5..365b2dd2d6 100644 --- a/src/security/security_util.c +++ b/src/security/security_util.c @@ -244,6 +244,19 @@ virSecuritySetRememberedLabel(const char *name, } +/** + * virSecurityMoveRememberedLabel: + * @name: security driver name + * @src: source file + * @dst: destination file + * + * For given security driver @name, move all XATTRs related to seclabel + * remembering from @src to @dst. However, if @dst is NULL, then XATTRs + * are just removed from @src. + * + * Returns: 0 on success, + * -1 otherwise. + */ int virSecurityMoveRememberedLabel(const char *name, const char *src, -- 2.21.0
On Wed, Aug 14, 2019 at 16:33:21 +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_util.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
This module contains function to get host boot time. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 4 +++ src/util/Makefile.inc.am | 2 ++ src/util/virhostuptime.c | 61 ++++++++++++++++++++++++++++++++++++++++ src/util/virhostuptime.h | 27 ++++++++++++++++++ 4 files changed, 94 insertions(+) create mode 100644 src/util/virhostuptime.c create mode 100644 src/util/virhostuptime.h diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7a3feb8efa..bed00c3cb8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2135,6 +2135,10 @@ virHostMemGetStats; virHostMemSetParameters; +# util/virhostuptime.h +virHostGetBootTime; + + # util/viridentity.h virIdentityGetAttr; virIdentityGetCurrent; diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am index a47f333a98..46866cf213 100644 --- a/src/util/Makefile.inc.am +++ b/src/util/Makefile.inc.am @@ -91,6 +91,8 @@ UTIL_SOURCES = \ util/virhostdev.h \ util/virhostmem.c \ util/virhostmem.h \ + util/virhostuptime.c \ + util/virhostuptime.h \ util/viridentity.c \ util/viridentity.h \ util/virinitctl.c \ diff --git a/src/util/virhostuptime.c b/src/util/virhostuptime.c new file mode 100644 index 0000000000..f48de672e6 --- /dev/null +++ b/src/util/virhostuptime.c @@ -0,0 +1,61 @@ +/* + * virhostuptime.c: helper APIs for host uptime + * + * Copyright (C) 2019 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> + +#if defined(__linux__) || defined(__FreeBSD__) +# include <utmpx.h> +#endif + +#include "virhostuptime.h" + + +/** + * virHostGetBootTime: + * @when: UNIX timestamp of boot time + * + * Get a UNIX timestamp of host boot time and store it at @when. + * Note that this function is not reentrant. + * + * Return: 0 on success, + * -1 otherwise. + */ +int +virHostGetBootTime(unsigned long long *when ATTRIBUTE_UNUSED) +{ +#if defined(__linux__) || defined(__FreeBSD__) + struct utmpx id = {.ut_type = BOOT_TIME}; + struct utmpx *res = NULL; + + if (!(res = getutxid(&id))) { + int theerrno = errno; + endutxent(); + return -theerrno; + } + + *when = res->ut_tv.tv_sec; + endutxent(); + return 0; +#else + /* Unfortunately, mingw lacks utmp */ + errno = ENOSYS; + return -errno; +#endif +} diff --git a/src/util/virhostuptime.h b/src/util/virhostuptime.h new file mode 100644 index 0000000000..03c1517a64 --- /dev/null +++ b/src/util/virhostuptime.h @@ -0,0 +1,27 @@ +/* + * virhostuptime.h: helper APIs for host uptime + * + * Copyright (C) 2019 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/>. + */ + +#pragma once + +#include "internal.h" + +int +virHostGetBootTime(unsigned long long *when) + ATTRIBUTE_NOINLINE; -- 2.21.0
On Wed, Aug 14, 2019 at 16:33:22 +0200, Michal Privoznik wrote:
This module contains function to get host boot time.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 4 +++ src/util/Makefile.inc.am | 2 ++ src/util/virhostuptime.c | 61 ++++++++++++++++++++++++++++++++++++++++ src/util/virhostuptime.h | 27 ++++++++++++++++++ 4 files changed, 94 insertions(+) create mode 100644 src/util/virhostuptime.c create mode 100644 src/util/virhostuptime.h
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7a3feb8efa..bed00c3cb8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2135,6 +2135,10 @@ virHostMemGetStats; virHostMemSetParameters;
+# util/virhostuptime.h +virHostGetBootTime; + + # util/viridentity.h virIdentityGetAttr; virIdentityGetCurrent; diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am index a47f333a98..46866cf213 100644 --- a/src/util/Makefile.inc.am +++ b/src/util/Makefile.inc.am @@ -91,6 +91,8 @@ UTIL_SOURCES = \ util/virhostdev.h \ util/virhostmem.c \ util/virhostmem.h \ + util/virhostuptime.c \ + util/virhostuptime.h \ util/viridentity.c \ util/viridentity.h \ util/virinitctl.c \ diff --git a/src/util/virhostuptime.c b/src/util/virhostuptime.c new file mode 100644 index 0000000000..f48de672e6 --- /dev/null +++ b/src/util/virhostuptime.c @@ -0,0 +1,61 @@ +/* + * virhostuptime.c: helper APIs for host uptime + * + * Copyright (C) 2019 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> + +#if defined(__linux__) || defined(__FreeBSD__) +# include <utmpx.h> +#endif + +#include "virhostuptime.h" + + +/** + * virHostGetBootTime: + * @when: UNIX timestamp of boot time + * + * Get a UNIX timestamp of host boot time and store it at @when. + * Note that this function is not reentrant. + * + * Return: 0 on success, + * -1 otherwise. + */ +int +virHostGetBootTime(unsigned long long *when ATTRIBUTE_UNUSED) +{ +#if defined(__linux__) || defined(__FreeBSD__)
I believe we usually use a bit different approach: #if ... int virHostGetBootTime(unsigned long long *when) { ... } #else int virHostGetBootTime(unsigned long long *when ATTRIBUTE_UNUSED) { return ...; } #endif
+ struct utmpx id = {.ut_type = BOOT_TIME}; + struct utmpx *res = NULL; + + if (!(res = getutxid(&id))) {
getutxid is not thread safe
+ int theerrno = errno; + endutxent(); + return -theerrno; + } + + *when = res->ut_tv.tv_sec; + endutxent(); + return 0; +#else + /* Unfortunately, mingw lacks utmp */ + errno = ENOSYS; + return -errno; +#endif +}
Jirka
On Wednesday, 14 August 2019 16:33:22 CEST Michal Privoznik wrote:
+int +virHostGetBootTime(unsigned long long *when ATTRIBUTE_UNUSED) +{ +#if defined(__linux__) || defined(__FreeBSD__) + struct utmpx id = {.ut_type = BOOT_TIME}; + struct utmpx *res = NULL; + + if (!(res = getutxid(&id))) { + int theerrno = errno; + endutxent(); + return -theerrno; + } + + *when = res->ut_tv.tv_sec; + endutxent(); + return 0; +#else + /* Unfortunately, mingw lacks utmp */ + errno = ENOSYS; + return -errno; +#endif
In addition to what Jirka said, IMHO it would be better to do a configure detection of the non-POSIX function, rather than assuming on which platforms/OSes it is available. -- Pino Toscano
It may happen that we leave some XATTRs behind. For instance, on a sudden power loss, the host just shuts down without calling restore on domain paths. This creates a problem, because when the host starts up again, the XATTRs are there but they don't reflect the true state and this may result in libvirt denying start of a domain. To solve this, save a unique timestamp among with our XATTRs. The timestamp consists of host UUID + boot timestamp. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_util.c | 202 ++++++++++++++++++++++++++++++++++- tests/qemusecuritymock.c | 12 +++ 2 files changed, 213 insertions(+), 1 deletion(-) diff --git a/src/security/security_util.c b/src/security/security_util.c index 365b2dd2d6..d063f526be 100644 --- a/src/security/security_util.c +++ b/src/security/security_util.c @@ -22,11 +22,16 @@ #include "virfile.h" #include "virstring.h" #include "virerror.h" +#include "virlog.h" +#include "viruuid.h" +#include "virhostuptime.h" #include "security_util.h" #define VIR_FROM_THIS VIR_FROM_SECURITY +VIR_LOG_INIT("security.security_util"); + /* There are four namespaces available on Linux (xattr(7)): * * user - can be modified by anybody, @@ -83,6 +88,157 @@ virSecurityGetRefCountAttrName(const char *name ATTRIBUTE_UNUSED) } +static char * +virSecurityGetTimestampAttrName(const char *name ATTRIBUTE_UNUSED) +{ + char *ret = NULL; +#ifdef XATTR_NAMESPACE + ignore_value(virAsprintf(&ret, XATTR_NAMESPACE".libvirt.security.timestamp_%s", name)); +#else + errno = ENOSYS; + virReportSystemError(errno, "%s", + _("Extended attributes are not supported on this system")); +#endif + return ret; +} + + +/* This global timestamp holds combination of host UUID + boot time so that we + * can detect stale XATTRs. For instance, on a sudden power loss, XATTRs are + * not going to change (nobody will call restoreLabel()) and thus they reflect + * state from just before the power loss and if there was a machine running, + * then XATTRs there are stale and no one will ever remove them. They don't + * reflect the true state (most notably the ref counter). + */ +static char *timestamp; + + +static int +virSecurityEnsureTimestamp(void) +{ + unsigned char uuid[VIR_UUID_BUFLEN] = {0}; + char uuidstr[VIR_UUID_STRING_BUFLEN] = {0}; + unsigned long long boottime = 0; + + if (timestamp) + return 0; + + if (virGetHostUUID(uuid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot get the host uuid")); + return -1; + } + + virUUIDFormat(uuid, uuidstr); + + if (virHostGetBootTime(&boottime) < 0) { + virReportSystemError(errno, "%s", + _("Unable to get host boot time")); + return -1; + } + + if (virAsprintf(×tamp, "%s-%llu", uuidstr, boottime) < 0) + return -1; + + return 0; +} + + +/** + * virSecurityValidateTimestamp: + * @name: security driver name + * @path: file name + * + * Check if remembered label on @path for security driver @name + * is valid, i.e. the label has been set since the last boot. If + * the label was set in previous runs, all XATTRs related to + * @name are removed so that clean slate is restored. + * + * Returns: 0 if remembered label is valid, + * 1 if remembered label was not valid, + * -1 otherwise. + */ +static int +virSecurityValidateTimestamp(const char *name, + const char *path) +{ + VIR_AUTOFREE(char *) timestamp_name = NULL; + VIR_AUTOFREE(char *) value = NULL; + + if (virSecurityEnsureTimestamp() < 0) + return -1; + + if (!(timestamp_name = virSecurityGetTimestampAttrName(name))) + return -1; + + errno = 0; + if (virFileGetXAttrQuiet(path, timestamp_name, &value) < 0) { + if (errno == ENOSYS || errno == ENOTSUP) { + /* XATTRs are not supported. */ + return -1; + } else if (errno != ENODATA) { + virReportSystemError(errno, + _("Unable to get XATTR %s on %s"), + timestamp_name, + path); + return -1; + } + + /* Timestamp is missing. We can continue and claim a valid timestamp. + * But then we would never remove stale XATTRs. Therefore, claim it + * invalid and have the code below remove all XATTRs. This of course + * means that we will not restore the original owner, but the plus side + * is that we reset refcounter which will represent the true state. + */ + } + + if (STREQ_NULLABLE(value, timestamp)) { + /* Hooray, XATTRs are valid. */ + VIR_DEBUG("XATTRs on %s secdriver=%s are valid", path, name); + return 0; + } + + VIR_WARN("Invalid XATTR timestamp detected on %s secdriver=%s", path, name); + + if (virSecurityMoveRememberedLabel(name, path, NULL) < 0) + return -1; + + return 1; +} + + +static int +virSecurityAddTimestamp(const char *name, + const char *path) +{ + VIR_AUTOFREE(char *) timestamp_name = NULL; + + if (virSecurityEnsureTimestamp() < 0) + return -1; + + if (!(timestamp_name = virSecurityGetTimestampAttrName(name))) + return -1; + + return virFileSetXAttr(path, timestamp_name, timestamp); +} + + +static int +virSecurityRemoveTimestamp(const char *name, + const char *path) +{ + VIR_AUTOFREE(char *) timestamp_name = NULL; + + if (!(timestamp_name = virSecurityGetTimestampAttrName(name))) + return -1; + + if (virFileRemoveXAttr(path, timestamp_name) < 0 && errno != ENOENT) + return -1; + + return 0; +} + + /** * virSecurityGetRememberedLabel: * @name: security driver name @@ -120,6 +276,12 @@ virSecurityGetRememberedLabel(const char *name, *label = NULL; + if (virSecurityValidateTimestamp(name, path) < 0) { + if (errno == ENOSYS || errno == ENOTSUP) + return -2; + return -1; + } + if (!(ref_name = virSecurityGetRefCountAttrName(name))) return -1; @@ -163,6 +325,9 @@ virSecurityGetRememberedLabel(const char *name, if (virFileRemoveXAttr(path, attr_name) < 0) return -1; + + if (virSecurityRemoveTimestamp(name, path) < 0) + return -1; } return 0; @@ -199,6 +364,12 @@ virSecuritySetRememberedLabel(const char *name, VIR_AUTOFREE(char *) value = NULL; unsigned int refcount = 0; + if (virSecurityValidateTimestamp(name, path) < 0) { + if (errno == ENOSYS || errno == ENOTSUP) + return -2; + return -1; + } + if (!(ref_name = virSecurityGetRefCountAttrName(name))) return -1; @@ -232,6 +403,9 @@ virSecuritySetRememberedLabel(const char *name, if (virFileSetXAttr(path, attr_name, label) < 0) return -1; + + if (virSecurityAddTimestamp(name, path) < 0) + return -1; } if (virAsprintf(&value, "%u", refcount) < 0) @@ -266,9 +440,12 @@ virSecurityMoveRememberedLabel(const char *name, VIR_AUTOFREE(char *) ref_value = NULL; VIR_AUTOFREE(char *) attr_name = NULL; VIR_AUTOFREE(char *) attr_value = NULL; + VIR_AUTOFREE(char *) timestamp_name = NULL; + VIR_AUTOFREE(char *) timestamp_value = NULL; if (!(ref_name = virSecurityGetRefCountAttrName(name)) || - !(attr_name = virSecurityGetAttrName(name))) + !(attr_name = virSecurityGetAttrName(name)) || + !(timestamp_name = virSecurityGetTimestampAttrName(name))) return -1; if (virFileGetXAttrQuiet(src, ref_name, &ref_value) < 0) { @@ -293,6 +470,17 @@ virSecurityMoveRememberedLabel(const char *name, } } + if (virFileGetXAttrQuiet(src, timestamp_name, ×tamp_value) < 0) { + if (errno == ENOSYS || errno == ENOTSUP) { + return -2; + } else if (errno != ENODATA) { + virReportSystemError(errno, + _("Unable to get XATTR %s on %s"), + attr_name, src); + return -1; + } + } + if (ref_value && virFileRemoveXAttr(src, ref_name) < 0) { return -1; @@ -303,6 +491,11 @@ virSecurityMoveRememberedLabel(const char *name, return -1; } + if (timestamp_value && + virFileRemoveXAttr(src, timestamp_name) < 0) { + return -1; + } + if (dst) { if (ref_value && virFileSetXAttr(dst, ref_name, ref_value) < 0) { @@ -314,6 +507,13 @@ virSecurityMoveRememberedLabel(const char *name, ignore_value(virFileRemoveXAttr(dst, ref_name)); return -1; } + + if (timestamp_value && + virFileSetXAttr(dst, timestamp_name, timestamp_value) < 0) { + ignore_value(virFileRemoveXAttr(dst, ref_name)); + ignore_value(virFileRemoveXAttr(dst, attr_name)); + return -1; + } } return 0; diff --git a/tests/qemusecuritymock.c b/tests/qemusecuritymock.c index a15eef29c9..373d64305a 100644 --- a/tests/qemusecuritymock.c +++ b/tests/qemusecuritymock.c @@ -32,6 +32,7 @@ #include "viralloc.h" #include "qemusecuritytest.h" #include "security/security_manager.h" +#include "virhostuptime.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -488,3 +489,14 @@ virProcessRunInFork(virProcessForkCallback cb, { return cb(-1, opaque); } + + +/* We don't really need to mock this function. The qemusecuritytest doesn't + * care about the actual value. However, travis runs build and tests in a + * container where utmp is missing and thus this function fails. */ +int +virHostGetBootTime(unsigned long long *when) +{ + *when = 1234567890; + return 0; +} -- 2.21.0
On Wed, Aug 14, 2019 at 16:33:23 +0200, Michal Privoznik wrote:
It may happen that we leave some XATTRs behind. For instance, on a sudden power loss, the host just shuts down without calling restore on domain paths. This creates a problem, because when the host starts up again, the XATTRs are there but they don't reflect the true state and this may result in libvirt denying start of a domain.
To solve this, save a unique timestamp among with our XATTRs. The timestamp consists of host UUID + boot timestamp.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_util.c | 202 ++++++++++++++++++++++++++++++++++- tests/qemusecuritymock.c | 12 +++ 2 files changed, 213 insertions(+), 1 deletion(-)
diff --git a/src/security/security_util.c b/src/security/security_util.c index 365b2dd2d6..d063f526be 100644 --- a/src/security/security_util.c +++ b/src/security/security_util.c @@ -22,11 +22,16 @@ #include "virfile.h" #include "virstring.h" #include "virerror.h" +#include "virlog.h" +#include "viruuid.h" +#include "virhostuptime.h"
#include "security_util.h"
#define VIR_FROM_THIS VIR_FROM_SECURITY
+VIR_LOG_INIT("security.security_util"); + /* There are four namespaces available on Linux (xattr(7)): * * user - can be modified by anybody, @@ -83,6 +88,157 @@ virSecurityGetRefCountAttrName(const char *name ATTRIBUTE_UNUSED) }
+static char * +virSecurityGetTimestampAttrName(const char *name ATTRIBUTE_UNUSED) +{ + char *ret = NULL; +#ifdef XATTR_NAMESPACE + ignore_value(virAsprintf(&ret, XATTR_NAMESPACE".libvirt.security.timestamp_%s", name)); +#else + errno = ENOSYS; + virReportSystemError(errno, "%s", + _("Extended attributes are not supported on this system")); +#endif + return ret; +}
Again, put #ifdef outside, please.
+ + +/* This global timestamp holds combination of host UUID + boot time so that we + * can detect stale XATTRs. For instance, on a sudden power loss, XATTRs are + * not going to change (nobody will call restoreLabel()) and thus they reflect + * state from just before the power loss and if there was a machine running, + * then XATTRs there are stale and no one will ever remove them. They don't + * reflect the true state (most notably the ref counter). + */ +static char *timestamp; + + +static int +virSecurityEnsureTimestamp(void) +{ + unsigned char uuid[VIR_UUID_BUFLEN] = {0}; + char uuidstr[VIR_UUID_STRING_BUFLEN] = {0}; + unsigned long long boottime = 0; + + if (timestamp) + return 0; + + if (virGetHostUUID(uuid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot get the host uuid")); + return -1; + } + + virUUIDFormat(uuid, uuidstr); + + if (virHostGetBootTime(&boottime) < 0) { + virReportSystemError(errno, "%s", + _("Unable to get host boot time")); + return -1; + } + + if (virAsprintf(×tamp, "%s-%llu", uuidstr, boottime) < 0) + return -1; + + return 0; +} + + +/** + * virSecurityValidateTimestamp: + * @name: security driver name + * @path: file name + * + * Check if remembered label on @path for security driver @name + * is valid, i.e. the label has been set since the last boot. If + * the label was set in previous runs, all XATTRs related to + * @name are removed so that clean slate is restored. + * + * Returns: 0 if remembered label is valid, + * 1 if remembered label was not valid, + * -1 otherwise. + */ +static int +virSecurityValidateTimestamp(const char *name, + const char *path) +{ + VIR_AUTOFREE(char *) timestamp_name = NULL; + VIR_AUTOFREE(char *) value = NULL; + + if (virSecurityEnsureTimestamp() < 0) + return -1; + + if (!(timestamp_name = virSecurityGetTimestampAttrName(name))) + return -1; + + errno = 0; + if (virFileGetXAttrQuiet(path, timestamp_name, &value) < 0) { + if (errno == ENOSYS || errno == ENOTSUP) { + /* XATTRs are not supported. */
Redundant comment.
+ return -1; + } else if (errno != ENODATA) { + virReportSystemError(errno, + _("Unable to get XATTR %s on %s"), + timestamp_name, + path); + return -1; + } + + /* Timestamp is missing. We can continue and claim a valid timestamp. + * But then we would never remove stale XATTRs. Therefore, claim it + * invalid and have the code below remove all XATTRs. This of course + * means that we will not restore the original owner, but the plus side + * is that we reset refcounter which will represent the true state. + */ + } + + if (STREQ_NULLABLE(value, timestamp)) { + /* Hooray, XATTRs are valid. */
Redundant comment.
+ VIR_DEBUG("XATTRs on %s secdriver=%s are valid", path, name); + return 0; + }
I believe the reason for having UUID in the timestamp is to be able to detect when the label was remembered on a different host. But here, you completely ignore this and always remove the remembered labels when it was remembered on a different host or after a reboot. And since you will need to check the UUID separately from the timestamp, I think it would make sense to store them separately rather than combining them in a single value.
+ + VIR_WARN("Invalid XATTR timestamp detected on %s secdriver=%s", path, name); + + if (virSecurityMoveRememberedLabel(name, path, NULL) < 0) + return -1; + + return 1; +}
Jirka
participants (3)
-
Jiri Denemark -
Michal Privoznik -
Pino Toscano