From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53977) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XlDgB-0003LO-HN for qemu-devel@nongnu.org; Mon, 03 Nov 2014 04:12:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XlDg1-00078H-H8 for qemu-devel@nongnu.org; Mon, 03 Nov 2014 04:12:03 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58336) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XlDg1-00076k-9u for qemu-devel@nongnu.org; Mon, 03 Nov 2014 04:11:53 -0500 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 sA39BqSj014463 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Mon, 3 Nov 2014 04:11:52 -0500 Message-ID: <545746D5.3070808@redhat.com> Date: Mon, 03 Nov 2014 10:11:49 +0100 From: Max Reitz MIME-Version: 1.0 References: <1414512220-19058-1-git-send-email-armbru@redhat.com> <1414512220-19058-3-git-send-email-armbru@redhat.com> <5452001E.9070907@redhat.com> <20141030092722.GB30746@stefanha-thinkpad.redhat.com> <20141030093635.GB9097@noname.str.redhat.com> <20141031112423.GE10332@stefanha-thinkpad.redhat.com> <20141031115639.GD4496@noname.str.redhat.com> <878ujs1x6y.fsf@blackfin.pond.sub.org> In-Reply-To: <878ujs1x6y.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit 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: Markus Armbruster , Kevin Wolf Cc: jcody@redhat.com, qemu-devel@nongnu.org, Stefan Hajnoczi On 2014-11-03 at 09:54, Markus Armbruster wrote: > Kevin Wolf writes: > >> Am 31.10.2014 um 12:24 hat Stefan Hajnoczi geschrieben: >>> On Thu, Oct 30, 2014 at 10:36:35AM +0100, Kevin Wolf wrote: >>>> Am 30.10.2014 um 10:27 hat Stefan Hajnoczi geschrieben: >>>>> The guest may legitimately use raw devices that contain image format >>>>> data. Imagine tools similar to libguestfs. >>>>> >>>>> It's perfectly okay for them to lay out image format data onto a raw >>>>> device. >>>>> >>>>> Probing is the problem, not putting image format data onto a raw device. >>>> Agreed, that's why any restrictions only apply when probing was used to >>>> detect a raw image. If you want to do anything exotic like storing a >>>> qcow2 image for nested virt on a disk that is a raw image in the host, >>>> then making sure to pass format=raw shouldn't be too much. >>> Because at that point the solution is way over-engineered. >>> >>> Probing checks should be in the QEMU command-line code, not sprinkled >>> across the codebase and even at run-time. >>> >>> Isn't Markus approach much simpler and cleaner? >> I don't think so. My code isn't "sprinkled across the codebase", it has >> the checks right where the problem arises, in the raw block driver. > Actually, that's not where the problem is. > > The guest writing funny things to its disks is *not* the problem, it's > perfectly normal operation. Probing untrusted images is the problem! I don't think making your hard disk a qcow2 image is a perfectly normal operation, but perhaps that's just me. I still think writing to sector 0 is not a perfectly normal operation, but rarely ever happens (at least for x86[1]), and if it does, there's not that much variety to what is written there. [1] I'm open for arguments about how on other platforms there's not necessarily a boot loader in sector 0 of an image and the host is therefore actually completely free to write whatever it likes and there's no telling of what it will be. >> It's with Markus's approach that we'll have to have code in many >> different places as I showed. Its fundamental assumption that there is >> always a filename string and the filename isn't passed in some QDict >> option is simply wrong. Specifying the image is driver-dependent and >> therefore you'd have to add functionality to each driver in order to get >> the filename extension (or the information that there isn't anything >> close enough to a filename). > The RFC patch is incomplete. Can't say how much code recording the > filename correctly will take until I've done it. > > I suspect the lack of an image filename already leads to rotten error > messages in places. > >> The only argument brought up so far that I can reasonably buy is that >> in the unlikely case of the restrictions applying it may be surprising >> for the user to see requests failing. To address this, we could print >> a warning when an image is opened in the "restricted raw" mode. This >> way the user knows what's going on, > Improves the virtual hardware from "crippled" to "openly crippled", and > my opinion on it from "I hate it" to "I hate it slightly less" :) > >> and at the same time we still >> effectively protect them instead of only printing a warning without real >> protection. > This isn't real protection, either. > > If the user runs with format=raw, the guest can write whatever it likes, > and that's a feature. If the user forgets format=raw just once, he's > p0wned. That's because your "protection" does not address the real > problem (probing of untrusted images), Well, that's your definition of the real problem. I think making a raw image effectively a non-raw image (according to what file(1) says) is a real problem as well. Detecting an image as qcow2 when file(1) says it is indeed qcow2 is not that much of a problem, in my very humble opinion. Probably the real real problem is that qemu supports raw images at all, in contrast to other VM software (which support it for CD and floppy at the most). Hey, let's just add a flat mode to qcow2 and get rid of raw altogether! (that was a joke) > but tries to prevent it by > refusing to created "bad" untrusted images, but can do so only in > special configurations, and not at all for images written by something > else. > > Unlike your proposal, mine does attack the real problem, and thus *can* > provide real protection. It's what you define to be the real problem. I for one think the definition of what a raw image is is the real problem. When does a raw image cease to be a raw image? My definition would be that it ceases to be raw when sufficiently complex probing detects another format. Your definition seems to be that it's always raw when the user wants it to be raw, and sometimes the user doesn't know that he/she wants it to be raw (because of legacy issues). Also, I think it's perfectly valid to prevent a guest from writing things to the first sector which this sufficiently complex probing(TM) detects to be some image format. You don't think so. That's why you say probing is the real problem, whereas I say allowing the guest to write a fake image header is the real problem (not the least because it may fool other tools as well). So for me, your patch only mitigates the problem but does not fix it. Mitigating it is better than doing nothing, however, which is why I'm fine with it. Max > Not immediately because we want to avoid > aprupt change, but eventually. Here's how: > > 1. Initially, we warn on insecure usage. This doesn't protect users, > but it guides them towards protecting themselves. > > 2. Eventually, we make the warning an error. This *does* protect users, > regardless of how they use or have used QEMU, *except* when they > explicitly ask for insecure probing with format=insecure-probe (assuming > we provide that option).