Jeff Cody <jc...@redhat.com> 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).
[...]
