Re: [Qemu-devel] Image probing: how it can be insecure, and what we could do about it

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 06.11.2014 um 13:26 hat Markus Armbruster geschrieben:
>> >> * Reuse the image *without* specifying the raw format.  QEMU guesses the
>> >>    format based on untrusted image contents.  Now QEMU guesses a format
>> >>    chosen by the guest, with meta-data chosen by the guest.  By
>> >>    controlling image meta-data, the malicious guest can access arbitrary
>> >>    files as QEMU, enlarge its storage, and more.  A non-malicious guest
>> >>    can accidentally DoS itself, by writing a pattern probing recognizes.
>> >
>> > Thank you for bringing that to my attention. This means that I'm even
>> > more in favor of using Kevin's patches because in fact they don't
>> > break anything.
>> 
>> They break things differently.  The difference may or may not matter.
>> 
>> Example: innocent guest writes a recognized pattern.
>> 
>>   Now: next restart fails, guest DoSed itself until host operator gets
>>   around to adding format=raw to the configuration.  Consequence:
>>   downtime (probably lengthy), but no data corruption.
>
> Depends.
>
> Another possible outcome is that the guest doesn't DoS itself, but
> writes a valid image header. You've argued before that a guest could be
> keeping a qcow2 image on a whole disk for whatever odd reason. In this
> case, the qemu restart will present the nested image instead of the
> top-level one to the guest, which can probably be labelled corruption.

Point taken.

>>   With Kevin's patch: write fails, guest may or may not handle the
>>   failure gracefully.  Consequences can range from "guest complains to
>>   its logs (who cares)" via "guest stops whatever it's doing and refuses
>>   to continue until its hardware gets fixed (downtime as above)" to
>>   "data corruption".
>> 
>> Example: innocent guest first writes a recognized pattern, then
>> overwrites it with a non-recognized pattern.
>> 
>>   Now: works.
>> 
>>   With Kevin's patch: as above.
>> 
>> Again, I'm not claiming the differences are serious in practice, only
>> that they exist.
>
> Yes, the failure scenario is different. The point still stands that if
> it were relevant in practice, we would likely have heard of it before.

I grant you unlikely.  But I abhor even the unlikeliest data corruptors.
So let's not argue about likelihood, let's concentrate on "it doesn't
really add risk, it mostly changes existing breakage".  That's a point I
can take.

We could then argue whether the changes are for the better.  However, if
we can agree to provide a global "insecure probing on/off" switch, my
interest in arguing these details declines sharply.

>> > You actually want to completely abolish probing? I thought we just
>> > wanted to use the filename as a second source of information and warn
>> > if the contents and the extension don't seem to match; and in the
>> > future, maybe make it an error (but we don't have to discuss that
>> > second part now, I think).
>> 
>> Yes, I propose to ditch it completely, after a suitable grace period.  I
>> tried to make that quite clear in my PATCH RFC 2/2.
>> 
>> Here's why.
>> 
>> Now: 1. probe
>>      4. open, error out if meta-data is bad
>> 
>> With my proposed patch:
>>      1. probe
>>      2. guess from trusted meta-data
>>      3. warn unless 1 and 2 match
>>      4. open, error out if meta-data is bad
>> 
>> Now make the warning an error:
>>      1. probe
>>      2. guess from trusted meta-data
>>      3. error out unless 1 and 2 match
>>      4. open, error out if meta-data is bad
>> 
>> I figure the following is equivalent, but simpler:
>> 
>>      2. guess from trusted meta-data
>>      4. open, error out if meta-data is bad
>> 
>> because open should detect all the errors the previous step 3 caught.
>> If not, things are broken with explicit format=.
>
> Not entirely true, see below.
>
>> > My conclusion: Don't ditch probing. It increases entropy, why would
>> > you ditch probing? Just combine it with the extension and if both
>> > don't seem to match, that's an error.
>> 
>> How does probing add value?
>> 
>> Compare
>> 
>>      1. probe
>>      2. guess from trusted meta-data
>>      3. error out unless 1 and 2 match
>>      4. open, error out if meta-data is bad
>> 
>> to just
>> 
>>      2. guess from trusted meta-data
>>      4. open, error out if meta-data is bad
>> 
>> Let P be the driver recommended by probe, and G be the driver
>> recommended by guess.
>
> P = qcow2, G = raw
>
>> If P == G, same result: we open with the same driver.
>
> No, they are not the same.
>
>> Else, if open with G fails, equivalent result: error out in step 3
>> vs. error out in step 4.
>
> No, raw accepts the image.
>
>> Else, we have an odd case: one driver's probe accepts (P's), yet a
>> different driver's open succeeds (G's).
>> 
>>     If G's probe rejects: is this a bug?  Shouldn't open always fail
>>     when probe rejects?
>
> No, raw's probe doesn't reject, it just has a very low score.
>
>>     If G's probe accepts, then probing chose P over G.  Maybe it
>>     shouldn't have.  Or maybe the probing functions are imprecise.
>>     Anyway, keeping probing around makes this an error.  Should it be
>>     one?
>
> Yes, raw being the fallback for everything is imprecise.

Okay, that's what I missed, thanks.

>                                                          It's the only
> way we have for probing raw.

I don't want to call it a probe, since it doesn't actually probe
anything.  But that's semantics.

>> Am I missing something?
>
> This is the safety measure that was missing in your proposal, against
> corruption caused by a qcow2 image stored in foo.img that is now
> unintentionally opened as raw.

> Same thing probably for qcow2 stored on LVs etc.

The transition period is a safety measure.  In this period, any usage
that is liable to be interpreted differently in the future warns.

We can try to do more.  We could refuse to use raw without an explicit
format=raw when any other format probe accepts.

Might be a good idea in general: refuse to use a format without an
explicit format= when any other non-raw format probe accepts.