[libvirt] [PATCH RFC 1/3] security_dac: Remember owner prior chown() and restore on relabel

Currently, if we label a file to match qemu process DAC label, we do not store the original owner anywhere. So when relabeling back, the only option we have is to relabel to root:root which is obviously wrong. However, bare remembering is not enough. We need to keep track of how many times we labeled a file so only the last restore chown()-s file back to the original owner. In order to not pollute domain XML, this info is kept in driver's private data in a hash table with path being key and pair <oldLabel, refcount> being value. --- src/security/security_dac.c | 351 ++++++++++++++++++++++++++++++++++------- src/security/security_driver.h | 3 + 2 files changed, 296 insertions(+), 58 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 0b274b7..4b8f0a2 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -42,8 +42,121 @@ struct _virSecurityDACData { uid_t user; gid_t group; bool dynamicOwnership; + virHashTablePtr oldOwners; /* to hold pairs <path, virOldLabelPtr> */ }; +struct _virOldLabel { + char *owner; + int refCount; +}; + +static void virOldLabelFree(virOldLabelPtr label) +{ + if (!label) + return; + + VIR_FREE(label->owner); + VIR_FREE(label); +} + +static void +hashDataFree(void *payload, const void *name ATTRIBUTE_UNUSED) +{ + virOldLabelFree(payload); +} + +/** + * virSecurityDACRememberLabel: + * @priv: private DAC driver data + * @path: path which is about to be relabelled + * + * Store the original DAC label of @path. + * Returns: number of references of @path, or -1 on error + */ +static int +virSecurityDACRememberLabel(virSecurityDACDataPtr priv, + const char *path) +{ + struct stat sb; + virOldLabelPtr oldLabel = NULL; + char *user = NULL, *group = NULL; + + oldLabel = virHashLookup(priv->oldOwners, path); + + if (oldLabel) { + /* just increment ref counter */ + oldLabel->refCount++; + goto cleanup; + } + + if (stat(path, &sb) < 0) { + virReportSystemError(errno, _("Unable to stat %s"), path); + goto cleanup; + } + + user = virGetUserName(sb.st_uid); + group = virGetGroupName(sb.st_gid); + if ((!user && (virAsprintf(&user, "+%ld", (long) sb.st_uid) < 0)) || + (!group && (virAsprintf(&group, "+%ld", (long) sb.st_gid) < 0)) || + (VIR_ALLOC(oldLabel) < 0) || + (virAsprintf(&oldLabel->owner, "%s:%s", user, group) < 0)) { + virReportOOMError(); + VIR_FREE(oldLabel); + goto cleanup; + } + + oldLabel->refCount = 1; + if (virHashUpdateEntry(priv->oldOwners, path, oldLabel) < 0) { + virOldLabelFree(oldLabel); + oldLabel = NULL; + } + +cleanup: + VIR_FREE(user); + VIR_FREE(group); + return oldLabel ? oldLabel->refCount : -1; +} + +static int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr); + +/** + * virSecurityDACGetRememberedLabel: + * @priv: private DAC driver data + * @path: path which we want to restore label on + * @user: original owner of @path + * @group: original owner of @path + * + * Decrements reference counter on @path. If this was the last + * reference, we need to restore the original label, in which + * case @user and @group are updated. + * Returns: the number of references of @path + * 0 if we need to restore the label + * -1 on error + */ +static int +virSecurityDACGetRememberedLabel(virSecurityDACDataPtr priv, + const char *path, + uid_t *user, + gid_t *group) +{ + int ret = -1; + virOldLabelPtr oldLabel = NULL; + + oldLabel = virHashLookup(priv->oldOwners, path); + if (!oldLabel) + goto cleanup; + + ret = --oldLabel->refCount; + + if (!ret) { + ret = parseIds(oldLabel->owner, user, group); + virHashRemoveEntry(priv->oldOwners, path); + } + +cleanup: + return ret; +} + void virSecurityDACSetUser(virSecurityManagerPtr mgr, uid_t user) { @@ -242,14 +355,20 @@ virSecurityDACProbe(const char *virtDriver ATTRIBUTE_UNUSED) } static int -virSecurityDACOpen(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) +virSecurityDACOpen(virSecurityManagerPtr mgr) { - return 0; + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + + priv->oldOwners = virHashCreate(0, hashDataFree); + return priv->oldOwners ? 0 : -1; } static int -virSecurityDACClose(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) +virSecurityDACClose(virSecurityManagerPtr mgr) { + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + + virHashFree(priv->oldOwners); return 0; } @@ -306,7 +425,9 @@ virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid) } static int -virSecurityDACRestoreSecurityFileLabel(const char *path) +virSecurityDACRestoreSecurityFileLabel(const char *path, + uid_t user, + gid_t group) { struct stat buf; int rc = -1; @@ -323,8 +444,7 @@ virSecurityDACRestoreSecurityFileLabel(const char *path) if (stat(newpath, &buf) != 0) goto err; - /* XXX record previous ownership */ - rc = virSecurityDACSetOwnership(newpath, 0, 0); + rc = virSecurityDACSetOwnership(newpath, user, group); err: VIR_FREE(newpath); @@ -345,7 +465,8 @@ virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, uid_t user; gid_t group; - if (virSecurityDACGetImageIds(def, priv, &user, &group)) + if (virSecurityDACGetImageIds(def, priv, &user, &group) || + (virSecurityDACRememberLabel(priv, path) < 0)) return -1; return virSecurityDACSetOwnership(path, user, group); @@ -382,26 +503,14 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, virDomainDiskDefPtr disk, int migrated) { + int ret; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + uid_t user = 0; + gid_t group = 0; - 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 @@ -411,7 +520,7 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, */ if (migrated) { int rc = virStorageFileIsSharedFS(disk->src); - if (rc < 0) + if (rc > 0) return -1; if (rc == 1) { VIR_DEBUG("Skipping image label restore on %s because FS is shared", @@ -420,7 +529,15 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, } } - return virSecurityDACRestoreSecurityFileLabel(disk->src); + ret = virSecurityDACGetRememberedLabel(priv, disk->src, &user, &group); + if (ret < 0) + return -1; + else if (ret > 0) { + VIR_DEBUG("Skipping image label restore on %s " + "because the image is still in use", disk->src); + return 0; + } + return virSecurityDACRestoreSecurityFileLabel(disk->src, user, group); } @@ -445,7 +562,8 @@ virSecurityDACSetSecurityPCILabel(virPCIDevicePtr dev ATTRIBUTE_UNUSED, uid_t user; gid_t group; - if (virSecurityDACGetIds(def, priv, &user, &group)) + if (virSecurityDACGetIds(def, priv, &user, &group) || + (virSecurityDACRememberLabel(priv, file) < 0)) return -1; return virSecurityDACSetOwnership(file, user, group); @@ -464,7 +582,8 @@ virSecurityDACSetSecurityUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED, uid_t user; gid_t group; - if (virSecurityDACGetIds(def, priv, &user, &group)) + if (virSecurityDACGetIds(def, priv, &user, &group) || + (virSecurityDACRememberLabel(priv, file) < 0)) return -1; return virSecurityDACSetOwnership(file, user, group); @@ -536,18 +655,46 @@ done: static int virSecurityDACRestoreSecurityPCILabel(virPCIDevicePtr dev ATTRIBUTE_UNUSED, const char *file, - void *opaque ATTRIBUTE_UNUSED) + void *opaque) { - return virSecurityDACRestoreSecurityFileLabel(file); + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(opaque); + uid_t user = 0; + gid_t group = 0; + int ret; + + ret = virSecurityDACGetRememberedLabel(priv, file, &user, &group); + if (ret < 0) + return -1; + else if (ret > 0) { + VIR_DEBUG("Skipping security label restore on %s " + "because the PCI device is still in use", file); + return 0; + } + + return virSecurityDACRestoreSecurityFileLabel(file, user, group); } static int virSecurityDACRestoreSecurityUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED, const char *file, - void *opaque ATTRIBUTE_UNUSED) + void *opaque) { - return virSecurityDACRestoreSecurityFileLabel(file); + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(opaque); + uid_t user = 0; + gid_t group = 0; + int ret; + + ret = virSecurityDACGetRememberedLabel(priv, file, &user, &group); + if (ret < 0) + return -1; + else if (ret > 0) { + VIR_DEBUG("Skipping security label restore on %s " + "because the USB device is still in use", file); + return 0; + } + + return virSecurityDACRestoreSecurityFileLabel(file, user, group); } @@ -630,6 +777,8 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, switch (dev->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: + if (virSecurityDACRememberLabel(priv, dev->data.file.path) < 0) + goto cleanup; ret = virSecurityDACSetOwnership(dev->data.file.path, user, group); break; @@ -637,16 +786,22 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, if ((virAsprintf(&in, "%s.in", dev->data.file.path) < 0) || (virAsprintf(&out, "%s.out", dev->data.file.path) < 0)) { virReportOOMError(); - goto done; + goto cleanup; } if (virFileExists(in) && virFileExists(out)) { + if ((virSecurityDACRememberLabel(priv, in) < 0) || + (virSecurityDACRememberLabel(priv, out) < 0)) + goto cleanup; if ((virSecurityDACSetOwnership(in, user, group) < 0) || (virSecurityDACSetOwnership(out, user, group) < 0)) { - goto done; + goto cleanup; } - } else if (virSecurityDACSetOwnership(dev->data.file.path, - user, group) < 0) { - goto done; + } else { + if (virSecurityDACRememberLabel(priv, dev->data.file.path) < 0) + goto cleanup; + if (virSecurityDACSetOwnership(dev->data.file.path, + user, group) < 0) + goto cleanup; } ret = 0; break; @@ -656,38 +811,85 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, break; } -done: +cleanup: VIR_FREE(in); VIR_FREE(out); return ret; } static int -virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, +virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr, virDomainChrSourceDefPtr dev) { + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); char *in = NULL, *out = NULL; int ret = -1; + uid_t user = 0; + gid_t group = 0; switch (dev->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - ret = virSecurityDACRestoreSecurityFileLabel(dev->data.file.path); + ret = virSecurityDACGetRememberedLabel(priv, dev->data.file.path, + &user, &group); + if (ret < 0) + return -1; + else if (ret > 0) { + VIR_DEBUG("Skipping security label restore on %s " + "because the chardev device is still in use", + dev->data.file.path); + return 0; + } + + ret = virSecurityDACRestoreSecurityFileLabel(dev->data.file.path, + user, group); break; case VIR_DOMAIN_CHR_TYPE_PIPE: if ((virAsprintf(&out, "%s.out", dev->data.file.path) < 0) || (virAsprintf(&in, "%s.in", dev->data.file.path) < 0)) { virReportOOMError(); - goto done; + goto cleanup; } if (virFileExists(in) && virFileExists(out)) { - if ((virSecurityDACRestoreSecurityFileLabel(out) < 0) || - (virSecurityDACRestoreSecurityFileLabel(in) < 0)) { - goto done; + ret = virSecurityDACGetRememberedLabel(priv, in, &user, &group); + if (ret < 0) + goto cleanup; + else if (ret > 0) { + VIR_DEBUG("Skipping security label restore on %s " + "because the chardev device is still in use", in); + goto cleanup; + } + ret = virSecurityDACGetRememberedLabel(priv, out, &user, &group); + if (ret < 0) + goto cleanup; + else if (ret > 0) { + VIR_DEBUG("Skipping security label restore on %s " + "because the chardev device is still in use", out); + goto cleanup; + } + if ((virSecurityDACRestoreSecurityFileLabel(out, user, group) < 0) || + (virSecurityDACRestoreSecurityFileLabel(in, user, group) < 0)) { + ret = -1; + goto cleanup; + } + } else { + ret = virSecurityDACGetRememberedLabel(priv, dev->data.file.path, + &user, &group); + if (ret < 0) + goto cleanup; + else if (ret > 0) { + VIR_DEBUG("Skipping security label restore on %s " + "because the chardev device is still in use", + dev->data.file.path); + goto cleanup; + } + + if (virSecurityDACRestoreSecurityFileLabel(dev->data.file.path, + user, group) < 0) { + ret = -1; + goto cleanup; } - } else if (virSecurityDACRestoreSecurityFileLabel(dev->data.file.path) < 0) { - goto done; } ret = 0; break; @@ -697,7 +899,7 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, break; } -done: +cleanup: VIR_FREE(in); VIR_FREE(out); return ret; @@ -723,6 +925,8 @@ virSecurityDACRestoreSecurityAllLabel(virSecurityManagerPtr mgr, virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); int i; int rc = 0; + uid_t user = 0; + gid_t group = 0; if (!priv->dynamicOwnership) return 0; @@ -752,13 +956,29 @@ virSecurityDACRestoreSecurityAllLabel(virSecurityManagerPtr mgr, mgr) < 0) rc = -1; - if (def->os.kernel && - virSecurityDACRestoreSecurityFileLabel(def->os.kernel) < 0) - rc = -1; + if (def->os.kernel) { + i = virSecurityDACGetRememberedLabel(priv, def->os.kernel, &user, &group); + if (i < 0) + rc = -1; + else if (i > 0) + VIR_DEBUG("Skipping security label restore on %s " + "because the kernel image is still in use", def->os.kernel); + else if (virSecurityDACRestoreSecurityFileLabel(def->os.kernel, + user, group) < 0) + rc = -1; + } - if (def->os.initrd && - virSecurityDACRestoreSecurityFileLabel(def->os.initrd) < 0) - rc = -1; + if (def->os.initrd) { + i = virSecurityDACGetRememberedLabel(priv, def->os.initrd, &user, &group); + if (i < 0) + rc = -1; + else if (i > 0) + VIR_DEBUG("Skipping security label restore on %s " + "because the initrd image is still in use", def->os.initrd); + else if (virSecurityDACRestoreSecurityFileLabel(def->os.initrd, + user, group) < 0) + rc = -1; + } return rc; } @@ -815,11 +1035,13 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr, return -1; if (def->os.kernel && - virSecurityDACSetOwnership(def->os.kernel, user, group) < 0) + ((virSecurityDACRememberLabel(priv, def->os.kernel) < 0) || + (virSecurityDACSetOwnership(def->os.kernel, user, group) < 0))) return -1; if (def->os.initrd && - virSecurityDACSetOwnership(def->os.initrd, user, group) < 0) + ((virSecurityDACRememberLabel(priv, def->os.initrd) < 0) || + (virSecurityDACSetOwnership(def->os.initrd, user, group) < 0))) return -1; return 0; @@ -835,7 +1057,8 @@ virSecurityDACSetSavedStateLabel(virSecurityManagerPtr mgr, gid_t group; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); - if (virSecurityDACGetImageIds(def, priv, &user, &group)) + if (virSecurityDACGetImageIds(def, priv, &user, &group) || + (virSecurityDACRememberLabel(priv, savefile) < 0)) return -1; return virSecurityDACSetOwnership(savefile, user, group); @@ -848,11 +1071,23 @@ virSecurityDACRestoreSavedStateLabel(virSecurityManagerPtr mgr, const char *savefile) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + uid_t user = 0; + gid_t group = 0; + int ret; if (!priv->dynamicOwnership) return 0; - return virSecurityDACRestoreSecurityFileLabel(savefile); + ret = virSecurityDACGetRememberedLabel(priv, savefile, &user, &group); + if (ret < 0) + return -1; + else if (ret > 0) { + VIR_DEBUG("Skipping security label restore on %s " + "because the saved state file is still in use", savefile); + return 0; + } + + return virSecurityDACRestoreSecurityFileLabel(savefile, user, group); } diff --git a/src/security/security_driver.h b/src/security/security_driver.h index cc401e1..d54f754 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -40,6 +40,9 @@ typedef enum { typedef struct _virSecurityDriver virSecurityDriver; typedef virSecurityDriver *virSecurityDriverPtr; +typedef struct _virOldLabel virOldLabel; +typedef virOldLabel *virOldLabelPtr; + typedef virSecurityDriverStatus (*virSecurityDriverProbe) (const char *virtDriver); typedef int (*virSecurityDriverOpen) (virSecurityManagerPtr mgr); typedef int (*virSecurityDriverClose) (virSecurityManagerPtr mgr); -- 1.8.1.4