Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 10/28/2014 10:03 AM, 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.
>
> May also be worth mentioning libvirt CVE-2010-2237, CVE-2010-2238,
> CVE-2010-2239, when libvirt was (mostly) patched to fix probing mishaps
> by defaulting to refusing to probe by default.  And as you say, we keep
> discovering variations on the theme (as recently as this year, I patched
> a bug where libvirt accidentally probes during a drive-mirror operation
> even when the user had explicitly marked an image as raw, although it
> was not given a CVE because of libvirt's default to refuse to probe).

I'll work that in, thanks.

>> 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.
>
> s/alltogether/altogether/

Okay.

>> Example 0: file=WHATEVER,format=F
>> 
>> Never warns, because the explicit format suppresses probing.
>
> Good, because this is what libvirt tries to use most of the time.
>
>> 
>> 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.
>
> Warning will be a false positive if the user misnamed their
> intentionally-created qcow2 file, but that's okay; the warning message
> tells them how to fix their setup.

I consider that a feature :)

>                                     However, I agree that most users
> pick .img or .iso for raw files, so when the probe finds something other
> than raw that the warning is worth it; better to catch the real problems
> in spite of the false negatives.

Positives, you mean.

>> 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.
>
> It's nice that it only warns the user when the probe came back with
> confusing results, while maintaining the convenience factor (no warning
> if probe matched expectations).  But this means the user is set up for
> late failure.
>
> Would it be any smarter to teach tools to automatically write in a
> backing format for any image that probed correctly where the backing
> format had not been previously recorded?  That is, if we are opening
> FOO.qcow2 for writing and it currently claims backing file BAR.img and
> no format, then before closing FOO.qcow2 we also write the backing
> format of raw into the metadata.  That way, the next time we open
> FOO.qcow2, we no longer have to probe BAR.img, and are ensured that we
> are reading the backing file with the same format as previously. (When
> opening FOO.qcow2 read-only, we don't have that luxury).  In other
> words, while we may still need to probe the TOP level file every time it
> is opened (for convenience for users that don't type the format on the
> command line every time), we should at LEAST minimize probing backing
> files to the just the first try.

Three separate cases, I think:

1. Creating an image without an explicitly specified backing file format

   Currently, we don't store a backing file format then.  You're
   proposing to record the probed format.  Example: qemu-img create.

2. Changing an image's backing file without explicitly specifying format

   Currently, we don't store a backing file format then.  You're
   proposing to record the probed format.  Example: qemu-img rebase

3. Opening an image read/write

   We don't mess with the stored backing file format then.  You're
   proposing to record the probed format.

Opinions?

> Not shown - what is the error message when a file extension that
> normally indicates a non-raw file comes up raw?  That is, if 'FOO.qcow2'
> does not actually contain qcow2, is that also worth a warning to the
> user?

This is the first of two possible warnings in example 2.

>        What happens where we don't have a file extension (such as when
> using block device names to hold qcow2 files, where there is no '.' in
> the name)?

Such names give no clue (in coding terms: extensions are matched
including the '.').  Therefore, the user has to specify the format.

We could use stat() in addition to the filename.  Say, block and
character devices are assumed raw, everything else is guessed from the
filename.  That way, the user has to specify the format only when he
uses devices in unusual ways.  Opinions?

>> 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.
>
> Yes.  For precedent, libvirt can be considered a tool on images for
> certain operations, and libvirt has been warning about probing since 2010.
>
> Also, while I agree that any tool that operates on ONLY one layer of an
> image, without ever trying to chase backing chains, can't be tricked
> into opening wrong files, I'm not sure I agree with the claim that
> "probing isn't insecure" -

I used a narrow definition of "insecure" here, namely the exact
CVE-2008-2004.  I shouldn't.

>> * I didn't specify recognized extensions for formats "bochs", "cloop",
>>   "parallels", "vpc", because I have no idea which ones to recognize.
>
> Fair enough.  If there are common enough extensions for a given format,
> I'm sure people can request they be recognized as future patches.

Or in review of this one :)

>> Additionally, some tests still need to be updated.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  block.c                   | 46 
>> ++++++++++++++++++++++++++++++++++++++++++++--
>>  block/dmg.c               |  1 +
>>  block/qcow.c              |  1 +
>>  block/qcow2.c             |  1 +
>>  block/qed.c               |  1 +
>>  block/raw_bsd.c           |  1 +
>>  block/vdi.c               |  1 +
>>  block/vhdx.c              |  1 +
>>  block/vmdk.c              |  1 +
>>  include/block/block_int.h |  1 +
>>  10 files changed, 53 insertions(+), 2 deletions(-)
>> 
>> diff --git a/block.c b/block.c
>> index 8da6e61..7a1c592 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -674,10 +674,37 @@ static BlockDriver *bdrv_probe_all(const uint8_t *buf, 
>> int buf_size,
>>      return drv;
>>  }
>>  
>> +/*
>> + * Guess image format from file name @fname
>> + */
>> +static BlockDriver *bdrv_guess_format(const char *fname)
>> +{
>> +    BlockDriver *d;
>> +    int i, ext;
>> +    size_t fn_len = strlen(fname);
>> +    size_t ext_len;
>> +
>> +    QLIST_FOREACH(d, &bdrv_drivers, list) {
>> +        for (i = 0; i < ARRAY_SIZE(d->fname_ext) && d->fname_ext[i]; i++) {
>> +            ext_len = strlen(d->fname_ext[i]);
>> +            if (fn_len <= ext_len) {
>> +                continue;
>> +            }
>> +            ext = fn_len - ext_len;
>> +            if (fname[ext - 1] == '.'
>> +                && !strcmp(fname + ext, d->fname_ext[i])) {
>> +                return d;
>
> What happens if more than one format tends to pick the same extension?
> For example, would you consider '.qcow' a typical extension for qcow2
> files, even though it would probably match the older qcow driver first?...

First one wins.  Therefore, adding the same extension to multiple
formats make no sense with this code.

>> +++ b/include/block/block_int.h
>> @@ -91,6 +91,7 @@ struct BlockDriver {
>>  
>>      int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char
>> *filename);
>>      int (*bdrv_probe_device)(const char *filename);
>> +    const char *fname_ext[2];
>
> ...Answering myself - so far, you haven't included any collision extensions.
>
> Overall, I like the idea of this patch, but still wonder if we can do
> better with the auto-amending of writable image metadata to record
> anything that we ever learned from an initial probe.

Thanks for your thoughtful review!