[libvirt] [PATCH 5/5] security_util: Remove stale XATTRs
Michal Privoznik
mprivozn at redhat.com
Wed Aug 14 14:33:23 UTC 2019
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 at 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
More information about the libvir-list
mailing list