qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Kevin Wolf
Subject: Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing
Date: Mon, 3 Nov 2014 12:00:28 +0100
User-agent: Mutt/1.5.21 (2010-09-15)

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

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

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

Kevin



reply via email to

[Prev in Thread] Current Thread [Next in Thread]