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

[Kevin Wolf]

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.

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.

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. If you disable
probing in qemu, you still allow qemu guests to write bad headers that
can fool other tools.

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.

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

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

And will you be sure that you'll have caught all places?

> I suspect the lack of an image filename already leads to rotten error
> messages in places.

Yes, we're in the middle of a conversion. I fully expect some messages
to be broken at the moment.

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

If you want to fully resolve the problem of trusted images turning into
something bad, you need to forbid raw. Period.

But we probably agree that we can't do that. What's left is that we can
try to make it less likely to happen.

> Unlike your proposal, mine does attack the real problem, and thus *can*
> provide real protection.  Not immediately because we want to avoid
> aprupt change, but eventually.

Your patch isn't attacking the real problem (using bad images). It also
isn't attacking the full special case (bad guest turning trusted images
into something bad). It's only attacking a special case of the special
case (turning a trusted image into something bad, and then exploiting
the bad image with qemu).

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

"Eventually" leaves users vulnerable for another year or two, even in
your special case of the special case.

And it will actually hit some users in practice with false positives (I
can remember BZs with command lines where qcow2 images don't have a
.qcow2 extension; and for LVs it's true anyway). I've yet to see
examples where my "restricted raw" patch would actually trigger in
practice outside of constructed nested virt examples with whole disks
formatted as qcow2 (seriously...) or actual attacks.

Kevin