[libvirt] [PATCH] security: Don't increase XATTRs refcounter on failure

Michal Privoznik posted 1 patch 4 years, 7 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/e3dcb01032a4dac86177295ec6ce4e5d3e9d0851.1566477030.git.mprivozn@redhat.com
src/security/security_dac.c     | 19 ++++++++++++++-----
src/security/security_selinux.c | 17 +++++++++++------
2 files changed, 25 insertions(+), 11 deletions(-)
[libvirt] [PATCH] security: Don't increase XATTRs refcounter on failure
Posted by Michal Privoznik 4 years, 7 months ago
If user has two domains, each have the same disk (configured for
RW) but each runs with different seclabel then we deny start of
the second domain because in order to do that we would need to
relabel the disk but that would cut the first domain off. Even if
we did not do that, qemu would fail to start because it would be
unable to lock the disk image for the second time. So far, this
behaviour is expected. But what is not expected is that we
increase the refcounter in XATTRs and leave it like that.

What happens is that when the second domain starts,
virSecuritySetRememberedLabel() is called, and since there are
XATTRs from the first domain it increments the refcounter and
returns it (refcounter == 2 at this point). Then callers
(virSecurityDACSetOwnership() and
virSecuritySELinuxSetFileconHelper()) realize that refcounter is
greater than 1 and desired seclabel doesn't match the one the
disk image already has and an error is produced. But the
refcounter is never decremented.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1740024

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/security/security_dac.c     | 19 ++++++++++++++-----
 src/security/security_selinux.c | 17 +++++++++++------
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 137daf5d28..b0070f7390 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -754,6 +754,8 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
     struct stat sb;
     int refcount;
     int rc;
+    bool rollback = false;
+    int ret = -1;
 
     if (!path && src && src->path &&
         virStorageSourceIsLocalStorage(src))
@@ -780,16 +782,18 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
         } else if (refcount < 0) {
             return -1;
         } else if (refcount > 1) {
+            rollback = true;
             /* Refcount is greater than 1 which means that there
              * is @refcount domains using the @path. Do not
              * change the label (as it would almost certainly
              * cause the other domains to lose access to the
-             * @path). */
+             * @path). However, the refcounter was incremented in
+             * XATTRs so decrease it. */
             if (sb.st_uid != uid || sb.st_gid != gid) {
                 virReportError(VIR_ERR_OPERATION_INVALID,
                                _("Setting different DAC user or group on %s "
                                  "which is already in use"), path);
-                return -1;
+                goto cleanup;
             }
         }
     }
@@ -797,7 +801,13 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
     VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
              NULLSTR(src ? src->path : path), (long)uid, (long)gid);
 
-    if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0) {
+    if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0)
+        goto cleanup;
+
+    ret = 0;
+
+ cleanup:
+    if (ret < 0 && rollback) {
         virErrorPtr origerr;
 
         virErrorPreserveLast(&origerr);
@@ -812,10 +822,9 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
                      NULLSTR(src ? src->path : path));
 
         virErrorRestore(&origerr);
-        return -1;
     }
 
-    return 0;
+    return ret;
 }
 
 
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index ea20373a90..0c6ace75fa 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1334,6 +1334,7 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr,
     security_context_t econ = NULL;
     int refcount;
     int rc;
