Commit 896997ad authored by Henrik Riomar's avatar Henrik Riomar Committed by Milan P. Stanić
Browse files

main/xen: fix XSA-115

This CVE-2020-29480
parent d73c439b
From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= <edvin.torok@citrix.com>
Subject: tools/ocaml/xenstored: ignore transaction id for [un]watch
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Instead of ignoring the transaction id for XS_WATCH and XS_UNWATCH
commands as it is documented in docs/misc/xenstore.txt, it is tested
for validity today.
Really ignore the transaction id for XS_WATCH and XS_UNWATCH.
This is part of XSA-115.
Signed-off-by: Edwin Török <edvin.torok@citrix.com>
Acked-by: Christian Lindig <christian.lindig@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml
index ff5c9484fc..2fa6798e3b 100644
--- a/tools/ocaml/xenstored/process.ml
+++ b/tools/ocaml/xenstored/process.ml
@@ -498,12 +498,19 @@ let retain_op_in_history ty =
| Xenbus.Xb.Op.Reset_watches
| Xenbus.Xb.Op.Invalid -> false
+let maybe_ignore_transaction = function
+ | Xenbus.Xb.Op.Watch | Xenbus.Xb.Op.Unwatch -> fun tid ->
+ if tid <> Transaction.none then
+ debug "Ignoring transaction ID %d for watch/unwatch" tid;
+ Transaction.none
+ | _ -> fun x -> x
+
(**
* Nothrow guarantee.
*)
let process_packet ~store ~cons ~doms ~con ~req =
let ty = req.Packet.ty in
- let tid = req.Packet.tid in
+ let tid = maybe_ignore_transaction ty req.Packet.tid in
let rid = req.Packet.rid in
try
let fct = function_of_type ty in
From e92f3dfeaae21a335e666c9247954424e34e5c56 Mon Sep 17 00:00:00 2001
From: Juergen Gross <jgross@suse.com>
Date: Thu, 11 Jun 2020 16:12:37 +0200
Subject: [PATCH 01/10] tools/xenstore: allow removing child of a node
exceeding quota
An unprivileged user of Xenstore is not allowed to write nodes with a
size exceeding a global quota, while privileged users like dom0 are
allowed to write such nodes. The size of a node is the needed space
to store all node specific data, this includes the names of all
children of the node.
When deleting a node its parent has to be modified by removing the
name of the to be deleted child from it.
This results in the strange situation that an unprivileged owner of a
node might not succeed in deleting that node in case its parent is
exceeding the quota of that unprivileged user (it might have been
written by dom0), as the user is not allowed to write the updated
parent node.
Fix that by not checking the quota when writing a node for the
purpose of removing a child's name only.
The same applies to transaction handling: a node being read during a
transaction is written to the transaction specific area and it should
not be tested for exceeding the quota, as it might not be owned by
the reader and presumably the original write would have failed if the
node is owned by the reader.
This is part of XSA-115.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Paul Durrant <paul@xen.org>
---
tools/xenstore/xenstored_core.c | 20 +++++++++++---------
tools/xenstore/xenstored_core.h | 3 ++-
tools/xenstore/xenstored_transaction.c | 2 +-
3 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 97ceabf9642d..b43e1018babd 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -417,7 +417,8 @@ static struct node *read_node(struct connection *conn, const void *ctx,
return node;
}
-int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node)
+int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node,
+ bool no_quota_check)
{
TDB_DATA data;
void *p;
@@ -427,7 +428,7 @@ int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node)
+ node->num_perms*sizeof(node->perms[0])
+ node->datalen + node->childlen;
- if (domain_is_unprivileged(conn) &&
+ if (!no_quota_check && domain_is_unprivileged(conn) &&
data.dsize >= quota_max_entry_size) {
errno = ENOSPC;
return errno;
@@ -455,14 +456,15 @@ int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node)
return 0;
}
-static int write_node(struct connection *conn, struct node *node)
+static int write_node(struct connection *conn, struct node *node,
+ bool no_quota_check)
{
TDB_DATA key;
if (access_node(conn, node, NODE_ACCESS_WRITE, &key))
return errno;
- return write_node_raw(conn, &key, node);
+ return write_node_raw(conn, &key, node, no_quota_check);
}
static enum xs_perm_type perm_for_conn(struct connection *conn,
@@ -999,7 +1001,7 @@ static struct node *create_node(struct connection *conn, const void *ctx,
/* We write out the nodes down, setting destructor in case
* something goes wrong. */
for (i = node; i; i = i->parent) {
- if (write_node(conn, i)) {
+ if (write_node(conn, i, false)) {
domain_entry_dec(conn, i);
return NULL;
}
@@ -1039,7 +1041,7 @@ static int do_write(struct connection *conn, struct buffered_data *in)
} else {
node->data = in->buffer + offset;
node->datalen = datalen;
- if (write_node(conn, node))
+ if (write_node(conn, node, false))
return errno;
}
@@ -1115,7 +1117,7 @@ static int remove_child_entry(struct connection *conn, struct node *node,
size_t childlen = strlen(node->children + offset);
memdel(node->children, offset, childlen + 1, node->childlen);
node->childlen -= childlen + 1;
- return write_node(conn, node);
+ return write_node(conn, node, true);
}
@@ -1254,7 +1256,7 @@ static int do_set_perms(struct connection *conn, struct buffered_data *in)
node->num_perms = num;
domain_entry_inc(conn, node);
- if (write_node(conn, node))
+ if (write_node(conn, node, false))
return errno;
fire_watches(conn, in, name, false);
@@ -1514,7 +1516,7 @@ static void manual_node(const char *name, const char *child)
if (child)
node->childlen = strlen(child) + 1;
- if (write_node(NULL, node))
+ if (write_node(NULL, node, false))
barf_perror("Could not create initial node %s", name);
talloc_free(node);
}
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 56a279cfbb47..3cb1c235a101 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -149,7 +149,8 @@ void send_ack(struct connection *conn, enum xsd_sockmsg_type type);
char *canonicalize(struct connection *conn, const void *ctx, const char *node);
/* Write a node to the tdb data base. */
-int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node);
+int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node,
+ bool no_quota_check);
/* Get this node, checking we have permissions. */
struct node *get_node(struct connection *conn,
diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index 2824f7b359b8..e87897573469 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -276,7 +276,7 @@ int access_node(struct connection *conn, struct node *node,
i->check_gen = true;
if (node->generation != NO_GENERATION) {
set_tdb_key(trans_name, &local_key);
- ret = write_node_raw(conn, &local_key, node);
+ ret = write_node_raw(conn, &local_key, node, true);
if (ret)
goto err;
i->ta_node = true;
--
2.17.1
From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= <edvin.torok@citrix.com>
Subject: tools/ocaml/xenstored: check privilege for XS_IS_DOMAIN_INTRODUCED
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The Xenstore command XS_IS_DOMAIN_INTRODUCED should be possible for privileged
domains only (the only user in the tree is the xenpaging daemon).
This is part of XSA-115.
Signed-off-by: Edwin Török <edvin.torok@citrix.com>
Acked-by: Christian Lindig <christian.lindig@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml
index 2fa6798e3b..fd79ef564f 100644
--- a/tools/ocaml/xenstored/process.ml
+++ b/tools/ocaml/xenstored/process.ml
@@ -166,7 +166,9 @@ let do_setperms con t _domains _cons data =
let do_error _con _t _domains _cons _data =
raise Define.Unknown_operation
-let do_isintroduced _con _t domains _cons data =
+let do_isintroduced con _t domains _cons data =
+ if not (Connection.is_dom0 con)
+ then raise Define.Permission_denied;
let domid =
match (split None '\000' data) with
| domid :: _ -> int_of_string domid
From e8076f73de65c4816f69d6ebf75839c706145fcd Mon Sep 17 00:00:00 2001
From: Juergen Gross <jgross@suse.com>
Date: Thu, 11 Jun 2020 16:12:38 +0200
Subject: [PATCH 02/10] tools/xenstore: ignore transaction id for [un]watch
Instead of ignoring the transaction id for XS_WATCH and XS_UNWATCH
commands as it is documented in docs/misc/xenstore.txt, it is tested
for validity today.
Really ignore the transaction id for XS_WATCH and XS_UNWATCH.
This is part of XSA-115.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Paul Durrant <paul@xen.org>
---
tools/xenstore/xenstored_core.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index b43e1018babd..bb2f9fd4e76e 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1268,13 +1268,17 @@ static int do_set_perms(struct connection *conn, struct buffered_data *in)
static struct {
const char *str;
int (*func)(struct connection *conn, struct buffered_data *in);
+ unsigned int flags;
+#define XS_FLAG_NOTID (1U << 0) /* Ignore transaction id. */
} const wire_funcs[XS_TYPE_COUNT] = {
[XS_CONTROL] = { "CONTROL", do_control },
[XS_DIRECTORY] = { "DIRECTORY", send_directory },
[XS_READ] = { "READ", do_read },
[XS_GET_PERMS] = { "GET_PERMS", do_get_perms },
- [XS_WATCH] = { "WATCH", do_watch },
- [XS_UNWATCH] = { "UNWATCH", do_unwatch },
+ [XS_WATCH] =
+ { "WATCH", do_watch, XS_FLAG_NOTID },
+ [XS_UNWATCH] =
+ { "UNWATCH", do_unwatch, XS_FLAG_NOTID },
[XS_TRANSACTION_START] = { "TRANSACTION_START", do_transaction_start },
[XS_TRANSACTION_END] = { "TRANSACTION_END", do_transaction_end },
[XS_INTRODUCE] = { "INTRODUCE", do_introduce },
@@ -1296,7 +1300,7 @@ static struct {
static const char *sockmsg_string(enum xsd_sockmsg_type type)
{
- if ((unsigned)type < XS_TYPE_COUNT && wire_funcs[type].str)
+ if ((unsigned int)type < ARRAY_SIZE(wire_funcs) && wire_funcs[type].str)
return wire_funcs[type].str;
return "**UNKNOWN**";
@@ -1311,7 +1315,14 @@ static void process_message(struct connection *conn, struct buffered_data *in)
enum xsd_sockmsg_type type = in->hdr.msg.type;
int ret;
- trans = transaction_lookup(conn, in->hdr.msg.tx_id);
+ if ((unsigned int)type >= XS_TYPE_COUNT || !wire_funcs[type].func) {
+ eprintf("Client unknown operation %i", type);
+ send_error(conn, ENOSYS);
+ return;
+ }
+
+ trans = (wire_funcs[type].flags & XS_FLAG_NOTID)
+ ? NULL : transaction_lookup(conn, in->hdr.msg.tx_id);
if (IS_ERR(trans)) {
send_error(conn, -PTR_ERR(trans));
return;
@@ -1320,12 +1331,7 @@ static void process_message(struct connection *conn, struct buffered_data *in)
assert(conn->transaction == NULL);
conn->transaction = trans;
- if ((unsigned)type < XS_TYPE_COUNT && wire_funcs[type].func)
- ret = wire_funcs[type].func(conn, in);
- else {
- eprintf("Client unknown operation %i", type);
- ret = ENOSYS;
- }
+ ret = wire_funcs[type].func(conn, in);
if (ret)
send_error(conn, ret);
--
2.17.1
From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= <edvin.torok@citrix.com>
Subject: tools/ocaml/xenstored: unify watch firing
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This will make it easier insert additional checks in a follow-up patch.
All watches are now fired from a single function.
This is part of XSA-115.
Signed-off-by: Edwin Török <edvin.torok@citrix.com>
Acked-by: Christian Lindig <christian.lindig@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
diff --git a/tools/ocaml/xenstored/connection.ml b/tools/ocaml/xenstored/connection.ml
index 24750ada43..e5df62d9e7 100644
--- a/tools/ocaml/xenstored/connection.ml
+++ b/tools/ocaml/xenstored/connection.ml
@@ -210,8 +210,7 @@ let fire_watch watch path =
end else
path
in
- let data = Utils.join_by_null [ new_path; watch.token; "" ] in
- send_reply watch.con Transaction.none 0 Xenbus.Xb.Op.Watchevent data
+ fire_single_watch { watch with path = new_path }
(* Search for a valid unused transaction id. *)
let rec valid_transaction_id con proposed_id =
From b8c6dbb67ebb449126023446a7d209eedf966537 Mon Sep 17 00:00:00 2001
From: Juergen Gross <jgross@suse.com>
Date: Thu, 11 Jun 2020 16:12:39 +0200
Subject: [PATCH 03/10] tools/xenstore: fix node accounting after failed node
creation
When a node creation fails the number of nodes of the domain should be
the same as before the failed node creation. In case of failure when
trying to create a node requiring to create one or more intermediate
nodes as well (e.g. when /a/b/c/d is to be created, but /a/b isn't
existing yet) it might happen that the number of nodes of the creating
domain is not reset to the value it had before.
So move the quota accounting out of construct_node() and into the node
write loop in create_node() in order to be able to undo the accounting
in case of an error in the intermediate node destructor.
This is part of XSA-115.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Paul Durrant <paul@xen.org>
Acked-by: Julien Grall <jgrall@amazon.com>
---
tools/xenstore/xenstored_core.c | 37 ++++++++++++++++++++++-----------
1 file changed, 25 insertions(+), 12 deletions(-)
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index bb2f9fd4e76e..db9b9ca7957d 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -925,11 +925,6 @@ static struct node *construct_node(struct connection *conn, const void *ctx,
if (!parent)
return NULL;
- if (domain_entry(conn) >= quota_nb_entry_per_domain) {
- errno = ENOSPC;
- return NULL;
- }
-
/* Add child to parent. */
base = basename(name);
baselen = strlen(base) + 1;
@@ -962,7 +957,6 @@ static struct node *construct_node(struct connection *conn, const void *ctx,
node->children = node->data = NULL;
node->childlen = node->datalen = 0;
node->parent = parent;
- domain_entry_inc(conn, node);
return node;
nomem:
@@ -982,6 +976,9 @@ static int destroy_node(void *_node)
key.dsize = strlen(node->name);
tdb_delete(tdb_ctx, key);
+
+ domain_entry_dec(talloc_parent(node), node);
+
return 0;
}
@@ -998,18 +995,34 @@ static struct node *create_node(struct connection *conn, const void *ctx,
node->data = data;
node->datalen = datalen;
- /* We write out the nodes down, setting destructor in case
- * something goes wrong. */
+ /*
+ * We write out the nodes bottom up.
+ * All new created nodes will have i->parent set, while the final
+ * node will be already existing and won't have i->parent set.
+ * New nodes are subject to quota handling.
+ * Initially set a destructor for all new nodes removing them from
+ * TDB again and undoing quota accounting for the case of an error
+ * during the write loop.
+ */
for (i = node; i; i = i->parent) {
- if (write_node(conn, i, false)) {
- domain_entry_dec(conn, i);
+ /* i->parent is set for each new node, so check quota. */
+ if (i->parent &&
+ domain_entry(conn) >= quota_nb_entry_per_domain) {
+ errno = ENOSPC;
return NULL;
}
- talloc_set_destructor(i, destroy_node);
+ if (write_node(conn, i, false))
+ return NULL;
+
+ /* Account for new node, set destructor for error case. */
+ if (i->parent) {
+ domain_entry_inc(conn, i);
+ talloc_set_destructor(i, destroy_node);
+ }
}
/* OK, now remove destructors so they stay around */
- for (i = node; i; i = i->parent)
+ for (i = node; i->parent; i = i->parent)
talloc_set_destructor(i, NULL);
return node;
}
--
2.17.1
From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= <edvin.torok@citrix.com>
Subject: tools/ocaml/xenstored: introduce permissions for special watches
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The special watches "@introduceDomain" and "@releaseDomain" should be
allowed for privileged callers only, as they allow to gain information
about presence of other guests on the host. So send watch events for
those watches via privileged connections only.
Start to address this by treating the special watches as regular nodes
in the tree, which gives them normal semantics for permissions. A later
change will restrict the handling, so that they can't be listed, etc.
This is part of XSA-115.
Signed-off-by: Edwin Török <edvin.torok@citrix.com>
Acked-by: Christian Lindig <christian.lindig@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml
index fd79ef564f..e528d1ecb2 100644
--- a/tools/ocaml/xenstored/process.ml
+++ b/tools/ocaml/xenstored/process.ml
@@ -420,7 +420,7 @@ let do_introduce con _t domains cons data =
else try
let ndom = Domains.create domains domid mfn port in
Connections.add_domain cons ndom;
- Connections.fire_spec_watches cons "@introduceDomain";
+ Connections.fire_spec_watches cons Store.Path.introduce_domain;
ndom
with _ -> raise Invalid_Cmd_Args
in
@@ -439,7 +439,7 @@ let do_release con _t domains cons data =
Domains.del domains domid;
Connections.del_domain cons domid;
if fire_spec_watches
- then Connections.fire_spec_watches cons "@releaseDomain"
+ then Connections.fire_spec_watches cons Store.Path.release_domain
else raise Invalid_Cmd_Args
let do_resume con _t domains _cons data =
diff --git a/tools/ocaml/xenstored/store.ml b/tools/ocaml/xenstored/store.ml
index 92b6289b5e..52b88b3ee1 100644
--- a/tools/ocaml/xenstored/store.ml
+++ b/tools/ocaml/xenstored/store.ml
@@ -214,6 +214,11 @@ let rec lookup node path fct =
let apply rnode path fct =
lookup rnode path fct
+
+let introduce_domain = "@introduceDomain"
+let release_domain = "@releaseDomain"
+let specials = List.map of_string [ introduce_domain; release_domain ]
+
end
(* The Store.t type *)
diff --git a/tools/ocaml/xenstored/utils.ml b/tools/ocaml/xenstored/utils.ml
index b252db799b..e8c9fe4e94 100644
--- a/tools/ocaml/xenstored/utils.ml
+++ b/tools/ocaml/xenstored/utils.ml
@@ -88,19 +88,17 @@ let read_file_single_integer filename =
Unix.close fd;
int_of_string (Bytes.sub_string buf 0 sz)
-let path_complete path connection_path =
- if String.get path 0 <> '/' then
- connection_path ^ path
- else
- path
-
+(* @path may be guest data and needs its length validating. @connection_path
+ * is generated locally in xenstored and always of the form "/local/domain/$N/" *)
let path_validate path connection_path =
- if String.length path = 0 || String.length path > 1024 then
- raise Define.Invalid_path
- else
- let cpath = path_complete path connection_path in
- if String.get cpath 0 <> '/' then
- raise Define.Invalid_path
- else
- cpath
+ let len = String.length path in
+
+ if len = 0 || len > 1024 then raise Define.Invalid_path;
+
+ let abs_path =
+ match String.get path 0 with
+ | '/' | '@' -> path
+ | _ -> connection_path ^ path
+ in
+ abs_path
diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml
index 7e7824761b..8d0c50bfa4 100644
--- a/tools/ocaml/xenstored/xenstored.ml
+++ b/tools/ocaml/xenstored/xenstored.ml
@@ -286,6 +286,8 @@ let _ =
let quit = ref false in
Logging.init_xenstored_log();
+ List.iter (fun path ->
+ Store.write store Perms.Connection.full_rights path "") Store.Path.specials;
let filename = Paths.xen_run_stored ^ "/db" in
if cf.restart && Sys.file_exists filename then (
@@ -335,7 +337,7 @@ let _ =
let (notify, deaddom) = Domains.cleanup domains in
List.iter (Connections.del_domain cons) deaddom;
if deaddom <> [] || notify then
- Connections.fire_spec_watches cons "@releaseDomain"
+ Connections.fire_spec_watches cons Store.Path.release_domain
)
else
let c = Connections.find_domain_by_port cons port in
From 318aa75bd0c05423e717ad0b64adb204282025db Mon Sep 17 00:00:00 2001
From: Juergen Gross <jgross@suse.com>
Date: Thu, 11 Jun 2020 16:12:40 +0200
Subject: [PATCH 04/10] tools/xenstore: simplify and rename check_event_node()
There is no path which allows to call check_event_node() without a
event name. So don't let the result depend on the name being NULL and
add an assert() covering that case.
Rename the function to check_special_event() to better match the
semantics.
This is part of XSA-115.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Paul Durrant <paul@xen.org>
---
tools/xenstore/xenstored_watch.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c
index 7dedca60dfd6..f2f1bed47cc6 100644
--- a/tools/xenstore/xenstored_watch.c
+++ b/tools/xenstore/xenstored_watch.c
@@ -47,13 +47,11 @@ struct watch
char *node;
};
-static bool check_event_node(const char *node)
+static bool check_special_event(const char *name)
{