Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing

[Markus Armbruster <armbru@redhat.com>]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Wed, Oct 29, 2014 at 02:54:32PM +0100, Markus Armbruster wrote:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 28.10.2014 um 17:03 hat Markus Armbruster geschrieben:
>> >> If the user neglects to specify the image format, QEMU probes the
>> >> image to guess it automatically, for convenience.
>> >> 
>> >> Relying on format probing is insecure for raw images (CVE-2008-2004).
>> >> If the guest writes a suitable header to the device, the next probe
>> >> will recognize a format chosen by the guest.  A malicious guest can
>> >> abuse this to gain access to host files, e.g. by crafting a QCOW2
>> >> header with backing file /etc/shadow.
>> >> 
>> >> Commit 1e72d3b (April 2008) provided -drive parameter format to let
>> >> users disable probing.  Commit f965509 (March 2009) extended QCOW2 to
>> >> optionally store the backing file format, to let users disable backing
>> >> file probing.  QED has had a flag to suppress probing since the
>> >> beginning (2010), set whenever a raw backing file is assigned.
>> >> 
>> >> Despite all this work (and time!), we're still insecure by default.  I
>> >> think we're doing our users a disservice by sticking to the fatally
>> >> flawed probing.  "Broken by default" is just wrong, and "convenience"
>> >> is no excuse.
>> >> 
>> >> I believe we can retain 90% of the convenience without compromising
>> >> security by keying on image file name instead of image contents: if
>> >> the file name ends in .img or .iso, assume raw, if it ends in .qcow2,
>> >> assume qcow2, and so forth.
>> >> 
>> >> Naturally, this would break command lines where the filename doesn't
>> >> provide the correct clue.  So don't do it just yet, only warn if the
>> >> the change would lead to a different result.  Looks like this:
>> >> 
>> >>     qemu: -drive file=my.img: warning: insecure format probing of image 'my.img'
>> >>     To get rid of this warning, specify format=qcow2 explicitly, or change
>> >>     the image name to end with '.qcow2'
>> >> 
>> >> This should steer users away from insecure format probing.  After a
>> >> suitable grace period, we can hopefully drop format probing
>> >> alltogether.
>> >> 
>> >> Example 0: file=WHATEVER,format=F
>> >> 
>> >> Never warns, because the explicit format suppresses probing.
>> >> 
>> >> Example 1: file=FOO.img
>> >> 
>> >> Warns when probing of FOO.img results in anything but raw.  In
>> >> particular, it warns when the guest just p0wned you.
>> >> 
>> >> Example 2: file=FOO.qcow2 with backing file name FOO.img and no
>> >> backing image format.
>> >> 
>> >> Warns when probing of FOO.qcow2 results in anything but qcow2, or
>> >> probing of FOO.img results in anything but raw.
>> >> 
>> >> This patch is RFC because of open questions:
>> >> 
>> >> * Should tools warn, too?  Probing isn't insecure there, but a "this
>> >>   may pick a different format in the future" warning may be
>> >>   appropriate.
>> >> 
>> >> * I didn't specify recognized extensions for formats "bochs", "cloop",
>> >>   "parallels", "vpc", because I have no idea which ones to recognize.
>> >> 
>> >> Additionally, some tests still need to be updated.
>> >> 
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >
>> > I still don't like this approach very much.
>> >
>> > The first of the reasons, and arguably the weakest one, is simply that I
>> > aesthetically dislike to encode semantics in filenames by relying on
>> > filename extensions. Feels like I'm being moved back to DOS times.
>> 
>> It's the least bad solution so far, in my opinion.
>> 
>> For what it's worth, gcc decides what to do with a file based on its
>> file name extension, too.
>> 
>> > The second one, though, is probably a show stopper for me. You assume
>> > that there even is a filename that could have an extension, and that it
>> > is passed in the legacy filename argument instead of the QDict. With your
>> > patches:
>> >
>> > $ qemu-img create -f qcow2 /tmp/test.img 64M
>> > Formatting '/tmp/test.img', fmt=qcow2 size=67108864 encryption=off
>> > cluster_size=65536 lazy_refcounts=off
>> > $ qemu-system-x86_64 -drive file=/tmp/test.img
>> > qemu-system-x86_64: -drive file=/tmp/test.img: warning: insecure
>> > format probing of image '/tmp/test.img'
>> > To get rid of this warning, specify format=qcow2 explicitly, or change
>> > the image name to end with '.qcow2'
>> > $ x86_64-softmmu/qemu-system-x86_64 -drive file.filename=/tmp/test.img
>> > Speicherzugriffsfehler (Speicherabzug geschrieben)
>> >
>> > Oops, dereferencing a NULL pointer. Now, that's surely fixable, but what
>> > I originally wanted to demonstrate is that you won't output a warning
>> > there. Even that would still be fixable, by adding code into raw-posix,
>> > but what do you do with '-drive file.driver=nbd,file.host=localhost'?

You're right, my patch's use of file name is naive.

Related bug:

static int dmg_probe(const uint8_t *buf, int buf_size, const char *filename)
{
    int len;

    if (!filename) {
        return 0;
    }

    len = strlen(filename);
    if (len > 4 && !strcmp(filename + len - 4, ".dmg")) {
        return 2;
    }
    return 0;
}

