[Xen-changelog] [xen stable-4.2] libxl: Restrict permissions on PV console device xenstore nodes

[patchbot@xxxxxxx]

commit 85fac79f83f96e5a626d9d0509c1533c6320ea5c
Author:     Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
AuthorDate: Thu Jun 27 17:25:18 2013 +0100
Commit:     Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
CommitDate: Thu Jun 27 17:25:18 2013 +0100

    libxl: Restrict permissions on PV console device xenstore nodes
    
    Matthew Daley has observed that the PV console protocol places sensitive 
host
    state into a guest writeable xenstore locations, this includes:
    
     - The pty used to communicate between the console backend daemon and its
       client, allowing the guest administrator to read and write arbitrary host
       files.
     - The output file, allowing the guest administrator to write arbitrary host
       files or to target arbitrary qemu chardevs which include sockets, udp, 
ptr,
       pipes etc (see -chardev in qemu(1) for a more complete list).
     - The maximum buffer size, allowing the guest administrator to consume more
       resources than the host administrator has configured.
     - The backend to use (qemu vs xenconsoled), potentially allowing the guest
       administrator to confuse host software.
    
    So we arrange to make the sensitive keys in the xenstore frontend directory
    read only for the guest. This is safe since the xenstore permissions model,
    unlike POSIX directory permissions, does not allow the guest to remove and
    recreate a node if it has write access to the containing directory.
    
    There are a few associated wrinkles:
    
     - The primary PV console is "special". It's xenstore node is not under the
       usual /devices/ subtree and it does not use the customary xenstore state
       machine protocol. Unfortunately its directory is used for other things,
       including the vnc-port node, which we do not want the guest to be able to
       write to. Rather than trying to track down all the possible secondary 
uses
       of this directory just make it r/o to the guest. All newly created
       subdirectories inherit these permissions and so are now safe by default.
    
     - The other serial consoles do use the customary xenstore state machine and
       therefore need write access to at least the "protocol" and "state" nodes,
       however they may also want to use arbitrary "feature-foo" nodes (although
       I'm not aware of any) and therefore we cannot simply lock down the entire
       frontend directory. Instead we add support to libxl__device_generic_add 
for
       frontend keys which are explicitly read only and use that to lock down 
the
       sensitive keys.
    
     - Minios' console frontend wants to write the "type" node, which it has no
       business doing since this is a host/toolstack level decision. This fails
       now that the node has become read only to the PV guest. Since the 
toolstack
       already writes this node just remove the attempt to set it.
    
    This is CVE-2013-2211 / XSA-57
    
    Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
    
    Conflicts:
        tools/libxl/libxl.c (no vtpm, free front_ro on error in
                             libxl__device_console_add)
---
 extras/mini-os/console/xenbus.c |    6 ----
 tools/libxl/libxl.c             |   58 ++++++++++++++++++++++++---------------
 tools/libxl/libxl_device.c      |   32 ++++++++++++++++-----
 tools/libxl/libxl_internal.h    |    7 ++++-
 tools/libxl/libxl_pci.c         |    3 +-
 tools/libxl/libxl_xshelp.c      |   14 ++++++++-
 6 files changed, 80 insertions(+), 40 deletions(-)

diff --git a/extras/mini-os/console/xenbus.c b/extras/mini-os/console/xenbus.c
index 77de82a..e65baf7 100644
--- a/extras/mini-os/console/xenbus.c
+++ b/extras/mini-os/console/xenbus.c
@@ -122,12 +122,6 @@ again:
         goto abort_transaction;
     }
 
