[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing
From: |
Jeff Cody |
Subject: |
Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing |
Date: |
Tue, 28 Oct 2014 15:42:24 -0400 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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.
>
Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing, Jeff Cody, 2014/10/28
Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing, Fam Zheng, 2014/10/28