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

[Markus Armbruster]

Jeff Cody <address@hidden> writes:

> On Fri, Nov 07, 2014 at 04:21:38PM +0100, Markus Armbruster wrote:
[...]
>>                let me refine / vary the hybrid approach I mentioned
>> under " Don't guess format from untrusted image contents" some.  I think
>> I can trace some inspiration to Max here.
>> 
>> Say we use trusted meta-data to compute a set of admissible formats, and
>> if the set has multiple members, use probing to pick one of them.
>> 
>> Example: foo.qcow2 -> { qcow2 }, no probing
>> 
>> Example: foo.qcow -> { qcow, qcow2 }, probe to pick one
>> 
>> Likewise for foo.vhdx and foo.vhd.
>> 
>> To ensure this actually knocks out condition (b), all members of the set
>> must have the image contents used by their members' probes within their
>> trust boundary.
>> 
>> Example: { qcow, qcow2 } is fine, because both formats have a header,
>> and each header covers the bytes either probe examines.
>> 
>> Example: { raw } is fine, because there is no probing.
>> 
>> Counterexample: { raw, qcow2 } is not possible, because qcow2 probes
>> outside raw's trusted metadata.
>> 
>> Note my careful wording "contents used by [other] probes".  Right now we
>> simply assume that the first 2048 bytes can be trusted.  This is not
>> obviously the case!  If I remember correctly, you proposed to cut it to
>> 512 bytes, which feels a lot safer, since any sane format probably
>> aligns (untrusted) image contents to at least a 512 byte boundary, but
>> is still theoretically unsound.
>> 
>> PRO and CON like my proposal to guess from trusted meta-data only.  The
>> difference is in what existing usage exactly it breaks.
>> 
>> Can be combined with "refuse to use a format without an explicit format=
>> when any other non-raw format probe accepts", just like everything else
>> proposed so far.
>> 
>
> I think I like this approach, when combined with the above paragraph.
> If I understand it correctly, it means it should also help protect a
> bit against incorrectly detecting raw.

The protection is actually strong: it "detects" raw only when the
trusted meta-data makes raw the only option.  Required to satisfy
condition (b), because if raw is an option, then the *entire* raw image
contents is untrusted, and probing it violates "(b) Don't guess format
from untrusted image contents".

> Looking at extension '.vhd':
>
> foo.vhd -> {vpc, vhdx}
>
> In practice, when probing by contents, we could get these results:
>
> image         | probe result
> ------------------------------
> vhdx               vhdx
> dynamic vhd        vpc
> fixed vhd          raw
>
> If the results are either vhdx or vpc, then that is unambiguous and
> OK.  If the result is raw, we would refuse to open it, because it is
> not in the accepted set of formats for type '.vhd'.

Correct.

> If we look at a generic extension, '.img':
>
> foo.img -> { raw, qcow2, qcow2, vhdx, vpc, qed, vmdk, parallels, unsupported }
>
> If foo.img probe returned qcow2, we would fail, because { raw, qcow2 }
> would both be valid.
>
> But if probe is negative, and returns raw, it is still unknown,
> because could mean {raw, vpc, unsupported}.
>
> So that would mean .img would always require format=, right?
>
> That also implies to me that the only extensions for raw that might
> not require format= would be .iso and .raw.

.img means what we choose it to mean.

If we choose "can mean anything, including raw", then .img always
requires an explicit format with this approach.

If we choose "means raw", then same as above, except you can omit
format=raw, and you become prone to opening existing non-raw formats
raw, which can be bad.

Tradeoff.

[...]