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

[Markus Armbruster]

Jeff Cody <address@hidden> writes:

> On Tue, Oct 28, 2014 at 12:56:37PM -0600, Eric Blake wrote:
>> 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.
>>
>
> True, but the warning doesn't prevent the probe.  My thinking is that
> if I am running 'qemu-img info' without specifying a format, I
> explicitly want the probe (how else to determine the format of a .img
> file, or other generic file/device?)
>
> But I am not hung up on this; a warning won't negate the usefulness of
> 'qemu-img info', so if others feel it is useful in that usage case, it
> is OK with me.

As far as I can tell, "qemu-img info" doesn't probe the backing file.

I'd prefer not to warn there.

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

Have you considered the "warn of future change" role?

> It'd also be nice if there was something that indicated the tree depth
> the warning was from - it may be confusing for the user if they run a
> qemu command on 'image.qcow2', and get a warning because a backing
> file image in the chain just had a generic '.img' extension.

This is how it looks now:

    qemu: -drive file=flawed.img,if=none: warning: insecure format probing of 
image 'flawed.img'
    To get rid of this warning, specify format=qcow2 explicitly, or change
    the image name to end with '.qcow2'
    qemu: -drive file=flawed.img,if=none: warning: insecure format probing of 
image 'backing.img'
    To get rid of this warning, specify format=qcow2 explicitly, or change
    the image name to end with '.qcow2'

Would be less clear with a differently named backing file.  Could you
sketch what you'd like to see?

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

Probing needs to die.  Leave it to file(1).

[...]