[libvirt] [PATCH v5 0/3] Keep original file label

Yet another rework of $subj. I am still not solving atomicity problem for now. See diff to the patches if you want to know what's changed. Michal Privoznik (3): virFile: Add APIs for extended attributes handling virfile: Introduce internal API for managing ACL security_dac: Favour ACLs over chown() configure.ac | 2 + libvirt.spec.in | 1 + m4/virt-acl.m4 | 9 ++ src/Makefile.am | 4 +- src/libvirt_private.syms | 6 + src/security/security_dac.c | 309 +++++++++++++++++++++++++++++++++++++++----- src/util/virfile.c | 295 ++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 28 ++++ 8 files changed, 623 insertions(+), 31 deletions(-) create mode 100644 m4/virt-acl.m4 -- 1.8.1.5

Currently, only three wrappers are being implemented: virFileSetAttr for setting attributes virFileGetAttr for querying attributes (note we need to call it twice, first time to get length of attribute value, second to get actual value) virFileRemoveAttr for removing attributes --- diff to v4: -drop errno setting diff to v3: -set errno=ENOSYS when building without WITH_ATTR for easier check within callee. diff to v2: -drop multiple check for libattr src/libvirt_private.syms | 3 ++ src/util/virfile.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 14 +++++++ 3 files changed, 122 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 21bc615..fd57fa0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1253,8 +1253,11 @@ virFileClose; virFileDirectFdFlag; virFileFclose; virFileFdopen; +virFileGetAttr; virFileLoopDeviceAssociate; +virFileRemoveAttr; virFileRewrite; +virFileSetAttr; virFileTouch; virFileUpdatePerm; virFileWrapperFdClose; diff --git a/src/util/virfile.c b/src/util/virfile.c index 4a9fa81..2409db4 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -37,6 +37,10 @@ # include <sys/ioctl.h> #endif +#ifdef WITH_ATTR +# include <attr/xattr.h> +#endif + #include "vircommand.h" #include "configmake.h" #include "viralloc.h" @@ -644,3 +648,104 @@ int virFileLoopDeviceAssociate(const char *file, } #endif /* __linux__ */ + +#ifdef WITH_ATTR +int +virFileSetAttr(const char *file, + const char *name, + const char *value) +{ + size_t valueSize = strlen(value); + if (setxattr(file, name, value, valueSize, 0) < 0) { + virReportSystemError(errno, + _("Unable to set extended attribute '%s' on '%s'"), + name, file); + return -1; + } + return 0; +} + +int +virFileGetAttr(const char *file, + const char *name, + char **value) +{ + int ret = -1; + char *buf = NULL; + ssize_t valueSize; + + /* get attribute length */ + if ((valueSize = getxattr(file, name, NULL, 0)) < 0) { + /* The Linux kernel does not define ENOATTR, but maps it to ENODATA. */ + if (errno == ENOATTR || errno == ENODATA) { + *value = NULL; + return 0; + } else { + virReportSystemError(errno, + _("Unable to get extended attribute '%s' on '%s'"), + name, file); + return ret; + } + } + + if (VIR_ALLOC_N(buf, valueSize) < 0) { + virReportOOMError(); + return ret; + } + + if ((ret = getxattr(file, name, buf, valueSize)) < 0) { + VIR_FREE(buf); + virReportSystemError(errno, + _("Unable to get extended attribute '%s' on '%s'"), + name, file); + } else { + *value = buf; + } + + return ret; +} + +int +virFileRemoveAttr(const char *file, + const char *name) +{ + if (removexattr(file, name) < 0) { + virReportSystemError(errno, + _("Unable to remove extended attribute '%s' on '%s'"), + name, file); + return -1; + } + return 0; +} + +#else /* WITH_ATTR */ + +int +virFileSetAttr(const char *file ATTRIBUTE_UNUSED, + const char *name ATTRIBUTE_UNUSED, + const char *value ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to set extended attributes")); + return -1; +} + +int +virFileGetAttr(const char *file ATTRIBUTE_UNUSED, + const char *name ATTRIBUTE_UNUSED, + char **value ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to get extended attributes")); + return -1; +} + +int +virFileRemoveAttr(const char *file ATTRIBUTE_UNUSED, + const char *name ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to remove extended attributes")); + return -1; +} +#endif /* WITH_ATTR */ diff --git a/src/util/virfile.h b/src/util/virfile.h index c885b73..9e0adf6 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -108,4 +108,18 @@ int virFileUpdatePerm(const char *path, int virFileLoopDeviceAssociate(const char *file, char **dev); +int virFileSetAttr(const char *file, + const char *name, + const char *value) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + +int virFileGetAttr(const char *file, + const char *name, + char **value) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + +int virFileRemoveAttr(const char *file, + const char *name) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + #endif /* __VIR_FILES_H */ -- 1.8.1.5

On Thu, Mar 21, 2013 at 05:50:47PM +0100, Michal Privoznik wrote:
Currently, only three wrappers are being implemented: virFileSetAttr for setting attributes virFileGetAttr for querying attributes (note we need to call it twice, first time to get length of attribute value, second to get actual value) virFileRemoveAttr for removing attributes --- diff to v4: -drop errno setting
diff to v3: -set errno=ENOSYS when building without WITH_ATTR for easier check within callee.
diff to v2: -drop multiple check for libattr src/libvirt_private.syms | 3 ++ src/util/virfile.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 14 +++++++ 3 files changed, 122 insertions(+)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

