Xen Security Advisory 440 v3 (CVE-2023-34323) - xenstored: A transaction conflict can crash C Xenstored

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

            Xen Security Advisory CVE-2023-34323 / XSA-440
                               version 3

        xenstored: A transaction conflict can crash C Xenstored

UPDATES IN VERSION 3
====================

Public release.

ISSUE DESCRIPTION
=================

When a transaction is committed, C Xenstored will first check
the quota is correct before attempting to commit any nodes.  It would
be possible that accounting is temporarily negative if a node has
been removed outside of the transaction.

Unfortunately, some versions of C Xenstored are assuming that the
quota cannot be negative and are using assert() to confirm it.  This
will lead to C Xenstored crash when tools are built without -DNDEBUG
(this is the default).

IMPACT
======

A malicious guest could craft a transaction that will hit the C
Xenstored bug and crash it.  This will result to the inability to
perform any further domain administration like starting new guests,
or adding/removing resources to or from any existing guest.

VULNERABLE SYSTEMS
==================

All versions of Xen up to and including 4.17 are vulnerable if XSA-326
was ingested.

All Xen systems using C Xenstored are vulnerable.  C Xenstored built
using -DNDEBUG (can be specified via EXTRA_CFLAGS_XEN_TOOLS=-DNDEBUG)
are not vulnerable.  Systems using the OCaml variant of Xenstored are
not vulnerable.

MITIGATION
==========

The problem can be avoided by using OCaml Xenstored variant.

CREDITS
=======

This issue was discovered by Stanislav Uschakow and Julien Grall, all
from Amazon.

RESOLUTION
==========

Applying the appropriate attached patch resolves this issue.

Note that patches for released versions are generally prepared to
apply to the stable branches, and may not apply cleanly to the most
recent release tarball.  Downstreams are encouraged to update to the
tip of the stable branch before applying these patches.

xsa440-4.17.patch      Xen 4.17.x - Xen 4.15.x.

$ sha256sum xsa440*
187b7edef4f509f3d7ec1662901fa638a900ab4213447438171fb2935f387014  xsa440.meta
431dab53baf2b57a299d1a151b330b62d9a007715d700e8515db71ff813d0037  xsa440-4.17.patch
$

DEPLOYMENT DURING EMBARGO
=========================

Deployment of the patches and/or mitigations described above (or
others which are substantially similar) is permitted during the
embargo, even on public-facing systems with untrusted guest users and
administrators.

But: Distribution of updated software is prohibited (except to other
members of the predisclosure list).

Predisclosure list members who wish to deploy significantly different
patches and/or mitigations, please contact the Xen Project Security
Team.

(Note: this during-embargo deployment notice is retained in
post-embargo publicly released Xen Project advisories, even though it
is then no longer applicable.  This is to enable the community to have
oversight of the Xen Project Security Team's decisionmaking.)

For more information about permissible uses of embargoed information,
consult the Xen Project community's agreed Security Policy:
  http://www.xenproject.org/security-policy.html
-----BEGIN PGP SIGNATURE-----

iQFABAEBCAAqFiEEI+MiLBRfRHX6gGCng/4UyVfoK9kFAmUlNOMMHHBncEB4ZW4u
b3JnAAoJEIP+FMlX6CvZy64IAIZBqlKJAGVeGMzSpuJfkP2YXLe9JNeR46HRG90e
mV94MWmsf+4kMu2ZhnXQaR2+lafjNfAQVdh9nXV0tdJu//yzLRfXnLfFWrroqBTS
g69/9zvgGRYvobHe6X/WmLwXCV8N27q04zLK7R9nYwntw2mJBBCvUfRPVHk/6lpH
4Ke6o0XbjmOjForl2PA3ISRqXKD5nB0pWp1cEfPt3PzCUV02kI/N3veWDRN2wyPN
jclvwlVVASJdCrcs0+NlOalN5XhD9+K5RN+VVGu3dchXpaa3qEOiTc/V5T1U5cX8
pqNqUBlo4ECFLygE2aUTITIX+dpLaGYD8rmFq0CPnsB6E5U=
=6W84
-----END PGP SIGNATURE-----

From 5d8b3d1ec98e56155d9650d7f4a70cd8ba9dc27d Mon Sep 17 00:00:00 2001
From: Julien Grall <[email protected]>
Date: Fri, 22 Sep 2023 11:32:16 +0100
Subject: tools/xenstored: domain_entry_fix(): Handle conflicting transaction

The function domain_entry_fix() will be initially called to check if the
quota is correct before attempt to commit any nodes. So it would be
possible that accounting is temporarily negative. This is the case
in the following sequence:

  1) Create 50 nodes
  2) Start two transactions
  3) Delete all the nodes in each transaction
  4) Commit the two transactions

Because the first transaction will have succeed and updated the
accounting, there is no guarantee that 'd->nbentry + num' will still
be above 0. So the assert() would be triggered.
The assert() was introduced in dbef1f748289 ("tools/xenstore: simplify
and fix per domain node accounting") with the assumption that the
value can't be negative. As this is not true revert to the original
check but restricted to the path where we don't update. Take the
opportunity to explain the rationale behind the check.

This CVE-2023-34323 / XSA-440.

Reported-by: Stanislav Uschakow <[email protected]>
Fixes: dbef1f748289 ("tools/xenstore: simplify and fix per domain node accounting")
Signed-off-by: Julien Grall <[email protected]>
Reviewed-by: Juergen Gross <[email protected]>

diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index aa86892fed9e..6074df210c6e 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -1094,10 +1094,20 @@ int domain_entry_fix(unsigned int domid, int num, bool update)
 	}
 
 	cnt = d->nbentry + num;
-	assert(cnt >= 0);
 
-	if (update)
+	if (update) {
+		assert(cnt >= 0);
 		d->nbentry = cnt;
+	} else if (cnt < 0) {
+		/*
+		 * In a transaction when a node is being added/removed AND
+		 * the same node has been added/removed outside the
+		 * transaction in parallel, the result value may be negative.
+		 * This is no problem, as the transaction will fail due to
+		 * the resulting conflict. So override 'cnt'.
+		 */
+		cnt = 0;
+	}
 
 	return domid_is_unprivileged(domid) ? cnt : 0;
 }