On Wed, Oct 29, 2014 at 02:54:32PM +0100, Markus Armbruster wrote: > Kevin Wolf <kw...@redhat.com> writes: > > > Am 28.10.2014 um 17:03 hat Markus Armbruster geschrieben: > >> If the user neglects to specify the image format, QEMU probes the > >> image to guess it automatically, for convenience. > >> > >> Relying on format probing is insecure for raw images (CVE-2008-2004). > >> If the guest writes a suitable header to the device, the next probe > >> will recognize a format chosen by the guest. A malicious guest can > >> abuse this to gain access to host files, e.g. by crafting a QCOW2 > >> header with backing file /etc/shadow. > >> > >> Commit 1e72d3b (April 2008) provided -drive parameter format to let > >> users disable probing. Commit f965509 (March 2009) extended QCOW2 to > >> optionally store the backing file format, to let users disable backing > >> file probing. QED has had a flag to suppress probing since the > >> beginning (2010), set whenever a raw backing file is assigned. > >> > >> Despite all this work (and time!), we're still insecure by default. I > >> think we're doing our users a disservice by sticking to the fatally > >> flawed probing. "Broken by default" is just wrong, and "convenience" > >> is no excuse. > >> > >> I believe we can retain 90% of the convenience without compromising > >> security by keying on image file name instead of image contents: if > >> the file name ends in .img or .iso, assume raw, if it ends in .qcow2, > >> assume qcow2, and so forth. > >> > >> Naturally, this would break command lines where the filename doesn't > >> provide the correct clue. So don't do it just yet, only warn if the > >> the change would lead to a different result. Looks like this: > >> > >> qemu: -drive file=my.img: warning: insecure format probing of image > >> 'my.img' > >> To get rid of this warning, specify format=qcow2 explicitly, or change > >> the image name to end with '.qcow2' > >> > >> This should steer users away from insecure format probing. After a > >> suitable grace period, we can hopefully drop format probing > >> alltogether. > >> > >> Example 0: file=WHATEVER,format=F > >> > >> Never warns, because the explicit format suppresses probing. > >> > >> Example 1: file=FOO.img > >> > >> Warns when probing of FOO.img results in anything but raw. In > >> particular, it warns when the guest just p0wned you. > >> > >> Example 2: file=FOO.qcow2 with backing file name FOO.img and no > >> backing image format. > >> > >> Warns when probing of FOO.qcow2 results in anything but qcow2, or > >> probing of FOO.img results in anything but raw. > >> > >> 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. > >> > >> * I didn't specify recognized extensions for formats "bochs", "cloop", > >> "parallels", "vpc", because I have no idea which ones to recognize. > >> > >> Additionally, some tests still need to be updated. > >> > >> Signed-off-by: Markus Armbruster <arm...@redhat.com> > > > > I still don't like this approach very much. > > > > The first of the reasons, and arguably the weakest one, is simply that I > > aesthetically dislike to encode semantics in filenames by relying on > > filename extensions. Feels like I'm being moved back to DOS times. > > It's the least bad solution so far, in my opinion. > > For what it's worth, gcc decides what to do with a file based on its > file name extension, too. > > > The second one, though, is probably a show stopper for me. You assume > > that there even is a filename that could have an extension, and that it > > is passed in the legacy filename argument instead of the QDict. With your > > patches: > > > > $ qemu-img create -f qcow2 /tmp/test.img 64M > > Formatting '/tmp/test.img', fmt=qcow2 size=67108864 encryption=off > > cluster_size=65536 lazy_refcounts=off > > $ qemu-system-x86_64 -drive file=/tmp/test.img > > qemu-system-x86_64: -drive file=/tmp/test.img: warning: insecure > > format probing of image '/tmp/test.img' > > To get rid of this warning, specify format=qcow2 explicitly, or change > > the image name to end with '.qcow2' > > $ x86_64-softmmu/qemu-system-x86_64 -drive file.filename=/tmp/test.img > > Speicherzugriffsfehler (Speicherabzug geschrieben) > > > > Oops, dereferencing a NULL pointer. Now, that's surely fixable, but what > > I originally wanted to demonstrate is that you won't output a warning > > there. Even that would still be fixable, by adding code into raw-posix, > > but what do you do with '-drive file.driver=nbd,file.host=localhost'? > > > > And how does your patch help for '-drive blkverify:hacked.img:good.img'? > > I'll reply to this as soon as I've had time to think.
If you are using fancy command-lines, you need to use format=. The probing feature is really useful for -hda test.qcow2. Anything fancier requires enough knowledge (and typing on the command-line) that format=qcow2 really isn't too much to ask for. > > Instead, let me try once more to sell my old proposal [1] from the > > thread you mentioned: > > > >> What if we let the raw driver know that it was probed and then it > >> enables a check that returns -EIO for any write on the first 2k if that > >> write would make the image look like a different format? > > > > Attacks the problem where it arises instead of trying to detect the > > outcome of it, and works in whatever way it is nested in the BDS graph > > and whatever way is used to address the image file. I think this is too clever. It's another thing to debug if a guest starts hitting EIO. My opinion on probing is: it's ugly but let's leave it for QEMU 3.0 at which point we implement Markus solution with exit(1). In the meantime the CVE has been known for a long time so vulnerable users (VM hosting, cloud, etc) have the information they need. Many are automatically protected by libvirt. Stefan
pgp4ussZhDdrW.pgp
Description: PGP signature
