Stefan Hajnoczi <stefa...@gmail.com> writes: > 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'?
You're right, my patch's use of file name is naive. Related bug: static int dmg_probe(const uint8_t *buf, int buf_size, const char *filename) { int len; if (!filename) { return 0; } len = strlen(filename); if (len > 4 && !strcmp(filename + len - 4, ".dmg")) { return 2; } return 0; } With -drive if=none,file=foo.dmg the probe succeeds, and the image is opened with driver bdrv_dmg. With file.filename=foo.dmg, the probe fails, and the image is opened with driver bdrv_raw. Oops. Two ideas for fixing this come to mind: * Probing must not use the file name. Remove the temptation by dropping the parameter from bdrv_probe(). Change dmg_probe() to probe the image contents instead. Backward incompatible. I suspect that could be just fine with you in this case ;-P * Probing may use the file name. Fix the code to pass the file name to ->bdrv_probe() whenever there is one. There certainly is one when we got our file descriptor from open(), i.e. in the simple case. Whenever we pass a file name to ->bdrv_probe(), we can just as well pass it to bdrv_guess_format(), too. It must still be prepared for a null name. There is a file name in your file.filename=/tmp/test.img example. There is none in your file.driver=nbd,file.host=localhost' example, because there we get the file descriptor from socket(). If you do fancy things like nbd, you need to specify format= to avoid the warning. I agree with Stefan: it's tolerable price to pay for "secure by default". >> > And how does your patch help for '-drive blkverify:hacked.img:good.img'? My patch puts "guess format from image file name" right next to "guess format from image contents" (a.k.a. probing). Therefore, it runs *exactly* when we probe an image. Your example: $ qemu-img create -f qcow2 good.img 1m Formatting 'good.img', fmt=qcow2 size=1048576 encryption=off cluster_size=65536 lazy_refcounts=off $ cp good.img hacked.img $ qemu -S -display none -monitor stdio -drive if=none,file=blkverify:hacked.img:good.img qemu: -drive if=none,file=blkverify:hacked.img:good.img: warning: insecure format probing of image 'good.img' To get rid of this warning, specify format=qcow2 explicitly, or change the image name to end with '.qcow2' blkverify: read sector_num=0 nb_sectors=4 contents mismatch in sector 0 Only good.img is probed. Why? Let's examine how this thing gets opened. First bdrv_open() has filename = "blkverify:hacked.img:good.img" options = {} flags = BDRV_O_RDWR | BDRV_O_CACHE_WB drv = NULL It calls itself recursively via bdrv_open_image(): Same parameters, except flags |= BDRV_O_ALLOW_RDWR | BDRV_O_UNMAP | BDRV_O_PROTOCOL. This parses the file name, and calls itself again, via bdrv_open_common(), blkverify_open(), bdrv_open_image(), two times. First: filename = "hacked.img" options = {} flags = as above bdrv_open() changes options to driver=file,filename=hacked.img, then opens hacked.img with driver "file", i.e. *without* probing. Next: filename = "good.img" options = {} flags = as above, less BDRV_O_PROTOCOL Once again, this calls itself, via bdrv_open_image(): Same parameters, except BDRV_O_PROTOCOL is back bdrv_open() changes options to driver=file,filename=good.img, then opens good.img with driver "file", i.e. *without* probing. Then it probes the image contents, resulting in driver bdrv_qcow2. My patch's code runs, and emits the warning. Then it calls bdrv_open_common() to put the qcow2 BDS on top of the file BDS. If it had a backing file, we'd call bdrv_open() some more. Back in the outermost bdrv_open(). It attempts to probe the image contents. Fails reading the contents, because the two children of the blkverify BDSs have different contents: good.img is read with qcow2 over file, while hacked.img is read with just file. If I create hacked.img with dd if=/dev/zero of=hacked.img bs=4M count=1 instead, then the read succeeds, and probing yields bdrv_raw. My patch's code runs, guesses bdrv_raw, and doesn't warn. Quite a jungle. >> 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). I regard my patch as a necessary preliminary step for that. Warn now, change behavior a couple of releases later. When exactly is debatable. > 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. The warning hopefully helps libvirt developers with keeping libvirt users fully protected.