[Xen-devel] [PATCH] QEMU "drive_init()" Disk Format Security Bypass

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Thanks to everyone for their comments.  It turns out that the
 file:/path/to/image
syntax doesn't work properly at the moment unless the image is a raw
image: although qemu-dm will treat it as a cow image, if it is in a
format it understands, blktap will not.  This means that the guest
would see the processed cow version via the emulated IDE controller
but the raw cow differences file, header and all, via the PV block
interface.

So it is fine for us to make  file:/path/to/image  always expect raw
images.   Non-raw images are done with  tap:qcow:/path/to/image
and that currently works.

Below is a patch, with suggested commit message.

The resulting code in xenstore.c is sadly not very simple.  This is
because it is untangling a mess made (entirely pointlessly) by xend,
as well as translating between the different naming schemes used by
xenstore and qemu for image formats.  I hope to remove this block
driver special case machinery from qemu as part of or shortly after I
complete my merge with qemu upstream.


ioemu: fix disk format security vulnerability

* make the xenstore reader in qemu-dm's startup determine which
  of qemu's block drivers to use according to the xenstore
  backend `type' field.  This `type' field typically comes from
  the front of the drive mapping string in ioemu.  The
  supported cases are:
    xm config file string      `type'  image format    qemu driver
     phy:[/dev/]<device>        phy     raw image       bdrv_raw
     file:<filename>            file    raw image       bdrv_raw
     tap:aio:<filename>         tap     raw image       bdrv_raw
     tap:qcow:<image>           tap     not raw         autoprobe
     tap:<cow-fmt>:<image>      tap     named format    bdrv_<cow-fmt>
  It is still necessary to autoprobe when the image is specified as
  `tap:qcow:<image>', because qemu distinguishes `qcow' and `qcow2'
  whereas blktap doesn't; `qcow' in xenstore typically means what
  qemu calls qcow2.  This is OK because qemu can safely distinguish
  the different cow formats provided we know it's not a raw image.

* Make the format autoprobing machinery never return `raw'.  This has
  two purposes: firstly, it arranges that the `tap:qcow:...' case
  above can be handled without accidentally falling back to raw
  format.  Secondly it prevents accidents in case the code changes in
  future: autoprobing will now always fail on supposed cow files which
  actually contain junk, rather than giving the guest access to the
  underlying file.

Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>


Ian.

diff -r c99a88623eda tools/ioemu/block.c
--- a/tools/ioemu/block.c       Thu May 08 14:33:31 2008 +0100
+++ b/tools/ioemu/block.c       Fri May 09 15:31:46 2008 +0100
@@ -254,7 +254,7 @@ static BlockDriver *find_protocol(const 
 #endif
     p = strchr(filename, ':');
     if (!p)
-        return &bdrv_raw;
+        return NULL; /* do not ever guess raw, it is a security problem! */
     len = p - filename;
     if (len > sizeof(protocol) - 1)
         len = sizeof(protocol) - 1;
diff -r c99a88623eda tools/ioemu/xenstore.c
--- a/tools/ioemu/xenstore.c    Thu May 08 14:33:31 2008 +0100
+++ b/tools/ioemu/xenstore.c    Fri May 09 16:32:43 2008 +0100
@@ -90,6 +90,7 @@ void xenstore_parse_domain_config(int hv
     int i, is_scsi, is_hdN = 0;
     unsigned int len, num, hd_index, pci_devid = 0;
     BlockDriverState *bs;
+    BlockDriver *format;
 
     for(i = 0; i < MAX_DISKS + MAX_SCSI_DISKS; i++)
         media_filename[i] = NULL;
@@ -135,6 +136,8 @@ void xenstore_parse_domain_config(int hv
     }
         
     for (i = 0; i < num; i++) {
+       format = NULL; /* don't know what the format is yet */
+
         /* read the backend path */
         if (pasprintf(&buf, "%s/device/vbd/%s/backend", path, e[i]) == -1)
             continue;
@@ -181,13 +184,20 @@ void xenstore_parse_domain_config(int hv
         drv = xs_read(xsh, XBT_NULL, buf, &len);
         if (drv == NULL)
             continue;
-        /* Strip off blktap sub-type prefix aio: - QEMU can autodetect this */
+        /* Obtain blktap sub-type prefix */
         if (!strcmp(drv, "tap") && params[0]) {
             char *offset = strchr(params, ':'); 
             if (!offset)
                 continue ;
+           free(drv);
+           drv = malloc(offset - params + 1);
+           memcpy(drv, params, offset - params);
+           drv[offset - params] = '\0';
+           if (!strcmp(drv, "aio"))
+               /* qemu does aio anyway if it can */
+               format = &bdrv_raw;
             memmove(params, offset+1, strlen(offset+1)+1 );
-            fprintf(logfile, "Strip off blktap sub-type prefix to %s\n", 
params); 
+            fprintf(logfile, "Strip off blktap sub-type prefix to %s (drv 
'%s')\n", params, drv); 
         }
         /* Prefix with /dev/ if needed */
         if (!strcmp(drv, "phy") && params[0] != '/') {
@@ -195,6 +205,7 @@ void xenstore_parse_domain_config(int hv
             sprintf(newparams, "/dev/%s", params);
             free(params);
             params = newparams;
+           format = &bdrv_raw;
         }
 
         /* 
@@ -240,8 +251,25 @@ void xenstore_parse_domain_config(int hv
 #endif
 
         if (params[0]) {
-            if (bdrv_open(bs, params, 0 /* snapshot */) < 0)
-                fprintf(stderr, "qemu: could not open vbd '%s' or hard disk 
image '%s'\n", buf, params);
+           if (!format) {
+               if (!drv) {
+                   fprintf(stderr, "qemu: type (image format) not specified 
for vbd '%s' or image '%s'\n", buf, params);
+                   continue;
+               }
+               if (!strcmp(drv,"qcow")) {
+                   /* autoguess qcow vs qcow2 */
+               } else if (!strcmp(drv,"file")) {
+                   format = &bdrv_raw;
+               } else {
+                   format = bdrv_find_format(drv);
+                   if (!format) {
+                       fprintf(stderr, "qemu: type (image format) '%s' unknown 
for vbd '%s' or image '%s'\n", drv, buf, params);
+                       continue;
+                   }
+               }
+           }
+            if (bdrv_open2(bs, params, 0 /* snapshot */, format) < 0)
+                fprintf(stderr, "qemu: could not open vbd '%s' or hard disk 
image '%s' (drv '%s')\n", buf, params, drv ? drv : "?");
         }
     }
 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel