Re: [Qemu-devel] [PATCH RFC for-2.3 1/1] block: New command line option --no-format-probing

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 23/03/2015 21:19, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> On 23/03/2015 18:48, Eric Blake wrote:
>>>>> Why can't libvirt just add ,format=raw instead of leaving out the
>>>>> format key altogether?
>>>>
>>>> Libvirt DOES add format=raw.  This patch is an extra insurance
>>>> policy to guarantee that libvirt does not have any code paths that
>>>> omit the explicit format (as we have had a couple of CVEs in
>>>> libvirt over the years where that was the case).
>>>
>>> And where's the extra insurance policy to guarantee that QEMU does not
>>> have any code paths that ignore the new command line option?
>> 
>> Right here (in the non-RFC patch, not this one):
>> 
>> @@ -751,6 +752,11 @@ static int find_image_format(BlockDriverState *bs, const char *filename,
>>          return ret;
>>      }
>>  
>> +    if (bdrv_image_probing_disabled) {
>> +        error_setg(errp, "Format not specified and image probing disabled");
>> +        return -EINVAL;
>> +    }
>> +
>>      ret = bdrv_pread(bs, 0, buf, sizeof(buf));
>>      if (ret < 0) {
>>          error_setg_errno(errp, -ret, "Could not read image for determining its "
>> 
>> The option sets bdrv_image_probing_disabled in a straightforward manner,
>> and bdrv_image_probing_disabled guards the probing code in an equally
>> straightforward manner.
>
> But what about migration from newer to older QEMU?  Libvirt even
> supports QEMU versions where the only way to specify disks is "-hda
> XYZ", so it is _impossible_ to honor the format=raw specifier.

If you migrate to a QEMU that doesn't understand the new option, libvirt
simply won't set it.  You lose the protection against libvirt bugs it
provides.  Guest won't notice.

If you somehow manage to find a QEMU old enough to make libvirt use
format-incapable interfaces, you'll be using insecure format probing on
the destination.  My patch makes no difference.  Good luck migrating to
such an old QEMU.

> Also, libvirt can start qemu-nbd and doesn't force format=raw in that
> case.  So the protection is far from complete.  This reinforces my
> opinion that the false sense of safety provided by this patch is worse
> than the "insurance" against future CVEs

The question whether the feature should be added to utilities is valid.
Me neglecting to add it in this patch can be a reason to reject this
patch, but hardly a reason for throwing out the idea wholesale.

>                                          (also, have there been any
> actual libvirt CVEs about this after 2010?  near misses don't count IMHO).

See Eric's testimony.

As to "near misses don't count": for me, what counts is actual users
telling me about their difficulties using QEMU securely.  Secure usage
shouldn't be hard.

>> How to get p0wned anyway:
>> 
>> 1. Use your raw image with format=raw.  Malicious guest writes qcow2
>> header.
>> 
>> 2. Use the image again without a format.  Probe returns qcow2, no
>> warning is printed.
>> 
>> Plausible attack?  Maybe not.  Worth stopping cold anyway?  I think so,
>> as long as it's easy.
>> 
>> My new option is a different kind of mitigation.  It's for users who
>> want more airtight protection, prefer writes to sector 0 just work,
>> funny or not, and are willing to always specify the format.  At least
>> one such user exists: libvirt.
>> 
>> Without this patch:
>> 
>> * Alternate use of raw images with and without format is still insecure;
>>   Kevin's mitigation can't protect you then.
>
> That's PEBKAC.

Secure usage shouldn't be hard.  When it is, then PEBKAC is blaming the
victim.

Today, secure usage requires you to always specify the format, on the
command line, in monitor commands, in every image referencing another
image (e.g. COW referencing backing file), and so forth.  There's images
all the way down.

Users tell us this is hard to get right and keep right, especially as
they race to keep up with our rapidly evolving block subsystem.

As long as they get it right, my proposed option doesn't make a
difference.  What it does is giving them a choice of failure modes for
when they fail:

1. The current failure mode, which maximizes backward compatibility and
   ease of use, except secure usage is inconveniently hard.  This is the
   default.

2. An alternate mode that fails early, hard and safe on insecure use.
   This is what libvirt wants to use.

> Paolo
>
>> * If you somehow miss specifying a format, and probing detects raw, you
>>   get a warning on stderr.  If your guest writes something funny to
>>   sector 0, the write fails.
>> 
>> With this patch:
>> 
>> * If you somehow miss specifying a format, open fails.  This is what
>>   libvirt wants.
>> 
>>>                       There certainly hasn't been enough discussion
>>> for this to get into 2.3.
>> 
>> Isn't that what were doing now?  :)

I chatted with Kevin, and we agree it's too late for 2.3 now, given the
ongoing discussion, but we'd like to pursue this further in 2.4.