[libvirt] [PATCH 0/3] Keep original file label
This is just a resurrection of my previous patchset. As of atomicity problem, I just realized there is none. The qemuProcessHook (which is responsible for locking the files) is called prior virSecurityManagerSetAllLabel (responsible for chown()-ing). Anyway, even if there's still one and it's pre-existing, it shouldn't block this set, should it? 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 | 297 ++++++++++++++++++++++++++++++++++++++----- src/util/virfile.c | 301 ++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 28 +++++ 8 files changed, 617 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 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 3 ++ src/util/virfile.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 13 ++++++ 3 files changed, 127 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d4006ae..c1a51b2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1369,6 +1369,7 @@ virFileExists; virFileFclose; virFileFdopen; virFileFindMountPoint; +virFileGetAttr; virFileHasSuffix; virFileIsAbsPath; virFileIsDir; @@ -1386,10 +1387,12 @@ virFileOpenTty; virFilePrintf; virFileReadAll; virFileReadLimFD; +virFileRemoveAttr; virFileResolveAllLinks; virFileResolveLink; virFileRewrite; virFileSanitizePath; +virFileSetAttr; virFileSkipRoot; virFileStripSuffix; virFileTouch; diff --git a/src/util/virfile.c b/src/util/virfile.c index 2b07ac9..064ea2f 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -48,6 +48,10 @@ # include <sys/ioctl.h> #endif +#ifdef WITH_ATTR +# include <attr/xattr.h> +#endif + #include "configmake.h" #include "viralloc.h" #include "vircommand.h" @@ -2362,3 +2366,110 @@ cleanup: return ret; } + +#ifdef WITH_ATTR +int +virFileSetAttr(const char *file, + const char *name, + const char *value) +{ + int ret = -1; + size_t valueSize = strlen(value); + + if (setxattr(file, name, value, valueSize, 0) < 0) { + virReportSystemError(errno, + _("Unable to set extended attribute '%s':'%s' on '%s'"), + name, value, file); + goto cleanup; + } + + ret = 0; +cleanup: + return ret; +} + +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 maps ENOATTR to ENODATA */ + if (errno == ENOATTR || errno == ENODATA) { + *value = NULL; + return 0; + } + virReportSystemError(errno, + _("Unable to get extended attribute '%s' on '%s'"), + name, file); + return ret; + } + + if (VIR_ALLOC_N(buf, valueSize) < 0) + 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) +{ + int ret = -1; + if (removexattr(file, name) < 0) { + virReportSystemError(errno, + _("Unable to remove extended attribute '%s' on '%s'"), + name, file); + goto cleanup; + } + + ret = 0; +cleanup: + return ret; +} + +#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 attributes")); + return -1; +} +#endif /* WITH_ATTR */ diff --git a/src/util/virfile.h b/src/util/virfile.h index 72d35ce..08c59b0 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -232,4 +232,17 @@ int virBuildPathInternal(char **path, ...) ATTRIBUTE_SENTINEL; int virFilePrintf(FILE *fp, const char *msg, ...) ATTRIBUTE_FMT_PRINTF(2, 3); +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_FILE_H */ -- 1.8.1.5
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. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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 | 15 ++++ 7 files changed, 222 insertions(+), 2 deletions(-) create mode 100644 m4/virt-acl.m4 diff --git a/configure.ac b/configure.ac index 94a2e19..7d4affd 100644 --- a/configure.ac +++ b/configure.ac @@ -162,6 +162,7 @@ LIBVIRT_COMPILE_PIE LIBVIRT_LINKER_RELRO LIBVIRT_LINKER_NO_INDIRECT +LIBVIRT_CHECK_ACL LIBVIRT_CHECK_APPARMOR LIBVIRT_CHECK_ATTR LIBVIRT_CHECK_AUDIT @@ -2589,6 +2590,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 85881ae..50f40d8 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -443,6 +443,7 @@ BuildRequires: libgcrypt-devel %endif BuildRequires: gnutls-devel BuildRequires: libattr-devel +BuildRequires: lobacl-devel %if %{with_libvirtd} # For pool-build probing for existing pools BuildRequires: libblkid-devel >= 2.17 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 d8b943d..6aceee3 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -901,12 +901,12 @@ 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) \ -I$(top_srcdir)/src/conf 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 c1a51b2..af3bdb6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1369,6 +1369,7 @@ virFileExists; virFileFclose; virFileFdopen; virFileFindMountPoint; +virFileGetACL; virFileGetAttr; virFileHasSuffix; virFileIsAbsPath; @@ -1387,11 +1388,13 @@ virFileOpenTty; virFilePrintf; virFileReadAll; virFileReadLimFD; +virFileRemoveACL; virFileRemoveAttr; virFileResolveAllLinks; virFileResolveLink; virFileRewrite; virFileSanitizePath; +virFileSetACL; virFileSetAttr; virFileSkipRoot; virFileStripSuffix; diff --git a/src/util/virfile.c b/src/util/virfile.c index 064ea2f..fe055d1 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -52,6 +52,10 @@ # include <attr/xattr.h> #endif +#ifdef WITH_ACL +# include <acl/libacl.h> +#endif + #include "configmake.h" #include "viralloc.h" #include "vircommand.h" @@ -2473,3 +2477,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 08c59b0..fcdf9fa 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -245,4 +245,19 @@ int virFileGetAttr(const char *file, 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_FILE_H */ -- 1.8.1.5
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. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 297 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 268 insertions(+), 29 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 8cbb083..913b68e 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -37,6 +37,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; @@ -203,6 +206,185 @@ virSecurityDACGetImageIds(virDomainDefPtr def, virSecurityDACDataPtr priv, return 0; } +static int +virSecurityDACGetXATTRRefcount(const char *path, + int *refCount) +{ + int ret = -1; + char *refCountStr = NULL; + + 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) { + /* refCount == 0 means we can remove the XATTR */ + return virFileRemoveAttr(path, SECURITY_DAC_XATTR_REFCOUNT); + } + + if (virAsprintf(&refCountStr, "%d", refCount) < 0 || + 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; + bool remove = false; + + 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 || + virFileSetAttr(path, SECURITY_DAC_XATTR_OLD_ACL, oldACL) < 0) + goto cleanup; + remove = true; + + if (virFileSetACL(path, uid, S_IRUSR | S_IWUSR) < 0) + goto cleanup; + + ret = 0; +cleanup: + if (ret < 0 && remove) + virFileRemoveAttr(path, SECURITY_DAC_XATTR_OLD_ACL); + VIR_FREE(oldACL); + return ret; +} + +static int +virSecurityDACRestoreACL(const char *path) +{ + int ret = -1; + char *c, *oldACL = NULL; + uid_t uid; + mode_t perms = 0; + + 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 || + 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, + gid_t *group) +{ + int ret = -1; + char *oldOwner; + + if (virFileGetAttr(path, SECURITY_DAC_XATTR_OLD_OWNER, &oldOwner) < 0 || + !oldOwner) + return ret; + + if (virParseOwnershipIds(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) @@ -253,11 +435,8 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr) } 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; @@ -294,29 +473,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; - /* XXX record previous ownership */ - rc = virSecurityDACSetOwnership(newpath, 0, 0); + 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); -err: + VIR_DEBUG("Falling back to chown"); + ret = virSecurityDACChown(newpath, uid, gid); + +cleanup: VIR_FREE(newpath); - return rc; + return ret; } @@ -372,24 +626,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 Wed, Aug 28, 2013 at 12:27:40PM +0200, 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.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 297 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 268 insertions(+), 29 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 8cbb083..913b68e 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -37,6 +37,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"
Use of "trusted." is non-portable, per the long mail thread last time this was posted[1]. The discussion there was never resolved in any way, so NACK to this patch as is. Daniel [1] https://www.redhat.com/archives/libvir-list/2013-March/msg01610.html -- |: 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 05.09.2013 16:14, Daniel P. Berrange wrote:
On Wed, Aug 28, 2013 at 12:27:40PM +0200, 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.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 297 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 268 insertions(+), 29 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 8cbb083..913b68e 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -37,6 +37,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"
Use of "trusted." is non-portable, per the long mail thread last time this was posted[1]. The discussion there was never resolved in any way, so NACK to this patch as is.
Daniel
[1] https://www.redhat.com/archives/libvir-list/2013-March/msg01610.html
Okay then, consider this squashed in: diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 47af4e8..acc80c4 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -37,9 +37,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" +#define SECURITY_DAC_XATTR_OLD_ACL "user.libvirt.dac.oldACL" +#define SECURITY_DAC_XATTR_OLD_OWNER "user.libvirt.dac.oldOwner" +#define SECURITY_DAC_XATTR_REFCOUNT "user.libvirt.dac.refCount" typedef struct _virSecurityDACData virSecurityDACData; typedef virSecurityDACData *virSecurityDACDataPtr; Michal
On Fri, Sep 06, 2013 at 09:50:25AM +0200, Michal Privoznik wrote:
On 05.09.2013 16:14, Daniel P. Berrange wrote:
On Wed, Aug 28, 2013 at 12:27:40PM +0200, 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.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 297 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 268 insertions(+), 29 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 8cbb083..913b68e 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -37,6 +37,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"
Use of "trusted." is non-portable, per the long mail thread last time this was posted[1]. The discussion there was never resolved in any way, so NACK to this patch as is.
Daniel
[1] https://www.redhat.com/archives/libvir-list/2013-March/msg01610.html
Okay then, consider this squashed in:
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 47af4e8..acc80c4 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -37,9 +37,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" +#define SECURITY_DAC_XATTR_OLD_ACL "user.libvirt.dac.oldACL" +#define SECURITY_DAC_XATTR_OLD_OWNER "user.libvirt.dac.oldOwner" +#define SECURITY_DAC_XATTR_REFCOUNT "user.libvirt.dac.refCount"
In the thread above we determined that this is insecure. We give QEMU write access to the files, so it would be able to change these attributes to cause libvirt todo incorrect thing in its security driver when shutting down. https://www.redhat.com/archives/libvir-list/2013-March/msg01612.html My conclusion was that we can never use xattrs on the disk files themselves for this, in a secure or portable manner. I suggested we could possibly use a parallel file to maintain it, but this is a little harder for block devices. https://www.redhat.com/archives/libvir-list/2013-March/msg01615.html 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.08.2013 12:27, Michal Privoznik wrote:
This is just a resurrection of my previous patchset. As of atomicity problem, I just realized there is none. The qemuProcessHook (which is responsible for locking the files) is called prior virSecurityManagerSetAllLabel (responsible for chown()-ing). Anyway, even if there's still one and it's pre-existing, it shouldn't block this set, should it?
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 | 297 ++++++++++++++++++++++++++++++++++++++----- src/util/virfile.c | 301 ++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 28 +++++ 8 files changed, 617 insertions(+), 31 deletions(-) create mode 100644 m4/virt-acl.m4
Ping?
participants (2)
-
Daniel P. Berrange -
Michal Privoznik