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

[Max Reitz <mreitz@redhat.com>]

On 2014-11-05 at 09:05, Markus Armbruster wrote:
> Jeff Cody <jcody@redhat.com> writes:
>
>> On Tue, Nov 04, 2014 at 10:39:36AM +0100, Markus Armbruster wrote:
>>> Kevin Wolf <kwolf@redhat.com> writes:
>>>
>>>> Am 30.10.2014 um 13:49 hat Markus Armbruster geschrieben:
>>>>> Kevin Wolf <kwolf@redhat.com> writes:
>>>>>
>>>>>> Am 29.10.2014 um 14:54 hat Markus Armbruster geschrieben:
>>>>>>> Kevin Wolf <kwolf@redhat.com> writes:
>>>>>>>> Instead, let me try once more to sell my old proposal [1] from the
>>>>>>>> thread you mentioned:
>>>>>>>>
>>>>>>>>> What if we let the raw driver know that it was probed and then it
>>>>>>>>> enables a check that returns -EIO for any write on the
>>>>>>>>> first 2k if that
>>>>>>>>> write would make the image look like a different format?
>>>>>>>> Attacks the problem where it arises instead of trying to detect the
>>>>>>>> outcome of it, and works in whatever way it is nested in the BDS graph
>>>>>>>> and whatever way is used to address the image file.
>>>>>>>>
>>>>>>>> Kevin
>>>>>>>>
>>>>>>>> [1]
>>>>>>>> https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg02548.html
>>>>>>> Arbitrarily breaking the virtual hardware that way is incompatible, too.
>>>>>>> It's a bigger no-no for me than changing user interface corner cases or
>>>>>>> deciding what to do with a file based on its file name extension.
>>>>>> It is arbitrarily breaking theoretical cases, while your patch is less
>>>>>> arbitrarily breaking common cases. I strongly prefer the former.
>>>>> I challenge "theoretical" as well as "common".
>>>>>
>>>>> For "theoretical", see below.
>>>>>
>>>>> As to "common": are unrecognizable image names really common?  I doubt
>>>>> it.  If I'm wrong, I expect users to complain about my warning, and then
>>>>> we can reconsider.
>>>> As I said in my other mail, I remember seeing BZs with command lines
>>>> that don't have a file extension. Block devices are affected anyway.
>>>>
>>>> Your RFC also misses the extension .raw that I personally use
>>>> occasionally, so I would get the warning even though the image name
>>>> isn't "unrecognizable" at all. And other people probably have other
>>>> extensions in use that we don't know of.
>>>>
>>>>>> Nobody will ever want to write a qcow2 header into their boot sector for
>>>>>> just running a guest:
>>>>>>
>>>>>> 0:   51                      push   %cx
>>>>>> 1:   46                      inc    %si
>>>>>> 2:   49                      dec    %cx
>>>>>> 3:   fb                      sti
>>>>>>
>>>>>> This code doesn't make any sense. It's similar for other formats. And if
>>>>>> they really have some odd reason to do that, the fix is easy: Either
>>>>>> don't use raw, or pass an explicit format=raw.
>>>>> A disk needn't start with a PC-style boot sector.  Even on a PC.  Not
>>>>> every disk needs to be bootable, or even partitioned.  Databases exist
>>>>> that like to work on raw devices.  Giving them /dev/sdb instead of a
>>>>> whole-disk partition /dev/sdb1 may not be the smartest thing to do, but
>>>>> it *works* with real hardware, so why not with virtual hardware?
>>>> Oh, it does. Just use a format that can represent this reliably (and as
>>>> a compromise, we're going to declare explicitly requested raw as such -
>>>> again, see my other mail for details).
>>> This is "opt in safety".  I dislike that for the same reason I dislike
>>> "opt in security".
>>>
>>> Insecure: we risk guest escaping isolation.
>>>
>>> Unsafe: we risk virtual hardware breakage.
>>>
>>>>> You either have to prevent *any* writing of the first 2048 bytes (the
>>>>> part that can be examined by a bdrv_probe() method, or your have to
>>>>> prevent writing anything a probe recognizes, or the user has to specify
>>>>> the format explicitly.
>>>>>
>>>>> If you do the former, you're way outside the realm of "theoretical".
>>>> Right, that's not an option.
>>>>
>>>>> If you do the latter, the degree of "theoreticalness" depends on the
>>>>> union of the patterns you prevent.  Issues:
>>>>>
>>>>> 1. Anthony's method of checking a few known signatures is fragile.  The
>>>>> only obviously robust way to test "is probing going to find something
>>>>> other than 'raw'?" is running the probes.  Feasible.
>>>> Yes, this is what I've been talking about.
>>>>
>>>>> 2. Whether the union of patterns qualifies as "theoretical" for all our
>>>>> targets is not obvious, and whether it'll remain "theoretical" for all
>>>>> future formats and target machines even less.
>>>> At least we don't have examples of it happening yet. And even if it
>>>> happens to become not theoretical at some point, the only thing that
>>>> happens is that they need to add format=raw - something that we already
>>>> know is sure to happen with your approach.
>>>>
>>>> Once we get to know of such cases, we can probably even resolve them by
>>>> improving the probing function of the problematic format.
>>> Your proposal "improves" things from "insecure by default" to "now less
>>> insecure, but additionally a bit unsafe".
>>>
>>> Your proposed remedy for "unsafe" seems to be "if it breaks, you get to
>>> keep the pieces, but you may ask us to narrow the conditions for
>>> breakage so it wouldn't have broken for you if you had imported the
>>> changed version from the future" ;)
>>>
>>>>> 3. A write access that works just fine in one QEMU version can be
>>>>> rejected by another version that recognizes more formats.  Or probes the
>>>>> same formats differently.
>>>> True. We're only sure to protect this binary, and we have good chances
>>>> of protecting some other qemu versions and some other tools, too.
>>>>
>>>> If the attacker has a time machine and knows what the next installed
>>>> qemu version will recognize (or if they exploit a file format that is
>>>> not a disk image, with a program other than qemu), we don't fully
>>>> protect the user. We'd have to completely forbid raw for that.
>>>>
>>>>> 4. Rejecting a write fails *late*, and looks like hardware failure to
>>>>> the guest.  With imperfect guest software, this risks data corruption.
>>>> Okay. So your common case is that you have a badly written database that
>>>> gets the full unpartitioned disk assigned and doesn't start to write at
>>>> the first block, but randomly, and when it finally tries to use block 0,
>>>> it gets deadly confused by the I/O error so that it will break the data
>>>> that it has already stored elsewhere.
>>>>
>>>> Yeah right.
>>>>
>>>> And you probably deserved it then for using such software. It would have
>>>> happened anyway sooner or later.
>>> Since you fail writes to sector 0 only when a "bad" pattern is written,
>>> your hypothesis that any software I should be using surely writes sector
>>> 0 early is irrelevant.
>>>
>>> "Common" doesn't matter either (and I never claimed it was common).  If
>>> I deploy software that uses whole disks, I'd consider disks that can
>>> fail writes to sector 0, even though the actual hardware is in good
>>> working order, defective, and I wouldn't buy them[*].  Regardless of how
>>> common the failure was.  Fortunately, no such disks exist.  Now you're
>>> proposing to create some, with a special "do not break" switch (factory
>>> default "do break").  Your excuse for doing so seems to be:
>>>
>>> (a) If I'm doing something *that* outlandish, I should know which disks
>>> to buy (i.e. anything but virtual raw images), or where to find the "do
>>> not break" switch (i.e. use format=raw).
>>>
>>> (b) If the breakage causes me harm, I deserve it, because my software
>>> sucks.
>>>
>>> For me, (a) is debatable, but (b) is simply bad attitude.  Let's not go
>>> there.
>>>
>>>
>>> [*] In fact, I'd consider them defective and wouldn't buy them no matter
>>> what software I plan to use.
>> The ultimate goal (correct me if I am wrong) of your patch is to get
>> rid of content probing during open, and rely solely on filename
>> extension.
> The goal is to separate raw and non-raw images sufficiently so we can
> exercise the proper care with untrusted raw image contents.
>
> Ditching probing is just a means.  I readily admit I rather like the
> means, since I never liked probing.

As I said before, I hope you won't ditch probing, because I'd strongly 
oppose that. I am however completely fine with adding more entropy to 
probing, i.e. using the filename extension as an additional source of 
information.

>> That leaves us open to guest data corruption in the opposite way -
>> identifying a qcow2 image as a raw image.  If I have qcow2 image named
>> mydisk.img, and QEMU 'detects' it as raw, then that means the first
>> guest data write will corrupt the image (especially to sector 0).
> I worked this issue into my "Image probing: how it can be insecure, and
> what we could do about it" treatise.
>
>> So if we are not specifying the image format, and allowing QEMU to
>> determine the format, Kevin's "restricted-raw" approach actually seems
>> safer to me, and less likely to break the guest.
> Relative likelihoods are of course debatable.
>
>> The other option is to just remove any sort of probing/format
>> autodetection (aside from qemu-img info) completely.  This may break
>> scripts, but switching to filename extensions as a detection mechanism
>> may break some scripts anyway.
> It breaks a whole lot more, and with fewer workarounds.  Fine with me,
> but Kevin hates it.

It would break my heart, too. (SCNR)

Max