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

[Max Reitz]

On 2014-10-29 at 08:36, Markus Armbruster wrote:
> 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.

Except for when you're using --backing-chain.

I don't really see the point in warning because qemu-img acts with the 
privileges of the invoking user and only passes data to that user, so 
there should not be any security issues here. However, we may want to 
warn anyway just so the user knows that he/she should rename the image file.

So for me it comes down to what is easier, and I think just always 
emitting the warning is easier.

Max

> > > 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).
>
> [...]