[libvirt] [PATCH 5/5] security_util: Remove stale XATTRs

[Jiri Denemark]

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

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(&timestamp, "%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