[libvirt] [PATCH v2 00/18] Implement original label remembering

This file implements wrappers over XATTR getter/setter. It
ensures the proper XATTR namespace is used.

Signed-off-by: Michal Privoznik <***@redhat.com>
---
src/security/Makefile.inc.am | 2 +
src/security/security_util.c | 226 +++++++++++++++++++++++++++++++++++
src/security/security_util.h | 32 +++++
3 files changed, 260 insertions(+)
create mode 100644 src/security/security_util.c
create mode 100644 src/security/security_util.h

diff --git a/src/security/Makefile.inc.am b/src/security/Makefile.inc.am
index f88b82df7b..0ade97d355 100644
--- a/src/security/Makefile.inc.am
+++ b/src/security/Makefile.inc.am
@@ -14,6 +14,8 @@ SECURITY_DRIVER_SOURCES = \
security/security_dac.c \
security/security_manager.h \
security/security_manager.c \
+ security/security_util.h \
+ security/security_util.c \
$(NULL)

SECURITY_DRIVER_SELINUX_SOURCES = \
diff --git a/src/security/security_util.c b/src/security/security_util.c
new file mode 100644
index 0000000000..7b3cef5e1a
--- /dev/null
+++ b/src/security/security_util.c
@@ -0,0 +1,226 @@
+/*
+ * 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/>.
+ */
+
+#include <config.h>
+
+#include "viralloc.h"
+#include "virfile.h"
+#include "virstring.h"
+#include "virerror.h"
+
+#include "security_util.h"
+
+#define VIR_FROM_THIS VIR_FROM_SECURITY
+
+/* There are four namespaces available on Linux (xattr(7)):
+ *
+ * user - can be modified by anybody,
+ * system - used by ACLs
+ * security - used by SELinux
+ * trusted - accessibly by CAP_SYS_ADMIN processes only
+ *
+ * Looks like the last one is way to go.
+ * Unfortunately, FreeBSD only supports:
+ *
+ * user - can be modified by anybody,
+ * system - accessible by CAP_SYS_ADMIN processes only
+ *
+ * Note that 'system' on FreeBSD corresponds to 'trusted' on
+ * Linux. So far the only point where FreeBSD and Linux can meet
+ * is NFS which still doesn't support XATTRs. Therefore we can
+ * use different namespace on each system. If NFS gains support
+ * for XATTRs then we have to find a way to deal with the
+ * different namespaces. But that is a problem for future me.
+ */
+#if defined(__linux__)
+# define XATTR_NAMESPACE "trusted"
+#elif defined(__FreeBSD__)
+# define XATTR_NAMESPACE "system"
+#endif
+
+static char *
+virSecurityGetAttrName(const char *name ATTRIBUTE_UNUSED)
+{
+ char *ret = NULL;
+#ifdef XATTR_NAMESPACE
+ ignore_value(virAsprintf(&ret, XATTR_NAMESPACE".libvirt.security.%s", name));
+#else
+ errno = ENOSYS;
+ virReportSystemError(errno, "%s",
+ _("Extended attributes are not supported on this system"));
+#endif
+ return ret;
+}
+
+
+static char *
+virSecurityGetRefCountAttrName(const char *name ATTRIBUTE_UNUSED)
+{
+ char *ret = NULL;
+#ifdef XATTR_NAMESPACE
+ ignore_value(virAsprintf(&ret, XATTR_NAMESPACE".libvirt.security.ref_%s", name));
+#else
+ errno = ENOSYS;
+ virReportSystemError(errno, "%s",
+ _("Extended attributes are not supported on this system"));
+#endif
+ return ret;
+}
+
+
+/**
+ * virSecurityGetRememberedLabel:
+ * @name: security driver name
+ * @path: file name
+ * @label: label
+ *
+ * For given @path and security driver (@name) fetch remembered
+ * @label. The caller must not restore label if an error is
+ * indicated or if @label is NULL upon return.
+ *
+ * Returns: 0 on success,
+ * -1 otherwise (with error reported)
+ */
+int
+virSecurityGetRememberedLabel(const char *name,
+ const char *path,
+ char **label)
+{
+ char *ref_name = NULL;
+ char *attr_name = NULL;
+ char *value = NULL;
+ unsigned int refcount = 0;
+ int ret = -1;
+
+ *label = NULL;
+
+ if (!(ref_name = virSecurityGetRefCountAttrName(name)))
+ goto cleanup;
+
+ if (virFileGetXAtrr(path, ref_name, &value) < 0) {
+ if (errno == ENOSYS || errno == ENODATA || errno == ENOTSUP) {
+ ret = 0;
+ } else {
+ virReportSystemError(errno,
+ _("Unable to get XATTR %s on %s"),
+ ref_name,
+ path);
+ }
+ goto cleanup;
+ }
+
+ if (virStrToLong_ui(value, NULL, 10, &refcount) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("malformed refcount %s on %s"),
+ value, path);
+ goto cleanup;
+ }
+
+ VIR_FREE(value);
+
+ refcount--;
+
+ if (refcount > 0) {
+ if (virAsprintf(&value, "%u", refcount) < 0)
+ goto cleanup;
+
+ if (virFileSetXAtrr(path, ref_name, value) < 0)
+ goto cleanup;
+ } else {
+ if (virFileRemoveXAttr(path, ref_name) < 0)
+ goto cleanup;
+
+ if (!(attr_name = virSecurityGetAttrName(name)))
+ goto cleanup;
+
+ if (virFileGetXAtrr(path, attr_name, label) < 0)
+ goto cleanup;
+
+ if (virFileRemoveXAttr(path, attr_name) < 0)
+ goto cleanup;
+ }
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(value);
+ VIR_FREE(attr_name);
+ VIR_FREE(ref_name);
+ return ret;
+}
+
+
+int
+virSecuritySetRememberedLabel(const char *name,
+ const char *path,
+ const char *label)
+{
+ char *ref_name = NULL;
+ char *attr_name = NULL;
+ char *value = NULL;
+ unsigned int refcount = 0;
+ int ret = -1;
+
+ if (!(ref_name = virSecurityGetRefCountAttrName(name)))
+ goto cleanup;
+
+ if (virFileGetXAtrr(path, ref_name, &value) < 0) {
+ if (errno == ENOSYS || errno == ENOTSUP) {
+ ret = 0;
+ goto cleanup;
+ } else if (errno != ENODATA) {
+ virReportSystemError(errno,
+ _("Unable to get XATTR %s on %s"),
+ ref_name,
+ path);
+ goto cleanup;
+ }
+ }
+
+ if (value &&
+ virStrToLong_ui(value, NULL, 10, &refcount) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("malformed refcount %s on %s"),
+ value, path);
+ goto cleanup;
+ }
+
+ VIR_FREE(value);
+
+ refcount++;
+
+ if (refcount == 1) {
+ if (!(attr_name = virSecurityGetAttrName(name)))
+ goto cleanup;
+
+ if (virFileSetXAtrr(path, attr_name, label) < 0)
+ goto cleanup;
+ }
+
+ if (virAsprintf(&value, "%u", refcount) < 0)
+ goto cleanup;
+
+ if (virFileSetXAtrr(path, ref_name, value) < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(value);
+ VIR_FREE(attr_name);
+ VIR_FREE(ref_name);
+ return ret;
+}
diff --git a/src/security/security_util.h b/src/security/security_util.h
new file mode 100644
index 0000000000..a6e67f4390
--- /dev/null
+++ b/src/security/security_util.h
@@ -0,0 +1,32 @@
+/*
+ * 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/>.
+ */
+
+#ifndef __SECURITY_UTIL_H__
+# define __SECURITY_UTIL_H__
+
+int
+virSecurityGetRememberedLabel(const char *name,
+ const char *path,
+ char **label);
+
+int
+virSecuritySetRememberedLabel(const char *name,
+ const char *path,
+ const char *label);
+
+#endif /* __SECURITY_UTIL_H__ */

--
2.18.1

Daniel P. Berrangé

2018-12-06 11:38:23 UTC

Post by Michal Privoznik
This file implements wrappers over XATTR getter/setter. It
ensures the proper XATTR namespace is used.
---
src/security/Makefile.inc.am | 2 +
src/security/security_util.c | 226 +++++++++++++++++++++++++++++++++++
src/security/security_util.h | 32 +++++
3 files changed, 260 insertions(+)
create mode 100644 src/security/security_util.c
create mode 100644 src/security/security_util.h
+/**
+ *
+ *
+ * Returns: 0 on success,
+ * -1 otherwise (with error reported)
+ */
+int
+virSecurityGetRememberedLabel(const char *name,
+ const char *path,
+ char **label)
+{
+ char *ref_name = NULL;
+ char *attr_name = NULL;
+ char *value = NULL;
+ unsigned int refcount = 0;
+ int ret = -1;
+
+ *label = NULL;
+
+ if (!(ref_name = virSecurityGetRefCountAttrName(name)))
+ goto cleanup;
+
+ if (virFileGetXAtrr(path, ref_name, &value) < 0) {
+ if (errno == ENOSYS || errno == ENODATA || errno == ENOTSUP) {
+ ret = 0;
+ } else {
+ virReportSystemError(errno,
+ _("Unable to get XATTR %s on %s"),
+ ref_name,
+ path);
+ }
+ goto cleanup;
+ }
+
+ if (virStrToLong_ui(value, NULL, 10, &refcount) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("malformed refcount %s on %s"),
+ value, path);
+ goto cleanup;
+ }
+
+ VIR_FREE(value);
+
+ refcount--;
+
+ if (refcount > 0) {
+ if (virAsprintf(&value, "%u", refcount) < 0)
+ goto cleanup;
+
+ if (virFileSetXAtrr(path, ref_name, value) < 0)
+ goto cleanup;
+ } else {
+ if (virFileRemoveXAttr(path, ref_name) < 0)
+ goto cleanup;
+
+ if (!(attr_name = virSecurityGetAttrName(name)))
+ goto cleanup;
+
+ if (virFileGetXAtrr(path, attr_name, label) < 0)
+ goto cleanup;

I'm not understanding why we only fetch the label value in
this half of the conditional ? Shouldn't we be unconditionally
getting the label, regardless of the updated refcount value.

If not, the method description could have an explanation of
intended behaviour.

Post by Michal Privoznik
+
+ if (virFileRemoveXAttr(path, attr_name) < 0)
+ goto cleanup;
+ }
+
+ ret = 0;
+ VIR_FREE(value);
+ VIR_FREE(attr_name);
+ VIR_FREE(ref_name);
+ return ret;
+}
+
+
+int
+virSecuritySetRememberedLabel(const char *name,
+ const char *path,
+ const char *label)
+{
+ char *ref_name = NULL;
+ char *attr_name = NULL;
+ char *value = NULL;
+ unsigned int refcount = 0;
+ int ret = -1;
+
+ if (!(ref_name = virSecurityGetRefCountAttrName(name)))
+ goto cleanup;
+
+ if (virFileGetXAtrr(path, ref_name, &value) < 0) {
+ if (errno == ENOSYS || errno == ENOTSUP) {
+ ret = 0;
+ goto cleanup;
+ } else if (errno != ENODATA) {
+ virReportSystemError(errno,
+ _("Unable to get XATTR %s on %s"),
+ ref_name,
+ path);
+ goto cleanup;
+ }
+ }
+
+ if (value &&
+ virStrToLong_ui(value, NULL, 10, &refcount) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("malformed refcount %s on %s"),
+ value, path);
+ goto cleanup;
+ }
+
+ VIR_FREE(value);
+
+ refcount++;
+
+ if (refcount == 1) {
+ if (!(attr_name = virSecurityGetAttrName(name)))
+ goto cleanup;
+
+ if (virFileSetXAtrr(path, attr_name, label) < 0)
+ goto cleanup;
+ }

