libxl: Do not trust frontend for channel in getinfo

libxl_device_channel_getinfo needs to examine devices without trusting
frontend-controlled data.  So:

* Use /libxl to find the backend path.
* Parse the backend path to find the backend domid, rather than
  reading it from the frontend.
* Tolerate FRONTEND/tty vanishing.

Note that there is a strange off-by-one error in the computation of
both fe_path and libxl_path in libxl_device_channel_getinfo: the
incoming channel->devid, which is copied to channelinfo->devid, has +1
applied to calculate the frontend path (and, after this patch, the
libxl path).  I.e., the devid passed to libxl_device_channel_getinfo
must be one less than the actual devid for the device being asked
about.

This is actually a bug which mirrors a bug in
libxl__append_channel_list, which fills in the devids of the channel
devices it finds with sequentially increasing numbers starting at 0.

In the usual case channels have real devids starting at 1 (because
there is the console, which is devid 0, but not a channel).  So these
bugs usually cancel out.

We do not address this problem at this time.  This bug does not have
any security implications.

This patch is part of XSA-175.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 6ffb173710f837a0c10f4de1a2e91c1a10b5d01b..2dd2467eb29c7f2fa354a8eddb23c5eabb536eef 100644 (file)
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3946,23 +3946,28 @@ int libxl_device_channel_getinfo(libxl_ctx *ctx, uint32_t domid,
                                  libxl_channelinfo *channelinfo)
 {
     GC_INIT(ctx);
-    char *dompath, *fe_path;
+    char *dompath, *fe_path, *libxl_path;
     char *val;
+    int rc;
 
     dompath = libxl__xs_get_dompath(gc, domid);
     channelinfo->devid = channel->devid;
 
-    fe_path = libxl__sprintf(gc, "%s/device/console/%d", dompath,
-                             channelinfo->devid + 1);
+    fe_path = GCSPRINTF("%s/device/console/%d", dompath,
+                        channelinfo->devid + 1);
+    libxl_path = GCSPRINTF("%s/device/console/%d",
+                           libxl__xs_libxl_path(gc, domid),
+                           channelinfo->devid + 1);
     channelinfo->backend = xs_read(ctx->xsh, XBT_NULL,
-                                   libxl__sprintf(gc, "%s/backend",
-                                   fe_path), NULL);
+                                   GCSPRINTF("%s/backend", libxl_path), NULL);
     if (!channelinfo->backend) {
         GC_FREE;
         return ERROR_FAIL;
     }
-    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/backend-id", fe_path));
-    channelinfo->backend_id = val ? strtoul(val, NULL, 10) : -1;
+    rc = libxl__backendpath_parse_domid(gc, channelinfo->backend,
+                                        &channelinfo->backend_id);
+    if (rc) goto out;
+
     val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/state", fe_path));
     channelinfo->state = val ? strtoul(val, NULL, 10) : -1;
     channelinfo->frontend = xs_read(ctx->xsh, XBT_NULL,
@@ -3980,13 +3985,36 @@ int libxl_device_channel_getinfo(libxl_ctx *ctx, uint32_t domid,
     switch (channel->connection) {
          case LIBXL_CHANNEL_CONNECTION_PTY:
              val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/tty", fe_path));
+             /*
+              * It is obviously very wrong for this value to be in the
+              * frontend.  But in XSA-175 we don't want to re-engineer
+              * this because other xenconsole code elsewhere (some
+              * even out of tree, perhaps) expects this node to be
+              * here.
+              *
+              * FE/pty is readonly for the guest.  It always exists if
+              * FE does because libxl__device_console_add
+              * unconditionally creates it and nothing deletes it.
+              *
+              * The guest can delete the whole FE (which it has write
+              * privilege on) but the containing directories
+              * /local/GUEST[/device[/console]] are also RO for the
+              * guest.  So if the guest deletes FE it cannot recreate
+              * it.
+              *
+              * Therefore the guest cannot cause FE/pty to contain bad
+              * data, although it can cause it to not exist.
+              */
+             if (!val) val = "/NO-SUCH-PATH";
              channelinfo->u.pty.path = strdup(val);
              break;
          default:
              break;
     }
+    rc = 0;
+ out:
     GC_FREE;
-    return 0;
+    return rc;
 }
 
 /******************************************************************************/