tools/ocaml/xenstored: do permission checks on xenstore root

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 5a8c377603467395f5c1a998bb9eebbb4b9ac806..6375a1c889913043ded6e52fcdc982d7f6627a8c 100644 (file)
--- 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;