[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing |
Date: |
Thu, 30 Oct 2014 10:07:26 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
Stefan Hajnoczi <address@hidden> writes:
> On Wed, Oct 29, 2014 at 02:54:32PM +0100, Markus Armbruster wrote:
>> Kevin Wolf <address@hidden> 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 <address@hidden>
>> >
>> > 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.
- Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing, (continued)
- Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing, Fam Zheng, 2014/10/28
- Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing, Kevin Wolf, 2014/10/29
- Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing, Kevin Wolf, 2014/10/30
- Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing, Markus Armbruster, 2014/10/31
- Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing, Stefan Hajnoczi, 2014/10/31
- Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing, Eric Blake, 2014/10/31
Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing, Max Reitz, 2014/10/30