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

[Jeff Cody <jcody@redhat.com>]

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.

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

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.

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