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

[Stefan Hajnoczi]

On Wed, Oct 29, 2014 at 02:54:32PM +0100, Markus Armbruster wrote:
> Kevin Wolf <address@hidden> 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 <address@hidden>
> >
> > 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'?
> >
> > And how does your patch help for '-drive blkverify:hacked.img:good.img'?
> 
> 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).

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.

Stefan

pgpyj58TTdJv1.pgp
Description: PGP signature