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

[Markus Armbruster]

Max Reitz <address@hidden> writes:

> On 2014-11-03 at 09:54, Markus Armbruster wrote:
>> Kevin Wolf <address@hidden> writes:
>>
>>> Am 31.10.2014 um 12:24 hat Stefan Hajnoczi geschrieben:
>>>> On Thu, Oct 30, 2014 at 10:36:35AM +0100, Kevin Wolf wrote:
>>>>> Am 30.10.2014 um 10:27 hat Stefan Hajnoczi geschrieben:
>>>>>> The guest may legitimately use raw devices that contain image format
>>>>>> data.  Imagine tools similar to libguestfs.
>>>>>>
>>>>>> It's perfectly okay for them to lay out image format data onto a raw
>>>>>> device.
>>>>>>
>>>>>> Probing is the problem, not putting image format data onto a raw device.
>>>>> Agreed, that's why any restrictions only apply when probing was used to
>>>>> detect a raw image. If you want to do anything exotic like storing a
>>>>> qcow2 image for nested virt on a disk that is a raw image in the host,
>>>>> then making sure to pass format=raw shouldn't be too much.
>>>> Because at that point the solution is way over-engineered.
>>>>
>>>> Probing checks should be in the QEMU command-line code, not sprinkled
>>>> across the codebase and even at run-time.
>>>>
>>>> Isn't Markus approach much simpler and cleaner?
>>> I don't think so. My code isn't "sprinkled across the codebase", it has
>>> the checks right where the problem arises, in the raw block driver.
>> Actually, that's not where the problem is.
>>
>> The guest writing funny things to its disks is *not* the problem, it's
>> perfectly normal operation.  Probing untrusted images is the problem!
>
> I don't think making your hard disk a qcow2 image is a perfectly
> normal operation, but perhaps that's just me.
>
> I still think writing to sector 0 is not a perfectly normal operation,
> but rarely ever happens (at least for x86[1]), and if it does, there's
> not that much variety to what is written there.
>
> [1] I'm open for arguments about how on other platforms there's not
> necessarily a boot loader in sector 0 of an image and the host is
> therefore actually completely free to write whatever it likes and
> there's no telling of what it will be.
>
>>> It's with Markus's approach that we'll have to have code in many
>>> different places as I showed. Its fundamental assumption that there is
>>> always a filename string and the filename isn't passed in some QDict
>>> option is simply wrong. Specifying the image is driver-dependent and
>>> therefore you'd have to add functionality to each driver in order to get
>>> the filename extension (or the information that there isn't anything
>>> close enough to a filename).
>> The RFC patch is incomplete.  Can't say how much code recording the
>> filename correctly will take until I've done it.
>>
>> I suspect the lack of an image filename already leads to rotten error
>> messages in places.
>>
>>> The only argument brought up so far that I can reasonably buy is that
>>> in the unlikely case of the restrictions applying it may be surprising
>>> for the user to see requests failing. To address this, we could print
>>> a warning when an image is opened in the "restricted raw" mode. This
>>> way the user knows what's going on,
>> Improves the virtual hardware from "crippled" to "openly crippled", and
>> my opinion on it from "I hate it" to "I hate it slightly less" :)
>>
>>>                                      and at the same time we still
>>> effectively protect them instead of only printing a warning without real
>>> protection.
>> This isn't real protection, either.
>>
>> If the user runs with format=raw, the guest can write whatever it likes,
>> and that's a feature.  If the user forgets format=raw just once, he's
>> p0wned.  That's because your "protection" does not address the real
>> problem (probing of untrusted images),
>
> Well, that's your definition of the real problem. I think making a raw
> image effectively a non-raw image (according to what file(1) says) is
> a real problem as well. Detecting an image as qcow2 when file(1) says
> it is indeed qcow2 is not that much of a problem, in my very humble
> opinion.
>
> Probably the real real problem is that qemu supports raw images at
> all, in contrast to other VM software (which support it for CD and
> floppy at the most). Hey, let's just add a flat mode to qcow2 and get
> rid of raw altogether! (that was a joke)
>
>> but tries to prevent it by
>> refusing to created "bad" untrusted images, but can do so only in
>> special configurations, and not at all for images written by something
>> else.
>>
>> Unlike your proposal, mine does attack the real problem, and thus *can*
>> provide real protection.
>
> It's what you define to be the real problem. I for one think the
> definition of what a raw image is is the real problem. When does a raw
> image cease to be a raw image? My definition would be that it ceases
> to be raw when sufficiently complex probing detects another
> format. Your definition seems to be that it's always raw when the user
> wants it to be raw, and sometimes the user doesn't know that he/she
> wants it to be raw (because of legacy issues).
>
> Also, I think it's perfectly valid to prevent a guest from writing
> things to the first sector which this sufficiently complex probing(TM)
> detects to be some image format. You don't think so. That's why you
> say probing is the real problem, whereas I say allowing the guest to
> write a fake image header is the real problem (not the least because
> it may fool other tools as well).
>
> So for me, your patch only mitigates the problem but does not fix
> it. Mitigating it is better than doing nothing, however, which is why
> I'm fine with it.

You're right, the *root* problem is the raw vs. other image formats
ambiguity.

This is an instance of the general raw data file vs. structured data
file ambiguity.  Whether a file contains raw data or not only the user
can know.  Users generally keep track of what's what by giving their
files suitable names.

The root problem becomes a QEMU security problem only because (a) we
support both raw and image formats, (b) we second-guess the user by
default, and (c) our guess can be controlled by a malicious guest.

We can try to fix it by attacking one or more of the three conditions:

(a) Outlaw raw images.  Nobody is seriously suggesting that.

(b) Make the second-guessing opt-in rather than opt-out.  This is my
approach.

(c) Prevent the guest from affecting the second-guessing.  This is
Kevin's approach.

I'll discuss these in more detail in a separate message.

>> Not immediately because we want to avoid
>> aprupt change, but eventually.  Here's how:
>>
>> 1. Initially, we warn on insecure usage.  This doesn't protect users,
>> but it guides them towards protecting themselves.
>>
>> 2. Eventually, we make the warning an error.  This *does* protect users,
>> regardless of how they use or have used QEMU, *except* when they
>> explicitly ask for insecure probing with format=insecure-probe (assuming
>> we provide that option).