Do we need to have a

} else {
.... check the currently remember label matches
what was passed into this method ?
}

if not, can you add API docs for this method explainng the
intended semantics when label is already remembered.

Post by Michal Privoznik
+
+ if (virAsprintf(&value, "%u", refcount) < 0)
+ goto cleanup;
+
+ if (virFileSetXAtrr(path, ref_name, value) < 0)
+ goto cleanup;
+
+ ret = 0;
+ VIR_FREE(value);
+ VIR_FREE(attr_name);
+ VIR_FREE(ref_name);
+ return ret;
+}

Regards,
Daniel

--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Michal Privoznik

2018-12-06 14:17:47 UTC

I'm not understanding why we only fetch the label value in
this half of the conditional ? Shouldn't we be unconditionally
getting the label, regardless of the updated refcount value.
If not, the method description could have an explanation of
intended behaviour.

The idea is that the first time we set a label for a file the owner is
stored in XATTRs and refcount is set to 1. Any other attempt to chown()
will increase the counter (and do the chown() too). Then, restore
decrements the counter and only if it reaches 0 the original owner is
read from XATTRs and passed to secdriver which is a signal to it that it
needs to actually restore the owner.

Anyway, I will put it into a comment.

Do we need to have a
} else {
.... check the currently remember label matches
what was passed into this method ?
}
if not, can you add API docs for this method explainng the
intended semantics when label is already remembered.

I don't think that's a good idea because that sets us off onto a path
where we'd have to determine whether a file is accessible or not. I
mean, if the current owner is UID_A:GID_A (and qemu has access through
both) and this new label passed into here is UID_B:GID_B that doesn't
necessarily mean that qemu loses the access (UID_A can be a member of
GID_B).

Sure, this doesn't prevent us from the following scenario:

1) set a seclabel for domA on path X
2) user tries to start domB with a different seclabel which also
requires path X
3) while starting domB, libvirt overwrites seclabel on X effectively
cutting of domA
4) imagine domB can't be started (or is destroyed later or whatever),
libvirt starts restore, but since X's refcount = 1, the original owner
is not restored (domA is still cut off)

But I don't see any easy solution to that. I mean:
a) As I say we don't want to check in 3) if the new seclabel would cut
off domA. That's for kernel to decide.
b) If we would want to rollback in 4) and set domA seclabel on X we
would need a very clever algorithm. We would need to store an array of
seclabels in XATTR and match them with domains that are doing the
restore. For instance: there are three domains domA, domB and domC and
assume they all share path X and have their own seclabels defined
(A:UID, B:UID, C:UID). Lets start them all and the XATTR on X would look
like this:

{[original owner],[A:UID],[B:UID]} (and the X is currently owned by C:UID)

Now, domA is shut off. How would I know what item in the array to
remove? Note that [A:UID] was added when starting domB, and in fact
[original owner] was added by domA.

Michal

Daniel P. Berrangé

2018-12-06 14:34:43 UTC

Post by Michal Privoznik
This file implements wrappers over XATTR getter/setter. It
ensures the proper XATTR namespace is used.
---
src/security/Makefile.inc.am | 2 +
src/security/security_util.c | 226 +++++++++++++++++++++++++++++++++++
src/security/security_util.h | 32 +++++
3 files changed, 260 insertions(+)
create mode 100644 src/security/security_util.c
create mode 100644 src/security/security_util.h
+int
+virSecuritySetRememberedLabel(const char *name,
+ const char *path,
+ const char *label)
+{
+ char *ref_name = NULL;
+ char *attr_name = NULL;
+ char *value = NULL;
+ unsigned int refcount = 0;
+ int ret = -1;
+
+ if (!(ref_name = virSecurityGetRefCountAttrName(name)))
+ goto cleanup;
+
+ if (virFileGetXAtrr(path, ref_name, &value) < 0) {
+ if (errno == ENOSYS || errno == ENOTSUP) {
+ ret = 0;
+ goto cleanup;
+ } else if (errno != ENODATA) {
+ virReportSystemError(errno,
+ _("Unable to get XATTR %s on %s"),
+ ref_name,
+ path);
+ goto cleanup;
+ }
+ }
+
+ if (value &&
+ virStrToLong_ui(value, NULL, 10, &refcount) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("malformed refcount %s on %s"),
+ value, path);
+ goto cleanup;
+ }
+
+ VIR_FREE(value);
+
+ refcount++;
+
+ if (refcount == 1) {
+ if (!(attr_name = virSecurityGetAttrName(name)))
+ goto cleanup;
+
+ if (virFileSetXAtrr(path, attr_name, label) < 0)
+ goto cleanup;
+ }

Do we need to have a
} else {
.... check the currently remember label matches
what was passed into this method ?
}
if not, can you add API docs for this method explainng the
intended semantics when label is already remembered.

I wasn't actually suggesting checking accessibility.

IMHO if you are going to have 2 guests both accessing the same file,
we should simply declare that they *MUST* both use the same label.

Simply reject the idea that the 2nd guest can change the label, using
a GID_B that magically still allows guest A to access it. Such scenarios
are way more likely to be an admin screw up than an intentional decision.

As an admin - the guest A might set svirt_image_t:s0:c12,c35. The guest B
then comes along and sets 'svirt_image_t:s0' which grants access to
all guests. This is a clearly a mistake because the first guests' label
was an exclusive access label, while the second guests label was a shared
access label. The lock manager should have blocked this, but I still
think we should also block it here, as the lock manager won't block it
until after you've already set the labels.

IOW we're justified in requiring strict equality of labels for all
guests, even if they technically might prevent something the kernel would
otherwise allow.

Post by Michal Privoznik
1) set a seclabel for domA on path X
2) user tries to start domB with a different seclabel which also
requires path X
3) while starting domB, libvirt overwrites seclabel on X effectively
cutting of domA
4) imagine domB can't be started (or is destroyed later or whatever),
libvirt starts restore, but since X's refcount = 1, the original owner
is not restored (domA is still cut off)
a) As I say we don't want to check in 3) if the new seclabel would cut
off domA. That's for kernel to decide.

Simply don't try to decide that. Require identical labels for all
guests sharing the disk.

Post by Michal Privoznik
b) If we would want to rollback in 4) and set domA seclabel on X we
would need a very clever algorithm. We would need to store an array of
seclabels in XATTR and match them with domains that are doing the
restore. For instance: there are three domains domA, domB and domC and
assume they all share path X and have their own seclabels defined
(A:UID, B:UID, C:UID). Lets start them all and the XATTR on X would look
{[original owner],[A:UID],[B:UID]} (and the X is currently owned by C:UID)
Now, domA is shut off. How would I know what item in the array to
remove? Note that [A:UID] was added when starting domB, and in fact
[original owner] was added by domA.

If we block guest B from starting unless its label is identical to that
originally used by guest A, we've nothing needing to rollback for guest
B.

Regards,
Daniel

Michal Privoznik

2018-12-06 15:12:45 UTC

Post by Michal Privoznik
This file implements wrappers over XATTR getter/setter. It
ensures the proper XATTR namespace is used.
---
src/security/Makefile.inc.am | 2 +
src/security/security_util.c | 226 +++++++++++++++++++++++++++++++++++
src/security/security_util.h | 32 +++++
3 files changed, 260 insertions(+)
create mode 100644 src/security/security_util.c
create mode 100644 src/security/security_util.h
+int
+virSecuritySetRememberedLabel(const char *name,
+ const char *path,
+ const char *label)
+{
+ char *ref_name = NULL;
+ char *attr_name = NULL;
+ char *value = NULL;
+ unsigned int refcount = 0;
+ int ret = -1;
+
+ if (!(ref_name = virSecurityGetRefCountAttrName(name)))
+ goto cleanup;
+
+ if (virFileGetXAtrr(path, ref_name, &value) < 0) {
+ if (errno == ENOSYS || errno == ENOTSUP) {
+ ret = 0;
+ goto cleanup;
+ } else if (errno != ENODATA) {
+ virReportSystemError(errno,
+ _("Unable to get XATTR %s on %s"),
+ ref_name,
+ path);
+ goto cleanup;
+ }
+ }
+
+ if (value &&
+ virStrToLong_ui(value, NULL, 10, &refcount) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("malformed refcount %s on %s"),
+ value, path);
+ goto cleanup;
+ }
+
+ VIR_FREE(value);
+
+ refcount++;
+
+ if (refcount == 1) {
+ if (!(attr_name = virSecurityGetAttrName(name)))
+ goto cleanup;
+
+ if (virFileSetXAtrr(path, attr_name, label) < 0)
+ goto cleanup;
+ }

