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

[Markus Armbruster]

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.

> Third, this is only a warning. In this form it doesn't help you much.
> You only see the message when it's already too late, and you might not
> see it at all because it can disappear somewhere deep in log files if
> there is some scripting around qemu (we're only talking about
> non-libvirt cases here anyway because libvirt always is explicit about
> the format).
>
> I know that your commit message says that you want to make it an error
> eventually, but we all know qemu's policies regarding compatibility, so:
> No, it won't happen, because there is no way to make it compatible.

I disagree.  "Backwards compatibility über alles" would be idiotic
policy, like pretty much every "über alles" is.  Letting backwards
compatibility win over making things secure by default has brought a
neverending chain of CVEs and bugs upon us.  Eric listed some.

No matter what we do, we're inconveniencing some users.

If we fix the user interface design flaw, some existing usage will
break.

If we do nothing, we expose users to a security risk they actively need
to opt out of.  Even the users who know that very well don't get it
right every time.  See again Eric's message for the libvirt-related CVEs
and bugs.

I'm arguing for limiting the inconvenience in scope and in time.  Time
means we actually fix the stupid thing instead of dragging it along
forever.  Scope means we try to keep as much usage as possible
unchanged.

> 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.
>
> Kevin
>
> [1] https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg02548.html

Arbitrarily breaking the virtual hardware that way is incompatible, too.
It's a bigger no-no for me than changing user interface corner cases or
deciding what to do with a file based on its file name extension.

Anthony tried something similar (commit 79368c8), but couldn't get it
right (commit 8b33d9e).