On Mon, Mar 11, 2013 at 05:13:27PM +0100, Michal Privoznik wrote: sys/types.h should never be required for any functionality - headers are self-contained. ACK if that is removed 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 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 | 184 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 11 +++ 7 files changed, 212 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 c1659a4..679c52b 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -763,11 +763,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) + $(DBUS_CFLAGS) $(LDEXP_LIBM) $(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) + $(SECDRIVER_LIBS) $(ACL_LIBS) noinst_LTLIBRARIES += libvirt_conf.la diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f787a12..e9dd7b6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1273,10 +1273,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 aae7101..e868e09 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -42,6 +42,12 @@ # include <sys/types.h> #endif +#ifdef HAVE_LIBACL +# include <acl/libacl.h> +# include <sys/acl.h> +# include <sys/types.h> +#endif + #include "vircommand.h" #include "configmake.h" #include "viralloc.h" @@ -750,3 +756,181 @@ virFileRemoveAttr(const char *file ATTRIBUTE_UNUSED, return -1; } #endif /* HAVE_LIBATTR */ + +#ifdef HAVE_LIBACL +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 (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 /* HAVE_LIBACL */ +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 /* HAVE_LIBACL */ diff --git a/src/util/virfile.h b/src/util/virfile.h index 3b4d672..fe46a7d 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -120,4 +120,15 @@ int virFileGetAttr(const char *file, int virFileRemoveAttr(const char *file, const char *name); +int virFileSetACL(const char *file, + uid_t user, + mode_t perms); + +int virFileGetACL(const char *file, + uid_t user, + mode_t *perms); + +int virFileRemoveACL(const char *file, + uid_t user); + #endif /* __VIR_FILES_H */ -- 1.8.1.5

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. 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.refCount and trusted.oldACL respectively. --- diff to v2: -basically squashed functionality of 2/4 and 4/4 from previous round src/security/security_dac.c | 209 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 182 insertions(+), 27 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 0b274b7..46cc686 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,8 @@ #define VIR_FROM_THIS VIR_FROM_SECURITY #define SECURITY_DAC_NAME "dac" +#define SECURITY_DAC_XATTR_OLD_ACL "trusted.oldACL" +#define SECURITY_DAC_XATTR_REFCOUNT "trusted.refCount" typedef struct _virSecurityDACData virSecurityDACData; typedef virSecurityDACData *virSecurityDACDataPtr; @@ -234,6 +237,144 @@ int virSecurityDACGetImageIds(virDomainDefPtr def, virSecurityDACDataPtr priv, return 0; } +static void +virSecurityDACCleanupLabel(const char *path) +{ + virFileRemoveAttr(path, SECURITY_DAC_XATTR_REFCOUNT); + virFileRemoveAttr(path, SECURITY_DAC_XATTR_OLD_ACL); +} + +static int +virSecurityDACSetACL(const char *path, + uid_t uid) +{ + int ret = -1; + char *refCountStr = NULL; + char *oldACL = NULL; + int refCount = 0; + + if (virFileGetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, &refCountStr) < 0) + return ret; + + if (!refCountStr) { + mode_t perms; + + 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; + } else if (virStrToLong_i(refCountStr, NULL, 10, &refCount) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Malformed %s attribute: %s"), + SECURITY_DAC_XATTR_REFCOUNT, + refCountStr); + goto cleanup; + } + + VIR_FREE(refCountStr); + if (virAsprintf(&refCountStr, "%u", ++refCount) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (virFileSetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, refCountStr) < 0) + goto cleanup; + + if (virFileSetACL(path, uid, S_IRUSR | S_IWUSR) < 0) + goto cleanup; + + ret = 0; +cleanup: + VIR_FREE(oldACL); + VIR_FREE(refCountStr); + return ret; +} + +static int +virSecurityDACRestoreACL(const char *path) +{ + int ret = -1; + char *refCountStr = NULL, *oldACL = NULL, *c; + int refCount = 0; + uid_t uid; + mode_t perms; + + if (virFileGetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, &refCountStr) < 0) + return ret; + + if (!refCountStr) { + /* Backward compatibility. If domain was started with older libvirt, + * it doesn't have any XATTR set so we should not fail here */ + 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; + } + VIR_FREE(refCountStr); + + if (--refCount) { + if (virAsprintf(&refCountStr, "%d", refCount) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (virFileSetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, refCountStr) < 0) + goto cleanup; + } else { + if (virFileGetAttr(path, SECURITY_DAC_XATTR_OLD_ACL, &oldACL) < 0) + goto cleanup; + + if (!oldACL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Attribute %s is missing"), + SECURITY_DAC_XATTR_OLD_ACL); + goto cleanup; + } + + 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; + + virSecurityDACCleanupLabel(path); + } + + ret = 0; +cleanup: + VIR_FREE(oldACL); + VIR_FREE(refCountStr); + return ret; +} static virSecurityDriverStatus virSecurityDACProbe(const char *virtDriver ATTRIBUTE_UNUSED) @@ -265,11 +406,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,6 +444,30 @@ virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid) } static int +virSecurityDACSetOwnership(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 (virSecurityDACSetACL(path, uid) == 0) { + /* ACL set successfully */ + return 0; + } else { + /* Oops, something went wrong. If ACL or XATTR are not + * supported fall back to chown then. */ + virErrorPtr err = virGetLastError(); + if (err && err->code != VIR_ERR_SYSTEM_ERROR && err->int1 != ENOSYS) { + virSecurityDACCleanupLabel(path); + return -1; + } + virSecurityDACCleanupLabel(path); + } + + VIR_DEBUG("Falling back to chown"); + return virSecurityDACChown(path, uid, gid); +} + +static int virSecurityDACRestoreSecurityFileLabel(const char *path) { struct stat buf; @@ -317,16 +479,24 @@ virSecurityDACRestoreSecurityFileLabel(const char *path) if (virFileResolveLink(path, &newpath) < 0) { virReportSystemError(errno, _("cannot resolve symlink %s"), path); - goto err; + goto cleanup; } if (stat(newpath, &buf) != 0) - goto err; + goto cleanup; + + if (virSecurityDACRestoreACL(newpath) == 0) { + /* ACL restored successfully */ + rc = 0; + goto cleanup; + } + + /* Oops, something went wrong. If ACL or XATTR are not + * supported fall back to chown then. */ - /* XXX record previous ownership */ - rc = virSecurityDACSetOwnership(newpath, 0, 0); + rc = virSecurityDACChown(path, 0, 0); -err: +cleanup: VIR_FREE(newpath); return rc; } @@ -384,24 +554,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