From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41318) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XjCf6-0004pm-IH for qemu-devel@nongnu.org; Tue, 28 Oct 2014 15:42:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XjCey-00046w-Ef for qemu-devel@nongnu.org; Tue, 28 Oct 2014 15:42:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58574) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XjCey-00046s-6n for qemu-devel@nongnu.org; Tue, 28 Oct 2014 15:42:28 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s9SJgRWT018108 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Tue, 28 Oct 2014 15:42:27 -0400 Date: Tue, 28 Oct 2014 15:42:24 -0400 From: Jeff Cody Message-ID: <20141028194224.GA23802@localhost.localdomain> References: <1414512220-19058-1-git-send-email-armbru@redhat.com> <1414512220-19058-3-git-send-email-armbru@redhat.com> <544FCC40.2020008@redhat.com> <20141028182955.GB26767@localhost.localdomain> <544FE6E5.6030605@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <544FE6E5.6030605@redhat.com> Subject: Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, Markus Armbruster , stefanha@redhat.com, qemu-devel@nongnu.org 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. >