-    err = xenbus_printf(xbt, nodename, "type", "%s", "ioemu");
-    if (err) {
-        message = "writing type";
-        goto abort_transaction;
-    }
-
     snprintf(path, sizeof(path), "%s/state", nodename);
     err = xenbus_switch_state(xbt, path, XenbusStateConnected);
     if (err) {
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index a6e9601..32d788a 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1920,8 +1920,9 @@ static void device_disk_add(libxl__egc *egc, uint32_t 
domid,
         flexarray_append(front, disk->is_cdrom ? "cdrom" : "disk");
 
         libxl__device_generic_add(gc, t, device,
-                            libxl__xs_kvs_of_flexarray(gc, back, back->count),
-                            libxl__xs_kvs_of_flexarray(gc, front, 
front->count));
+                                  libxl__xs_kvs_of_flexarray(gc, back, 
back->count),
+                                  libxl__xs_kvs_of_flexarray(gc, front, 
front->count),
+                                  NULL);
 
         rc = libxl__xs_transaction_commit(gc, &t);
         if (!rc) break;
@@ -2633,8 +2634,9 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t 
domid,
     flexarray_append(front, libxl__sprintf(gc,
                                     LIBXL_MAC_FMT, LIBXL_MAC_BYTES(nic->mac)));
     libxl__device_generic_add(gc, XBT_NULL, device,
-                             libxl__xs_kvs_of_flexarray(gc, back, back->count),
-                             libxl__xs_kvs_of_flexarray(gc, front, 
front->count));
+                              libxl__xs_kvs_of_flexarray(gc, back, 
back->count),
+                              libxl__xs_kvs_of_flexarray(gc, front, 
front->count),
+                              NULL);
 
     aodev->dev = device;
     aodev->action = DEVICE_CONNECT;
@@ -2830,7 +2832,7 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t 
domid,
                               libxl__device_console *console,
                               libxl__domain_build_state *state)
 {
-    flexarray_t *front;
+    flexarray_t *front, *ro_front;
     flexarray_t *back;
     libxl__device device;
     int rc;
@@ -2845,6 +2847,11 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t 
domid,
         rc = ERROR_NOMEM;
         goto out;
     }
+    ro_front = flexarray_make(16, 1);
+    if (!ro_front) {
+        rc = ERROR_NOMEM;
+        goto out;
+    }
     back = flexarray_make(16, 1);
     if (!back) {
         rc = ERROR_NOMEM;
@@ -2871,21 +2878,24 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t 
domid,
 
     flexarray_append(front, "backend-id");
     flexarray_append(front, libxl__sprintf(gc, "%d", console->backend_domid));
-    flexarray_append(front, "limit");
-    flexarray_append(front, libxl__sprintf(gc, "%d", LIBXL_XENCONSOLE_LIMIT));
-    flexarray_append(front, "type");
+
+    flexarray_append(ro_front, "limit");
+    flexarray_append(ro_front, libxl__sprintf(gc, "%d", 
LIBXL_XENCONSOLE_LIMIT));
+    flexarray_append(ro_front, "type");
     if (console->consback == LIBXL__CONSOLE_BACKEND_XENCONSOLED)
-        flexarray_append(front, "xenconsoled");
+        flexarray_append(ro_front, "xenconsoled");
     else
-        flexarray_append(front, "ioemu");
-    flexarray_append(front, "output");
-    flexarray_append(front, console->output);
+        flexarray_append(ro_front, "ioemu");
+    flexarray_append(ro_front, "output");
+    flexarray_append(ro_front, console->output);
+    flexarray_append(ro_front, "tty");
+    flexarray_append(ro_front, "");
 
     if (state) {
-        flexarray_append(front, "port");
-        flexarray_append(front, libxl__sprintf(gc, "%"PRIu32, 
state->console_port));
-        flexarray_append(front, "ring-ref");
-        flexarray_append(front, libxl__sprintf(gc, "%lu", state->console_mfn));
+        flexarray_append(ro_front, "port");
+        flexarray_append(ro_front, libxl__sprintf(gc, "%"PRIu32, 
state->console_port));
+        flexarray_append(ro_front, "ring-ref");
+        flexarray_append(ro_front, libxl__sprintf(gc, "%lu", 
state->console_mfn));
     } else {
         flexarray_append(front, "state");
         flexarray_append(front, libxl__sprintf(gc, "%d", 1));
@@ -2894,11 +2904,13 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t 
domid,
     }
 
     libxl__device_generic_add(gc, XBT_NULL, &device,
-                             libxl__xs_kvs_of_flexarray(gc, back, back->count),
-                             libxl__xs_kvs_of_flexarray(gc, front, 
front->count));
+                              libxl__xs_kvs_of_flexarray(gc, back, 
back->count),
+                              libxl__xs_kvs_of_flexarray(gc, front, 
front->count),
+                              libxl__xs_kvs_of_flexarray(gc, ro_front, 
ro_front->count));
     rc = 0;
 out_free:
     flexarray_free(back);
