Re: [libvirt] [PATCH v2 2/4] security_dac: Remember owner prior chown() and restore on relabel

Currently, on domain startup a labeling of domain resources (e.g. disk images, sockets, ...) is done. However, we don't store the original owner anywhere. So when domain is shutting down, we cannot restore the original owner of files we have chown()-ed. This patch resolves this issue for DAC driver. --- src/security/security_dac.c | 187 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 153 insertions(+), 34 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 0b274b7..76a1dc6 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_OWNER "trusted.oldOwner" +#define SECURITY_DAC_XATTR_REFCOUNT "trusted.refCount" typedef struct _virSecurityDACData virSecurityDACData; typedef virSecurityDACData *virSecurityDACDataPtr; @@ -234,6 +237,117 @@ int virSecurityDACGetImageIds(virDomainDefPtr def, virSecurityDACDataPtr priv, return 0; } +static int +virSecurityDACRememberLabel(const char *path) +{ + int ret = -1; + char *refCountStr = NULL; + char *oldOwner = NULL; + unsigned int refCount = 0; + + if (virFileGetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, &refCountStr) < 0) { + /* Not all filesystems support XATTR */ + if (virStorageFileIsSharedFS(path)) + return 0; + else + return ret; + } + + if (!refCountStr) { + struct stat sb; + + 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; + } else if (virStrToLong_ui(refCountStr, NULL, 10, &refCount) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Malformed %s attribute: %s"), + SECURITY_DAC_XATTR_REFCOUNT, + refCountStr); + goto cleanup; + } + + if (virAsprintf(&refCountStr, "%u", refCount+1) < 0) { + virReportOOMError(); + return ret; + } + + if (virFileSetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, refCountStr) < 0) + goto cleanup; + + ret = 0; +cleanup: + VIR_FREE(oldOwner); + VIR_FREE(refCountStr); + return 0; +} + +static int +virSecurityDACGetRememberedLabel(const char *path, + uid_t *user, + uid_t *group) +{ + int ret = -1; + char *refCountStr = NULL; + char *oldOwner = NULL; + unsigned int refCount; + + 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_ui(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, "%u", refCount) < 0) { + virReportOOMError(); + return ret; + } + if (virFileSetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, refCountStr) < 0) + goto cleanup; + ret = refCount; + } else { + 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_REFCOUNT); + virFileRemoveAttr(path, SECURITY_DAC_XATTR_OLD_OWNER); + } + + ret = refCount; + +cleanup: + VIR_FREE(oldOwner); + VIR_FREE(refCountStr); + return ret; +} + static virSecurityDriverStatus virSecurityDACProbe(const char *virtDriver ATTRIBUTE_UNUSED) @@ -265,11 +379,18 @@ static const char * virSecurityDACGetDOI(virSecurityManagerPtr mgr ATTRIBUTE_UNU } static int -virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid) +virSecurityDACSetOwnership(const char *path, + uid_t uid, + gid_t gid, + bool remember)

{ VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", path, (long) uid, (long) gid); + if (remember && + virSecurityDACRememberLabel(path) < 0) + return -1; + if (chown(path, uid, gid) < 0) { struct stat sb; int chown_errno = errno; @@ -310,23 +431,35 @@ virSecurityDACRestoreSecurityFileLabel(const char *path) { struct stat buf; int rc = -1; + int ret; char *newpath = NULL; + uid_t user = (uid_t) -1; + gid_t group = (gid_t) -1; VIR_INFO("Restoring DAC user and group on '%s'", 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; - /* XXX record previous ownership */ - rc = virSecurityDACSetOwnership(newpath, 0, 0); + ret = virSecurityDACGetRememberedLabel(newpath, &user, &group); + if (ret < 0) + goto cleanup; + else if (ret > 0) { + VIR_DEBUG("Skipping label restore on '%s' as it is still " + "in use (refCount=%d)", newpath, ret); + rc = 0; + goto cleanup; + } + + rc = virSecurityDACSetOwnership(newpath, user, group, false); -err: +cleanup: VIR_FREE(newpath); return rc; } @@ -348,7 +481,7 @@ virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, if (virSecurityDACGetImageIds(def, priv, &user, &group)) return -1; - return virSecurityDACSetOwnership(path, user, group); + return virSecurityDACSetOwnership(path, user, group, true);