From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33897) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XmMiC-0004IQ-8Q for qemu-devel@nongnu.org; Thu, 06 Nov 2014 08:02:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XmMi6-0008PZ-3d for qemu-devel@nongnu.org; Thu, 06 Nov 2014 08:02:52 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42378) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XmMi5-0008P9-S8 for qemu-devel@nongnu.org; Thu, 06 Nov 2014 08:02:46 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sA6D2iYi000860 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Thu, 6 Nov 2014 08:02:44 -0500 Date: Thu, 6 Nov 2014 14:02:41 +0100 From: Kevin Wolf Message-ID: <20141106130241.GC4409@noname.redhat.com> References: <87lhnq3iul.fsf@blackfin.pond.sub.org> <5459E210.2020008@redhat.com> <87a944y0od.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87a944y0od.fsf@blackfin.pond.sub.org> Subject: Re: [Qemu-devel] Image probing: how it can be insecure, and what we could do about it List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Jeff Cody , qemu-devel@nongnu.org, Stefan Hajnoczi , Max Reitz Am 06.11.2014 um 13:26 hat Markus Armbruster geschrieben: > >> * Reuse the image *without* specifying the raw format. QEMU guesses the > >> format based on untrusted image contents. Now QEMU guesses a format > >> chosen by the guest, with meta-data chosen by the guest. By > >> controlling image meta-data, the malicious guest can access arbitrary > >> files as QEMU, enlarge its storage, and more. A non-malicious guest > >> can accidentally DoS itself, by writing a pattern probing recognizes. > > > > Thank you for bringing that to my attention. This means that I'm even > > more in favor of using Kevin's patches because in fact they don't > > break anything. > > They break things differently. The difference may or may not matter. > > Example: innocent guest writes a recognized pattern. > > Now: next restart fails, guest DoSed itself until host operator gets > around to adding format=raw to the configuration. Consequence: > downtime (probably lengthy), but no data corruption. Depends. Another possible outcome is that the guest doesn't DoS itself, but writes a valid image header. You've argued before that a guest could be keeping a qcow2 image on a whole disk for whatever odd reason. In this case, the qemu restart will present the nested image instead of the top-level one to the guest, which can probably be labelled corruption. > With Kevin's patch: write fails, guest may or may not handle the > failure gracefully. Consequences can range from "guest complains to > its logs (who cares)" via "guest stops whatever it's doing and refuses > to continue until its hardware gets fixed (downtime as above)" to > "data corruption". > > Example: innocent guest first writes a recognized pattern, then > overwrites it with a non-recognized pattern. > > Now: works. > > With Kevin's patch: as above. > > Again, I'm not claiming the differences are serious in practice, only > that they exist. Yes, the failure scenario is different. The point still stands that if it were relevant in practice, we would likely have heard of it before. > > You actually want to completely abolish probing? I thought we just > > wanted to use the filename as a second source of information and warn > > if the contents and the extension don't seem to match; and in the > > future, maybe make it an error (but we don't have to discuss that > > second part now, I think). > > Yes, I propose to ditch it completely, after a suitable grace period. I > tried to make that quite clear in my PATCH RFC 2/2. > > Here's why. > > Now: 1. probe > 4. open, error out if meta-data is bad > > With my proposed patch: > 1. probe > 2. guess from trusted meta-data > 3. warn unless 1 and 2 match > 4. open, error out if meta-data is bad > > Now make the warning an error: > 1. probe > 2. guess from trusted meta-data > 3. error out unless 1 and 2 match > 4. open, error out if meta-data is bad > > I figure the following is equivalent, but simpler: > > 2. guess from trusted meta-data > 4. open, error out if meta-data is bad > > because open should detect all the errors the previous step 3 caught. > If not, things are broken with explicit format=. Not entirely true, see below. > > My conclusion: Don't ditch probing. It increases entropy, why would > > you ditch probing? Just combine it with the extension and if both > > don't seem to match, that's an error. > > How does probing add value? > > Compare > > 1. probe > 2. guess from trusted meta-data > 3. error out unless 1 and 2 match > 4. open, error out if meta-data is bad > > to just > > 2. guess from trusted meta-data > 4. open, error out if meta-data is bad > > Let P be the driver recommended by probe, and G be the driver > recommended by guess. P = qcow2, G = raw > If P == G, same result: we open with the same driver. No, they are not the same. > Else, if open with G fails, equivalent result: error out in step 3 > vs. error out in step 4. No, raw accepts the image. > Else, we have an odd case: one driver's probe accepts (P's), yet a > different driver's open succeeds (G's). > > If G's probe rejects: is this a bug? Shouldn't open always fail > when probe rejects? No, raw's probe doesn't reject, it just has a very low score. > If G's probe accepts, then probing chose P over G. Maybe it > shouldn't have. Or maybe the probing functions are imprecise. > Anyway, keeping probing around makes this an error. Should it be > one? Yes, raw being the fallback for everything is imprecise. It's the only way we have for probing raw. > Am I missing something? This is the safety measure that was missing in your proposal, against corruption caused by a qcow2 image stored in foo.img that is now unintentionally opened as raw. Same thing probably for qcow2 stored on LVs etc. Kevin