Do we need to have a
} else {
.... check the currently remember label matches
what was passed into this method ?
}
if not, can you add API docs for this method explainng the
intended semantics when label is already remembered.

I wasn't actually suggesting checking accessibility.
IMHO if you are going to have 2 guests both accessing the same file,
we should simply declare that they *MUST* both use the same label.
Simply reject the idea that the 2nd guest can change the label, using
a GID_B that magically still allows guest A to access it. Such scenarios
are way more likely to be an admin screw up than an intentional decision.
As an admin - the guest A might set svirt_image_t:s0:c12,c35. The guest B
then comes along and sets 'svirt_image_t:s0' which grants access to
all guests. This is a clearly a mistake because the first guests' label
was an exclusive access label, while the second guests label was a shared
access label. The lock manager should have blocked this, but I still
think we should also block it here, as the lock manager won't block it
until after you've already set the labels.
IOW we're justified in requiring strict equality of labels for all
guests, even if they technically might prevent something the kernel would
otherwise allow.

Okay then, but this is not the correct place for that check then. In the
XATTRs there is stored the original owner, not the current one. If domA
is already running, then domB doesn't need to check if its seclabel ==
original owner rather than if its seclabel == current seclabel.

What I can do is to have virSecuritySetRememberedLabel() return the
updated value of the ref counter and if it is greater than 1 require the
seclabel to be the one that @path currently has (in the caller). I mean,
these virSecurity{Get,Set}RememberedLabel APIs really treat seclabels as
an opaque data. I rather not have them understand seclabels.

Simply don't try to decide that. Require identical labels for all
guests sharing the disk.

If we block guest B from starting unless its label is identical to that
originally used by guest A, we've nothing needing to rollback for guest
B.

Fair enough.

Michal

Daniel P. Berrangé

2018-12-06 15:17:13 UTC

Post by Michal Privoznik
This file implements wrappers over XATTR getter/setter. It
ensures the proper XATTR namespace is used.
---
src/security/Makefile.inc.am | 2 +
src/security/security_util.c | 226 +++++++++++++++++++++++++++++++++++
src/security/security_util.h | 32 +++++
3 files changed, 260 insertions(+)
create mode 100644 src/security/security_util.c
create mode 100644 src/security/security_util.h
+int
+virSecuritySetRememberedLabel(const char *name,
+ const char *path,
+ const char *label)
+{
+ char *ref_name = NULL;
+ char *attr_name = NULL;
+ char *value = NULL;
+ unsigned int refcount = 0;
+ int ret = -1;
+
+ if (!(ref_name = virSecurityGetRefCountAttrName(name)))
+ goto cleanup;
+
+ if (virFileGetXAtrr(path, ref_name, &value) < 0) {
+ if (errno == ENOSYS || errno == ENOTSUP) {
+ ret = 0;
+ goto cleanup;
+ } else if (errno != ENODATA) {
+ virReportSystemError(errno,
+ _("Unable to get XATTR %s on %s"),
+ ref_name,
+ path);
+ goto cleanup;
+ }
+ }
+
+ if (value &&
+ virStrToLong_ui(value, NULL, 10, &refcount) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("malformed refcount %s on %s"),
+ value, path);
+ goto cleanup;
+ }
+
+ VIR_FREE(value);
+
+ refcount++;
+
+ if (refcount == 1) {
+ if (!(attr_name = virSecurityGetAttrName(name)))
+ goto cleanup;
+
+ if (virFileSetXAtrr(path, attr_name, label) < 0)
+ goto cleanup;
+ }

Do we need to have a
} else {
.... check the currently remember label matches
what was passed into this method ?
}
if not, can you add API docs for this method explainng the
intended semantics when label is already remembered.

I wasn't actually suggesting checking accessibility.
IMHO if you are going to have 2 guests both accessing the same file,
we should simply declare that they *MUST* both use the same label.
Simply reject the idea that the 2nd guest can change the label, using
a GID_B that magically still allows guest A to access it. Such scenarios
are way more likely to be an admin screw up than an intentional decision.
As an admin - the guest A might set svirt_image_t:s0:c12,c35. The guest B
then comes along and sets 'svirt_image_t:s0' which grants access to
all guests. This is a clearly a mistake because the first guests' label
was an exclusive access label, while the second guests label was a shared
access label. The lock manager should have blocked this, but I still
think we should also block it here, as the lock manager won't block it
until after you've already set the labels.
IOW we're justified in requiring strict equality of labels for all
guests, even if they technically might prevent something the kernel would
otherwise allow.

Okay then, but this is not the correct place for that check then. In the
XATTRs there is stored the original owner, not the current one. If domA
is already running, then domB doesn't need to check if its seclabel ==
original owner rather than if its seclabel == current seclabel.
What I can do is to have virSecuritySetRememberedLabel() return the
updated value of the ref counter and if it is greater than 1 require the
these virSecurity{Get,Set}RememberedLabel APIs really treat seclabels as
an opaque data. I rather not have them understand seclabels.

Yes, that makes sense to me.

Regards,
Daniel

Michal Privoznik

2018-11-29 13:52:21 UTC

It helps whe trying to match calls with virSecurityDACSetAllLabel
if the order in which devices are set/restored is the same in
both functions.

Signed-off-by: Michal Privoznik <***@redhat.com>
---
src/security/security_dac.c | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 82b16f96ee..9b3069e60c 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1665,24 +1665,6 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr,
VIR_DEBUG("Restoring security label on %s migrated=%d",
def->name, migrated);

- for (i = 0; i < def->nhostdevs; i++) {
- if (virSecurityDACRestoreHostdevLabel(mgr,
- def,
- def->hostdevs[i],
- NULL) < 0)
- rc = -1;
- }
-
- for (i = 0; i < def->ngraphics; i++) {
- if (virSecurityDACRestoreGraphicsLabel(mgr, def, def->graphics[i]) < 0)
- return -1;
- }
-
- for (i = 0; i < def->ninputs; i++) {
- if (virSecurityDACRestoreInputLabel(mgr, def, def->inputs[i]) < 0)
- rc = -1;
- }
-
for (i = 0; i < def->ndisks; i++) {
if (virSecurityDACRestoreImageLabelInt(mgr,
def,
@@ -1691,6 +1673,24 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr,
rc = -1;
}

+ for (i = 0; i < def->ngraphics; i++) {
+ if (virSecurityDACRestoreGraphicsLabel(mgr, def, def->graphics[i]) < 0)
+ return -1;
+ }
+
+ for (i = 0; i < def->ninputs; i++) {
+ if (virSecurityDACRestoreInputLabel(mgr, def, def->inputs[i]) < 0)
+ rc = -1;
+ }
+
+ for (i = 0; i < def->nhostdevs; i++) {
+ if (virSecurityDACRestoreHostdevLabel(mgr,
+ def,
+ def->hostdevs[i],
+ NULL) < 0)
+ rc = -1;
+ }
+
for (i = 0; i < def->nmems; i++) {
if (virSecurityDACRestoreMemoryLabel(mgr,
def,

--
2.18.1

Daniel P. Berrangé

2018-12-06 11:54:49 UTC

Post by Michal Privoznik
It helps whe trying to match calls with virSecurityDACSetAllLabel
if the order in which devices are set/restored is the same in
both functions.
---
src/security/security_dac.c | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)

Reviewed-by: Daniel P. Berrangé <***@redhat.com>

Regards,
Daniel

Michal Privoznik

2018-11-29 13:52:19 UTC

This post might be inappropriate. Click to display it.

Daniel P. Berrangé

2018-12-06 11:53:06 UTC

Post by Michal Privoznik
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.
---
src/security/security_dac.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 6b64d2c07a..8155c6d58a 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -718,7 +718,25 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
NULLSTR(src ? src->path : path), (long)uid, (long)gid);
- return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid);
+ if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 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 (virSecurityDACRestoreFileLabelInternal(mgr, src, path) < 0)
+ VIR_WARN("Unable to restore label on '%s'. "
+ "XATTRs might have been left in inconsistent state.",
+ NULLSTR(src ? src->path : path));
+
+ virErrorRestore(&origerr);
+ return -1;
+ }
+
+ return 0;
}

Reviewed-by: Daniel P. Berrangé <***@redhat.com>

THough I feel this could probably just be squashed into the patch that
integrates the label remembering, as it doesn't really do anything useful
on its own.

Regards,
Daniel

Michal Privoznik

2018-11-29 13:52:16 UTC

To avoid including a header file twice the following pattern is
used:

#ifndef __SOMETHING__
# define __SOMETHING__

where __SOMETHING__ should correspond to the header file name.
However, some of our header files break that pattern.

Signed-off-by: Michal Privoznik <***@redhat.com>
---
src/security/security_apparmor.h | 6 +++---
src/security/security_dac.h | 6 +++---
src/security/security_driver.h | 6 +++---
src/security/security_manager.h | 6 +++---
src/security/security_nop.h | 6 +++---
src/security/security_selinux.h | 6 +++---
src/security/security_stack.h | 6 +++---
7 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/src/security/security_apparmor.h b/src/security/security_apparmor.h
index 7872588f64..6b454d1b5c 100644
--- a/src/security/security_apparmor.h
+++ b/src/security/security_apparmor.h
@@ -19,8 +19,8 @@
* Jamie Strandboge <***@canonical.com>
*
*/
-#ifndef __VIR_SECURITY_APPARMOR_H__
-# define __VIR_SECURITY_APPARMOR_H__
+#ifndef __SECURITY_APPARMOR_H__
+# define __SECURITY_APPARMOR_H__

# include "security_driver.h"