+    bool rollback = false;
     int ret = -1;
 
     if ((rc = virSecuritySELinuxTransactionAppend(path, tcon,
@@ -1358,11 +1359,13 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr,
             } else if (refcount < 0) {
                 goto cleanup;
             } else if (refcount > 1) {
+                rollback = true;
                 /* Refcount is greater than 1 which means that there
                  * is @refcount domains using the @path. Do not
                  * change the label (as it would almost certainly
                  * cause the other domains to lose access to the
-                 * @path). */
+                 * @path). However, the refcounter was
+                 * incremented in XATTRs so decrease it. */
                 if (STRNEQ(econ, tcon)) {
                     virReportError(VIR_ERR_OPERATION_INVALID,
                                    _("Setting different SELinux label on %s "
@@ -1373,7 +1376,12 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr,
         }
     }
 
-    if (virSecuritySELinuxSetFileconImpl(path, tcon, optional, privileged) < 0) {
+    if (virSecuritySELinuxSetFileconImpl(path, tcon, optional, privileged) < 0)
+        goto cleanup;
+
+    ret = 0;
+ cleanup:
+    if (ret < 0 && rollback) {
         virErrorPtr origerr;
 
         virErrorPreserveLast(&origerr);
@@ -1388,11 +1396,8 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr,
                      path);
 
         virErrorRestore(&origerr);
-        goto cleanup;
-    }
 
-    ret = 0;
- cleanup:
+    }
     freecon(econ);
     return ret;
 }
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] security: Don't increase XATTRs refcounter on failure
Posted by Martin Kletzander 4 years, 7 months ago
On Thu, Aug 22, 2019 at 02:30:30PM +0200, Michal Privoznik wrote:
>If user has two domains, each have the same disk (configured for
>RW) but each runs with different seclabel then we deny start of
>the second domain because in order to do that we would need to
>relabel the disk but that would cut the first domain off. Even if
>we did not do that, qemu would fail to start because it would be
>unable to lock the disk image for the second time. So far, this
>behaviour is expected. But what is not expected is that we
>increase the refcounter in XATTRs and leave it like that.
>
>What happens is that when the second domain starts,
>virSecuritySetRememberedLabel() is called, and since there are
>XATTRs from the first domain it increments the refcounter and
>returns it (refcounter == 2 at this point). Then callers
>(virSecurityDACSetOwnership() and
>virSecuritySELinuxSetFileconHelper()) realize that refcounter is
>greater than 1 and desired seclabel doesn't match the one the
>disk image already has and an error is produced. But the
>refcounter is never decremented.
>
>Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1740024
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/security/security_dac.c     | 19 ++++++++++++++-----
> src/security/security_selinux.c | 17 +++++++++++------
> 2 files changed, 25 insertions(+), 11 deletions(-)
>
>diff --git a/src/security/security_dac.c b/src/security/security_dac.c
>index 137daf5d28..b0070f7390 100644
>--- a/src/security/security_dac.c
>+++ b/src/security/security_dac.c
>@@ -754,6 +754,8 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
>     struct stat sb;
>     int refcount;
>     int rc;
>+    bool rollback = false;
>+    int ret = -1;
>
>     if (!path && src && src->path &&
>         virStorageSourceIsLocalStorage(src))
>@@ -780,16 +782,18 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
>         } else if (refcount < 0) {
>             return -1;
>         } else if (refcount > 1) {
>+            rollback = true;
>             /* Refcount is greater than 1 which means that there
>              * is @refcount domains using the @path. Do not
>              * change the label (as it would almost certainly
>              * cause the other domains to lose access to the
>-             * @path). */
>+             * @path). However, the refcounter was incremented in
>+             * XATTRs so decrease it. */
>             if (sb.st_uid != uid || sb.st_gid != gid) {
>                 virReportError(VIR_ERR_OPERATION_INVALID,
>                                _("Setting different DAC user or group on %s "
>                                  "which is already in use"), path);
>-                return -1;
>+                goto cleanup;
>             }
>         }
>     }
>@@ -797,7 +801,13 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
>     VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
>              NULLSTR(src ? src->path : path), (long)uid, (long)gid);
>
>-    if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0) {
>+    if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0)
>+        goto cleanup;
>+
>+    ret = 0;
>+
>+ cleanup:
>+    if (ret < 0 && rollback) {
>         virErrorPtr origerr;
>
>         virErrorPreserveLast(&origerr);
>@@ -812,10 +822,9 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
>                      NULLSTR(src ? src->path : path));
>
>         virErrorRestore(&origerr);
>-        return -1;
>     }
>
>-    return 0;
>+    return ret;

The whole cleanup here is used only in error case (as opposed to the selinux
driver where it free()'s something, so if you want it to look nicer change it to
error:, remove the `ret < 0` from the condition...  Well, you might just as well
remove the `ret` variable and so on.  If you want (which I know you do).

With that (optionally, but hopefully) fixed:

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list