+    flexarray_free(ro_front);
     flexarray_free(front);
 out:
     return rc;
@@ -2982,8 +2994,9 @@ int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid,
     flexarray_append(front, libxl__sprintf(gc, "%d", 1));
 
     libxl__device_generic_add(gc, XBT_NULL, &device,
-                             libxl__xs_kvs_of_flexarray(gc, back, back->count),
-                             libxl__xs_kvs_of_flexarray(gc, front, 
front->count));
+                              libxl__xs_kvs_of_flexarray(gc, back, 
back->count),
+                              libxl__xs_kvs_of_flexarray(gc, front, 
front->count),
+                              NULL);
     rc = 0;
 out_free:
     flexarray_free(back);
@@ -3096,8 +3109,9 @@ int libxl__device_vfb_add(libxl__gc *gc, uint32_t domid, 
libxl_device_vfb *vfb)
     flexarray_append_pair(front, "state", libxl__sprintf(gc, "%d", 1));
 
     libxl__device_generic_add(gc, XBT_NULL, &device,
-                             libxl__xs_kvs_of_flexarray(gc, back, back->count),
-                             libxl__xs_kvs_of_flexarray(gc, front, 
front->count));
+                              libxl__xs_kvs_of_flexarray(gc, back, 
back->count),
+                              libxl__xs_kvs_of_flexarray(gc, front, 
front->count),
+                              NULL);
     rc = 0;
 out_free:
     flexarray_free(front);
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index c3283f1..1c04a21 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -84,11 +84,12 @@ out:
 }
 
 int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
-        libxl__device *device, char **bents, char **fents)
+        libxl__device *device, char **bents, char **fents, char **ro_fents)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     char *frontend_path, *backend_path;
     struct xs_permissions frontend_perms[2];
+    struct xs_permissions ro_frontend_perms[2];
     struct xs_permissions backend_perms[2];
     int create_transaction = t == XBT_NULL;
 
