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

[Kevin Wolf <kwolf@redhat.com>]

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.

>   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.

> > 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. It's the only
way we have for probing raw.

> 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.

Kevin