[libvirt] [PATCH v3 3/3] security_dac: Favour ACLs over chown()

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