@@ -100,22 +101,37 @@ int libxl__device_generic_add(libxl__gc *gc, 
xs_transaction_t t,
     frontend_perms[1].id = device->backend_domid;
     frontend_perms[1].perms = XS_PERM_READ;
 
-    backend_perms[0].id = device->backend_domid;
-    backend_perms[0].perms = XS_PERM_NONE;
-    backend_perms[1].id = device->domid;
-    backend_perms[1].perms = XS_PERM_READ;
+    ro_frontend_perms[0].id = backend_perms[0].id = device->backend_domid;
+    ro_frontend_perms[0].perms = backend_perms[0].perms = XS_PERM_NONE;
+    ro_frontend_perms[1].id = backend_perms[1].id = device->domid;
+    ro_frontend_perms[1].perms = backend_perms[1].perms = XS_PERM_READ;
 
 retry_transaction:
     if (create_transaction)
         t = xs_transaction_start(ctx->xsh);
     /* FIXME: read frontend_path and check state before removing stuff */
 
-    if (fents) {
+    if (fents || ro_fents) {
         xs_rm(ctx->xsh, t, frontend_path);
         xs_mkdir(ctx->xsh, t, frontend_path);
-        xs_set_permissions(ctx->xsh, t, frontend_path, frontend_perms, 
ARRAY_SIZE(frontend_perms));
+        /* Console 0 is a special case. It doesn't use the regular PV
+         * state machine but also the frontend directory has
+         * historically contained other information, such as the
+         * vnc-port, which we don't want the guest fiddling with.
+         */
+        if (device->kind == LIBXL__DEVICE_KIND_CONSOLE && device->devid == 0)
+            xs_set_permissions(ctx->xsh, t, frontend_path,
+                               ro_frontend_perms, 
ARRAY_SIZE(ro_frontend_perms));
+        else
+            xs_set_permissions(ctx->xsh, t, frontend_path,
+                               frontend_perms, ARRAY_SIZE(frontend_perms));
         xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/backend", frontend_path), 
backend_path, strlen(backend_path));
-        libxl__xs_writev(gc, t, frontend_path, fents);
+        if (fents)
+            libxl__xs_writev_perms(gc, t, frontend_path, fents,
+                                   frontend_perms, ARRAY_SIZE(frontend_perms));
+        if (ro_fents)
+            libxl__xs_writev_perms(gc, t, frontend_path, ro_fents,
+                                   ro_frontend_perms, 
ARRAY_SIZE(ro_frontend_perms));
     }
 
     if (bents) {
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 13fa509..ae96a74 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -516,6 +516,11 @@ _hidden char **libxl__xs_kvs_of_flexarray(libxl__gc *gc, 
flexarray_t *array, int
 /* treats kvs as pairs of keys and values and writes each to dir. */
 _hidden int libxl__xs_writev(libxl__gc *gc, xs_transaction_t t,
                              const char *dir, char **kvs);
+/* as writev but also sets the permissions on each path */
+_hidden int libxl__xs_writev_perms(libxl__gc *gc, xs_transaction_t t,
+                                   const char *dir, char *kvs[],
+                                   struct xs_permissions *perms,
+                                   unsigned int num_perms);
 /* _atonce creates a transaction and writes all keys at once */
 _hidden int libxl__xs_writev_atonce(libxl__gc *gc,
                              const char *dir, char **kvs);
@@ -930,7 +935,7 @@ _hidden int libxl__device_console_add(libxl__gc *gc, 
uint32_t domid,
                                       libxl__domain_build_state *state);
 
 _hidden int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
-        libxl__device *device, char **bents, char **fents);
+        libxl__device *device, char **bents, char **fents, char **ro_fents);
 _hidden char *libxl__device_backend_path(libxl__gc *gc, libxl__device *device);
 _hidden char *libxl__device_frontend_path(libxl__gc *gc, libxl__device 
*device);
 _hidden int libxl__parse_backend_path(libxl__gc *gc, const char *path,
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 48986f3..d373b4d 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -106,7 +106,8 @@ int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
 
     libxl__device_generic_add(gc, XBT_NULL, &device,
                               libxl__xs_kvs_of_flexarray(gc, back, 
back->count),
-                              libxl__xs_kvs_of_flexarray(gc, front, 
front->count));
+                              libxl__xs_kvs_of_flexarray(gc, front, 
front->count),
+                              NULL);
 
 out:
     if (back)
diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
index 52af484..d7eaa66 100644
--- a/tools/libxl/libxl_xshelp.c
+++ b/tools/libxl/libxl_xshelp.c
@@ -41,8 +41,10 @@ char **libxl__xs_kvs_of_flexarray(libxl__gc *gc, flexarray_t 
*array, int length)
     return kvs;
 }
 
-int libxl__xs_writev(libxl__gc *gc, xs_transaction_t t,
-                     const char *dir, char *kvs[])
+int libxl__xs_writev_perms(libxl__gc *gc, xs_transaction_t t,
+                           const char *dir, char *kvs[],
+                           struct xs_permissions *perms,
+                           unsigned int num_perms)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     char *path;
@@ -56,11 +58,19 @@ int libxl__xs_writev(libxl__gc *gc, xs_transaction_t t,
         if (path && kvs[i + 1]) {
             int length = strlen(kvs[i + 1]);
             xs_write(ctx->xsh, t, path, kvs[i + 1], length);
+            if (perms)
+                xs_set_permissions(ctx->xsh, t, path, perms, num_perms);
         }
     }
     return 0;
 }
 
+int libxl__xs_writev(libxl__gc *gc, xs_transaction_t t,
+                     const char *dir, char *kvs[])
+{
+    return libxl__xs_writev_perms(gc, t, dir, kvs, NULL, 0);
+}
+
 int libxl__xs_writev_atonce(libxl__gc *gc,
                             const char *dir, char *kvs[])
 {
--
generated by git-patchbot for /home/xen/git/xen.git#stable-4.2

_______________________________________________
Xen-changelog mailing list
Xen-changelog@xxxxxxxxxxxxx
http://lists.xensource.com/xen-changelog