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 (host boot time) among
with our XATTRs.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/security/security_util.c    | 196 +++++++++++++++++++++++++++++++-
 tests/qemusecuritymock.c        |  12 ++
 tools/libvirt_recover_xattrs.sh |   2 +-
 3 files changed, 208 insertions(+), 2 deletions(-)

diff --git a/src/security/security_util.c b/src/security/security_util.c
index 365b2dd2d6..c65e27e6d4 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,151 @@ virSecurityGetRefCountAttrName(const char *name ATTRIBUTE_UNUSED)
 }
 
 
+#ifdef XATTR_NAMESPACE
+static char *
+virSecurityGetTimestampAttrName(const char *name)
+{
+    char *ret = NULL;
+    ignore_value(virAsprintf(&ret, XATTR_NAMESPACE".libvirt.security.timestamp_%s", name));
+    return ret;
+}
+#else /* !XATTR_NAMESPACE */
+static char *
+virSecurityGetTimestampAttrName(const char *name ATTRIBUTE_UNUSED)
+{
+    errno = ENOSYS;
+    virReportSystemError(errno, "%s",
+                         _("Extended attributes are not supported on this system"));
+    return NULL;
+}
+#endif /* !XATTR_NAMESPACE */
+
+
+static char *
+virSecurityGetTimestamp(void)
+{
+    unsigned long long boottime = 0;
+    char *ret = NULL;
+
+    if (virHostGetBootTime(&boottime) < 0) {
+        virReportSystemError(errno, "%s",
+                             _("Unable to get host boot time"));
+        return NULL;
+    }
+
+    ignore_value(virAsprintf(&ret, "%llu", boottime));
+    return ret;
+}
+
+
+/**
+ * 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.
+ *
+ * This is done having extra attribute timestamp_$SECDRIVER which
+ * contains the host boot time. Its value is then compared to
+ * actual host boot time. If these two values don't match then
+ * XATTRs are considered as stale and thus invalid.
+ *
+ * In ideal world, where there network file systems have XATTRs
+ * using plain host boot time is not enough as it may lead to a
+ * situation where a freshly started host sees XATTRs, sees the
+ * timestamp put there by some longer running host and considers
+ * the XATTRs invalid. Well, there is not an easy way out. We
+ * would need to somehow check if the longer running host is
+ * still there and is the @path (how?).
+ * Fortunately, there is only one network file system which
+ * supports XATTRs currently (GlusterFS via FUSE) and it is used
+ * so rarely that it's almost a corner case.
+ * The worst thing that happens there is that we remove XATTRs
+ * and thus return @path to the default label for $SECDRIVER.
+ *
+ * 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 *) expected_timestamp = NULL;
+    VIR_AUTOFREE(char *) timestamp_name = NULL;
+    VIR_AUTOFREE(char *) value = NULL;
+
+    if (!(expected_timestamp = virSecurityGetTimestamp()) ||
+        !(timestamp_name = virSecurityGetTimestampAttrName(name)))
+        return -1;
+
+    errno = 0;
+    if (virFileGetXAttrQuiet(path, timestamp_name, &value) < 0) {
+        if (errno == ENOSYS || errno == ENOTSUP) {
+            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, expected_timestamp)) {
+        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;
+    VIR_AUTOFREE(char *) timestamp_value = NULL;
+
+    if (!(timestamp_name = virSecurityGetTimestampAttrName(name)))
+        return -1;
+
+    return virFileSetXAttr(path, timestamp_name, timestamp_value);
+}
+
+
+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 +270,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 +319,9 @@ virSecurityGetRememberedLabel(const char *name,
 
         if (virFileRemoveXAttr(path, attr_name) < 0)
             return -1;
+
+        if (virSecurityRemoveTimestamp(name, path) < 0)
+            return -1;
     }
 
     return 0;
@@ -199,6 +358,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 +397,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 +434,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 +464,17 @@ virSecurityMoveRememberedLabel(const char *name,
         }
     }
 
+    if (virFileGetXAttrQuiet(src, timestamp_name, &timestamp_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 +485,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 +501,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;
+}
diff --git a/tools/libvirt_recover_xattrs.sh b/tools/libvirt_recover_xattrs.sh
index 58f02f8dfb..3907413c63 100755
--- a/tools/libvirt_recover_xattrs.sh
+++ b/tools/libvirt_recover_xattrs.sh
@@ -74,7 +74,7 @@ fi
 declare -a XATTRS
 for i in "dac" "selinux"; do
     for p in ${LIBVIRT_XATTR_PREFIXES[@]}; do
-        XATTRS+=("$p.$i" "$p.ref_$i")
+        XATTRS+=("$p.$i" "$p.ref_$i" "$p.timestamp_$i")
     done
 done
 
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Aug 22, 2019 at 13:15:33 +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 (host boot time) among
> with our XATTRs.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/security/security_util.c    | 196 +++++++++++++++++++++++++++++++-
>  tests/qemusecuritymock.c        |  12 ++
>  tools/libvirt_recover_xattrs.sh |   2 +-
>  3 files changed, 208 insertions(+), 2 deletions(-)
> 
> diff --git a/src/security/security_util.c b/src/security/security_util.c
> index 365b2dd2d6..c65e27e6d4 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,151 @@ virSecurityGetRefCountAttrName(const char *name ATTRIBUTE_UNUSED)
>  }
>  
>  
> +#ifdef XATTR_NAMESPACE
> +static char *
> +virSecurityGetTimestampAttrName(const char *name)
> +{
> +    char *ret = NULL;
> +    ignore_value(virAsprintf(&ret, XATTR_NAMESPACE".libvirt.security.timestamp_%s", name));

I'd put a space between XATTR_NAMESPACE and ".

> +    return ret;
> +}
> +#else /* !XATTR_NAMESPACE */
> +static char *
> +virSecurityGetTimestampAttrName(const char *name ATTRIBUTE_UNUSED)
> +{
> +    errno = ENOSYS;
> +    virReportSystemError(errno, "%s",
> +                         _("Extended attributes are not supported on this system"));
> +    return NULL;
> +}
> +#endif /* !XATTR_NAMESPACE */
> +
> +
> +static char *
> +virSecurityGetTimestamp(void)
> +{
> +    unsigned long long boottime = 0;
> +    char *ret = NULL;
> +
> +    if (virHostGetBootTime(&boottime) < 0) {
> +        virReportSystemError(errno, "%s",
> +                             _("Unable to get host boot time"));
> +        return NULL;
> +    }
> +
> +    ignore_value(virAsprintf(&ret, "%llu", boottime));
> +    return ret;
> +}
> +
> +
> +/**
> + * 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.
> + *
> + * This is done having extra attribute timestamp_$SECDRIVER which
> + * contains the host boot time. Its value is then compared to
> + * actual host boot time. If these two values don't match then
> + * XATTRs are considered as stale and thus invalid.
> + *
> + * In ideal world, where there network file systems have XATTRs
> + * using plain host boot time is not enough as it may lead to a
> + * situation where a freshly started host sees XATTRs, sees the
> + * timestamp put there by some longer running host and considers
> + * the XATTRs invalid. Well, there is not an easy way out. We
> + * would need to somehow check if the longer running host is
> + * still there and is the @path (how?).

Looks like there is a missing word in the sentence above.

> + * Fortunately, there is only one network file system which
> + * supports XATTRs currently (GlusterFS via FUSE) and it is used
> + * so rarely that it's almost a corner case.
> + * The worst thing that happens there is that we remove XATTRs
> + * and thus return @path to the default label for $SECDRIVER.
> + *
> + * 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 *) expected_timestamp = NULL;
> +    VIR_AUTOFREE(char *) timestamp_name = NULL;
> +    VIR_AUTOFREE(char *) value = NULL;
> +
> +    if (!(expected_timestamp = virSecurityGetTimestamp()) ||
> +        !(timestamp_name = virSecurityGetTimestampAttrName(name)))
> +        return -1;
> +
> +    errno = 0;
> +    if (virFileGetXAttrQuiet(path, timestamp_name, &value) < 0) {
> +        if (errno == ENOSYS || errno == ENOTSUP) {
> +            return -1;

This is the only path which returns -1 without setting an error. I know
it's because the caller will check errno and do something else then with
other paths that return -1, but it's weird. I think it would be much
better to just return -2 here and let the caller use only the return
value without looking at errno.

> +        } 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.

s/can/could/

> +         * 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, expected_timestamp)) {
> +        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;
> +    VIR_AUTOFREE(char *) timestamp_value = NULL;
> +
> +    if (!(timestamp_name = virSecurityGetTimestampAttrName(name)))

Missing timestamp_value = virSecurityGetTimestamp()

> +        return -1;
> +
> +    return virFileSetXAttr(path, timestamp_name, timestamp_value);
> +}
> +
> +
> +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 +270,12 @@ virSecurityGetRememberedLabel(const char *name,
>  
>      *label = NULL;
>  
> +    if (virSecurityValidateTimestamp(name, path) < 0) {

Here you could just return what virSecurityValidateTimestamp returned if
you follow what I suggested above.

> +        if (errno == ENOSYS || errno == ENOTSUP)
> +            return -2;
> +        return -1;
> +    }
> +
>      if (!(ref_name = virSecurityGetRefCountAttrName(name)))
>          return -1;
>  
> @@ -163,6 +319,9 @@ virSecurityGetRememberedLabel(const char *name,
>  
>          if (virFileRemoveXAttr(path, attr_name) < 0)
>              return -1;
> +
> +        if (virSecurityRemoveTimestamp(name, path) < 0)
> +            return -1;
>      }
>  
>      return 0;
> @@ -199,6 +358,12 @@ virSecuritySetRememberedLabel(const char *name,
>      VIR_AUTOFREE(char *) value = NULL;
>      unsigned int refcount = 0;
>  
> +    if (virSecurityValidateTimestamp(name, path) < 0) {

Here you could just return what virSecurityValidateTimestamp returned if
you follow what I suggested above.

> +        if (errno == ENOSYS || errno == ENOTSUP)
> +            return -2;
> +        return -1;
> +    }
> +
>      if (!(ref_name = virSecurityGetRefCountAttrName(name)))
>          return -1;
>  
...

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list