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

[Max Reitz]

On 2014-10-28 at 17:03, Markus Armbruster wrote:
> 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>
> ---
>   block.c                   | 46 ++++++++++++++++++++++++++++++++++++++++++++--
>   block/dmg.c               |  1 +
>   block/qcow.c              |  1 +
>   block/qcow2.c             |  1 +
>   block/qed.c               |  1 +
>   block/raw_bsd.c           |  1 +
>   block/vdi.c               |  1 +
>   block/vhdx.c              |  1 +
>   block/vmdk.c              |  1 +
>   include/block/block_int.h |  1 +
>   10 files changed, 53 insertions(+), 2 deletions(-)


So I guess it's my turn to give yet another opinion (or just something 
in between of what has been already said).

First, I'm fine with this patch, or at least the idea as there were yet 
some quirks.

But I oppose turning the warning into an error eventually. I want to be 
able to use -hda $filename and it should work. As Kevin said, a warning 
alone will not help a lot, though. I don't know about that, I think it 
should work. This warning should only appear when you're using qemu 
directly because management tools should already use -drive with format= 
or driver=; and if you're using qemu directly you should be watching stderr.

The only way I'd be fine with turning this into an error would be to 
make an exception for -hda and the like. There were already plans of 
introducing pseudo block drivers for format and protocol probing; if we 
do that we can use those block drivers for -hda. We should still emit 
the warning, but in my opinion it should never be an error with -hda, 
-cdrom etc.. For me, this is not about backwards compatibility but 
because I'm using -hda myself.

On the other hand, I'm very fine with Kevin's proposal. Bootloaders 
normally don't change that often and there's not that much variety 
(aside from hand-written hobby stuff) so we should be able to regularly 
make sure that nothing breaks. I'd like to increase the entropy, though, 
so we should take another look into the block drivers' probing functions 
and make them more specific, if possible. I don't feel like testing four 
bytes is enough (although in practice it should be).

Also, I like Kevin's proposal/Anthony's approach a lot more because of 
its principle. If a guest can overwrite the beginning of the image so it 
looks like an image format, that's the real bug. Afterwards, anyone will 
recognize that image as non-raw and they'd be correct.

Max