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

[Markus Armbruster]

Stefan Hajnoczi <address@hidden> writes:

> On Mon, Nov 03, 2014 at 11:25:10AM +0100, Kevin Wolf wrote:
>> Am 03.11.2014 um 09:54 hat Markus Armbruster geschrieben:
>> > 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 think we all know it, but let's clearly state the real problem once
>> again to remind us what we're trying to do on a high level. The security
>> problem we're talking about is that some image formats can contain file
>> names, and the content of these files can be exposed (in some cases even
>> r/w) to the guest.
>
> That's too specific.  There are other cases:
>
> Imagine you are paying for a 10 GB disk KVM virtual server with a
> hosting provider.
>
> The hosting provider did not configure KVM correctly and they use
> probing.  You can write a qcow2 header with a 100 GB disk size to the
> raw disk and reboot the VM.
>
> Suddenly your guest has access to 100 GB of disk space even though you
> are only paying for 10 GB!

Yes.

>> Therefore not probing, but using untrusted images is the root problem -
>> but it's not one that we can fix. If the users downloads (or otherwise
>> obtains) an untrusted image with a bad backing file name, and of course
>> the correct file extension, we don't have a chance to protect them.
>
> This is driving the discussion into the wrong direction.  You are right
> that QEMU cannot solve that but it's also a well-known problem that
> OpenStack and co solved a long time ago.

I agree.  Conflating different threat vectors isn't going to help.
I think we got two here:

1. Trusting data from untrusted sources

   As Stefan said, this is a very general problem we can't solve in
   QEMU.  But it's also a well-known problem.

2. Letting the guest turn untrusted data into trusted data

   Unlike 1., this is a very specific problem, which *can* be solved in
   QEMU, at some cost.

   It's a guest isolation failure.  Since isolating guests is one of the
   points of virtualization, I believe users can reasonably expect
   working isolation.  Thus, isolation failure is an unexpected rather
   than a well-known problem.

Working definition of "trusted image" in this context: you trust the
image meta-data, but not the (guest-controlled) image contents.

Example: in a trusted QCOW2 image, you trust everything but the contents
of the actual data blocks.

Example: in a "trusted" raw image, you can trust nothing but file name
and attributes.

>> What we can do something about is a special case of this, where a
>> malicious guest tries to turn a (trusted) image into something that
>> isn't trustworthy any more. In my book, the bad action is modifying the
>> image so that it looks like a different format with a bad header, it's
>> not treating an image file like what it looks like.

I acknowledge your point of view that modifying the image in certain
ways is a bad action.  I just don't share it.  In my view, the guest can
legitimately write whatever it pleases.  Making it work with proper
isolation is our job.

>>                                                     If you disable
>> probing in qemu, you still allow qemu guests to write bad headers that
>> can fool other tools.

Yes.  Both your and my patch allow that.

Your patch prevents it sometimes, for some patterns.  When it does, it
protects all future users of the raw image from this particular bad
pattern.

My patch doesn't prevent it.  Instead, it immunizes QEMU against writes.
Only protects QEMU.  But then only QEMU has an untrusted guest inside.

>> The problem really isn't all the non-raw formats that have a header
>> which can contain file names. The problem is with raw which doesn't have
>> a header and allows the guest to turn its image file into any other file
>> type it wants and can potentially trick the user. raw is really a bad
>> format (or actually, it's a non-format), and if I could choose I
>> wouldn't support it, or at least only r/o. We should instead have used a
>> format with a header and then the raw file shifted by some offset. It's
>> too late for that, though.

Too late indeed.

>> But coming back to your specific statement: Probing images isn't the
>> problem. The guest writing funny things isn't the problem either. Using
>> a format that can't reliably represent those funny things is the
>> problem. And if the format can't reliably represent something, it should
>> return an error when you try to do it anyway. If you need the feature of
>> storing funny things, use a format that supports it reliably. (And
>> calling explicit format=raw reliable isn't quite right either, but it's
>> a compromise I'd be willing to make.)
>
> I don't follow this logic.  You end up with "raw is bad because it
> doesn't have a header".  That doesn't get us anywhere.
>
> (There are other formats that can also be confused.  For example, you
> can create a qcow2 file that is also a vpc file because vpc allows files
> that have only a footer.)

That way is madness.

> Fact remains that you must not probe untrusted guest disk images.
>
> The argument that there might not be a traditional filename doesn't make
> sense to me.  When there is no filename the command-line is already
> sufficiently complex and usage is fancy enough that probing adds no
> convenience, the user can just specify the format.

Yes.  Unknown file name can and should fail gracefully with my solution:
ask the user to specify a format.  Code to make the file name known in
more cases may be nice to have anyway.

> Anyway, does this sound reasonable:
>
> In QEMU 3.0, require the format= option for -drive.  Keep probing the
> way it is for non-drive options because they are used for convenience by
> local users.

We've discussed the problem in some depth, and several solutions have
been proposed.  I'll try to describe the solution space in a separate
message.