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

[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.

>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.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


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