From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= <edvin.torok@citrix.com>
Subject: tools/ocaml/xenstored: do permission checks on xenstore root
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This was lacking in a disappointing number of places.

The xenstore root node is treated differently from all other nodes, because it
doesn't have a parent, and mutation requires changing the parent.

Unfortunately this lead to open-coding the special case for root into every
single xenstore operation, and out of all the xenstore operations only read
did a permission check when handling the root node.

This means that an unprivileged guest can:

 * xenstore-chmod / to its liking and subsequently write new arbitrary nodes
   there (subject to quota)
 * xenstore-rm -r / deletes almost the entire xenstore tree (xenopsd quickly
   refills some, but you are left with a broken system)
 * DIRECTORY on / lists all children when called through python
   bindings (xenstore-ls stops at /local because it tries to list recursively)
 * get-perms on / works too, but that is just a minor information leak

Add the missing permission checks, but this should really be refactored to do
the root handling and permission checks on the node only once from a single
function, instead of getting it wrong nearly everywhere.

This is XSA-353.

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/store.ml b/tools/ocaml/xenstored/store.ml
index f299ec6461..92b6289b5e 100644
--- a/tools/ocaml/xenstored/store.ml
+++ b/tools/ocaml/xenstored/store.ml
@@ -273,15 +273,17 @@ let path_rm store perm path =
 			Node.del_childname node name
 		with Not_found ->
 			raise Define.Doesnt_exist in
-	if path = [] then
+	if path = [] then (
+		Node.check_perm store.root perm Perms.WRITE;
 		Node.del_all_children store.root
-	else
+	) else
 		Path.apply_modify store.root path do_rm
 
 let path_setperms store perm path perms =
-	if path = [] then
+	if path = [] then (
+		Node.check_perm store.root perm Perms.WRITE;
 		Node.set_perms store.root perms
-	else
+	) else
 		let do_setperms node name =
 			let c = Node.find node name in
 			Node.check_owner c perm;
@@ -313,9 +315,10 @@ let read store perm path =
 
 let ls store perm path =
 	let children =
-		if path = [] then
-			(Node.get_children store.root)
-		else
+		if path = [] then (
+			Node.check_perm store.root perm Perms.READ;
+			Node.get_children store.root
+		) else
 			let do_ls node name =
 				let cnode = Node.find node name in
 				Node.check_perm cnode perm Perms.READ;
@@ -324,9 +327,10 @@ let ls store perm path =
 	List.rev (List.map (fun n -> Symbol.to_string n.Node.name) children)
 
 let getperms store perm path =
-	if path = [] then
-		(Node.get_perms store.root)
-	else
+	if path = [] then (
+		Node.check_perm store.root perm Perms.READ;
+		Node.get_perms store.root
+	) else
 		let fct n name =
 			let c = Node.find n name in
 			Node.check_perm c perm Perms.READ;