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

> This is really borderline security theater.

I don't think so.  It's optional insurance agains libvirt bugs, and
libvirt wants to buy it.

>                                              Bugs happen, we fix them.

Yes.  Doesn't mean we shouldn't proactively mitigate bugs.

>  Even better, Kevin now has implemented a strong mitigation for CVEs
> like this, that won't allow guests to transmute a probed raw image
> into another format.

Yes.  As we discussed at some length back then, Kevin's mitigation is
not airtight and not without risk, but we decided it's worth having all
the same, in particular since it provides a good amount of protection
for users unwilling or unable to always specify formats.

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.

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