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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3706 bytes --]

On 10/28/2014 12:29 PM, Jeff Cody wrote:

>>> 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.
>>
>> Yes.  For precedent, libvirt can be considered a tool on images for
>> certain operations, and libvirt has been warning about probing since 2010.
>>
> 
> I think at least the invocation 'qemu-img info' should be exempt from
> the warning; doing a format probe is arguably part of its intended
> usage.
> 
>> Also, while I agree that any tool that operates on ONLY one layer of an
>> image, without ever trying to chase backing chains, can't be tricked
>> into opening wrong files, I'm not sure I agree with the claim that
>> "probing isn't insecure" -
>>
> 
> Maybe we should draw the distinction at tools that write data?
> Without a guest running, a tool that simply reads files should be safe
> to probe.

Misprobing a top-level raw file as qcow2 can result in opening and
reading a backing file, even when the top-level file was opened with
read-only intent.  If the guest can stick some sort of /proc filesystem
name as a qcow2 backing file for interpretation for a bogus probe of a
raw file, you can result in hanging the process in trying to read the
backing file.  Even if you aren't leaking data about what was read, this
could still possibly constitute a denial of service attack.

I was about to propose these two rules as something I'd still feel more
comfortable with:

if it is the top-level file, then warn for read-write access doing a
probe where the probe differed from filename heuristics, be silent for
read-only access doing a probe (whether or not the file claims to have a
backing image)

if it is chasing the backing chain (necessarily read-only access of the
backing), then warn if the backing format was not specified and the
probe differs from filename heuristics

But that still has the drawback that if the backing file is some /proc
name that will cause the process to hang, you don't want to print the
message until after you read the file to discover that the probe
differed from heuristics, but it is doing the open/read that determines
the hang.  So I don't see an elegant way to break the chicken-and-egg
problem.


>> What happens if more than one format tends to pick the same extension?
>> For example, would you consider '.qcow' a typical extension for qcow2
>> files, even though it would probably match the older qcow driver first?...
>>
> 
> I think this could arguably end up being the case for VHD and VHDX
> (i.e., both using .vhd).
> 
> I guess the question is, in the case of common extensions, should the
> priority be on the probe, or on the extension?  With the way the code
> is written, the priority is all going to depend on the order the
> driver is registered, so it may or may not warn.

Technically, don't we correctly probe both VHD and VHDX files?  It is
only files that start out raw and later get mis-probed as non-raw where
we have an issue, so I'd rather treat the probe as accurate if it
matches a common extension for that format, and NOT treat the extension
as dictating a single required format.

> 
> Currently, the code does a format probe, does an independent extension
> lookup, and checks if the two agree.  Instead, would it be sufficient
> to do the format probe, and then just verify the detected driver has a
> compatible extension name?

Yes, that was what I was thinking as well.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]