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.

diff --git a/fish/alloc.c b/fish/alloc.c
index 156fcc0..7799e4e 100644 (file)
--- 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 (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;
     }
       unlink (filename);
       return -1;
     }

diff --git a/fish/fish.c b/fish/fish.c
index 6aadd56..7cb2e71 100644 (file)
--- a/fish/fish.c
+++ b/fish/fish.c
@@ -51,6 +51,7 @@ struct drv {
   union {
     struct {
       char *filename;       /* disk filename */
   union {
     struct {
       char *filename;       /* disk filename */

+      const char *format;   /* format (NULL == autodetect) */

     } a;
     struct {
       char *guest;          /* guest name */
     } 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"
              "  -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"
              "  -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' },
     { "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 },
     { "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;
   struct mp *mps = NULL;
   struct mp *mp;
   char *p, *file = NULL;

+  const char *format = NULL;

   int c;
   int option_index;
   struct sigaction sa;
   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;
         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);
       } 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->type = drv_a;
       drv->a.filename = optarg;

+      drv->a.format = format;

       drv->next = drvs;
       drvs = drv;
       break;
       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->type = drv_a;
         drv->a.filename = argv[optind];

+        drv->a.format = NULL;

         drv->next = drvs;
         drvs = drv;
       } else {                  /* simulate -d option */
         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;
 add_drives (struct drv *drv, char next_drive)
 {
   int r;

+  struct guestfs_add_drive_opts_argv ad_optargs;

 
   if (next_drive > 'z') {
     fprintf (stderr,
 
   if (next_drive > 'z') {
     fprintf (stderr,


@@ -644,10 +656,16 @@ add_drives (struct drv *drv, char next_drive)
 
     switch (drv->type) {
     case drv_a:
 
     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);
 
       if (r == -1)
         exit (EXIT_FAILURE);
 


@@ -663,6 +681,11 @@ add_drives (struct drv *drv, char next_drive)
       break;
 
     case drv_N:
       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)
       /* -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) {
   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);
   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 c92953b..4f4e1f0 100644 (file)
--- 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.
 
 
 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
 =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
 
 
  #!/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
 =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 9c4ce1a..d915d22 100644 (file)
--- a/fish/virt.c
+++ b/fish/virt.c
@@ -32,8 +32,6 @@
 
 #include "fish.h"
 
 
 #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
 /* 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;
   }
 
     LIBXML_TEST_VERSION;
   }
 

-  int r = -1, nr_added = 0;
+  int r = -1, nr_added = 0, i;

   virErrorPtr err;
   virConnectPtr conn = NULL;
   virDomainPtr dom = NULL;
   virErrorPtr err;
   virConnectPtr conn = NULL;
   virDomainPtr dom = NULL;


@@ -121,25 +119,81 @@ add_libvirt_drives (const char *guest)
     goto cleanup;
   }
 
     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;
   }
 
   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"),
 
   if (nr_added == 0) {
     fprintf (stderr, _("guestfish: libvirt domain '%s' has no disks\n"),


@@ -160,32 +214,3 @@ cleanup:
 
   return r;
 }
 
   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;
-}

diff --git a/regressions/test-guestfish-a.sh b/regressions/test-guestfish-a.sh
index 8847b85..887d1aa 100755 (executable)
--- a/regressions/test-guestfish-a.sh
+++ b/regressions/test-guestfish-a.sh
@@ -20,9 +20,26 @@
 
 set -e
 
 
 set -e
 

-rm -f test.img
+rm -f test.out

 
 

-truncate -s 10M test.img
-../fish/guestfish -a test.img </dev/null
+../fish/guestfish -x -a /dev/null </dev/null >test.out 2>&1

 
 

-rm -f test.img
+! grep -sq '^add_drive.*format' test.out
+
+../fish/guestfish -x --format=qcow2 -a /dev/null </dev/null >test.out 2>&1
+
+grep -sq '^add_drive.*format:qcow2' test.out
+
+../fish/guestfish -x --ro --format=qcow2 -a /dev/null </dev/null >test.out 2>&1
+
+grep -sq '^add_drive.*readonly:true.*format:qcow2' test.out
+
+../fish/guestfish -x --format -a /dev/null </dev/null >test.out 2>&1
+
+! grep -sq '^add_drive.*format' test.out
+
+../fish/guestfish -x -a /dev/null --format=qcow2 </dev/null >test.out 2>&1
+
+! grep -sq '^add_drive.*format' test.out
+
+rm -f test.out

diff --git a/regressions/test-guestfish-d.sh b/regressions/test-guestfish-d.sh
index bf5d514..be20748 100755 (executable)
--- a/regressions/test-guestfish-d.sh
+++ b/regressions/test-guestfish-d.sh
@@ -20,11 +20,11 @@
 
 set -e
 
 
 set -e
 

-rm -f test.img test.xml test.out
+rm -f test1.img test2.img test3.img test.xml test.out

 
 cwd="$(pwd)"
 
 
 cwd="$(pwd)"
 

-truncate -s 10M test.img
+truncate -s 1M test1.img test2.img test3.img

 
 # Libvirt test XML, see libvirt.git/examples/xml/test/testnode.xml
 cat > test.xml <<EOF
 
 # Libvirt test XML, see libvirt.git/examples/xml/test/testnode.xml
 cat > test.xml <<EOF


@@ -38,9 +38,19 @@ cat > test.xml <<EOF
     <memory>524288</memory>
     <devices>
       <disk type="file">
     <memory>524288</memory>
     <devices>
       <disk type="file">

-        <source file="$cwd/test.img"/>
+        <source file="$cwd/test1.img"/>

         <target dev="hda"/>
       </disk>
         <target dev="hda"/>
       </disk>

+      <disk type="file">
+        <driver name="qemu" type="raw"/>
+        <source file="$cwd/test2.img"/>
+        <target dev="hdb"/>
+      </disk>
+      <disk type="file">
+        <driver name="qemu" type="qcow2"/>
+        <source file="$cwd/test3.img"/>
+        <target dev="hdc"/>
+      </disk>

     </devices>
   </domain>
 </node>
     </devices>
   </domain>
 </node>


@@ -48,6 +58,9 @@ EOF
 
 ../fish/guestfish -c "test://$cwd/test.xml" --ro -d guest -x \
   </dev/null >test.out 2>&1
 
 ../fish/guestfish -c "test://$cwd/test.xml" --ro -d guest -x \
   </dev/null >test.out 2>&1

-grep -sq '^add_drive_ro.*test.img' test.out
+grep -sq '^add_drive.*test1.img.*readonly:true' test.out
+! grep -sq '^add_drive.*test1.img.*format' test.out
+grep -sq '^add_drive.*test2.img.*readonly:true.*format:raw' test.out
+grep -sq '^add_drive.*test3.img.*readonly:true.*format:qcow2' test.out

 
 

-rm -f test.img test.xml test.out
+rm -f test1.img test2.img test3.img test.xml test.out