Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
src/security/Makefile.inc.am | 2 +
src/security/security_util.c | 198 +++++++++++++++++++++++++++++++++++
src/security/security_util.h | 32 ++++++
3 files changed, 232 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..4178fdff81
--- /dev/null
+++ b/src/security/security_util.c
@@ -0,0 +1,198 @@
+/*
+ * 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 "security_util.h"
+
+#define VIR_FROM_THIS VIR_FROM_SECURITY
+
+/* There are four namespaces available (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.
+ */
+#define XATTR_NAMESPACE "trusted"
+
+static char *
+virSecurityGetAttrName(const char *name)
+{
+ char *ret;
+ ignore_value(virAsprintf(&ret, XATTR_NAMESPACE".libvirt.security.%s", name));
+ return ret;
+}
+
+
+static char *
+virSecurityGetRefCountAttrName(const char *name)
+{
+ char *ret;
+ ignore_value(virAsprintf(&ret, XATTR_NAMESPACE".libvirt.security.ref_%s", name));
+ 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 Fri, Nov 23, 2018 at 09:43:21AM +0100, Michal Privoznik wrote: > +/* There are four namespaces available (xattr(7)): s/available/available on Linux/ FreeBSD only supports 'user' and 'system' namespaces > + * > + * 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. That prevents the QEMU driver using this functionality on any non-Linux host. The key problem we obviously face is that of the QEMU process being able to modify the xattrs maliciously. 'trusted' namespace solves this for Linux but unsolved for BSD/macOS. I can only think of two alternative ways to deal with this - Use a sidecar file. eg $FILEPATH.libvirt.json Works ok for plain files. Troublesome for device nodes. Would have to use a file in /var/run/libvirt/devs/$DEVNODE perhaps ? - Use 'user' label but add a cryptographic signature as a further attribute. Doesn't prevent tampering but lets us throw away the data when tempering is detected. Did you consider either of these, or any other possible options ? I'm still loathe to bake in a solution that will only work on Linux, despite 99% of our userbase being Linux. 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 11/27/18 4:20 PM, Daniel P. Berrangé wrote: > On Fri, Nov 23, 2018 at 09:43:21AM +0100, Michal Privoznik wrote: > >> +/* There are four namespaces available (xattr(7)): > > s/available/available on Linux/ > > FreeBSD only supports 'user' and 'system' namespaces D'oh! > >> + * >> + * 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. > > That prevents the QEMU driver using this functionality on any > non-Linux host. > > The key problem we obviously face is that of the QEMU process > being able to modify the xattrs maliciously. 'trusted' namespace > solves this for Linux but unsolved for BSD/macOS. > > I can only think of two alternative ways to deal with this > > - Use a sidecar file. eg $FILEPATH.libvirt.json > Works ok for plain files. Troublesome for device nodes. > Would have to use a file in /var/run/libvirt/devs/$DEVNODE > perhaps ? I rather not create files. If there is a bug and libvirt doesn't remove the file on the last restore (or it gets refcounting wrong or something) then it requires user intervention. Not only that, but the file would need to be owned by root:root (in order to avoid malicious users mangling its contents) which might not be possible at all times (e.g. root squashed NFS). On the other hand, NFS itself doesn't support XATTRs currently so these patches do not with that filesystem. > > - Use 'user' label but add a cryptographic signature > as a further attribute. Doesn't prevent tampering > but lets us throw away the data when tempering is > detected. I'm not sure how this would work with multiple daemons. They would have to have the key shared among them. I mean, if one daemon sets the attribute and signs it with its own key and then comes another daemon and updates the attribute it needs to sign it with the very same key otherwise the signature would be invalidated from the first daemon POV. > > Did you consider either of these, or any other possible > options ? I'm still loathe to bake in a solution that will > only work on Linux, despite 99% of our userbase being Linux. Not really. I focused on Linux and haven't realized BSD doesn't support trusted. Now that I am playing with this it looks like 'system' while being available on both Linux and BSD can't really be set on Linux. But it can be set on BSD (for privileged user only of course). I'm wondering if using 'trusted' on Linux and 'system' on BSD is the way to go. This could work except for cases where BSD and Linux meet, which can be only on NFS (right?) which doesn't support XATTRs anyway. Therefore I think we are safe, aren't we? Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Nov 28, 2018 at 09:35:49AM +0100, Michal Privoznik wrote: > On 11/27/18 4:20 PM, Daniel P. Berrangé wrote: > > On Fri, Nov 23, 2018 at 09:43:21AM +0100, Michal Privoznik wrote: > > > >> +/* There are four namespaces available (xattr(7)): > > > > s/available/available on Linux/ > > > > FreeBSD only supports 'user' and 'system' namespaces > > D'oh! > > > > >> + * > >> + * 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. > > > > That prevents the QEMU driver using this functionality on any > > non-Linux host. > > > > The key problem we obviously face is that of the QEMU process > > being able to modify the xattrs maliciously. 'trusted' namespace > > solves this for Linux but unsolved for BSD/macOS. > > > > I can only think of two alternative ways to deal with this > > > > - Use a sidecar file. eg $FILEPATH.libvirt.json > > Works ok for plain files. Troublesome for device nodes. > > Would have to use a file in /var/run/libvirt/devs/$DEVNODE > > perhaps ? > > I rather not create files. If there is a bug and libvirt doesn't remove > the file on the last restore (or it gets refcounting wrong or something) > then it requires user intervention. Isn't that true of this series too - ie if we update the xattr for refcount wrong, or fail to remove the xattr on last restore ? Feels like we have the same possible source of bugs, just against an unlink() call rather than a setxattr() call. The existance of libvirt's sidecar files on disk would be more obviously to an admin. That said tellin people to "rm" the file is kinda dangerous if they typo and rm the real disk file that's alongside. Using a completely 100% separate directory would solve that. > Not only that, but the file would need to be owned by root:root (in > order to avoid malicious users mangling its contents) which might not be > possible at all times (e.g. root squashed NFS). On the other hand, NFS > itself doesn't support XATTRs currently so these patches do not with > that filesystem. Yeah, root sqaush NFS is horrible :-( > > > > > - Use 'user' label but add a cryptographic signature > > as a further attribute. Doesn't prevent tampering > > but lets us throw away the data when tempering is > > detected. > > I'm not sure how this would work with multiple daemons. They would have > to have the key shared among them. I mean, if one daemon sets the > attribute and signs it with its own key and then comes another daemon > and updates the attribute it needs to sign it with the very same key > otherwise the signature would be invalidated from the first daemon POV. Yeah, it would need shared keys in some manner which is a very big pain point. > > > > > Did you consider either of these, or any other possible > > options ? I'm still loathe to bake in a solution that will > > only work on Linux, despite 99% of our userbase being Linux. > > Not really. I focused on Linux and haven't realized BSD doesn't support > trusted. > > Now that I am playing with this it looks like 'system' while being > available on both Linux and BSD can't really be set on Linux. But it can > be set on BSD (for privileged user only of course). I'm wondering if > using 'trusted' on Linux and 'system' on BSD is the way to go. This > could work except for cases where BSD and Linux meet, which can be only > on NFS (right?) which doesn't support XATTRs anyway. Therefore I think > we are safe, aren't we? I expect eventually NFSv4 may get xattr support - there's a 1 year old RFC for it: https://tools.ietf.org/html/rfc8276 Any app using xattr is going to face this same problem unless they only use the common "user" namespace Maybe justusing trusted (Linux) and system (BSD) is good enough for us to get started, and we can allow for a different system (side car files, or attribute signatures) in future if it becomes needed. 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 11/28/18 11:00 AM, Daniel P. Berrangé wrote: > On Wed, Nov 28, 2018 at 09:35:49AM +0100, Michal Privoznik wrote: >> On 11/27/18 4:20 PM, Daniel P. Berrangé wrote: >>> On Fri, Nov 23, 2018 at 09:43:21AM +0100, Michal Privoznik wrote: >>> >>>> +/* There are four namespaces available (xattr(7)): >>> >>> s/available/available on Linux/ >>> >>> FreeBSD only supports 'user' and 'system' namespaces >> >> D'oh! >> >>> >>>> + * >>>> + * 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. >>> >>> That prevents the QEMU driver using this functionality on any >>> non-Linux host. >>> >>> The key problem we obviously face is that of the QEMU process >>> being able to modify the xattrs maliciously. 'trusted' namespace >>> solves this for Linux but unsolved for BSD/macOS. >>> >>> I can only think of two alternative ways to deal with this >>> >>> - Use a sidecar file. eg $FILEPATH.libvirt.json >>> Works ok for plain files. Troublesome for device nodes. >>> Would have to use a file in /var/run/libvirt/devs/$DEVNODE >>> perhaps ? >> >> I rather not create files. If there is a bug and libvirt doesn't remove >> the file on the last restore (or it gets refcounting wrong or something) >> then it requires user intervention. > > Isn't that true of this series too - ie if we update the xattr for > refcount wrong, or fail to remove the xattr on last restore ? Yes, this the same. If there is a bug then unlink() and setxattr() don't differ from this POV. > > Feels like we have the same possible source of bugs, just against > an unlink() call rather than a setxattr() call. > > The existance of libvirt's sidecar files on disk would be more > obviously to an admin. > > That said tellin people to "rm" the file is kinda dangerous if > they typo and rm the real disk file that's alongside. Using a > completely 100% separate directory would solve that. Except we can't do that. If there is a file accessible to two daemons (e.g. running on the same host in two different containers) the daemons can't have their private DB, it has to be shared -> because of refcounting. If the DB was private then one daemon could not increment/decrement the refcounter inside the other daemon (consider two daemons, one starting domain A, the other starting domain B, then the first destroying domain A). I mean, this is the sole reason I chose XATTRs and not some in memory list/mapping of original owners. > >> Not only that, but the file would need to be owned by root:root (in >> order to avoid malicious users mangling its contents) which might not be >> possible at all times (e.g. root squashed NFS). On the other hand, NFS >> itself doesn't support XATTRs currently so these patches do not with >> that filesystem. > > Yeah, root sqaush NFS is horrible :-( > >> >>> >>> - Use 'user' label but add a cryptographic signature >>> as a further attribute. Doesn't prevent tampering >>> but lets us throw away the data when tempering is >>> detected. >> >> I'm not sure how this would work with multiple daemons. They would have >> to have the key shared among them. I mean, if one daemon sets the >> attribute and signs it with its own key and then comes another daemon >> and updates the attribute it needs to sign it with the very same key >> otherwise the signature would be invalidated from the first daemon POV. > > Yeah, it would need shared keys in some manner which is a very big > pain point. > >> >>> >>> Did you consider either of these, or any other possible >>> options ? I'm still loathe to bake in a solution that will >>> only work on Linux, despite 99% of our userbase being Linux. >> >> Not really. I focused on Linux and haven't realized BSD doesn't support >> trusted. >> >> Now that I am playing with this it looks like 'system' while being >> available on both Linux and BSD can't really be set on Linux. But it can >> be set on BSD (for privileged user only of course). I'm wondering if >> using 'trusted' on Linux and 'system' on BSD is the way to go. This >> could work except for cases where BSD and Linux meet, which can be only >> on NFS (right?) which doesn't support XATTRs anyway. Therefore I think >> we are safe, aren't we? > > I expect eventually NFSv4 may get xattr support - there's a 1 year old > RFC for it: > > https://tools.ietf.org/html/rfc8276 > > Any app using xattr is going to face this same problem unless they only > use the common "user" namespace > > Maybe justusing trusted (Linux) and system (BSD) is good enough for > us to get started, and we can allow for a different system (side car > files, or attribute signatures) in future if it becomes needed. Agreed. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.