For now, only three APIs are implemented: virFileGetACL to retrieve permission for a specific user virFileSetACL for setting requested permissions for a specific user, virFileRemoveACL to remove those permissions. --- diff to v4: -drop errno setting diff to v3: -set errno=ENOSYS when building without WITH_ATTR for easier check within callee. -ACL mask is deleted prior recalc as after removing our entry, mask may be not required anymore. diff to v2: -Introduced m4 macro to check for libacl -new virFileGetACL API -ACL mask recalc offloaded to libacl configure.ac | 2 + libvirt.spec.in | 1 + m4/virt-acl.m4 | 9 +++ src/Makefile.am | 4 +- src/libvirt_private.syms | 3 + src/util/virfile.c | 190 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 14 ++++ 7 files changed, 221 insertions(+), 2 deletions(-) create mode 100644 m4/virt-acl.m4 diff --git a/configure.ac b/configure.ac index 9d366e9..4e6627c 100644 --- a/configure.ac +++ b/configure.ac @@ -142,6 +142,7 @@ AC_MSG_RESULT([$VERSION_SCRIPT_FLAGS]) LIBVIRT_COMPILE_WARNINGS +LIBVIRT_CHECK_ACL LIBVIRT_CHECK_APPARMOR LIBVIRT_CHECK_ATTR LIBVIRT_CHECK_AUDIT @@ -2462,6 +2463,7 @@ fi AC_MSG_NOTICE([]) AC_MSG_NOTICE([Libraries]) AC_MSG_NOTICE([]) +LIBVIRT_RESULT_ACL LIBVIRT_RESULT_APPARMOR LIBVIRT_RESULT_ATTR LIBVIRT_RESULT_AUDIT diff --git a/libvirt.spec.in b/libvirt.spec.in index 9fb753a..222674d 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -411,6 +411,7 @@ BuildRequires: gettext BuildRequires: libtasn1-devel BuildRequires: gnutls-devel BuildRequires: libattr-devel +BuildRequires: libacl-devel %if 0%{?fedora} >= 12 || 0%{?rhel} >= 6 # for augparse, optionally used in testing BuildRequires: augeas diff --git a/m4/virt-acl.m4 b/m4/virt-acl.m4 new file mode 100644 index 0000000..7f16dca --- /dev/null +++ b/m4/virt-acl.m4 @@ -0,0 +1,9 @@ +dnl The libacl.so library + +AC_DEFUN([LIBVIRT_CHECK_ACL],[ + LIBVIRT_CHECK_LIB([ACL], [acl], [acl_init], [sys/acl.h]) +]) + +AC_DEFUN([LIBVIRT_RESULT_ACL],[ + LIBVIRT_RESULT_LIB([ACL]) +]) diff --git a/src/Makefile.am b/src/Makefile.am index 3f69d39..921ef94 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -768,11 +768,11 @@ libvirt_util_la_SOURCES = \ $(UTIL_SOURCES) libvirt_util_la_CFLAGS = $(CAPNG_CFLAGS) $(YAJL_CFLAGS) $(LIBNL_CFLAGS) \ $(AM_CFLAGS) $(AUDIT_CFLAGS) $(DEVMAPPER_CFLAGS) \ - $(DBUS_CFLAGS) $(LDEXP_LIBM) $(NUMACTL_CFLAGS) + $(DBUS_CFLAGS) $(LDEXP_LIBM) $(NUMACTL_CFLAGS) $(ACL_CFLAGS) libvirt_util_la_LIBADD = $(CAPNG_LIBS) $(YAJL_LIBS) $(LIBNL_LIBS) \ $(THREAD_LIBS) $(AUDIT_LIBS) $(DEVMAPPER_LIBS) \ $(LIB_CLOCK_GETTIME) $(DBUS_LIBS) $(MSCOM_LIBS) $(LIBXML_LIBS) \ - $(SECDRIVER_LIBS) $(NUMACTL_LIBS) + $(SECDRIVER_LIBS) $(NUMACTL_LIBS) $(ACL_LIBS) noinst_LTLIBRARIES += libvirt_conf.la diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fd57fa0..db20591 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1253,10 +1253,13 @@ virFileClose; virFileDirectFdFlag; virFileFclose; virFileFdopen; +virFileGetACL; virFileGetAttr; virFileLoopDeviceAssociate; +virFileRemoveACL; virFileRemoveAttr; virFileRewrite; +virFileSetACL; virFileSetAttr; virFileTouch; virFileUpdatePerm; diff --git a/src/util/virfile.c b/src/util/virfile.c index 2409db4..f8f0e20 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -41,6 +41,10 @@ # include <attr/xattr.h> #endif +#ifdef WITH_ACL +# include <acl/libacl.h> +#endif + #include "vircommand.h" #include "configmake.h" #include "viralloc.h" @@ -749,3 +753,189 @@ virFileRemoveAttr(const char *file ATTRIBUTE_UNUSED, return -1; } #endif /* WITH_ATTR */ + +#ifdef WITH_ACL +static acl_entry_t +virFileACLFindEntry(acl_t acl, acl_tag_t type, id_t id) +{ + acl_entry_t ent; + acl_tag_t e_type; + id_t *e_id_p; + + /* acl_get_entry returns 1 if there's an entry in @acl */ + if (acl_get_entry(acl, ACL_FIRST_ENTRY, &ent) != 1) + return NULL; + + do { + acl_get_tag_type(ent, &e_type); + if (e_type == type) { + if (id == ACL_UNDEFINED_ID) + return ent; + + if (!(e_id_p = acl_get_qualifier(ent))) + return NULL; + if (*e_id_p == id) { + acl_free(e_id_p); + return ent; + } + acl_free(e_id_p); + } + } while (acl_get_entry(acl, ACL_NEXT_ENTRY, &ent) == 1); + + return NULL; +} + +static void +virFileACLSetPerms(acl_entry_t ent, mode_t perms) +{ + acl_permset_t set; + + acl_get_permset(ent, &set); + if (perms & S_IRUSR) + acl_add_perm(set, ACL_READ); + else + acl_delete_perm(set, ACL_READ); + if (perms & S_IWUSR) + acl_add_perm(set, ACL_WRITE); + else + acl_delete_perm(set, ACL_WRITE); + if (perms & S_IXUSR) + acl_add_perm(set, ACL_EXECUTE); + else + acl_delete_perm(set, ACL_EXECUTE); +} + +static void +virFileACLGetPerms(acl_entry_t ent, mode_t *perms) +{ + acl_permset_t set; + + *perms = 0; + acl_get_permset(ent, &set); + if (acl_get_perm(set, ACL_READ)) + *perms |= S_IRUSR; + if (acl_get_perm(set, ACL_WRITE)) + *perms |= S_IWUSR; + if (acl_get_perm(set, ACL_EXECUTE)) + *perms |= S_IXUSR; +} + +static int +virFileACLSetOrRemove(const char *path, + uid_t user, + mode_t perms, + bool set) +{ + int ret = -1; + acl_t acl; + acl_entry_t ent; + + if (!(acl = acl_get_file(path, ACL_TYPE_ACCESS))) { + virReportSystemError(errno, _("Unable to get ACL on %s"), path); + return ret; + } + + ent = virFileACLFindEntry(acl, ACL_USER, user); + if (set) { + if (!ent && acl_create_entry(&acl, &ent) < 0) { + virReportSystemError(errno, "%s", _("Unable to create ACL entity")); + goto cleanup; + } + acl_set_tag_type(ent, ACL_USER); + acl_set_qualifier(ent, &user); + + virFileACLSetPerms(ent, perms); + + } else if (ent) { + if (acl_delete_entry(acl, ent) < 0) { + virReportSystemError(errno, "%s", _("Unable to delete ACL entity")); + goto cleanup; + } + } + + if ((ent = virFileACLFindEntry(acl, ACL_MASK, ACL_UNDEFINED_ID)) && + acl_delete_entry(acl, ent) < 0) { + virReportSystemError(errno, "%s", _("Unable to delete ACL mask")); + goto cleanup; + } + + if (acl_equiv_mode(acl, NULL) && acl_calc_mask(&acl) < 0) { + virReportSystemError(errno, "%s", _("Unable to calculate ACL mask")); + goto cleanup; + } + + if (acl_set_file(path, ACL_TYPE_ACCESS, acl) < 0) { + virReportSystemError(errno, _("Unable to set ACL on %s"), path); + goto cleanup; + } + + ret = 0; +cleanup: + acl_free(acl); + return ret; +} + +int +virFileSetACL(const char *file, + uid_t user, + mode_t perms) +{ + return virFileACLSetOrRemove(file, user, perms, true); +} + +int +virFileRemoveACL(const char *file, + uid_t user) +{ + return virFileACLSetOrRemove(file, user, 0, false); +} + +int +virFileGetACL(const char *file, + uid_t user, + mode_t *perms) +{ + acl_t acl; + acl_entry_t ent; + + if (!(acl = acl_get_file(file, ACL_TYPE_ACCESS))) { + virReportSystemError(errno, _("Unable to get ACL on %s"), file); + return -1; + } + + if ((ent = virFileACLFindEntry(acl, ACL_USER, user))) + virFileACLGetPerms(ent, perms); + else + *perms = 0; + + acl_free(acl); + return 0; +} + +#else /* WITH_ACL */ + +int +virFileSetACL(const char *file ATTRIBUTE_UNUSED, + uid_t user ATTRIBUTE_UNUSED, + mode_t perms ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", _("Unable to set ACL")); + return -1; +} + +int +virFileRemoveACL(const char *file ATTRIBUTE_UNUSED, + uid_t user ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", _("Unable to remove ACL")); + return -1; +} +int +virFileGetACL(const char *file ATTRIBUTE_UNUSED, + uid_t user ATTRIBUTE_UNUSED, + mode_t *perms ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", _("Unable to get ACL")); + return -1; +} +#endif /* WITH_ACL */ diff --git a/src/util/virfile.h b/src/util/virfile.h index 9e0adf6..2a322b5 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -122,4 +122,18 @@ int virFileRemoveAttr(const char *file, const char *name) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int virFileSetACL(const char *file, + uid_t user, + mode_t perms) + ATTRIBUTE_NONNULL(1); + +int virFileGetACL(const char *file, + uid_t user, + mode_t *perms) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); + +int virFileRemoveACL(const char *file, + uid_t user) + ATTRIBUTE_NONNULL(1); + #endif /* __VIR_FILES_H */ -- 1.8.1.5

