qemu-devel
[Top][All Lists]
Advanced

[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: Mon, 03 Nov 2014 09:11:54 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)

Jeff Cody <address@hidden> writes:

> On Wed, Oct 29, 2014 at 07:37:02AM +0100, Markus Armbruster wrote:
>> Jeff Cody <address@hidden> writes:
>> 
>> > On Tue, Oct 28, 2014 at 05:03:40PM +0100, Markus Armbruster wrote:
>> >> 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.
>> >>
>> >
>> > Format 'vpc' should probably recognize both extensions "vpc" and
>> > "vhd".  The actual format is VHD, so most MS tools will probably
>> > create files with .vhd extensions.
>> >
>> > (This creates an overlap with vhdx; see my response to Eric's email).
>> 
>> Going to discuss it there.
>> 
>> >> Additionally, some tests still need to be updated.
>> >> 
>> >> Signed-off-by: Markus Armbruster <address@hidden>
>> >
>> >
>> > [ ...]
>> >
>> >> diff --git a/block/vhdx.c b/block/vhdx.c
>> >> index 12bfe75..d2c3a20 100644
>> >> --- a/block/vhdx.c
>> >> +++ b/block/vhdx.c
>> >> @@ -1945,6 +1945,7 @@ static BlockDriver bdrv_vhdx = {
>> >>      .format_name            = "vhdx",
>> >>      .instance_size          = sizeof(BDRVVHDXState),
>> >>      .bdrv_probe             = vhdx_probe,
>> >> +    .fname_ext              = { "vhd" },
>> >
>> > This should also have "vhdx", I think.  
>> 
>> Okay.  I looked for confirmation in Wikipedia, and found:
>> 
>>     Hyper-V, like Microsoft Virtual Server and Windows Virtual PC, saves
>>     each guest OS to a single virtual hard disk file with the extension
>>     .VHD, except in Windows 8 and Windows Server 2012 where it can be
>>     the newer .vhdx.
>> 
>> https://en.wikipedia.org/wiki/Hyper-V#VHD_compatibility_with_Virtual_Server_2005_and_Virtual_PC_2004.2F2007
>> 
>> Makes me wonder whether .vhd is really used for both vhdx and vpc format
>> images.  What have you seen in the wild?
>>
>
> I need to resurrect my Windows Server Hyper-V test machine, and see
> what it generates by default.  Most likely '.vhdx'
>
> However, even so, it seems entirely plausible that a 4-letter
> extension may end up represented as a 3-digit extension, and be .vhd,
> even if that is not the 'official' name.

If .vhd turns out to be ambiguous in practice, we need to deal with it.
I got an idea how, but I'd prefer to discuss it after we figured out how
to address our "inscure by default" issue.


>> >>      .bdrv_open              = vhdx_open,
>> >>      .bdrv_close             = vhdx_close,
>> >>      .bdrv_reopen_prepare    = vhdx_reopen_prepare,
>> [...]



reply via email to

[Prev in Thread] Current Thread [Next in Thread]