@@ -30,4 +30,4 @@ extern virSecurityDriver virAppArmorSecurityDriver;
# define PROFILE_NAME_SIZE 8 + VIR_UUID_STRING_BUFLEN /* AA_PREFIX + uuid */
# define MAX_FILE_LEN (1024*1024*10) /* 10MB limit for sanity check */

-#endif /* __VIR_SECURITY_APPARMOR_H__ */
+#endif /* __SECURITY_APPARMOR_H__ */
diff --git a/src/security/security_dac.h b/src/security/security_dac.h
index 97681c9610..8007bde000 100644
--- a/src/security/security_dac.h
+++ b/src/security/security_dac.h
@@ -20,8 +20,8 @@

#include "security_driver.h"

-#ifndef __VIR_SECURITY_DAC
-# define __VIR_SECURITY_DAC
+#ifndef __SECURITY_DAC__
+# define __SECURITY_DAC__

extern virSecurityDriver virSecurityDriverDAC;

@@ -38,4 +38,4 @@ void virSecurityDACSetMountNamespace(virSecurityManagerPtr mgr,
void virSecurityDACSetChownCallback(virSecurityManagerPtr mgr,
virSecurityManagerDACChownCallback chownCallback);

-#endif /* __VIR_SECURITY_DAC */
+#endif /* __SECURITY_DAC__ */
diff --git a/src/security/security_driver.h b/src/security/security_driver.h
index cd221f1c78..25d49bb0f4 100644
--- a/src/security/security_driver.h
+++ b/src/security/security_driver.h
@@ -19,8 +19,8 @@
* James Morris <***@namei.org>
*
*/
-#ifndef __VIR_SECURITY_H__
-# define __VIR_SECURITY_H__
+#ifndef __SECURITY_DRIVER_H__
+# define __SECURITY_DRIVER_H__

# include "internal.h"
# include "domain_conf.h"
@@ -226,4 +226,4 @@ struct _virSecurityDriver {
virSecurityDriverPtr virSecurityDriverLookup(const char *name,
const char *virtDriver);

-#endif /* __VIR_SECURITY_H__ */
+#endif /* __SECURITY_DRIVER_H__ */
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index 7e82304689..139b70ec10 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -20,8 +20,8 @@
* Author: Daniel P. Berrange <***@redhat.com>
*/

-#ifndef VIR_SECURITY_MANAGER_H__
-# define VIR_SECURITY_MANAGER_H__
+#ifndef __SECURITY_MANAGER_H__
+# define __SECURITY_MANAGER_H__

# include "domain_conf.h"
# include "vircommand.h"
@@ -210,4 +210,4 @@ void
virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr,
virSecurityManagerMetadataLockStatePtr *state);

-#endif /* VIR_SECURITY_MANAGER_H__ */
+#endif /* __SECURITY_MANAGER_H__ */
diff --git a/src/security/security_nop.h b/src/security/security_nop.h
index 514b339467..7b2ded2292 100644
--- a/src/security/security_nop.h
+++ b/src/security/security_nop.h
@@ -17,11 +17,11 @@
*
*/

-#ifndef __VIR_SECURITY_NOP_H__
-# define __VIR_SECURITY_NOP_H__
+#ifndef __SECURITY_NOP_H__
+# define __SECURITY_NOP_H__

# include "security_driver.h"

extern virSecurityDriver virSecurityDriverNop;

-#endif /* __VIR_SECURITY_NOP_H__ */
+#endif /* __SECURITY_NOP_H__ */
diff --git a/src/security/security_selinux.h b/src/security/security_selinux.h
index 1700d8c661..11b62acb52 100644
--- a/src/security/security_selinux.h
+++ b/src/security/security_selinux.h
@@ -19,9 +19,9 @@
* James Morris <***@namei.org>
*
*/
-#ifndef __VIR_SECURITY_SELINUX_H__
-# define __VIR_SECURITY_SELINUX_H__
+#ifndef __SECURITY_SELINUX_H__
+# define __SECURITY_SELINUX_H__

extern virSecurityDriver virSecurityDriverSELinux;

-#endif /* __VIR_SECURITY_SELINUX_H__ */
+#endif /* __SECURITY_SELINUX_H__ */
diff --git a/src/security/security_stack.h b/src/security/security_stack.h
index b38f9a9481..7e6ab3d93e 100644
--- a/src/security/security_stack.h
+++ b/src/security/security_stack.h
@@ -20,8 +20,8 @@

#include "security_driver.h"

-#ifndef __VIR_SECURITY_STACK
-# define __VIR_SECURITY_STACK
+#ifndef __SECURITY_STACK__
+# define __SECURITY_STACK__

extern virSecurityDriver virSecurityDriverStack;

@@ -35,4 +35,4 @@ virSecurityStackGetPrimary(virSecurityManagerPtr mgr);
virSecurityManagerPtr*
virSecurityStackGetNested(virSecurityManagerPtr mgr);

-#endif /* __VIR_SECURITY_STACK */
+#endif /* __SECURITY_STACK__ */

--
2.18.1

Daniel P. Berrangé

2018-12-06 11:16:54 UTC

Post by Michal Privoznik
To avoid including a header file twice the following pattern is
#ifndef __SOMETHING__
# define __SOMETHING__
where __SOMETHING__ should correspond to the header file name.
However, some of our header files break that pattern.

Looking at the git tree as a whole, we're all over the place
with the naming of these cnoditionals. There's many othuer
files using a __VIR prefix which don't have 'vir' in the
filename:

$ git grep ifndef '*.h' | grep VIR | grep -v vir | wc -l
103

So I don't think this is something we should really change here.
I think it points to the need for a syntax-check rule to enforce
a given convention and then a tree-wide fixup to comply.

Post by Michal Privoznik
---
src/security/security_apparmor.h | 6 +++---
src/security/security_dac.h | 6 +++---
src/security/security_driver.h | 6 +++---
src/security/security_manager.h | 6 +++---
src/security/security_nop.h | 6 +++---
src/security/security_selinux.h | 6 +++---
src/security/security_stack.h | 6 +++---
7 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/src/security/security_apparmor.h b/src/security/security_apparmor.h
index 7872588f64..6b454d1b5c 100644
--- a/src/security/security_apparmor.h
+++ b/src/security/security_apparmor.h
@@ -19,8 +19,8 @@
*
*/
-#ifndef __VIR_SECURITY_APPARMOR_H__
-# define __VIR_SECURITY_APPARMOR_H__
+#ifndef __SECURITY_APPARMOR_H__
+# define __SECURITY_APPARMOR_H__
# include "security_driver.h"
@@ -30,4 +30,4 @@ extern virSecurityDriver virAppArmorSecurityDriver;
# define PROFILE_NAME_SIZE 8 + VIR_UUID_STRING_BUFLEN /* AA_PREFIX + uuid */
# define MAX_FILE_LEN (1024*1024*10) /* 10MB limit for sanity check */
-#endif /* __VIR_SECURITY_APPARMOR_H__ */
+#endif /* __SECURITY_APPARMOR_H__ */
diff --git a/src/security/security_dac.h b/src/security/security_dac.h
index 97681c9610..8007bde000 100644
--- a/src/security/security_dac.h
+++ b/src/security/security_dac.h
@@ -20,8 +20,8 @@
#include "security_driver.h"
-#ifndef __VIR_SECURITY_DAC
-# define __VIR_SECURITY_DAC
+#ifndef __SECURITY_DAC__
+# define __SECURITY_DAC__
extern virSecurityDriver virSecurityDriverDAC;
@@ -38,4 +38,4 @@ void virSecurityDACSetMountNamespace(virSecurityManagerPtr mgr,
void virSecurityDACSetChownCallback(virSecurityManagerPtr mgr,
virSecurityManagerDACChownCallback chownCallback);
-#endif /* __VIR_SECURITY_DAC */
+#endif /* __SECURITY_DAC__ */
diff --git a/src/security/security_driver.h b/src/security/security_driver.h
index cd221f1c78..25d49bb0f4 100644
--- a/src/security/security_driver.h
+++ b/src/security/security_driver.h
@@ -19,8 +19,8 @@
*
*/
-#ifndef __VIR_SECURITY_H__
-# define __VIR_SECURITY_H__
+#ifndef __SECURITY_DRIVER_H__
+# define __SECURITY_DRIVER_H__
# include "internal.h"
# include "domain_conf.h"
@@ -226,4 +226,4 @@ struct _virSecurityDriver {
virSecurityDriverPtr virSecurityDriverLookup(const char *name,
const char *virtDriver);
-#endif /* __VIR_SECURITY_H__ */
+#endif /* __SECURITY_DRIVER_H__ */
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index 7e82304689..139b70ec10 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -20,8 +20,8 @@
*/
-#ifndef VIR_SECURITY_MANAGER_H__
-# define VIR_SECURITY_MANAGER_H__
+#ifndef __SECURITY_MANAGER_H__
+# define __SECURITY_MANAGER_H__
# include "domain_conf.h"
# include "vircommand.h"
@@ -210,4 +210,4 @@ void
virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr,
virSecurityManagerMetadataLockStatePtr *state);
-#endif /* VIR_SECURITY_MANAGER_H__ */
+#endif /* __SECURITY_MANAGER_H__ */
diff --git a/src/security/security_nop.h b/src/security/security_nop.h
index 514b339467..7b2ded2292 100644
--- a/src/security/security_nop.h
+++ b/src/security/security_nop.h
@@ -17,11 +17,11 @@
*
*/
-#ifndef __VIR_SECURITY_NOP_H__
-# define __VIR_SECURITY_NOP_H__
+#ifndef __SECURITY_NOP_H__
+# define __SECURITY_NOP_H__
# include "security_driver.h"
extern virSecurityDriver virSecurityDriverNop;
-#endif /* __VIR_SECURITY_NOP_H__ */
+#endif /* __SECURITY_NOP_H__ */
diff --git a/src/security/security_selinux.h b/src/security/security_selinux.h
index 1700d8c661..11b62acb52 100644
--- a/src/security/security_selinux.h
+++ b/src/security/security_selinux.h
@@ -19,9 +19,9 @@
*
*/
-#ifndef __VIR_SECURITY_SELINUX_H__
-# define __VIR_SECURITY_SELINUX_H__
+#ifndef __SECURITY_SELINUX_H__
+# define __SECURITY_SELINUX_H__
extern virSecurityDriver virSecurityDriverSELinux;
-#endif /* __VIR_SECURITY_SELINUX_H__ */
+#endif /* __SECURITY_SELINUX_H__ */
diff --git a/src/security/security_stack.h b/src/security/security_stack.h
index b38f9a9481..7e6ab3d93e 100644
--- a/src/security/security_stack.h
+++ b/src/security/security_stack.h
@@ -20,8 +20,8 @@
#include "security_driver.h"
-#ifndef __VIR_SECURITY_STACK
-# define __VIR_SECURITY_STACK
+#ifndef __SECURITY_STACK__
+# define __SECURITY_STACK__
extern virSecurityDriver virSecurityDriverStack;
@@ -35,4 +35,4 @@ virSecurityStackGetPrimary(virSecurityManagerPtr mgr);
virSecurityManagerPtr*
virSecurityStackGetNested(virSecurityManagerPtr mgr);
-#endif /* __VIR_SECURITY_STACK */
+#endif /* __SECURITY_STACK__ */
--
2.18.1
--
libvir-list mailing list
https://www.redhat.com/mailman/listinfo/libvir-list

Regards,
Daniel

Michal Privoznik

2018-11-29 13:52:22 UTC

We are setting label on kernel, initrd, dtb and slic_table files.
But we never restored it.

Signed-off-by: Michal Privoznik <***@redhat.com>
---
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 9b3069e60c..de12a1e351 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1720,6 +1720,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.18.1

Daniel P. Berrangé

2018-12-06 11:55:17 UTC

Post by Michal Privoznik
We are setting label on kernel, initrd, dtb and slic_table files.
But we never restored it.
---
src/security/security_dac.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)

Reviewed-by: Daniel P. Berrangé <***@redhat.com>

Regards,
Daniel

Michal Privoznik

2018-11-29 13:52:17 UTC

Signed-off-by: Michal Privoznik <***@redhat.com>
---
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 5018a13e9c..8e5b610ab1 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1827,6 +1827,7 @@ virFileGetACLs;
virFileGetHugepageSize;
virFileGetMountReverseSubtree;
virFileGetMountSubtree;
+virFileGetXAtrr;
virFileHasSuffix;
virFileInData;
virFileIsAbsPath;
@@ -1866,6 +1867,7 @@ virFileReadValueUint;
virFileRelLinkPointsTo;
virFileRemove;
virFileRemoveLastComponent;
+virFileRemoveXAttr;
virFileResolveAllLinks;
virFileResolveLink;
virFileRewrite;
@@ -1873,6 +1875,7 @@ virFileRewriteStr;
virFileSanitizePath;
virFileSetACLs;
virFileSetupDev;
+virFileSetXAtrr;
virFileSkipRoot;
virFileStripSuffix;
virFileTouch;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index f6f9e4ceda..9df5f70c60 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
+/**
+ * virFileGetXAtrr;
+ * @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
+virFileGetXAtrr(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. */
+ do {
+ 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;
+ } while (1);
+
+ VIR_STEAL_PTR(*value, buf);
+ ret = 0;
+ cleanup:
+ VIR_FREE(buf);
+ return ret;
+}
+
+/**
+ * virFileSetXAtrr:
+ * @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
+virFileSetXAtrr(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
+virFileGetXAtrr(const char *path ATTRIBUTE_UNUSED,
+ const char *name ATTRIBUTE_UNUSED,
+ char **value ATTRIBUTE_UNUSED)
+{
+ errno = ENOSYS;
+ return -1;
+}
+
+int
+virFileSetXAtrr(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..9cd1bc3a5f 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 virFileGetXAtrr(const char *path,
+ const char *name,
+ char **value);
+
+int virFileSetXAtrr(const char *path,
+ const char *name,
+ const char *value);
+
+int virFileRemoveXAttr(const char *path,
+ const char *name);
+
#endif /* __VIR_FILE_H */

