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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 30.10.2014 um 13:49 hat Markus Armbruster geschrieben:
>> Kevin Wolf <address@hidden> writes:
>> 
>> > Am 29.10.2014 um 14:54 hat Markus Armbruster geschrieben:
>> >> Kevin Wolf <address@hidden> 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.