With -drive if=none,file=foo.dmg the probe succeeds, and the image is
opened with driver bdrv_dmg.  With file.filename=foo.dmg, the probe
fails, and the image is opened with driver bdrv_raw.  Oops.

Two ideas for fixing this come to mind:

* Probing must not use the file name.  Remove the temptation by dropping
  the parameter from bdrv_probe().  Change dmg_probe() to probe the
  image contents instead.  Backward incompatible.  I suspect that could
  be just fine with you in this case ;-P

* Probing may use the file name.  Fix the code to pass the file name to
  ->bdrv_probe() whenever there is one.  There certainly is one when we
  got our file descriptor from open(), i.e. in the simple case.

  Whenever we pass a file name to ->bdrv_probe(), we can just as well
  pass it to bdrv_guess_format(), too.  It must still be prepared for a
  null name.

  There is a file name in your file.filename=/tmp/test.img example.

  There is none in your file.driver=nbd,file.host=localhost' example,
  because there we get the file descriptor from socket().

  If you do fancy things like nbd, you need to specify format= to avoid
  the warning.  I agree with Stefan: it's tolerable price to pay for
  "secure by default".

>> > And how does your patch help for '-drive blkverify:hacked.img:good.img'?

My patch puts "guess format from image file name" right next to "guess
format from image contents" (a.k.a. probing).  Therefore, it runs
*exactly* when we probe an image.

Your example:

    $ qemu-img create -f qcow2 good.img 1m
    Formatting 'good.img', fmt=qcow2 size=1048576 encryption=off cluster_size=65536 lazy_refcounts=off 
    $ cp good.img hacked.img
    $ qemu -S -display none -monitor stdio -drive if=none,file=blkverify:hacked.img:good.img
    qemu: -drive if=none,file=blkverify:hacked.img:good.img: warning: insecure format probing of image 'good.img'
    To get rid of this warning, specify format=qcow2 explicitly, or change
    the image name to end with '.qcow2'
    blkverify: read sector_num=0 nb_sectors=4 contents mismatch in sector 0

Only good.img is probed.  Why?  Let's examine how this thing gets
opened.

First bdrv_open() has
    filename = "blkverify:hacked.img:good.img"
    options = {}
    flags = BDRV_O_RDWR | BDRV_O_CACHE_WB
    drv = NULL

    It calls itself recursively via bdrv_open_image():
        Same parameters, except flags |= BDRV_O_ALLOW_RDWR |
        BDRV_O_UNMAP | BDRV_O_PROTOCOL.

        This parses the file name, and calls itself again, via
        bdrv_open_common(), blkverify_open(), bdrv_open_image(), two
        times.

            First:
                filename = "hacked.img"
                options = {}
                flags = as above

                bdrv_open() changes options to
                driver=file,filename=hacked.img, then opens hacked.img
                with driver "file", i.e. *without* probing.

            Next:
                filename = "good.img"
                options = {}
                flags = as above, less BDRV_O_PROTOCOL

                Once again, this calls itself, via bdrv_open_image():
                    Same parameters, except BDRV_O_PROTOCOL is back
            
                    bdrv_open() changes options to
                    driver=file,filename=good.img, then opens
                    good.img with driver "file", i.e. *without*
                    probing.

                Then it probes the image contents, resulting in driver
                bdrv_qcow2.  My patch's code runs, and emits the
                warning.

                Then it calls bdrv_open_common() to put the qcow2 BDS on
                top of the file BDS.  If it had a backing file, we'd
                call bdrv_open() some more.

        Back in the outermost bdrv_open().  It attempts to probe the
        image contents.  Fails reading the contents, because the two
        children of the blkverify BDSs have different contents: good.img
        is read with qcow2 over file, while hacked.img is read with just
        file.

        If I create hacked.img with

            dd if=/dev/zero of=hacked.img bs=4M count=1

        instead, then the read succeeds, and probing yields bdrv_raw.
        My patch's code runs, guesses bdrv_raw, and doesn't warn.

Quite a jungle.

>> I'll reply to this as soon as I've had time to think.
>
> If you are using fancy command-lines, you need to use format=.
>
> The probing feature is really useful for -hda test.qcow2.  Anything
> fancier requires enough knowledge (and typing on the command-line) that
> format=qcow2 really isn't too much to ask for.
>
>> > Instead, let me try once more to sell my old proposal [1] from the
>> > thread you mentioned:
>> >
>> >> What if we let the raw driver know that it was probed and then it
>> >> enables a check that returns -EIO for any write on the first 2k if that
>> >> write would make the image look like a different format?
>> >
>> > Attacks the problem where it arises instead of trying to detect the
>> > outcome of it, and works in whatever way it is nested in the BDS graph
>> > and whatever way is used to address the image file.
>
> I think this is too clever.  It's another thing to debug if a guest
> starts hitting EIO.
>
> My opinion on probing is: it's ugly but let's leave it for QEMU 3.0 at
> which point we implement Markus solution with exit(1).

I regard my patch as a necessary preliminary step for that.  Warn now,
change behavior a couple of releases later.  When exactly is debatable.

> In the meantime the CVE has been known for a long time so vulnerable
> users (VM hosting, cloud, etc) have the information they need.  Many are
> automatically protected by libvirt.

The warning hopefully helps libvirt developers with keeping libvirt
users fully protected.