--
2.18.1

Daniel P. Berrangé

2018-12-06 11:30:44 UTC

Post by Michal Privoznik
---
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 5018a13e9c..8e5b610ab1 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1827,6 +1827,7 @@ virFileGetACLs;
virFileGetHugepageSize;
virFileGetMountReverseSubtree;
virFileGetMountSubtree;
+virFileGetXAtrr;

s/Atrr/Attr/

Post by Michal Privoznik
@@ -1873,6 +1875,7 @@ virFileRewriteStr;
virFileSanitizePath;
virFileSetACLs;
virFileSetupDev;
+virFileSetXAtrr;

s/Atrr/Attr/

Post by Michal Privoznik
+#if HAVE_LIBATTR
+/**
+ * virFileGetXAtrr;
+ *
+ *
+ * Returns: 0 on success,
+ * -1 otherwise (with errno set).
+ */
+int
+virFileGetXAtrr(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. */
+ do {
+ 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;
+ } while (1);

Nitpick, I'd expect the while(1) at the top. Only use this style of
loop when the checked loop condition would mistakenly prevent the first
iteration running.

Post by Michal Privoznik
+
+ VIR_STEAL_PTR(*value, buf);
+ ret = 0;
+ VIR_FREE(buf);
+ return ret;
+}

Regards,
Daniel

Michal Privoznik

2018-11-29 13:52:24 UTC

Signed-off-by: Michal Privoznik <***@redhat.com>
---
src/security/security_dac.c | 48 ++++++++++++++++++++++++++++++-------
1 file changed, 40 insertions(+), 8 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index cdbe07543c..9d31faa9d4 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"
@@ -415,11 +416,26 @@ virSecurityDACGetImageIds(virSecurityLabelDefPtr seclabel,
*/
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)
+ goto cleanup;
+
+ if (virSecuritySetRememberedLabel(SECURITY_DAC_NAME,
+ path, label) < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(label);
+ return ret;
}

/**
@@ -439,11 +455,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

--
2.18.1

Daniel P. Berrangé

2018-12-06 12:03:07 UTC

Post by Michal Privoznik
---
src/security/security_dac.c | 48 ++++++++++++++++++++++++++++++-------
1 file changed, 40 insertions(+), 8 deletions(-)

Reviewed-by: Daniel P. Berrangé <***@redhat.com>

Regards,
Daniel

Michal Privoznik

2018-11-29 13:52:26 UTC

It is going to be important to know if the current transaction we
are running is a restore operation or set label operation.

Signed-off-by: Michal Privoznik <***@redhat.com>
---
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.18.1

Daniel P. Berrangé

2018-12-06 12:08:35 UTC

Post by Michal Privoznik
It is going to be important to know if the current transaction we
are running is a restore operation or set label operation.

Might be worth saying why it is important :-)

Post by Michal Privoznik
---
src/security/security_selinux.c | 36 +++++++++++++++++++++++----------
1 file changed, 25 insertions(+), 11 deletions(-)

Reviewed-by: Daniel P. Berrangé <***@redhat.com>

Regards,
Daniel

Michal Privoznik

2018-11-29 13:52:25 UTC

Now that we have seclabel remembering we can safely restore
labels for shared and RO disks. In fact we need to do that to
keep seclabel refcount stored in XATTRs in sync with reality.

Signed-off-by: Michal Privoznik <***@redhat.com>
---
src/security/security_dac.c | 8 --------
1 file changed, 8 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 9d31faa9d4..60adfaf526 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -921,14 +921,6 @@ virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr,
if (!priv->dynamicOwnership)
return 0;

- /* Don't restore labels on readoly/shared disks, because other VMs may
- * still be accessing these. Alternatively we could iterate over all
- * running domains and try to figure out if it is in use, but this would
- * not work for clustered filesystems, since we can't see running VMs using
- * the file on other nodes. Safest bet is thus to skip the restore step. */
- if (src->readonly || src->shared)
- return 0;
-
secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
if (secdef && !secdef->relabel)
return 0;

--
2.18.1

Daniel P. Berrangé

2018-12-06 12:06:00 UTC

Post by Michal Privoznik
Now that we have seclabel remembering we can safely restore
labels for shared and RO disks. In fact we need to do that to
keep seclabel refcount stored in XATTRs in sync with reality.
---
src/security/security_dac.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 9d31faa9d4..60adfaf526 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -921,14 +921,6 @@ virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr,
if (!priv->dynamicOwnership)
return 0;
- /* Don't restore labels on readoly/shared disks, because other VMs may
- * still be accessing these. Alternatively we could iterate over all
- * running domains and try to figure out if it is in use, but this would
- * not work for clustered filesystems, since we can't see running VMs using
- * the file on other nodes. Safest bet is thus to skip the restore step. */
- if (src->readonly || src->shared)
- return 0;
-
secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
if (secdef && !secdef->relabel)
return 0;

Reviewed-by: Daniel P. Berrangé <***@redhat.com>

though I'd probably squash this into the previous commit, otherwise the
bisection has a point in time where it leaks attrs for shared/readonly
disk

Regards,
Daniel

Michal Privoznik

2018-11-29 13:52:27 UTC

Signed-off-by: Michal Privoznik <***@redhat.com>
---
src/security/security_selinux.c | 161 ++++++++++++++++++++++----------
1 file changed, 114 insertions(+), 47 deletions(-)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 715d9a428b..4990d94b5f 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -33,6 +33,7 @@

#include "security_driver.h"
#include "security_selinux.h"
+#include "security_util.h"
#include "virerror.h"
#include "viralloc.h"
#include "virlog.h"
@@ -197,14 +198,40 @@ virSecuritySELinuxTransactionAppend(const char *path,
}

