[prev in list] [next in list] [prev in thread] [next in thread]
List: oss-security
Subject: [oss-security] Xen Security Advisory 325 v3 (CVE-2020-29483) - Xenstore: guests can disturb domain c
From: Xen.org security team <security () xen ! org>
Date: 2020-12-15 12:20:19
Message-ID: E1kp9JT-00072J-Kk () xenbits ! xenproject ! org
[Download RAW message or body]
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Xen Security Advisory CVE-2020-29483 / XSA-325
version 3
Xenstore: guests can disturb domain cleanup
UPDATES IN VERSION 3
====================
Public release.
ISSUE DESCRIPTION
=================
Xenstored and guests communicate via a shared memory page using a
specific protocol. When a guest violates this protocol, xenstored will
drop the connection to that guest.
Unfortunately this is done by just removing the guest from xenstored's
internal management, resulting in the same actions as if the guest had
been destroyed, including sending an @releaseDomain event.
@releaseDomain events do not say guest has been removed. All watchers
of this event must look at the states of all guests to find the guest
which has been removed. When an @releaseDomain is generated due to
domain xenstored protocol violation, As the guest is still running, so
the watchers will not react.
Later, when the guest is actually destroyed, xenstored will no longer
have it stored in its internal data base, so no further @releaseDomain
event will be sent. This can lead to a zombie domain; memory mappings
of that guest's memory will not be removed, due to the missing
event. This zombie domain will be cleaned up only after another domain
is destroyed, as that will trigger another @releaseDomain event.
If the device model of the guest which violated the Xenstore protocol
is running in a stub-domain, a use-after-free case could happen in
xenstored, after having removed the guest from its internal data base,
possibly resulting in a crash of xenstored.
IMPACT
======
A malicious guest can block resources of the host for a period after
its own death.
Guests with a stub domain device model can eventually crash xenstored,
resulting in a more serious denial of service (the prevention of any
further domain management operations).
VULNERABLE SYSTEMS
==================
All versions of Xen are affected.
Only the C variant of Xenstore is affected, the Ocaml variant is not
affected.
Only HVM guests with a stubdom device model can cause a serious DoS.
MITIGATION
==========
Using the Ocaml variant of Xenstore (oxenstored) avoids the issue;
Running HVM domains with a dom0 device model rather than a stubdom
device model will avoid the more serious DoS.
However, given the other vulnerabilities in both versions of xenstored
being reported at this time, changing xenstored implementation, or
switching to dom0 xenstored, is not a recommended approach to
mitigation of individual issues.
CREDITS
=======
This issue was discovered by Pawel Wieczorkiewicz of 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.
xsa325.patch xen-unstable
xsa325-4.14.patch Xen 4.14 - 4.10
$ sha256sum xsa325*
29a81606e9c0e036dcc39b2a7e6ec0b1ce7d658972a368907b02d56f2aae3dc2 xsa325.meta
56e09d92fa3d623b2896fd6e6a08805514b2ff9b1cde526968be3925fda28705 xsa325.patch
702f0f4c20e685d2e23a9c1a31c0e0fda1824c9209bd8affca9dd3489dfbd23d xsa325-4.14.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/4UyVfoK9kFAl/Yqd4MHHBncEB4ZW4u
b3JnAAoJEIP+FMlX6CvZ7AEH/0fHBNU0Sd9iVVcGmZvJblI3mKy9TA3Z8vcdiN7I
j0TXOQlmjp90WPC8nYo/XtsFpCx5dhg0yLX1Unxe1R0twvt2OrXWRZTa0dbVFcou
t8yq3lSRiOqzwNK186wzS2LSyAH7yit9CpWLGsXuL6WnocL84Hb3PSsJBP4nTZzm
dcol+h85SvfQ5S+aMUTPqxdm+uE9qoSAN6rJU2Fill3jCThpJSfRUy1vIz5CDYes
oD8Oq+H1sdfzCtDHGzgRveDqkHTr6rxCmlenxAI3UCshkhM6VJypoNQ4jQpS/yfN
nrim4XntIOdy1HR4UgnHRYcnFOnn2qs7dkIU449KVzs1KCg=
=83j/
-----END PGP SIGNATURE-----
["xsa325.meta" (application/octet-stream)]
["xsa325.patch" (application/octet-stream)]
From: Harsha Shamsundara Havanur <havanur@amazon.com>
Subject: tools/xenstore: Preserve bad client until they are destroyed
XenStored will kill any connection that it thinks has misbehaved,
this is currently happening in two places:
* In `handle_input()` if the sanity check on the ring and the message
fails.
* In `handle_output()` when failing to write the response in the ring.
As the domain structure is a child of the connection, XenStored will
destroy its view of the domain when killing the connection. This will
result in sending @releaseDomain event to all the watchers.
As the watch event doesn't carry which domain has been released,
the watcher (such as XenStored) will generally go through the list of
domains registers and check if one of them is shutting down/dying.
In the case of a client misbehaving, the domain will likely to be
running, so no action will be performed.
When the domain is effectively destroyed, XenStored will not be aware of
the domain anymore. So the watch event is not going to be sent.
By consequence, the watchers of the event will not release mappings
they may have on the domain. This will result in a zombie domain.
In order to send @releaseDomain event at the correct time, we want
to keep the domain structure until the domain is effectively
shutting-down/dying.
We also want to keep the connection around so we could possibly revive
the connection in the future.
A new flag 'is_ignored' is added to mark whether a connection should be
ignored when checking if there are work to do. Additionally any
transactions, watches, buffers associated to the connection will be
freed as you can't do much with them (restarting the connection will
likely need a reset).
As a side note, when the device model were running in a stubdomain, a
guest would have been able to introduce a use-after-free because there
is two parents for a guest connection.
This is XSA-325.
Reported-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Signed-off-by: Harsha Shamsundara Havanur <havanur@amazon.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Paul Durrant <paul@xen.org>
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index c929cbbc3b..746a1247b3 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1337,6 +1337,32 @@ static struct {
[XS_DIRECTORY_PART] = { "DIRECTORY_PART", send_directory_part },
};
+/*
+ * Keep the connection alive but stop processing any new request or sending
+ * reponse. This is to allow sending @releaseDomain watch event at the correct
+ * moment and/or to allow the connection to restart (not yet implemented).
+ *
+ * All watches, transactions, buffers will be freed.
+ */
+static void ignore_connection(struct connection *conn)
+{
+ struct buffered_data *out, *tmp;
+
+ trace("CONN %p ignored\n", conn);
+
+ conn->is_ignored = true;
+ conn_delete_all_watches(conn);
+ conn_delete_all_transactions(conn);
+
+ list_for_each_entry_safe(out, tmp, &conn->out_list, list) {
+ list_del(&out->list);
+ talloc_free(out);
+ }
+
+ talloc_free(conn->in);
+ conn->in = NULL;
+}
+
static const char *sockmsg_string(enum xsd_sockmsg_type type)
{
if ((unsigned int)type < ARRAY_SIZE(wire_funcs) && wire_funcs[type].str)
@@ -1395,8 +1421,10 @@ static void consider_message(struct connection *conn)
assert(conn->in == NULL);
}
-/* Errors in reading or allocating here mean we get out of sync, so we
- * drop the whole client connection. */
+/*
+ * Errors in reading or allocating here means we get out of sync, so we mark
+ * the connection as ignored.
+ */
static void handle_input(struct connection *conn)
{
int bytes;
@@ -1453,14 +1481,14 @@ static void handle_input(struct connection *conn)
return;
bad_client:
- /* Kill it. */
- talloc_free(conn);
+ ignore_connection(conn);
}
static void handle_output(struct connection *conn)
{
+ /* Ignore the connection if an error occured */
if (!write_messages(conn))
- talloc_free(conn);
+ ignore_connection(conn);
}
struct connection *new_connection(connwritefn_t *write, connreadfn_t *read)
@@ -1475,6 +1503,7 @@ struct connection *new_connection(connwritefn_t *write, connreadfn_t *read)
new->pollfd_idx = -1;
new->write = write;
new->read = read;
+ new->is_ignored = false;
new->transaction_started = 0;
INIT_LIST_HEAD(&new->out_list);
INIT_LIST_HEAD(&new->watches);
@@ -2136,8 +2165,9 @@ int main(int argc, char *argv[])
if (fds[conn->pollfd_idx].revents
& ~(POLLIN|POLLOUT))
talloc_free(conn);
- else if (fds[conn->pollfd_idx].revents
- & POLLIN)
+ else if ((fds[conn->pollfd_idx].revents
+ & POLLIN) &&
+ !conn->is_ignored)
handle_input(conn);
}
if (talloc_free(conn) == 0)
@@ -2149,8 +2179,9 @@ int main(int argc, char *argv[])
if (fds[conn->pollfd_idx].revents
& ~(POLLIN|POLLOUT))
talloc_free(conn);
- else if (fds[conn->pollfd_idx].revents
- & POLLOUT)
+ else if ((fds[conn->pollfd_idx].revents
+ & POLLOUT) &&
+ !conn->is_ignored)
handle_output(conn);
}
if (talloc_free(conn) == 0)
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 6c21d5bb9a..4c6c3d6f20 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -77,6 +77,9 @@ struct connection
/* Who am I? 0 for socket connections. */
unsigned int id;
+ /* Is this connection ignored? */
+ bool is_ignored;
+
/* Buffered incoming data. */
struct buffered_data *in;
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 7169da9851..7d348d57f3 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -286,6 +286,10 @@ bool domain_can_read(struct connection *conn)
if (domain_is_unprivileged(conn) && conn->domain->wrl_credit < 0)
return false;
+
+ if (conn->is_ignored)
+ return false;
+
return (intf->req_cons != intf->req_prod);
}
@@ -303,6 +307,10 @@ bool domain_is_unprivileged(struct connection *conn)
bool domain_can_write(struct connection *conn)
{
struct xenstore_domain_interface *intf = conn->domain->interface;
+
+ if (conn->is_ignored)
+ return false;
+
return ((intf->rsp_prod - intf->rsp_cons) != XENSTORE_RING_SIZE);
}
["xsa325-4.14.patch" (application/octet-stream)]
From: Harsha Shamsundara Havanur <havanur@amazon.com>
Subject: tools/xenstore: Preserve bad client until they are destroyed
XenStored will kill any connection that it thinks has misbehaved,
this is currently happening in two places:
* In `handle_input()` if the sanity check on the ring and the message
fails.
* In `handle_output()` when failing to write the response in the ring.
As the domain structure is a child of the connection, XenStored will
destroy its view of the domain when killing the connection. This will
result in sending @releaseDomain event to all the watchers.
As the watch event doesn't carry which domain has been released,
the watcher (such as XenStored) will generally go through the list of
domains registers and check if one of them is shutting down/dying.
In the case of a client misbehaving, the domain will likely to be
running, so no action will be performed.
When the domain is effectively destroyed, XenStored will not be aware of
the domain anymore. So the watch event is not going to be sent.
By consequence, the watchers of the event will not release mappings
they may have on the domain. This will result in a zombie domain.
In order to send @releaseDomain event at the correct time, we want
to keep the domain structure until the domain is effectively
shutting-down/dying.
We also want to keep the connection around so we could possibly revive
the connection in the future.
A new flag 'is_ignored' is added to mark whether a connection should be
ignored when checking if there are work to do. Additionally any
transactions, watches, buffers associated to the connection will be
freed as you can't do much with them (restarting the connection will
likely need a reset).
As a side note, when the device model were running in a stubdomain, a
guest would have been able to introduce a use-after-free because there
is two parents for a guest connection.
This is XSA-325.
Reported-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Signed-off-by: Harsha Shamsundara Havanur <havanur@amazon.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Paul Durrant <paul@xen.org>
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index af3d17004b3f..27d8f15b6b76 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1355,6 +1355,32 @@ static struct {
[XS_DIRECTORY_PART] = { "DIRECTORY_PART", send_directory_part },
};
+/*
+ * Keep the connection alive but stop processing any new request or sending
+ * reponse. This is to allow sending @releaseDomain watch event at the correct
+ * moment and/or to allow the connection to restart (not yet implemented).
+ *
+ * All watches, transactions, buffers will be freed.
+ */
+static void ignore_connection(struct connection *conn)
+{
+ struct buffered_data *out, *tmp;
+
+ trace("CONN %p ignored\n", conn);
+
+ conn->is_ignored = true;
+ conn_delete_all_watches(conn);
+ conn_delete_all_transactions(conn);
+
+ list_for_each_entry_safe(out, tmp, &conn->out_list, list) {
+ list_del(&out->list);
+ talloc_free(out);
+ }
+
+ talloc_free(conn->in);
+ conn->in = NULL;
+}
+
static const char *sockmsg_string(enum xsd_sockmsg_type type)
{
if ((unsigned int)type < ARRAY_SIZE(wire_funcs) && wire_funcs[type].str)
@@ -1413,8 +1439,10 @@ static void consider_message(struct connection *conn)
assert(conn->in == NULL);
}
-/* Errors in reading or allocating here mean we get out of sync, so we
- * drop the whole client connection. */
+/*
+ * Errors in reading or allocating here means we get out of sync, so we mark
+ * the connection as ignored.
+ */
static void handle_input(struct connection *conn)
{
int bytes;
@@ -1471,14 +1499,14 @@ static void handle_input(struct connection *conn)
return;
bad_client:
- /* Kill it. */
- talloc_free(conn);
+ ignore_connection(conn);
}
static void handle_output(struct connection *conn)
{
+ /* Ignore the connection if an error occured */
if (!write_messages(conn))
- talloc_free(conn);
+ ignore_connection(conn);
}
struct connection *new_connection(connwritefn_t *write, connreadfn_t *read)
@@ -1494,6 +1522,7 @@ struct connection *new_connection(connwritefn_t *write, connreadfn_t *read)
new->write = write;
new->read = read;
new->can_write = true;
+ new->is_ignored = false;
new->transaction_started = 0;
INIT_LIST_HEAD(&new->out_list);
INIT_LIST_HEAD(&new->watches);
@@ -2186,8 +2215,9 @@ int main(int argc, char *argv[])
if (fds[conn->pollfd_idx].revents
& ~(POLLIN|POLLOUT))
talloc_free(conn);
- else if (fds[conn->pollfd_idx].revents
- & POLLIN)
+ else if ((fds[conn->pollfd_idx].revents
+ & POLLIN) &&
+ !conn->is_ignored)
handle_input(conn);
}
if (talloc_free(conn) == 0)
@@ -2199,8 +2229,9 @@ int main(int argc, char *argv[])
if (fds[conn->pollfd_idx].revents
& ~(POLLIN|POLLOUT))
talloc_free(conn);
- else if (fds[conn->pollfd_idx].revents
- & POLLOUT)
+ else if ((fds[conn->pollfd_idx].revents
+ & POLLOUT) &&
+ !conn->is_ignored)
handle_output(conn);
}
if (talloc_free(conn) == 0)
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index eb19b71f5f46..196a6fd2b0be 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -80,6 +80,9 @@ struct connection
/* Is this a read-only connection? */
bool can_write;
+ /* Is this connection ignored? */
+ bool is_ignored;
+
/* Buffered incoming data. */
struct buffered_data *in;
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index dc635e9be30c..d5e1e3e9d42d 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -286,6 +286,10 @@ bool domain_can_read(struct connection *conn)
if (domain_is_unprivileged(conn) && conn->domain->wrl_credit < 0)
return false;
+
+ if (conn->is_ignored)
+ return false;
+
return (intf->req_cons != intf->req_prod);
}
@@ -303,6 +307,10 @@ bool domain_is_unprivileged(struct connection *conn)
bool domain_can_write(struct connection *conn)
{
struct xenstore_domain_interface *intf = conn->domain->interface;
+
+ if (conn->is_ignored)
+ return false;
+
return ((intf->rsp_prod - intf->rsp_cons) != XENSTORE_RING_SIZE);
}
--
2.17.1
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic