[libvirt] [PATCH v4 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. Patch 1/3 has been already ACKed, however, I've changed it slightly. 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 | 302 +++++++++++++++++++++++++++++++++++++++----- src/util/virfile.c | 301 +++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 28 ++++ 8 files changed, 622 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 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 | 108 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 14 ++++++ 3 files changed, 125 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5cad990..5a2cbe8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1252,8 +1252,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..be50e83 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,107 @@ 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) +{ + errno = ENOSYS; + virReportSystemError(errno, "%s", + _("Unable to set extended attributes")); + return -1; +} + +int +virFileGetAttr(const char *file ATTRIBUTE_UNUSED, + const char *name ATTRIBUTE_UNUSED, + char **value ATTRIBUTE_UNUSED) +{ + errno = ENOSYS; + virReportSystemError(errno, "%s", + _("Unable to get extended attributes")); + return -1; +} + +int +virFileRemoveAttr(const char *file ATTRIBUTE_UNUSED, + const char *name ATTRIBUTE_UNUSED) +{ + errno = ENOSYS; + virReportSystemError(errno, "%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 Fri, Mar 15, 2013 at 03:12:01PM +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 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 | 108 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 14 ++++++ 3 files changed, 125 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5cad990..5a2cbe8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1252,8 +1252,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..be50e83 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,107 @@ 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) +{ + errno = ENOSYS; + virReportSystemError(errno, "%s", + _("Unable to set extended attributes")); + return -1; +} + +int +virFileGetAttr(const char *file ATTRIBUTE_UNUSED, + const char *name ATTRIBUTE_UNUSED, + char **value ATTRIBUTE_UNUSED) +{ + errno = ENOSYS; + virReportSystemError(errno, "%s", + _("Unable to get extended attributes")); + return -1; +} + +int +virFileRemoveAttr(const char *file ATTRIBUTE_UNUSED, + const char *name ATTRIBUTE_UNUSED) +{ + errno = ENOSYS;
NACK to this addition. Callers have absolutely no business accessing 'errno' for any function which uses libvirt error reporting - we make no guarnatees that the value will be preserved by any cleanup code in such methods. If callers want to check errno values they should do this: virErrorPtr err = virGetLastError() if (err && err->code == VIR_ERR_SYSTEM_ERROR && err->int1 == ENOSYS) .... 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 :|

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 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 | 193 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 14 ++++ 7 files changed, 224 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 0c0dfb3..0ddc128 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -764,11 +764,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 5a2cbe8..e1ec774 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1252,10 +1252,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 be50e83..7f50328 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" @@ -752,3 +756,192 @@ 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) +{ + errno = ENOSYS; + virReportSystemError(errno, "%s", _("Unable to set ACL")); + return -1; +} + +int +virFileRemoveACL(const char *file ATTRIBUTE_UNUSED, + uid_t user ATTRIBUTE_UNUSED) +{ + errno = ENOSYS; + virReportSystemError(errno, "%s", _("Unable to remove ACL")); + return -1; +} +int +virFileGetACL(const char *file ATTRIBUTE_UNUSED, + uid_t user ATTRIBUTE_UNUSED, + mode_t *perms ATTRIBUTE_UNUSED) +{ + errno = ENOSYS; + virReportSystemError(errno, "%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 Fri, Mar 15, 2013 at 03:12:02PM +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 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 | 193 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 14 ++++ 7 files changed, 224 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 0c0dfb3..0ddc128 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -764,11 +764,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 5a2cbe8..e1ec774 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1252,10 +1252,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 be50e83..7f50328 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" @@ -752,3 +756,192 @@ 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) +{ + errno = ENOSYS;
As with previous patch, NACK to setting 'errno' here. 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 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 | 302 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 273 insertions(+), 29 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 0b274b7..4914baa 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,97 @@ 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; + + VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", + path, (long) uid, (long) gid); + + if (virSecurityDACGetXATTRRefcount(path, &refCount) < 0) { + if (errno != ENOSYS && errno != ENOTSUP) + return -1; + 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 */ + if (errno != ENOSYS && errno != ENOTSUP) + return -1; + + 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 +643,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 Fri, Mar 15, 2013 at 03:12:03PM +0100, Michal Privoznik wrote:
static int +virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid) +{ + int refCount = 0; + bool xattrSupported = true; + + VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", + path, (long) uid, (long) gid); + + if (virSecurityDACGetXATTRRefcount(path, &refCount) < 0) { + if (errno != ENOSYS && errno != ENOTSUP) + return -1;
It is unsafe to check errno. You must use the virErrorPtr only, and if you decide to ignore the error, you should also call virResetLastError() to clear it.
+ 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 */ + if (errno != ENOSYS && errno != ENOTSUP) + return -1; + + 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; +}
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 :|
participants (2)
-
Daniel P. Berrange
-
Michal Privoznik