>From 283bb008bdef624d9bb5bb9c8b0ce868f00939ff Mon Sep 17 00:00:00 2001 From: Richard W.M. Jones Date: Thu, 21 Oct 2010 23:05:26 +0100 Subject: [PATCH 3/8] 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. --- fish/alloc.c | 4 +- fish/fish.c | 33 ++++++++++-- fish/guestfish.pod | 26 +++++++++ fish/virt.c | 111 ++++++++++++++++++++++++--------------- regressions/test-guestfish-a.sh | 25 +++++++-- regressions/test-guestfish-d.sh | 23 ++++++-- 6 files changed, 164 insertions(+), 58 deletions(-) diff --git a/fish/alloc.c b/fish/alloc.c index 156fcc0..7799e4e 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 6aadd56..7cb2e71 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 c92953b..4f4e1f0 100644 --- a/fish/guestfish.pod +++ b/fish/guestfish.pod @@ -165,6 +165,9 @@ Displays detailed help on a single command C. 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. + + guestfish --format=raw -a disk.img --format -a another.img + +forces raw format (no auto-detection) for C and reverts to +auto-detection for C. + +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. + =item B<-i> | B<--inspector> Using L code, inspect the disks looking for diff --git a/fish/virt.c b/fish/virt.c index 9c4ce1a..d915d22 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 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 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 or 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; -} diff --git a/regressions/test-guestfish-a.sh b/regressions/test-guestfish-a.sh index 8847b85..887d1aa 100755 --- a/regressions/test-guestfish-a.sh +++ b/regressions/test-guestfish-a.sh @@ -20,9 +20,26 @@ set -e -rm -f test.img +rm -f test.out -truncate -s 10M test.img -../fish/guestfish -a test.img test.out 2>&1 -rm -f test.img +! grep -sq '^add_drive.*format' test.out + +../fish/guestfish -x --format=qcow2 -a /dev/null test.out 2>&1 + +grep -sq '^add_drive.*format:qcow2' test.out + +../fish/guestfish -x --ro --format=qcow2 -a /dev/null test.out 2>&1 + +grep -sq '^add_drive.*readonly:true.*format:qcow2' test.out + +../fish/guestfish -x --format -a /dev/null test.out 2>&1 + +! grep -sq '^add_drive.*format' test.out + +../fish/guestfish -x -a /dev/null --format=qcow2 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 --- a/regressions/test-guestfish-d.sh +++ b/regressions/test-guestfish-d.sh @@ -20,11 +20,11 @@ set -e -rm -f test.img test.xml test.out +rm -f test1.img test2.img test3.img test.xml test.out 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 < test.xml <524288 - + + + + + + + + + + + @@ -48,6 +58,9 @@ EOF ../fish/guestfish -c "test://$cwd/test.xml" --ro -d guest -x \ 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 -- 1.7.3.1