This file implements wrappers over XATTR getter/setter. It
ensures the proper XATTR namespace is used.
Signed-off-by: Michal Privoznik <mprivozn@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Nov 29, 2018 at 02:52:18PM +0100, Michal Privoznik wrote:
> This file implements wrappers over XATTR getter/setter. It
> ensures the proper XATTR namespace is used.
>
> Signed-off-by: Michal Privoznik <mprivozn@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
>
> +/**
> + * 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;
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.
> +
> + 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;
> + }
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.
> +
> + 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;
> +}
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/6/18 12:38 PM, Daniel P. Berrangé wrote:
> On Thu, Nov 29, 2018 at 02:52:18PM +0100, Michal Privoznik wrote:
>> This file implements wrappers over XATTR getter/setter. It
>> ensures the proper XATTR namespace is used.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@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
>>
>
>
>> +/**
>> + * 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;
>
> 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.
>
>> +
>> + 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;
>> + }
>
> 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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Dec 06, 2018 at 03:17:47PM +0100, Michal Privoznik wrote:
> On 12/6/18 12:38 PM, Daniel P. Berrangé wrote:
> > On Thu, Nov 29, 2018 at 02:52:18PM +0100, Michal Privoznik wrote:
> >> This file implements wrappers over XATTR getter/setter. It
> >> ensures the proper XATTR namespace is used.
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn@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
> >> +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 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).
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.
> 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.
Simply don't try to decide that. Require identical labels for all
guests sharing the disk.
> 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.
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
--
|: 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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/6/18 3:34 PM, Daniel P. Berrangé wrote:
> On Thu, Dec 06, 2018 at 03:17:47PM +0100, Michal Privoznik wrote:
>> On 12/6/18 12:38 PM, Daniel P. Berrangé wrote:
>>> On Thu, Nov 29, 2018 at 02:52:18PM +0100, Michal Privoznik wrote:
>>>> This file implements wrappers over XATTR getter/setter. It
>>>> ensures the proper XATTR namespace is used.
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn@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
>
>>>> +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 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).
>
> 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.
>
>> 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.
>
> Simply don't try to decide that. Require identical labels for all
> guests sharing the disk.
>
>> 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.
>
> 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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Dec 06, 2018 at 04:12:45PM +0100, Michal Privoznik wrote:
> On 12/6/18 3:34 PM, Daniel P. Berrangé wrote:
> > On Thu, Dec 06, 2018 at 03:17:47PM +0100, Michal Privoznik wrote:
> >> On 12/6/18 12:38 PM, Daniel P. Berrangé wrote:
> >>> On Thu, Nov 29, 2018 at 02:52:18PM +0100, Michal Privoznik wrote:
> >>>> This file implements wrappers over XATTR getter/setter. It
> >>>> ensures the proper XATTR namespace is used.
> >>>>
> >>>> Signed-off-by: Michal Privoznik <mprivozn@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
> >
> >>>> +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 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).
> >
> > 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.
Yes, that makes sense to me.
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.