[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]