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