+static int
+virSecuritySELinuxRememberLabel(const char *path,
+ const security_context_t con)
+{
+ return virSecuritySetRememberedLabel(SECURITY_SELINUX_NAME,
+ path, con);
+}
+
+
+static int
+virSecuritySELinuxRecallLabel(const char *path,
+ security_context_t *con)
+{
+ if (virSecurityGetRememberedLabel(SECURITY_SELINUX_NAME,
+ path, con) < 0)
+ return -1;
+
+ if (!con)
+ return 1;
+
+ return 0;
+}
+
+
static int virSecuritySELinuxSetFileconHelper(const char *path,
const char *tcon,
bool optional,
- bool privileged);
+ bool privileged,
+ bool remember);

static int virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr,
- const char *path);
+ const char *path,
+ bool recall);

/**
@@ -255,10 +282,12 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
rv = virSecuritySELinuxSetFileconHelper(item->path,
item->tcon,
item->optional,
- privileged);
+ privileged,
+ list->lock);
} else {
rv = virSecuritySELinuxRestoreFileLabel(list->manager,
- item->path);
+ item->path,
+ list->lock);
}

if (rv < 0)
@@ -1275,16 +1304,38 @@ virSecuritySELinuxSetFileconImpl(const char *path, const char *tcon,

static int
virSecuritySELinuxSetFileconHelper(const char *path, const char *tcon,
- bool optional, bool privileged)
+ bool optional, bool privileged, bool remember)
{
+ security_context_t econ = NULL;
int rc;
+ int ret = -1;

if ((rc = virSecuritySELinuxTransactionAppend(path, tcon, optional, false)) < 0)
return -1;
else if (rc > 0)
return 0;

- return virSecuritySELinuxSetFileconImpl(path, tcon, optional, privileged);
+ if (remember) {
+ if (getfilecon_raw(path, &econ) < 0 &&
+ errno != ENOTSUP && errno != ENODATA) {
+ virReportSystemError(errno,
+ _("unable to get SELinux context of %s"),
+ path);
+ goto cleanup;
+ }
+
+ if (econ &&
+ virSecuritySELinuxRememberLabel(path, econ) < 0)
+ goto cleanup;
+ }
+
+ if (virSecuritySELinuxSetFileconImpl(path, tcon, optional, privileged) < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ freecon(econ);
+ return ret;
}

@@ -1293,7 +1344,7 @@ virSecuritySELinuxSetFileconOptional(virSecurityManagerPtr mgr,
const char *path, const char *tcon)
{
bool privileged = virSecurityManagerGetPrivileged(mgr);
- return virSecuritySELinuxSetFileconHelper(path, tcon, true, privileged);
+ return virSecuritySELinuxSetFileconHelper(path, tcon, true, privileged, false);
}

static int
@@ -1301,7 +1352,7 @@ virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr,
const char *path, const char *tcon)
{
bool privileged = virSecurityManagerGetPrivileged(mgr);
- return virSecuritySELinuxSetFileconHelper(path, tcon, false, privileged);
+ return virSecuritySELinuxSetFileconHelper(path, tcon, false, privileged, false);
}

static int
@@ -1362,7 +1413,8 @@ getContext(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
* errors that the caller(s) are already dealing with */
static int
virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr,
- const char *path)
+ const char *path,
+ bool recall)
{
bool privileged = virSecurityManagerGetPrivileged(mgr);
struct stat buf;
@@ -1386,26 +1438,35 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr,
goto cleanup;
}

- if (stat(newpath, &buf) != 0) {
- VIR_WARN("cannot stat %s: %s", newpath,
- virStrerror(errno, ebuf, sizeof(ebuf)));
- goto cleanup;
- }
-
- if (getContext(mgr, newpath, buf.st_mode, &fcon) < 0) {
- /* Any user created path likely does not have a default label,
- * which makes this an expected non error
- */
- VIR_WARN("cannot lookup default selinux label for %s", newpath);
- ret = 0;
- goto cleanup;
- }
-
- if ((rc = virSecuritySELinuxTransactionAppend(path, fcon, false, true)) < 0)
+ if ((rc = virSecuritySELinuxTransactionAppend(path, NULL, false, true)) < 0)
return -1;
else if (rc > 0)
return 0;

+ if (recall) {
+ if ((rc = virSecuritySELinuxRecallLabel(newpath, &fcon)) < 0) {
+ goto cleanup;
+ } else if (rc > 0) {
+ ret = 0;
+ goto cleanup;
+ }
+ } else {
+ if (stat(newpath, &buf) != 0) {
+ VIR_WARN("cannot stat %s: %s", newpath,
+ virStrerror(errno, ebuf, sizeof(ebuf)));
+ goto cleanup;
+ }
+
+ if (getContext(mgr, newpath, buf.st_mode, &fcon) < 0) {
+ /* Any user created path likely does not have a default label,
+ * which makes this an expected non error
+ */
+ VIR_WARN("cannot lookup default selinux label for %s", newpath);
+ ret = 0;
+ goto cleanup;
+ }
+ }
+
if (virSecuritySELinuxSetFileconImpl(newpath, fcon, false, privileged) < 0)
goto cleanup;

@@ -1460,7 +1521,7 @@ virSecuritySELinuxRestoreInputLabel(virSecurityManagerPtr mgr,

switch ((virDomainInputType)input->type) {
case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH:
- rc = virSecuritySELinuxRestoreFileLabel(mgr, input->source.evdev);
+ rc = virSecuritySELinuxRestoreFileLabel(mgr, input->source.evdev, false);
break;

case VIR_DOMAIN_INPUT_TYPE_MOUSE:
@@ -1516,7 +1577,7 @@ virSecuritySELinuxRestoreMemoryLabel(virSecurityManagerPtr mgr,
if (!seclabel || !seclabel->relabel)
return 0;

- ret = virSecuritySELinuxRestoreFileLabel(mgr, mem->nvdimmPath);
+ ret = virSecuritySELinuxRestoreFileLabel(mgr, mem->nvdimmPath, false);
break;

case VIR_DOMAIN_MEMORY_MODEL_DIMM:
@@ -1595,10 +1656,10 @@ virSecuritySELinuxRestoreTPMFileLabelInt(virSecurityManagerPtr mgr,
switch (tpm->type) {
case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
tpmdev = tpm->data.passthrough.source.data.file.path;
- rc = virSecuritySELinuxRestoreFileLabel(mgr, tpmdev);
+ rc = virSecuritySELinuxRestoreFileLabel(mgr, tpmdev, false);

if ((cancel_path = virTPMCreateCancelPath(tpmdev)) != NULL) {
- if (virSecuritySELinuxRestoreFileLabel(mgr, cancel_path) < 0)
+ if (virSecuritySELinuxRestoreFileLabel(mgr, cancel_path, false) < 0)
rc = -1;
VIR_FREE(cancel_path);
}
@@ -1665,7 +1726,7 @@ virSecuritySELinuxRestoreImageLabelInt(virSecurityManagerPtr mgr,
}
}

- return virSecuritySELinuxRestoreFileLabel(mgr, src->path);
+ return virSecuritySELinuxRestoreFileLabel(mgr, src->path, false);
}

@@ -2053,7 +2114,7 @@ virSecuritySELinuxRestorePCILabel(virPCIDevicePtr dev ATTRIBUTE_UNUSED,
{
virSecurityManagerPtr mgr = opaque;

- return virSecuritySELinuxRestoreFileLabel(mgr, file);
+ return virSecuritySELinuxRestoreFileLabel(mgr, file, false);
}

static int
@@ -2063,7 +2124,7 @@ virSecuritySELinuxRestoreUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED,
{
virSecurityManagerPtr mgr = opaque;

- return virSecuritySELinuxRestoreFileLabel(mgr, file);
+ return virSecuritySELinuxRestoreFileLabel(mgr, file, false);
}

@@ -2080,7 +2141,7 @@ virSecuritySELinuxRestoreSCSILabel(virSCSIDevicePtr dev,
if (virSCSIDeviceGetShareable(dev) || virSCSIDeviceGetReadonly(dev))
return 0;

- return virSecuritySELinuxRestoreFileLabel(mgr, file);
+ return virSecuritySELinuxRestoreFileLabel(mgr, file, false);
}

static int
@@ -2090,7 +2151,7 @@ virSecuritySELinuxRestoreHostLabel(virSCSIVHostDevicePtr dev ATTRIBUTE_UNUSED,
{
virSecurityManagerPtr mgr = opaque;

- return virSecuritySELinuxRestoreFileLabel(mgr, file);
+ return virSecuritySELinuxRestoreFileLabel(mgr, file, false);
}

@@ -2194,7 +2255,7 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr,
if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdevsrc->uuidstr)))
goto done;

- ret = virSecuritySELinuxRestoreFileLabel(mgr, vfiodev);
+ ret = virSecuritySELinuxRestoreFileLabel(mgr, vfiodev, false);

VIR_FREE(vfiodev);
break;
@@ -2228,7 +2289,7 @@ virSecuritySELinuxRestoreHostdevCapsLabel(virSecurityManagerPtr mgr,
if (VIR_STRDUP(path, dev->source.caps.u.storage.block) < 0)
return -1;
}
- ret = virSecuritySELinuxRestoreFileLabel(mgr, path);
+ ret = virSecuritySELinuxRestoreFileLabel(mgr, path, false);
VIR_FREE(path);
break;
}
@@ -2242,7 +2303,7 @@ virSecuritySELinuxRestoreHostdevCapsLabel(virSecurityManagerPtr mgr,
if (VIR_STRDUP(path, dev->source.caps.u.misc.chardev) < 0)
return -1;
}
- ret = virSecuritySELinuxRestoreFileLabel(mgr, path);
+ ret = virSecuritySELinuxRestoreFileLabel(mgr, path, false);
VIR_FREE(path);
break;
}
@@ -2390,14 +2451,18 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr,
switch (dev_source->type) {
case VIR_DOMAIN_CHR_TYPE_DEV:
case VIR_DOMAIN_CHR_TYPE_FILE:
- if (virSecuritySELinuxRestoreFileLabel(mgr, dev_source->data.file.path) < 0)
+ if (virSecuritySELinuxRestoreFileLabel(mgr,
+ dev_source->data.file.path,
+ false) < 0)
goto done;
ret = 0;
break;

case VIR_DOMAIN_CHR_TYPE_UNIX:
if (!dev_source->data.nix.listen) {
- if (virSecuritySELinuxRestoreFileLabel(mgr, dev_source->data.file.path) < 0)
+ if (virSecuritySELinuxRestoreFileLabel(mgr,
+ dev_source->data.file.path,
+ false) < 0)
goto done;
}
ret = 0;
@@ -2408,11 +2473,13 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr,
(virAsprintf(&in, "%s.in", dev_source->data.file.path) < 0))
goto done;
if (virFileExists(in) && virFileExists(out)) {
- if ((virSecuritySELinuxRestoreFileLabel(mgr, out) < 0) ||
- (virSecuritySELinuxRestoreFileLabel(mgr, in) < 0)) {
+ if ((virSecuritySELinuxRestoreFileLabel(mgr, out, false) < 0) ||
+ (virSecuritySELinuxRestoreFileLabel(mgr, in, false) < 0)) {
goto done;
}
- } else if (virSecuritySELinuxRestoreFileLabel(mgr, dev_source->data.file.path) < 0) {
+ } else if (virSecuritySELinuxRestoreFileLabel(mgr,
+ dev_source->data.file.path,
+ false) < 0) {
goto done;
}
ret = 0;
@@ -2464,7 +2531,7 @@ virSecuritySELinuxRestoreSecuritySmartcardCallback(virDomainDefPtr def,
database = dev->data.cert.database;
if (!database)
database = VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE;
- return virSecuritySELinuxRestoreFileLabel(mgr, database);
+ return virSecuritySELinuxRestoreFileLabel(mgr, database, false);

case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
return virSecuritySELinuxRestoreChardevLabel(mgr, def,
@@ -2559,7 +2626,7 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr mgr,
rc = -1;

if (def->os.loader && def->os.loader->nvram &&
- virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram) < 0)
+ virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram, false) < 0)
rc = -1;

