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

[Paolo Bonzini]

On 23/03/2015 21:19, Markus Armbruster wrote:
> Paolo Bonzini <address@hidden> 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.

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 (also, have there been any
actual libvirt CVEs about this after 2010?  near misses don't count IMHO).

> 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.

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?  :)
> 
>