On Thu, Mar 21, 2013 at 05:50:48PM +0100, Michal Privoznik wrote:
For now, only three APIs are implemented: virFileGetACL to retrieve permission for a specific user virFileSetACL for setting requested permissions for a specific user, virFileRemoveACL to remove those permissions. --- diff to v4: -drop errno setting
diff to v3: -set errno=ENOSYS when building without WITH_ATTR for easier check within callee. -ACL mask is deleted prior recalc as after removing our entry, mask may be not required anymore.
diff to v2: -Introduced m4 macro to check for libacl -new virFileGetACL API -ACL mask recalc offloaded to libacl configure.ac | 2 + libvirt.spec.in | 1 + m4/virt-acl.m4 | 9 +++ src/Makefile.am | 4 +- src/libvirt_private.syms | 3 + src/util/virfile.c | 190 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 14 ++++ 7 files changed, 221 insertions(+), 2 deletions(-) create mode 100644 m4/virt-acl.m4
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

From implementation POV, a reference counter is introduced, so ACL is restored only on the last restore attempt in order to not cut off other domains. And since a file may had an ACL for a user already set, we need to keep this as well. Both these, the reference counter and original ACL are stored as extended attributes named trusted.libvirt.dac.refCount and
On filesystems supporting ACLs we don't need to do a chown but we can just set ACLs to gain access for qemu. However, since we are setting these on too low level, where we don't know if disk is just a read only or read write, we set read write access unconditionally. trusted.libvirt.dac.oldACL respectively. However, some filesystems doesn't support ACLs, XATTRs, or both. So the code is made to favour ACLs among with tracking the reference count. If this fails, we fall back to chown() with best effort to remember the original owner of file. --- diff to v4: -adapt to changed error reporting in 1/3 and 2/3 diff to v3: -Dan's suggestions worked in diff to v2: -basically squashed functionality of 2/4 and 4/4 from previous round src/security/security_dac.c | 309 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 280 insertions(+), 29 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 35b90da..989dc50 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -25,6 +25,7 @@ #include "security_dac.h" #include "virerror.h" +#include "virfile.h" #include "virutil.h" #include "viralloc.h" #include "virlog.h" @@ -34,6 +35,9 @@ #define VIR_FROM_THIS VIR_FROM_SECURITY #define SECURITY_DAC_NAME "dac" +#define SECURITY_DAC_XATTR_OLD_ACL "trusted.libvirt.dac.oldACL" +#define SECURITY_DAC_XATTR_OLD_OWNER "trusted.libvirt.dac.oldOwner" +#define SECURITY_DAC_XATTR_REFCOUNT "trusted.libvirt.dac.refCount" typedef struct _virSecurityDACData virSecurityDACData; typedef virSecurityDACData *virSecurityDACDataPtr; @@ -234,6 +238,196 @@ int virSecurityDACGetImageIds(virDomainDefPtr def, virSecurityDACDataPtr priv, return 0; } +static int +virSecurityDACGetXATTRRefcount(const char *path, + int *refCount) +{ + int ret = -1; + char *refCountStr; + + if (virFileGetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, &refCountStr) < 0) + return ret; + + VIR_DEBUG("path=%s refCountStr=%s", path, NULLSTR(refCountStr)); + + if (!refCountStr) { + *refCount = 0; + return 0; + } + + if (virStrToLong_i(refCountStr, NULL, 10, refCount) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Malformed %s attribute: %s"), + SECURITY_DAC_XATTR_REFCOUNT, + refCountStr); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(refCountStr); + return ret; +} + +static int +virSecurityDACSetXATTRRefcount(const char *path, + int refCount) +{ + int ret = -1; + char *refCountStr; + + VIR_DEBUG("path=%s refCount=%d", path, refCount); + + if (refCount == 0) { + virFileRemoveAttr(path, SECURITY_DAC_XATTR_REFCOUNT); + return 0; + } + + if (virAsprintf(&refCountStr, "%u", refCount) < 0) { + virReportOOMError(); + return ret; + } + + if (virFileSetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, refCountStr) < 0) + goto cleanup; + + ret = 0; +cleanup: + VIR_FREE(refCountStr); + return ret; +} + +static int +virSecurityDACSetACL(const char *path, + uid_t uid) +{ + int ret = -1; + char *oldACL = NULL; + mode_t perms; + + VIR_DEBUG("path=%s uid=%u", path, uid); + + if (virFileGetACL(path, uid, &perms) < 0) { + /* error getting ACL entry for @uid */ + goto cleanup; + } + + if (virAsprintf(&oldACL, "%u:0%o", uid, perms) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (virFileSetAttr(path, SECURITY_DAC_XATTR_OLD_ACL, oldACL) < 0) + goto cleanup; + + if (virFileSetACL(path, uid, S_IRUSR | S_IWUSR) < 0) + goto cleanup; + + ret = 0; +cleanup: + if (ret < 0) + virFileRemoveAttr(path, SECURITY_DAC_XATTR_OLD_ACL); + VIR_FREE(oldACL); + return ret; +} + +static int +virSecurityDACRestoreACL(const char *path) +{ + int ret = -1; + char *oldACL = NULL, *c; + uid_t uid; + mode_t perms; + + VIR_DEBUG("path=%s", path); + + if (virFileGetAttr(path, SECURITY_DAC_XATTR_OLD_ACL, &oldACL) < 0) + return ret; + + if (!oldACL) { + VIR_WARN("Attribute %s is missing", SECURITY_DAC_XATTR_OLD_ACL); + return ret; + } + + if (!(c = strchr(oldACL, ':'))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Malformed %s attribute: %s"), + SECURITY_DAC_XATTR_OLD_ACL, oldACL); + goto cleanup; + } + + *c = '\0'; + c++; + + if (virStrToLong_ui(oldACL, NULL, 10, &uid) < 0 || + virStrToLong_ui(c, NULL, 8, &perms) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Malformed %s attribute: %s"), + SECURITY_DAC_XATTR_OLD_ACL, oldACL); + goto cleanup; + } + + if ((perms && virFileSetACL(path, uid, perms) < 0) || + (!perms && virFileRemoveACL(path, uid) < 0)) + goto cleanup; + + virFileRemoveAttr(path, SECURITY_DAC_XATTR_OLD_ACL); + ret = 0; +cleanup: + VIR_FREE(oldACL); + return ret; +} + +static int +virSecurityDACRememberLabel(const char *path) +{ + int ret = -1; + struct stat sb; + char *oldOwner = NULL; + + VIR_DEBUG("path=%s", path); + + if (stat(path, &sb) < 0) { + virReportSystemError(errno, _("Unable to stat '%s'"), path); + return ret; + } + + if (virAsprintf(&oldOwner, "+%u:+%u", + (unsigned) sb.st_uid, (unsigned) sb.st_gid) < 0) { + virReportOOMError(); + return ret; + } + + if (virFileSetAttr(path, SECURITY_DAC_XATTR_OLD_OWNER, oldOwner) < 0) + goto cleanup; + + ret = 0; +cleanup: + VIR_FREE(oldOwner); + return ret; +} + +static int +virSecurityDACGetRememberedLabel(const char *path, + uid_t *user, + uid_t *group) +{ + int ret = -1; + char *oldOwner = NULL; + + if (virFileGetAttr(path, SECURITY_DAC_XATTR_OLD_OWNER, &oldOwner) < 0 || + !oldOwner) + return ret; + + if (parseIds(oldOwner, user, group) < 0) + goto cleanup; + + virFileRemoveAttr(path, SECURITY_DAC_XATTR_OLD_OWNER); + ret = 0; +cleanup: + VIR_FREE(oldOwner); + return ret; +} static virSecurityDriverStatus virSecurityDACProbe(const char *virtDriver ATTRIBUTE_UNUSED) @@ -265,11 +459,8 @@ static const char * virSecurityDACGetDOI(virSecurityManagerPtr mgr ATTRIBUTE_UNU } static int -virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid) +virSecurityDACChown(const char *path, uid_t uid, gid_t gid) { - VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", - path, (long) uid, (long) gid); - if (chown(path, uid, gid) < 0) { struct stat sb; int chown_errno = errno; @@ -306,29 +497,104 @@ virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid) } static int +virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid) +{ + int refCount = 0; + bool xattrSupported = true; + virErrorPtr err; + + VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", + path, (long) uid, (long) gid); + + if (virSecurityDACGetXATTRRefcount(path, &refCount) < 0) { + err = virGetLastError(); + if (!err || err->code != VIR_ERR_SYSTEM_ERROR || + (err->int1 != ENOSYS && err->int1 != ENOTSUP)) + return -1; + virResetLastError(); + xattrSupported = false; + } + + if (refCount || virSecurityDACSetACL(path, uid) == 0) { + if (xattrSupported && + virSecurityDACSetXATTRRefcount(path, refCount + 1) < 0) { + /* Clear out oldACL XATTR */ + return -1; + } + return 0; + } + + /* Setting ACL failed. If the cause is libvirt was build without ACL + * support, or filesystem does not support ACLs fall back to chown */ + err = virGetLastError(); + if (!err || err->code != VIR_ERR_SYSTEM_ERROR || + (err->int1 != ENOSYS && err->int1 != ENOTSUP)) + return -1; + virResetLastError(); + + VIR_DEBUG("Falling back to chown"); + if (xattrSupported && virSecurityDACRememberLabel(path) < 0) + return -1; + + if (virSecurityDACChown(path, uid, gid) < 0 || + (xattrSupported && + virSecurityDACSetXATTRRefcount(path, refCount + 1) < 0)) { + /* XXX Clear our oldOwner XATTR */ + return -1; + } + return 0; +} + +static int virSecurityDACRestoreSecurityFileLabel(const char *path) { struct stat buf; - int rc = -1; + int ret = -1; + int refCount; char *newpath = NULL; + uid_t uid = 0; + gid_t gid = 0; + bool xattrSupported = true; VIR_INFO("Restoring DAC user and group on '%s'", path); if (virFileResolveLink(path, &newpath) < 0) { virReportSystemError(errno, _("cannot resolve symlink %s"), path); - goto err; + return ret; } if (stat(newpath, &buf) != 0) - goto err; + goto cleanup; + + if (virSecurityDACGetXATTRRefcount(newpath, &refCount) < 0) { + if (errno != ENOSYS && errno != ENOTSUP) + return ret; + xattrSupported = false; + } + + /* Backward compatibility. If domain was started with older libvirt, + * it doesn't have any XATTR set, which means refCount == 0 */ + if (refCount) + refCount--; + + if (refCount || virSecurityDACRestoreACL(newpath) == 0) { + if (xattrSupported) + ret = virSecurityDACSetXATTRRefcount(newpath, refCount); + else + ret = 0; + goto cleanup; + } + + if (virSecurityDACGetRememberedLabel(newpath, &uid, &gid) == 0) + virSecurityDACSetXATTRRefcount(newpath, refCount); - /* XXX record previous ownership */ - rc = virSecurityDACSetOwnership(newpath, 0, 0); + VIR_DEBUG("Falling back to chown"); + ret = virSecurityDACChown(newpath, uid, gid); -err: +cleanup: VIR_FREE(newpath); - return rc; + return ret; } @@ -384,24 +650,9 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); - if (!priv->dynamicOwnership) - return 0; - - if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) - 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 (disk->readonly || disk->shared) - return 0; - - if (!disk->src) + if (!priv->dynamicOwnership || + disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK || + !disk->src) return 0; /* If we have a shared FS & doing migrated, we must not -- 1.8.1.5