return rc;
@@ -2619,7 +2686,7 @@ virSecuritySELinuxRestoreSavedStateLabel(virSecurityManagerPtr mgr,
if (!secdef || !secdef->relabel)
return 0;

- return virSecuritySELinuxRestoreFileLabel(mgr, savefile);
+ return virSecuritySELinuxRestoreFileLabel(mgr, savefile, false);
}

@@ -3214,7 +3281,7 @@ virSecuritySELinuxRestoreFileLabels(virSecurityManagerPtr mgr,
char *filename = NULL;
DIR *dir;

- if ((ret = virSecuritySELinuxRestoreFileLabel(mgr, path)))
+ if ((ret = virSecuritySELinuxRestoreFileLabel(mgr, path, false)))
return ret;

if (!virFileIsDir(path))
@@ -3231,7 +3298,7 @@ virSecuritySELinuxRestoreFileLabels(virSecurityManagerPtr mgr,
ret = -1;
break;
}
- ret = virSecuritySELinuxRestoreFileLabel(mgr, filename);
+ ret = virSecuritySELinuxRestoreFileLabel(mgr, filename, false);
VIR_FREE(filename);
if (ret < 0)
break;

--
2.18.1

Daniel P. Berrangé

2018-12-06 12:09:10 UTC

Post by Michal Privoznik
---
src/security/security_selinux.c | 161 ++++++++++++++++++++++----------
1 file changed, 114 insertions(+), 47 deletions(-)

Reviewed-by: Daniel P. Berrangé <***@redhat.com>

Regards,
Daniel

Michal Privoznik

2018-11-29 13:52:20 UTC

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 <***@redhat.com>
---
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 8155c6d58a..82b16f96ee 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.18.1

Daniel P. Berrangé

2018-12-06 11:54:01 UTC

Post by Michal Privoznik
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.
---
src/security/security_dac.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <***@redhat.com>

Regards,
Daniel

Michal Privoznik

2018-11-29 13:52:30 UTC

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 <***@redhat.com>
---
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 0cf8164265..553fc852db 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -2604,8 +2604,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;
}

@@ -2627,11 +2630,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.18.1

Daniel P. Berrangé

2018-12-06 12:10:23 UTC

Post by Michal Privoznik
It helps whe trying to match calls with virSecuritySELinuxSetAllLabel
if the order in which devices are set/restored is the same in
both functions.
---
src/security/security_selinux.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)

Reviewed-by: Daniel P. Berrangé <***@redhat.com>

Regards,
Daniel

Michal Privoznik

2018-11-29 13:52:32 UTC

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 <***@redhat.com>
---
tools/Makefile.am | 1 +
tools/libvirt_recover_xattrs.sh | 89 +++++++++++++++++++++++++++++++++
2 files changed, 90 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..c4a8b27cbc
--- /dev/null
+++ b/tools/libvirt_recover_xattrs.sh
@@ -0,0 +1,89 @@
+#!/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
+
+XATTRS=("trusted.libvirt.security.dac"
+ "trusted.libvirt.security.ref_dac"
+ "trusted.libvirt.security.selinux"
+ "trusted.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.18.1

Daniel P. Berrangé

2018-12-06 12:14:04 UTC

Post by Michal Privoznik
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.

On this point, it would be a nice idea to be able to write some
unit tests to exercise the security drivers, as this is something
we're significantly lacking coverage of.

With mocking of the chown/setxattr/etc methods we can easily
detect some ofthe bugs you fixed here, such as forgetting to
restore labels of certain resource types.

Post by Michal Privoznik
---
tools/Makefile.am | 1 +
tools/libvirt_recover_xattrs.sh | 89 +++++++++++++++++++++++++++++++++
2 files changed, 90 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)
+XATTRS=("trusted.libvirt.security.dac"
+ "trusted.libvirt.security.ref_dac"
+ "trusted.libvirt.security.selinux"
+ "trusted.libvirt.security.ref_selinux")

Needs updating to account for FreeBSD naming now

Regards,
Daniel

Michal Privoznik

2018-11-29 13:52:23 UTC

Because the implementation that will be used for label
remembering/recall is not atomic we have to give callers a chance
to enable or disable it. That is, enable it if and only if
metadata locking is enabled. Otherwise the feature MUST be turned
off.

Signed-off-by: Michal Privoznik <***@redhat.com>
---
src/security/security_dac.c | 74 ++++++++++++++++++++++---------------
1 file changed, 45 insertions(+), 29 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index de12a1e351..cdbe07543c 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -182,11 +182,13 @@ static int virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
const virStorageSource *src,
const char *path,
uid_t uid,
- gid_t gid);
+ gid_t gid,
+ bool remember);

static int virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr,
const virStorageSource *src,
- const char *path);
+ const char *path,
+ bool recall);
/**
* virSecurityDACTransactionRun:
* @pid: process pid
@@ -234,11 +236,13 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
item->src,
item->path,
item->uid,
- item->gid);
+ item->gid,
+ list->lock);
} else {
rv = virSecurityDACRestoreFileLabelInternal(list->manager,
item->src,
- item->path);
+ item->path,
+ list->lock);
}

if (rv < 0)
@@ -251,7 +255,8 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
if (!item->restore) {
virSecurityDACRestoreFileLabelInternal(list->manager,
item->src,
- item->path);
+ item->path,
+ list->lock);
} else {
VIR_WARN("Ignoring failed restore attempt on %s",
NULLSTR(item->src ? item->src->path : item->path));
@@ -699,7 +704,8 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
const virStorageSource *src,
const char *path,
uid_t uid,
- gid_t gid)
+ gid_t gid,
+ bool remember)
{
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
struct stat sb;
@@ -717,7 +723,7 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
else if (rc > 0)
return 0;

- if (path) {
+ if (remember && path) {
if (stat(path, &sb) < 0) {
virReportSystemError(errno, _("unable to stat: %s"), path);
return -1;
@@ -739,7 +745,7 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
* 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 (virSecurityDACRestoreFileLabelInternal(mgr, src, path) < 0)
+ if (virSecurityDACRestoreFileLabelInternal(mgr, src, path, remember) < 0)
VIR_WARN("Unable to restore label on '%s'. "
"XATTRs might have been left in inconsistent state.",
NULLSTR(src ? src->path : path));
@@ -755,7 +761,8 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
static int
virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr,
const virStorageSource *src,
- const char *path)
+ const char *path,
+ bool recall)
{
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
int rv;
@@ -774,7 +781,7 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr,
else if (rv > 0)
return 0;

- if (path) {
+ if (recall && path) {
rv = virSecurityDACRecallLabel(priv, path, &uid, &gid);
if (rv < 0)
return -1;
@@ -793,7 +800,7 @@ static int
virSecurityDACRestoreFileLabel(virSecurityManagerPtr mgr,
const char *path)
{
- return virSecurityDACRestoreFileLabelInternal(mgr, NULL, path);
+ return virSecurityDACRestoreFileLabelInternal(mgr, NULL, path, false);
}

@@ -840,7 +847,7 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr,
return -1;
}

- return virSecurityDACSetOwnership(mgr, src, NULL, user, group);
+ return virSecurityDACSetOwnership(mgr, src, NULL, user, group, false);
}

@@ -920,7 +927,7 @@ virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr,
}
}

- return virSecurityDACRestoreFileLabelInternal(mgr, src, NULL);
+ return virSecurityDACRestoreFileLabelInternal(mgr, src, NULL, false);
}

@@ -956,7 +963,7 @@ virSecurityDACSetHostdevLabelHelper(const char *file,
if (virSecurityDACGetIds(secdef, priv, &user, &group, NULL, NULL) < 0)
return -1;

- return virSecurityDACSetOwnership(mgr, NULL, file, user, group);
+ return virSecurityDACSetOwnership(mgr, NULL, file, user, group, false);
}

@@ -1332,7 +1339,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
case VIR_DOMAIN_CHR_TYPE_FILE:
ret = virSecurityDACSetOwnership(mgr, NULL,
dev_source->data.file.path,
- user, group);
+ user, group, false);
break;

case VIR_DOMAIN_CHR_TYPE_PIPE:
@@ -1340,12 +1347,12 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
virAsprintf(&out, "%s.out", dev_source->data.file.path) < 0)
goto done;
if (virFileExists(in) && virFileExists(out)) {
- if (virSecurityDACSetOwnership(mgr, NULL, in, user, group) < 0 ||
- virSecurityDACSetOwnership(mgr, NULL, out, user, group) < 0)
+ if (virSecurityDACSetOwnership(mgr, NULL, in, user, group, false) < 0 ||
+ virSecurityDACSetOwnership(mgr, NULL, out, user, group, false) < 0)
goto done;
} else if (virSecurityDACSetOwnership(mgr, NULL,
dev_source->data.file.path,
- user, group) < 0) {
+ user, group, false) < 0) {
goto done;
}
ret = 0;
@@ -1360,7 +1367,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
* and passed via FD */
if (virSecurityDACSetOwnership(mgr, NULL,
dev_source->data.nix.path,
- user, group) < 0)
+ user, group, false) < 0)
goto done;
}
ret = 0;
@@ -1543,7 +1550,7 @@ virSecurityDACSetGraphicsLabel(virSecurityManagerPtr mgr,
gfx->data.spice.rendernode) {
if (virSecurityDACSetOwnership(mgr, NULL,
gfx->data.spice.rendernode,
- user, group) < 0)
+ user, group, false) < 0)
return -1;
}

