fish: Specify format of disks (RHBZ#642934,CVE-2010-3851).

For libvirt guests, the disk format is copied from libvirt (if
libvirt knows it).

For command line disk images, you can use --format to override
format auto-detection.

4 files changed, 125 insertions, 49 deletions

diff --git a/fish/alloc.c b/fish/alloc.c
index 156fcc00..7799e4e1 100644
--- a/fish/alloc.c
+++ b/fish/alloc.c
@@ -133,7 +133,9 @@ alloc_disk (const char *filename, const char *size_str, int add, int sparse)
   }
 
   if (add) {
-    if (guestfs_add_drive (g, filename) == -1) {
+    if (guestfs_add_drive_opts (g, filename,
+                                GUESTFS_ADD_DRIVE_OPTS_FORMAT, "raw",
+                                -1) == -1) {
       unlink (filename);
       return -1;
     }
diff --git a/fish/fish.c b/fish/fish.c
index 6aadd560..7cb2e714 100644
--- a/fish/fish.c
+++ b/fish/fish.c
@@ -51,6 +51,7 @@ struct drv {
   union {
     struct {
       char *filename;       /* disk filename */
+      const char *format;   /* format (NULL == autodetect) */
     } a;
     struct {
       char *guest;          /* guest name */
@@ -137,6 +138,7 @@ usage (int status)
              "  -D|--no-dest-paths   Don't tab-complete paths from guest fs\n"
              "  --echo-keys          Don't turn off echo for passphrases\n"
              "  -f|--file file       Read commands from file\n"
+             "  --format[=raw|..]    Force disk format for -a option\n"
              "  -i|--inspector       Automatically mount filesystems\n"
              "  --keys-from-stdin    Read passphrases from stdin\n"
              "  --listen             Listen for remote commands\n"
@@ -183,6 +185,7 @@ main (int argc, char *argv[])
     { "domain", 1, 0, 'd' },
     { "echo-keys", 0, 0, 0 },
     { "file", 1, 0, 'f' },
+    { "format", 2, 0, 0 },
     { "help", 0, 0, HELP_OPTION },
     { "inspector", 0, 0, 'i' },
     { "keys-from-stdin", 0, 0, 0 },
@@ -205,6 +208,7 @@ main (int argc, char *argv[])
   struct mp *mps = NULL;
   struct mp *mp;
   char *p, *file = NULL;
+  const char *format = NULL;
   int c;
   int option_index;
   struct sigaction sa;
@@ -284,6 +288,11 @@ main (int argc, char *argv[])
         override_progress_bars = 0;
       } else if (STREQ (long_options[option_index].name, "echo-keys")) {
         echo_keys = 1;
+      } else if (STREQ (long_options[option_index].name, "format")) {
+        if (!optarg || STREQ (optarg, ""))
+          format = NULL;
+        else
+          format = optarg;
       } else {
         fprintf (stderr, _("%s: unknown long option: %s (%d)\n"),
                  program_name, long_options[option_index].name, option_index);
@@ -303,6 +312,7 @@ main (int argc, char *argv[])
       }
       drv->type = drv_a;
       drv->a.filename = optarg;
+      drv->a.format = format;
       drv->next = drvs;
       drvs = drv;
       break;
@@ -442,6 +452,7 @@ main (int argc, char *argv[])
         }
         drv->type = drv_a;
         drv->a.filename = argv[optind];
+        drv->a.format = NULL;
         drv->next = drvs;
         drvs = drv;
       } else {                  /* simulate -d option */
@@ -632,6 +643,7 @@ static char
 add_drives (struct drv *drv, char next_drive)
 {
   int r;
+  struct guestfs_add_drive_opts_argv ad_optargs;
 
   if (next_drive > 'z') {
     fprintf (stderr,
@@ -644,10 +656,16 @@ add_drives (struct drv *drv, char next_drive)
 
     switch (drv->type) {
     case drv_a:
-      if (!read_only)
-        r = guestfs_add_drive (g, drv->a.filename);
-      else
-        r = guestfs_add_drive_ro (g, drv->a.filename);
+      ad_optargs.bitmask = 0;
+      if (read_only) {
+        ad_optargs.bitmask |= GUESTFS_ADD_DRIVE_OPTS_READONLY_BITMASK;
+        ad_optargs.readonly = 1;
+      }
+      if (drv->a.format) {
+        ad_optargs.bitmask |= GUESTFS_ADD_DRIVE_OPTS_FORMAT_BITMASK;
+        ad_optargs.format = drv->a.format;
+      }
+      r = guestfs_add_drive_opts_argv (g, drv->a.filename, &ad_optargs);
       if (r == -1)
         exit (EXIT_FAILURE);
 
@@ -663,6 +681,11 @@ add_drives (struct drv *drv, char next_drive)
       break;
 
     case drv_N:
+      /* guestfs_add_drive (ie. autodetecting) should be safe here
+       * since we have just created the prepared disk.  At the moment
+       * it will always be "raw" but in a theoretical future we might
+       * create other formats.
+       */
       /* -N option is not affected by --ro */
       r = guestfs_add_drive (g, drv->N.filename);
       if (r == -1)
@@ -701,7 +724,7 @@ free_drives (struct drv *drv)
   free_drives (drv->next);
 
   switch (drv->type) {
-  case drv_a: /* a.filename is optarg, don't free it */ break;
+  case drv_a: /* a.filename and a.format are optargs, don't free them */ break;
   case drv_d: /* d.filename is optarg, don't free it */ break;
   case drv_N:
     free (drv->N.filename);
diff --git a/fish/guestfish.pod b/fish/guestfish.pod
index c92953b6..4f4e1f0b 100644
--- a/fish/guestfish.pod
+++ b/fish/guestfish.pod
@@ -165,6 +165,9 @@ Displays detailed help on a single command C<cmd>.
 
 Add a block device or virtual machine image to the shell.
 
+The format of the disk image is auto-detected.  To override this and
+force a particular format use the I<--format=..> option.
+
 =item B<-c URI> | B<--connect URI>
 
 When used in conjunction with the I<-d> option, this specifies
@@ -198,6 +201,29 @@ scripts, use:
 
  #!/usr/bin/guestfish -f
 
+=item B<--format=raw|qcow2|..> | B<--format>
+
+The default for the I<-a> option is to auto-detect the format of the
+disk image.  Using this forces the disk format for I<-a> options which
+follow on the command line.  Using I<--format> with no argument
+switches back to auto-detection for subsequent I<-a> options.
+
+For example:
+
+ guestfish --format=raw -a disk.img
+
+forces raw format (no auto-detection) for C<disk.img>.
+
+ guestfish --format=raw -a disk.img --format -a another.img
+
+forces raw format (no auto-detection) for C<disk.img> and reverts to
+auto-detection for C<another.img>.
+
+If you have untrusted raw-format guest disk images, you should use
+this option to specify the disk format.  This avoids a possible
+security problem with malicious guests (CVE-2010-3851).  See also
+L</add-drive-opts>.
+
 =item B<-i> | B<--inspector>
 
 Using L<virt-inspector(1)> code, inspect the disks looking for
diff --git a/fish/virt.c b/fish/virt.c
index 9c4ce1ad..d915d220 100644
--- a/fish/virt.c
+++ b/fish/virt.c
@@ -32,8 +32,6 @@
 
 #include "fish.h"
 
-static int add_drives_from_node_set (xmlDocPtr doc, xmlNodeSetPtr nodes);
-
 /* Implements the guts of the '-d' option.
  *
  * Note that we have to observe the '--ro' flag in two respects: by
@@ -56,7 +54,7 @@ add_libvirt_drives (const char *guest)
     LIBXML_TEST_VERSION;
   }
 
-  int r = -1, nr_added = 0;
+  int r = -1, nr_added = 0, i;
   virErrorPtr err;
   virConnectPtr conn = NULL;
   virDomainPtr dom = NULL;
@@ -121,25 +119,81 @@ add_libvirt_drives (const char *guest)
     goto cleanup;
   }
 
-  xpathObj = xmlXPathEvalExpression (BAD_CAST "//devices/disk/source/@dev",
-                                     xpathCtx);
+  /* This gives us a set of all the <disk> nodes. */
+  xpathObj = xmlXPathEvalExpression (BAD_CAST "//devices/disk", xpathCtx);
   if (xpathObj == NULL) {
     fprintf (stderr, _("guestfish: unable to evaluate XPath expression\n"));
     goto cleanup;
   }
 
-  nr_added += add_drives_from_node_set (doc, xpathObj->nodesetval);
+  xmlNodeSetPtr nodes = xpathObj->nodesetval;
+  for (i = 0; i < nodes->nodeNr; ++i) {
+    xmlXPathObjectPtr xpfilename;
+    xmlXPathObjectPtr xpformat;
+
+    /* Change the context to the current <disk> node.
+     * DV advises to reset this before each search since older versions of
+     * libxml2 might overwrite it.
+     */
+    xpathCtx->node = nodes->nodeTab[i];
+
+    /* Filename can be in <source dev=..> or <source file=..> attribute. */
+    xpfilename = xmlXPathEvalExpression (BAD_CAST "./source/@dev", xpathCtx);
+    if (xpfilename == NULL ||
+        xpfilename->nodesetval == NULL ||
+        xpfilename->nodesetval->nodeNr == 0) {
+      xmlXPathFreeObject (xpfilename);
+      xpathCtx->node = nodes->nodeTab[i];
+      xpfilename = xmlXPathEvalExpression (BAD_CAST "./source/@file", xpathCtx);
+      if (xpfilename == NULL ||
+          xpfilename->nodesetval == NULL ||
+          xpfilename->nodesetval->nodeNr == 0) {
+        xmlXPathFreeObject (xpfilename);
+        continue;               /* disk filename not found, skip this */
+      }
+    }
 
-  xmlXPathFreeObject (xpathObj); xpathObj = NULL;
+    assert (xpfilename->nodesetval->nodeTab[0]);
+    assert (xpfilename->nodesetval->nodeTab[0]->type == XML_ATTRIBUTE_NODE);
+    xmlAttrPtr attr = (xmlAttrPtr) xpfilename->nodesetval->nodeTab[0];
+    char *filename = (char *) xmlNodeListGetString (doc, attr->children, 1);
+
+    /* Get the disk format (may not be set). */
+    xpathCtx->node = nodes->nodeTab[i];
+    xpformat = xmlXPathEvalExpression (BAD_CAST "./driver/@type", xpathCtx);
+    char *format = NULL;
+    if (xpformat != NULL &&
+        xpformat->nodesetval &&
+        xpformat->nodesetval->nodeNr > 0) {
+      assert (xpformat->nodesetval->nodeTab[0]);
+      assert (xpformat->nodesetval->nodeTab[0]->type == XML_ATTRIBUTE_NODE);
+      attr = (xmlAttrPtr) xpformat->nodesetval->nodeTab[0];
+      format = (char *) xmlNodeListGetString (doc, attr->children, 1);
+    }
 
-  xpathObj = xmlXPathEvalExpression (BAD_CAST "//devices/disk/source/@file",
-                                     xpathCtx);
-  if (xpathObj == NULL) {
-    fprintf (stderr, _("guestfish: unable to evaluate XPath expression\n"));
-    goto cleanup;
-  }
+    /* Add the disk, with optional format. */
+    struct guestfs_add_drive_opts_argv optargs = { .bitmask = 0 };
+    if (read_only) {
+      optargs.bitmask |= GUESTFS_ADD_DRIVE_OPTS_READONLY_BITMASK;
+      optargs.readonly = read_only;
+    }
+    if (format) {
+      optargs.bitmask |= GUESTFS_ADD_DRIVE_OPTS_FORMAT_BITMASK;
+      optargs.format = format;
+    }
+
+    int t = guestfs_add_drive_opts_argv (g, filename, &optargs);
+
+    xmlFree (filename);
+    xmlFree (format);
+    xmlXPathFreeObject (xpfilename);
+    xmlXPathFreeObject (xpformat);
+
+    if (t == -1)
+      goto cleanup;
 
-  nr_added += add_drives_from_node_set (doc, xpathObj->nodesetval);
+    nr_added++;
+  }
 
   if (nr_added == 0) {
     fprintf (stderr, _("guestfish: libvirt domain '%s' has no disks\n"),
@@ -160,32 +214,3 @@ cleanup:
 
   return r;
 }
-
-static int
-add_drives_from_node_set (xmlDocPtr doc, xmlNodeSetPtr nodes)
-{
-  if (!nodes)
-    return 0;
-
-  int i;
-
-  for (i = 0; i < nodes->nodeNr; ++i) {
-    assert (nodes->nodeTab[i]);
-    assert (nodes->nodeTab[i]->type == XML_ATTRIBUTE_NODE);
-    xmlAttrPtr attr = (xmlAttrPtr) nodes->nodeTab[i];
-
-    char *device = (char *) xmlNodeListGetString (doc, attr->children, 1);
-
-    int r;
-    if (!read_only)
-      r = guestfs_add_drive (g, device);
-    else
-      r = guestfs_add_drive_ro (g, device);
-    if (r == -1)
-      exit (EXIT_FAILURE);
-
-    xmlFree (device);
-  }
-
-  return nodes->nodeNr;
-}