On Thu, Mar 21, 2013 at 05:50:49PM +0100, Michal Privoznik wrote:
On filesystems supporting ACLs we don't need to do a chown but we can just set ACLs to gain access for qemu. However, since we are setting these on too low level, where we don't know if disk is just a read only or read write, we set read write access unconditionally.
From implementation POV, a reference counter is introduced, so ACL is restored only on the last restore attempt in order to not cut off other domains. And since a file may had an ACL for a user already set, we need to keep this as well. Both these, the reference counter and original ACL are stored as extended attributes named trusted.libvirt.dac.refCount and trusted.libvirt.dac.oldACL respectively.
However, some filesystems doesn't support ACLs, XATTRs, or both. So the code is made to favour ACLs among with tracking the reference count. If this fails, we fall back to chown() with best effort to remember the original owner of file.
I'm reminded that we need to do the same thing for the SELinux driver. ie we should record the original selinux label in an xattr and apply it on restore, instead of guessing the default label on restore. No need to block this though - it can be done as a separate patch.
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 35b90da..989dc50 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -25,6 +25,7 @@
#include "security_dac.h" #include "virerror.h" +#include "virfile.h" #include "virutil.h" #include "viralloc.h" #include "virlog.h" @@ -34,6 +35,9 @@
#define VIR_FROM_THIS VIR_FROM_SECURITY #define SECURITY_DAC_NAME "dac" +#define SECURITY_DAC_XATTR_OLD_ACL "trusted.libvirt.dac.oldACL" +#define SECURITY_DAC_XATTR_OLD_OWNER "trusted.libvirt.dac.oldOwner" +#define SECURITY_DAC_XATTR_REFCOUNT "trusted.libvirt.dac.refCount"
typedef struct _virSecurityDACData virSecurityDACData; typedef virSecurityDACData *virSecurityDACDataPtr; @@ -234,6 +238,196 @@ int virSecurityDACGetImageIds(virDomainDefPtr def, virSecurityDACDataPtr priv, return 0; }
+static int +virSecurityDACGetXATTRRefcount(const char *path, + int *refCount) +{ + int ret = -1; + char *refCountStr; + + if (virFileGetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, &refCountStr) < 0) + return ret; + + VIR_DEBUG("path=%s refCountStr=%s", path, NULLSTR(refCountStr)); + + if (!refCountStr) { + *refCount = 0; + return 0; + } + + if (virStrToLong_i(refCountStr, NULL, 10, refCount) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Malformed %s attribute: %s"), + SECURITY_DAC_XATTR_REFCOUNT, + refCountStr); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(refCountStr); + return ret; +} + +static int +virSecurityDACSetXATTRRefcount(const char *path, + int refCount) +{ + int ret = -1; + char *refCountStr; + + VIR_DEBUG("path=%s refCount=%d", path, refCount); + + if (refCount == 0) { + virFileRemoveAttr(path, SECURITY_DAC_XATTR_REFCOUNT); + return 0; + } + + if (virAsprintf(&refCountStr, "%u", refCount) < 0) { + virReportOOMError(); + return ret; + } + + if (virFileSetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, refCountStr) < 0) + goto cleanup; + + ret = 0; +cleanup: + VIR_FREE(refCountStr); + return ret; +} + +static int +virSecurityDACSetACL(const char *path, + uid_t uid) +{ + int ret = -1; + char *oldACL = NULL; + mode_t perms; + + VIR_DEBUG("path=%s uid=%u", path, uid); + + if (virFileGetACL(path, uid, &perms) < 0) { + /* error getting ACL entry for @uid */ + goto cleanup; + } + + if (virAsprintf(&oldACL, "%u:0%o", uid, perms) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (virFileSetAttr(path, SECURITY_DAC_XATTR_OLD_ACL, oldACL) < 0) + goto cleanup; + + if (virFileSetACL(path, uid, S_IRUSR | S_IWUSR) < 0) + goto cleanup;
Hmm, one thing we didn't do in the past was to correctly honour the 'readonly' attribute. Now we are using ACLs we should do that. ie not set S_IWUSR if def->readonly is true.
+ + ret = 0; +cleanup: + if (ret < 0) + virFileRemoveAttr(path, SECURITY_DAC_XATTR_OLD_ACL); + VIR_FREE(oldACL); + return ret; +} + +static int +virSecurityDACRestoreACL(const char *path) +{ + int ret = -1; + char *oldACL = NULL, *c; + uid_t uid; + mode_t perms; + + VIR_DEBUG("path=%s", path); + + if (virFileGetAttr(path, SECURITY_DAC_XATTR_OLD_ACL, &oldACL) < 0) + return ret; + + if (!oldACL) { + VIR_WARN("Attribute %s is missing", SECURITY_DAC_XATTR_OLD_ACL); + return ret; + } + + if (!(c = strchr(oldACL, ':'))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Malformed %s attribute: %s"), + SECURITY_DAC_XATTR_OLD_ACL, oldACL); + goto cleanup; + } + + *c = '\0'; + c++; + + if (virStrToLong_ui(oldACL, NULL, 10, &uid) < 0 || + virStrToLong_ui(c, NULL, 8, &perms) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Malformed %s attribute: %s"), + SECURITY_DAC_XATTR_OLD_ACL, oldACL); + goto cleanup; + } + + if ((perms && virFileSetACL(path, uid, perms) < 0) || + (!perms && virFileRemoveACL(path, uid) < 0)) + goto cleanup; + + virFileRemoveAttr(path, SECURITY_DAC_XATTR_OLD_ACL); + ret = 0; +cleanup: + VIR_FREE(oldACL); + return ret; +} + +static int +virSecurityDACRememberLabel(const char *path) +{ + int ret = -1; + struct stat sb; + char *oldOwner = NULL; + + VIR_DEBUG("path=%s", path); + + if (stat(path, &sb) < 0) { + virReportSystemError(errno, _("Unable to stat '%s'"), path); + return ret; + } + + if (virAsprintf(&oldOwner, "+%u:+%u", + (unsigned) sb.st_uid, (unsigned) sb.st_gid) < 0) { + virReportOOMError(); + return ret; + } + + if (virFileSetAttr(path, SECURITY_DAC_XATTR_OLD_OWNER, oldOwner) < 0) + goto cleanup; + + ret = 0; +cleanup: + VIR_FREE(oldOwner); + return ret; +} + +static int +virSecurityDACGetRememberedLabel(const char *path, + uid_t *user, + uid_t *group) +{ + int ret = -1; + char *oldOwner = NULL; + + if (virFileGetAttr(path, SECURITY_DAC_XATTR_OLD_OWNER, &oldOwner) < 0 || + !oldOwner) + return ret; + + if (parseIds(oldOwner, user, group) < 0) + goto cleanup; + + virFileRemoveAttr(path, SECURITY_DAC_XATTR_OLD_OWNER); + ret = 0; +cleanup: + VIR_FREE(oldOwner); + return ret; +}
static virSecurityDriverStatus virSecurityDACProbe(const char *virtDriver ATTRIBUTE_UNUSED) @@ -265,11 +459,8 @@ static const char * virSecurityDACGetDOI(virSecurityManagerPtr mgr ATTRIBUTE_UNU }
static int -virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid) +virSecurityDACChown(const char *path, uid_t uid, gid_t gid) { - VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", - path, (long) uid, (long) gid); - if (chown(path, uid, gid) < 0) { struct stat sb; int chown_errno = errno; @@ -306,29 +497,104 @@ virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid) }
static int +virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid) +{ + int refCount = 0; + bool xattrSupported = true; + virErrorPtr err; + + VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", + path, (long) uid, (long) gid); + + if (virSecurityDACGetXATTRRefcount(path, &refCount) < 0) { + err = virGetLastError(); + if (!err || err->code != VIR_ERR_SYSTEM_ERROR || + (err->int1 != ENOSYS && err->int1 != ENOTSUP)) + return -1; + virResetLastError(); + xattrSupported = false; + } + + if (refCount || virSecurityDACSetACL(path, uid) == 0) {
Just checking refCount isn't sufficient - we'll need to validate that the ACL matches what we expect. ie if the existing ACL is allowing read+write, but this VM is configured read-only we must reject it, since the ACLs are incompatible.
+ if (xattrSupported && + virSecurityDACSetXATTRRefcount(path, refCount + 1) < 0) { + /* Clear out oldACL XATTR */ + return -1; + } + return 0; + }
Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Thu, Mar 21, 2013 at 05:50:49PM +0100, Michal Privoznik wrote:
#define VIR_FROM_THIS VIR_FROM_SECURITY #define SECURITY_DAC_NAME "dac" +#define SECURITY_DAC_XATTR_OLD_ACL "trusted.libvirt.dac.oldACL" +#define SECURITY_DAC_XATTR_OLD_OWNER "trusted.libvirt.dac.oldOwner" +#define SECURITY_DAC_XATTR_REFCOUNT "trusted.libvirt.dac.refCount"
IMHO we don't need the 'trusted.' prefix on these. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 28.03.2013 10:46, Daniel P. Berrange wrote:
On Thu, Mar 21, 2013 at 05:50:49PM +0100, Michal Privoznik wrote:
#define VIR_FROM_THIS VIR_FROM_SECURITY #define SECURITY_DAC_NAME "dac" +#define SECURITY_DAC_XATTR_OLD_ACL "trusted.libvirt.dac.oldACL" +#define SECURITY_DAC_XATTR_OLD_OWNER "trusted.libvirt.dac.oldOwner" +#define SECURITY_DAC_XATTR_REFCOUNT "trusted.libvirt.dac.refCount"
IMHO we don't need the 'trusted.' prefix on these.
Daniel
An XATTR has to have a prefix. We can choose from several prefixes. attr(5) says: Currently the security, system, trusted, and user extended attribute classes are defined as described below. Additional classes may be added in the future. security - should be used by kernel security modules, such as Security Enhanced Linux. As long as libvirt doesn't provide kernel module, we should not be polluting this prefix. system - used by the kernel to store system objects such as Access Control Lists and Capabilities. Again, we are not kernel. trusted - visible and accessible only to processes that have the CAP_SYS_ADMIN capability (the super user usually has this capability). Attributes in this class are used to implement mechanisms in user space (i.e., outside the kernel) which keep information in extended attributes to which ordinary processes should not have access. Note, that the three above can be touched only by root (or CAP_SYS_ADMIN'ed process). user - may be assigned to files and directories for storing arbitrary additional information such as the mime type, character set or encoding of a file. The user. can be manipulated by ordinary user. My rationale for not allowing ordinary user to play with our XATTRs is to prevent them chowning to arbitrary user. Michal

On Thu, Mar 28, 2013 at 11:38:04AM +0100, Michal Privoznik wrote:
On 28.03.2013 10:46, Daniel P. Berrange wrote:
On Thu, Mar 21, 2013 at 05:50:49PM +0100, Michal Privoznik wrote:
#define VIR_FROM_THIS VIR_FROM_SECURITY #define SECURITY_DAC_NAME "dac" +#define SECURITY_DAC_XATTR_OLD_ACL "trusted.libvirt.dac.oldACL" +#define SECURITY_DAC_XATTR_OLD_OWNER "trusted.libvirt.dac.oldOwner" +#define SECURITY_DAC_XATTR_REFCOUNT "trusted.libvirt.dac.refCount"
IMHO we don't need the 'trusted.' prefix on these.
Daniel
An XATTR has to have a prefix. We can choose from several prefixes. attr(5) says:
Currently the security, system, trusted, and user extended attribute classes are defined as described below. Additional classes may be added in the future.
security - should be used by kernel security modules, such as Security Enhanced Linux. As long as libvirt doesn't provide kernel module, we should not be polluting this prefix.
system - used by the kernel to store system objects such as Access Control Lists and Capabilities. Again, we are not kernel.
trusted - visible and accessible only to processes that have the CAP_SYS_ADMIN capability (the super user usually has this capability). Attributes in this class are used to implement mechanisms in user space (i.e., outside the kernel) which keep information in extended attributes to which ordinary processes should not have access.
Note, that the three above can be touched only by root (or CAP_SYS_ADMIN'ed process).
user - may be assigned to files and directories for storing arbitrary additional information such as the mime type, character set or encoding of a file.
The user. can be manipulated by ordinary user.
My rationale for not allowing ordinary user to play with our XATTRs is to prevent them chowning to arbitrary user.
Ok, that makes more sense now. I wonder how portable this list of prefixes is though - does anyone know if *BSD use the same conventions ? Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 28.03.2013 12:12, Daniel P. Berrange wrote:
On Thu, Mar 28, 2013 at 11:38:04AM +0100, Michal Privoznik wrote:
On 28.03.2013 10:46, Daniel P. Berrange wrote:
On Thu, Mar 21, 2013 at 05:50:49PM +0100, Michal Privoznik wrote:
#define VIR_FROM_THIS VIR_FROM_SECURITY #define SECURITY_DAC_NAME "dac" +#define SECURITY_DAC_XATTR_OLD_ACL "trusted.libvirt.dac.oldACL" +#define SECURITY_DAC_XATTR_OLD_OWNER "trusted.libvirt.dac.oldOwner" +#define SECURITY_DAC_XATTR_REFCOUNT "trusted.libvirt.dac.refCount"
IMHO we don't need the 'trusted.' prefix on these.
Daniel
An XATTR has to have a prefix. We can choose from several prefixes. attr(5) says:
Currently the security, system, trusted, and user extended attribute classes are defined as described below. Additional classes may be added in the future.
security - should be used by kernel security modules, such as Security Enhanced Linux. As long as libvirt doesn't provide kernel module, we should not be polluting this prefix.
system - used by the kernel to store system objects such as Access Control Lists and Capabilities. Again, we are not kernel.
trusted - visible and accessible only to processes that have the CAP_SYS_ADMIN capability (the super user usually has this capability). Attributes in this class are used to implement mechanisms in user space (i.e., outside the kernel) which keep information in extended attributes to which ordinary processes should not have access.
Note, that the three above can be touched only by root (or CAP_SYS_ADMIN'ed process).
user - may be assigned to files and directories for storing arbitrary additional information such as the mime type, character set or encoding of a file.
The user. can be manipulated by ordinary user.
My rationale for not allowing ordinary user to play with our XATTRs is to prevent them chowning to arbitrary user.
Ok, that makes more sense now. I wonder how portable this list of prefixes is though - does anyone know if *BSD use the same conventions ?
Daniel
Aah. On BSD they support just 'system' and 'user': http://svnweb.freebsd.org/base/head/sys/sys/extattr.h?revision=184413&view=markup Does it mean we should move from 'trusted' to 'system'? Or is conditional prefix ('trusted' on linux, 'system' on BSD) sufficient? Michal

On Thu, Mar 28, 2013 at 12:47:25PM +0100, Michal Privoznik wrote:
On 28.03.2013 12:12, Daniel P. Berrange wrote:
On Thu, Mar 28, 2013 at 11:38:04AM +0100, Michal Privoznik wrote:
On 28.03.2013 10:46, Daniel P. Berrange wrote:
On Thu, Mar 21, 2013 at 05:50:49PM +0100, Michal Privoznik wrote:
#define VIR_FROM_THIS VIR_FROM_SECURITY #define SECURITY_DAC_NAME "dac" +#define SECURITY_DAC_XATTR_OLD_ACL "trusted.libvirt.dac.oldACL" +#define SECURITY_DAC_XATTR_OLD_OWNER "trusted.libvirt.dac.oldOwner" +#define SECURITY_DAC_XATTR_REFCOUNT "trusted.libvirt.dac.refCount"
IMHO we don't need the 'trusted.' prefix on these.
Daniel
An XATTR has to have a prefix. We can choose from several prefixes. attr(5) says:
Currently the security, system, trusted, and user extended attribute classes are defined as described below. Additional classes may be added in the future.
security - should be used by kernel security modules, such as Security Enhanced Linux. As long as libvirt doesn't provide kernel module, we should not be polluting this prefix.
system - used by the kernel to store system objects such as Access Control Lists and Capabilities. Again, we are not kernel.
trusted - visible and accessible only to processes that have the CAP_SYS_ADMIN capability (the super user usually has this capability). Attributes in this class are used to implement mechanisms in user space (i.e., outside the kernel) which keep information in extended attributes to which ordinary processes should not have access.
Note, that the three above can be touched only by root (or CAP_SYS_ADMIN'ed process).
user - may be assigned to files and directories for storing arbitrary additional information such as the mime type, character set or encoding of a file.
The user. can be manipulated by ordinary user.
My rationale for not allowing ordinary user to play with our XATTRs is to prevent them chowning to arbitrary user.
Ok, that makes more sense now. I wonder how portable this list of prefixes is though - does anyone know if *BSD use the same conventions ?
Daniel
Aah. On BSD they support just 'system' and 'user':
http://svnweb.freebsd.org/base/head/sys/sys/extattr.h?revision=184413&view=markup
Does it mean we should move from 'trusted' to 'system'? Or is conditional prefix ('trusted' on linux, 'system' on BSD) sufficient?
You're not able to use 'system.' from userspace. # setfattr -n user.eek -v bar foo # setfattr -n system.eek -v bar foo setfattr: foo: Operation not supported So 'user.' is the only option here for BSD. If we consider the (admittedly unlikely) possibility of an NFS server access by 2 libvirt clients one running Linux and one running BSD, we want compatibility. So this says to me that we should use 'user.' as the prefer everywhere. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 28.03.2013 12:52, Daniel P. Berrange wrote:
On Thu, Mar 28, 2013 at 12:47:25PM +0100, Michal Privoznik wrote:
On 28.03.2013 12:12, Daniel P. Berrange wrote:
On Thu, Mar 28, 2013 at 11:38:04AM +0100, Michal Privoznik wrote:
On 28.03.2013 10:46, Daniel P. Berrange wrote:
On Thu, Mar 21, 2013 at 05:50:49PM +0100, Michal Privoznik wrote:
#define VIR_FROM_THIS VIR_FROM_SECURITY #define SECURITY_DAC_NAME "dac" +#define SECURITY_DAC_XATTR_OLD_ACL "trusted.libvirt.dac.oldACL" +#define SECURITY_DAC_XATTR_OLD_OWNER "trusted.libvirt.dac.oldOwner" +#define SECURITY_DAC_XATTR_REFCOUNT "trusted.libvirt.dac.refCount"
IMHO we don't need the 'trusted.' prefix on these.
Daniel
An XATTR has to have a prefix. We can choose from several prefixes. attr(5) says:
Currently the security, system, trusted, and user extended attribute classes are defined as described below. Additional classes may be added in the future.
security - should be used by kernel security modules, such as Security Enhanced Linux. As long as libvirt doesn't provide kernel module, we should not be polluting this prefix.
system - used by the kernel to store system objects such as Access Control Lists and Capabilities. Again, we are not kernel.
trusted - visible and accessible only to processes that have the CAP_SYS_ADMIN capability (the super user usually has this capability). Attributes in this class are used to implement mechanisms in user space (i.e., outside the kernel) which keep information in extended attributes to which ordinary processes should not have access.
Note, that the three above can be touched only by root (or CAP_SYS_ADMIN'ed process).
user - may be assigned to files and directories for storing arbitrary additional information such as the mime type, character set or encoding of a file.
The user. can be manipulated by ordinary user.
My rationale for not allowing ordinary user to play with our XATTRs is to prevent them chowning to arbitrary user.
Ok, that makes more sense now. I wonder how portable this list of prefixes is though - does anyone know if *BSD use the same conventions ?
Daniel
Aah. On BSD they support just 'system' and 'user':
http://svnweb.freebsd.org/base/head/sys/sys/extattr.h?revision=184413&view=markup
Does it mean we should move from 'trusted' to 'system'? Or is conditional prefix ('trusted' on linux, 'system' on BSD) sufficient?
You're not able to use 'system.' from userspace.
# setfattr -n user.eek -v bar foo # setfattr -n system.eek -v bar foo setfattr: foo: Operation not supported
So 'user.' is the only option here for BSD. If we consider the (admittedly unlikely) possibility of an NFS server access by 2 libvirt clients one running Linux and one running BSD, we want compatibility. So this says to me that we should use 'user.' as the prefer everywhere.
I don't think we should use 'user.' at all. It smells of CVE as soon as users find out they can effectively let libvirt chown a file for them in case ACLs are disabled. So the other solution is to not enable this on BSD (and other systems) unless they learn proper XATTRs. Michal
Regards, Daniel

On Thu, Mar 28, 2013 at 01:06:12PM +0100, Michal Privoznik wrote:
On 28.03.2013 12:52, Daniel P. Berrange wrote:
On Thu, Mar 28, 2013 at 12:47:25PM +0100, Michal Privoznik wrote:
On 28.03.2013 12:12, Daniel P. Berrange wrote:
On Thu, Mar 28, 2013 at 11:38:04AM +0100, Michal Privoznik wrote:
On 28.03.2013 10:46, Daniel P. Berrange wrote:
On Thu, Mar 21, 2013 at 05:50:49PM +0100, Michal Privoznik wrote: > #define VIR_FROM_THIS VIR_FROM_SECURITY > #define SECURITY_DAC_NAME "dac" > +#define SECURITY_DAC_XATTR_OLD_ACL "trusted.libvirt.dac.oldACL" > +#define SECURITY_DAC_XATTR_OLD_OWNER "trusted.libvirt.dac.oldOwner" > +#define SECURITY_DAC_XATTR_REFCOUNT "trusted.libvirt.dac.refCount"
IMHO we don't need the 'trusted.' prefix on these.
Daniel
An XATTR has to have a prefix. We can choose from several prefixes. attr(5) says:
Currently the security, system, trusted, and user extended attribute classes are defined as described below. Additional classes may be added in the future.
security - should be used by kernel security modules, such as Security Enhanced Linux. As long as libvirt doesn't provide kernel module, we should not be polluting this prefix.
system - used by the kernel to store system objects such as Access Control Lists and Capabilities. Again, we are not kernel.
trusted - visible and accessible only to processes that have the CAP_SYS_ADMIN capability (the super user usually has this capability). Attributes in this class are used to implement mechanisms in user space (i.e., outside the kernel) which keep information in extended attributes to which ordinary processes should not have access.
Note, that the three above can be touched only by root (or CAP_SYS_ADMIN'ed process).
user - may be assigned to files and directories for storing arbitrary additional information such as the mime type, character set or encoding of a file.
The user. can be manipulated by ordinary user.
My rationale for not allowing ordinary user to play with our XATTRs is to prevent them chowning to arbitrary user.
Ok, that makes more sense now. I wonder how portable this list of prefixes is though - does anyone know if *BSD use the same conventions ?
Daniel
Aah. On BSD they support just 'system' and 'user':
http://svnweb.freebsd.org/base/head/sys/sys/extattr.h?revision=184413&view=markup
Does it mean we should move from 'trusted' to 'system'? Or is conditional prefix ('trusted' on linux, 'system' on BSD) sufficient?
You're not able to use 'system.' from userspace.
# setfattr -n user.eek -v bar foo # setfattr -n system.eek -v bar foo setfattr: foo: Operation not supported
So 'user.' is the only option here for BSD. If we consider the (admittedly unlikely) possibility of an NFS server access by 2 libvirt clients one running Linux and one running BSD, we want compatibility. So this says to me that we should use 'user.' as the prefer everywhere.
I don't think we should use 'user.' at all. It smells of CVE as soon as users find out they can effectively let libvirt chown a file for them in case ACLs are disabled. So the other solution is to not enable this on BSD (and other systems) unless they learn proper XATTRs.
It doesn't let any user do this. They have to have write access to the files in question, which they will not ordinarily have. That said we're actually giving QEMU such access, which is a problem. I don't like the fact that we'd be forced into what is effectively a Linux only solution for xattrs here. It makes me wonder if our entire approach here is basically wrong. We decided on using xattrs, instead of an in-memory record, because we want the data to be accessible to multiple libvirtd daemons on different hosts. This does not imply we actually need to store the xattrs on the files themselves. Perhaps we should have been creating some parallel files to record the original ownership, which are permanently root owned. eg For a file /var/lib/libvirt/images/foo.img add a /var/lib/libvirt/images/.libvirt.dac.foo.img file to record the original information. Or as with the lock manager, just use a single directory like /var/lib/libvirt/dac/ and create files in there based on the SHA256SUM of the filename, and declare that you must share that directory between hosts ? Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 03/28/2013 06:21 AM, Daniel P. Berrange wrote:
We decided on using xattrs, instead of an in-memory record, because we want the data to be accessible to multiple libvirtd daemons on different hosts. This does not imply we actually need to store the xattrs on the files themselves. Perhaps we should have been creating some parallel files to record the original ownership, which are permanently root owned.
eg For a file /var/lib/libvirt/images/foo.img add a /var/lib/libvirt/images/.libvirt.dac.foo.img file to record the original information.
Or as with the lock manager, just use a single directory like /var/lib/libvirt/dac/ and create files in there based on the SHA256SUM of the filename, and declare that you must share that directory between hosts ?
Out of the box, libvirt is not set up for sharing unless you mount /var/lib/libvirt/images on shared storage or add your own storage pool pointing to shared storage. Since setting up shared storage pools is already an admin action, I would be okay with also requiring an admin action to set up a shared directory that tracks ownership information across a shared pool. It might also be nice to make it easy to associate a shared subdirectory for metainformation inside any storage pool based on top of a shared directory (although that approach doesn't really scale to other storage pools such as iscsi or LVM where the pool isn't really exposing a file system to hold a subdirectory). For that matter, there have already been requests to allow 'virsh managedsave' dump memory into a storage pool, rather than into /etc, since saved state can occupy a lot more space than the / partition is prepared to handle. Also, we have the question of coming up with a default name for saved state files in external snapshots. Both of these problems would also benefit from the ability to designate where libvirt should stick metadata associated with a storage pool, but where a fallback to a default within /var/lib/libvirt are still okay for out-of-the-box installation on a single machine. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange
-
Eric Blake
-
Michal Privoznik