@@ -1585,7 +1592,9 @@ virSecurityDACSetInputLabel(virSecurityManagerPtr mgr,
if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0)
return -1;

- ret = virSecurityDACSetOwnership(mgr, NULL, input->source.evdev, user, group);
+ ret = virSecurityDACSetOwnership(mgr, NULL,
+ input->source.evdev,
+ user, group, false);
break;

case VIR_DOMAIN_INPUT_TYPE_MOUSE:
@@ -1773,7 +1782,9 @@ virSecurityDACSetMemoryLabel(virSecurityManagerPtr mgr,
if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0)
return -1;

- ret = virSecurityDACSetOwnership(mgr, NULL, mem->nvdimmPath, user, group);
+ ret = virSecurityDACSetOwnership(mgr, NULL,
+ mem->nvdimmPath,
+ user, group, false);
break;

case VIR_DOMAIN_MEMORY_MODEL_DIMM:
@@ -1862,27 +1873,32 @@ virSecurityDACSetAllLabel(virSecurityManagerPtr mgr,

if (def->os.loader && def->os.loader->nvram &&
virSecurityDACSetOwnership(mgr, NULL,
- def->os.loader->nvram, user, group) < 0)
+ def->os.loader->nvram,
+ user, group, false) < 0)
return -1;

if (def->os.kernel &&
virSecurityDACSetOwnership(mgr, NULL,
- def->os.kernel, user, group) < 0)
+ def->os.kernel,
+ user, group, false) < 0)
return -1;

if (def->os.initrd &&
virSecurityDACSetOwnership(mgr, NULL,
- def->os.initrd, user, group) < 0)
+ def->os.initrd,
+ user, group, false) < 0)
return -1;

if (def->os.dtb &&
virSecurityDACSetOwnership(mgr, NULL,
- def->os.dtb, user, group) < 0)
+ def->os.dtb,
+ user, group, false) < 0)
return -1;

if (def->os.slic_table &&
virSecurityDACSetOwnership(mgr, NULL,
- def->os.slic_table, user, group) < 0)
+ def->os.slic_table,
+ user, group, false) < 0)
return -1;

return 0;
@@ -1904,7 +1920,7 @@ virSecurityDACSetSavedStateLabel(virSecurityManagerPtr mgr,
if (virSecurityDACGetImageIds(secdef, priv, &user, &group) < 0)
return -1;

- return virSecurityDACSetOwnership(mgr, NULL, savefile, user, group);
+ return virSecurityDACSetOwnership(mgr, NULL, savefile, user, group, false);
}

@@ -2224,7 +2240,7 @@ virSecurityDACDomainSetPathLabel(virSecurityManagerPtr mgr,
if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0)
return -1;

- return virSecurityDACSetOwnership(mgr, NULL, path, user, group);
+ return virSecurityDACSetOwnership(mgr, NULL, path, user, group, false);
}

virSecurityDriver virSecurityDriverDAC = {

--
2.18.1

Daniel P. Berrangé

2018-12-06 12:00:36 UTC

Post by Michal Privoznik
Because the implementation that will be used for label
remembering/recall is not atomic we have to give callers a chance
to enable or disable it. That is, enable it if and only if
metadata locking is enabled. Otherwise the feature MUST be turned
off.
---
src/security/security_dac.c | 74 ++++++++++++++++++++++---------------
1 file changed, 45 insertions(+), 29 deletions(-)

Reviewed-by: Daniel P. Berrangé <***@redhat.com>

Regards,
Daniel

Michal Privoznik

2018-11-29 13:52:28 UTC

This post might be inappropriate. Click to display it.

Daniel P. Berrangé

2018-12-06 12:09:38 UTC

Reviewed-by: Daniel P. Berrangé <***@redhat.com>

Regards,
Daniel

Michal Privoznik

2018-11-29 13:52:29 UTC

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 <***@redhat.com>
---
src/security/security_selinux.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 290faba9d6..0cf8164265 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -276,7 +276,6 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
for (i = 0; i < list->nItems; i++) {
virSecuritySELinuxContextItemPtr item = list->items[i];

- /* TODO Implement rollback */
if (!item->restore) {
rv = virSecuritySELinuxSetFileconHelper(list->manager,
item->path,
@@ -293,6 +292,18 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
break;
}

+ for (; rv < 0 && i > 0; i--) {
+ virSecuritySELinuxContextItemPtr item = list->items[i - 1];
+
+ if (!item->restore) {
+ virSecuritySELinuxRestoreFileLabel(list->manager,
+ item->path,
+ list->lock);
+ } else {
+ VIR_WARN("Ignoring failed restore attempt on %s", item->path);
+ }
+ }
+
if (list->lock)
virSecurityManagerMetadataUnlock(list->manager, &state);

--
2.18.1

Daniel P. Berrangé

2018-12-06 12:10:00 UTC

Post by Michal Privoznik
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.
---
src/security/security_selinux.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <***@redhat.com>

Regards,
Daniel

Michal Privoznik

2018-11-29 13:52:31 UTC

We are setting label on kernel, initrd, dtb and slic_table files.
But we never restored it.

Signed-off-by: Michal Privoznik <***@redhat.com>
---
src/security/security_selinux.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 553fc852db..5f2fab73bc 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -2656,6 +2656,22 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr mgr,
virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram, false) < 0)
rc = -1;

+ if (def->os.kernel &&
+ virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel, false) < 0)
+ rc = -1;
+
+ if (def->os.initrd &&
+ virSecuritySELinuxRestoreFileLabel(mgr, def->os.initrd, false) < 0)
+ rc = -1;
+
+ if (def->os.dtb &&
+ virSecuritySELinuxRestoreFileLabel(mgr, def->os.dtb, false) < 0)
+ rc = -1;
+
+ if (def->os.slic_table &&
+ virSecuritySELinuxRestoreFileLabel(mgr, def->os.slic_table, false) < 0)
+ rc = -1;
+
return rc;
}

--
2.18.1

Daniel P. Berrangé

2018-12-06 12:11:05 UTC

Post by Michal Privoznik
We are setting label on kernel, initrd, dtb and slic_table files.
But we never restored it.
---
src/security/security_selinux.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)

Reviewed-by: Daniel P. Berrangé <***@redhat.com>

Regards,
Daniel

Michal Privoznik

2018-11-29 13:52:33 UTC

Signed-off-by: Michal Privoznik <***@redhat.com>
---
src/qemu/libvirtd_qemu.aug | 1 +
src/qemu/qemu.conf | 6 ++++++
src/qemu/qemu_conf.c | 4 ++++
src/qemu/test_libvirtd_qemu.aug.in | 1 +
4 files changed, 12 insertions(+)

diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index ddc4bbfd1d..8a5b39e568 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -71,6 +71,7 @@ module Libvirtd_qemu =
| str_entry "user"
| str_entry "group"
| bool_entry "dynamic_ownership"
+ | bool_entry "remember_owner"
| str_array_entry "cgroup_controllers"
| str_array_entry "cgroup_device_acl"
| int_entry "seccomp_sandbox"
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 8391332cb4..31e8d8476b 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -450,6 +450,12 @@
# Set to 0 to disable file ownership changes.
#dynamic_ownership = 1

+# Whether libvirt should remember and restore the original
+# ownership over files it is relabeling. Be aware that with the
+# current implementation this requires exclusive access to the
+# files which might hurt performance a bit in some cases.
+# Defaults to 1, set to 0 to disable the feature.
+#remember_owner = 1

# What cgroup controllers to make use of with QEMU guests
#
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index a946b05d5d..89491a37b7 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -147,6 +147,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
cfg->group = (gid_t)-1;
}
cfg->dynamicOwnership = privileged;
+ cfg->rememberOwner = true;

cfg->cgroupControllers = -1; /* -1 == auto-detect */

@@ -730,6 +731,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
if (virConfGetValueBool(conf, "dynamic_ownership", &cfg->dynamicOwnership) < 0)
goto cleanup;

+ if (virConfGetValueBool(conf, "remember_owner", &cfg->rememberOwner) < 0)
+ goto cleanup;
+
if (virConfGetValueStringList(conf, "cgroup_controllers", false,
&controllers) < 0)
goto cleanup;
diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
index f1e8806ad2..92a8ae1192 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -43,6 +43,7 @@ module Test_libvirtd_qemu =
{ "user" = "root" }
{ "group" = "root" }
{ "dynamic_ownership" = "1" }
+{ "remember_owner" = "1" }
{ "cgroup_controllers"
{ "1" = "cpu" }
{ "2" = "devices" }

--
2.18.1

Daniel P. Berrangé

2018-12-06 12:18:47 UTC