[PATCH 0/3] Tighten qemu-img rules on missing backing format

Eric Blake posted 3 patches 4 years, 2 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
block.c                       | 31 ++++++++++++++++++++---
block/qcow2.c                 |  2 +-
block/stream.c                |  2 +-
blockdev.c                    |  3 ++-
include/block/block.h         |  4 +--
qemu-deprecated.texi          | 12 +++++++++
qemu-img.c                    | 10 ++++++--
tests/qemu-iotests/017        |  2 +-
tests/qemu-iotests/017.out    |  2 +-
tests/qemu-iotests/018        |  2 +-
tests/qemu-iotests/018.out    |  2 +-
tests/qemu-iotests/019        |  5 ++--
tests/qemu-iotests/019.out    |  2 +-
tests/qemu-iotests/020        |  2 +-
tests/qemu-iotests/020.out    |  2 +-
tests/qemu-iotests/024        |  8 +++---
tests/qemu-iotests/024.out    |  5 ++--
tests/qemu-iotests/028        |  4 +--
tests/qemu-iotests/028.out    |  2 +-
tests/qemu-iotests/030        | 26 +++++++++++++------
tests/qemu-iotests/034        |  2 +-
tests/qemu-iotests/034.out    |  2 +-
tests/qemu-iotests/037        |  2 +-
tests/qemu-iotests/037.out    |  2 +-
tests/qemu-iotests/038        |  2 +-
tests/qemu-iotests/038.out    |  2 +-
tests/qemu-iotests/039        |  3 ++-
tests/qemu-iotests/039.out    |  2 +-
tests/qemu-iotests/040        | 47 +++++++++++++++++++++++++----------
tests/qemu-iotests/041        | 37 ++++++++++++++++++---------
tests/qemu-iotests/042        |  4 +--
tests/qemu-iotests/043        | 18 +++++++-------
tests/qemu-iotests/043.out    | 16 +++++++-----
tests/qemu-iotests/046        |  2 +-
tests/qemu-iotests/046.out    |  2 +-
tests/qemu-iotests/050        |  4 +--
tests/qemu-iotests/050.out    |  2 +-
tests/qemu-iotests/051        |  2 +-
tests/qemu-iotests/051.out    |  2 +-
tests/qemu-iotests/051.pc.out |  2 +-
tests/qemu-iotests/060        |  2 +-
tests/qemu-iotests/060.out    |  2 +-
tests/qemu-iotests/061        | 10 ++++----
tests/qemu-iotests/061.out    | 10 ++++----
tests/qemu-iotests/069        |  2 +-
tests/qemu-iotests/069.out    |  2 +-
tests/qemu-iotests/073        |  2 +-
tests/qemu-iotests/073.out    |  2 +-
tests/qemu-iotests/082        | 16 +++++++-----
tests/qemu-iotests/082.out    | 16 ++++++------
tests/qemu-iotests/085        |  4 +--
tests/qemu-iotests/085.out    |  6 ++---
tests/qemu-iotests/089        |  2 +-
tests/qemu-iotests/089.out    |  2 +-
tests/qemu-iotests/095        |  4 +--
tests/qemu-iotests/095.out    |  4 +--
tests/qemu-iotests/097        |  4 +--
tests/qemu-iotests/097.out    | 16 ++++++------
tests/qemu-iotests/098        |  2 +-
tests/qemu-iotests/098.out    |  8 +++---
tests/qemu-iotests/110        |  4 +--
tests/qemu-iotests/110.out    |  4 +--
tests/qemu-iotests/114        |  4 +--
tests/qemu-iotests/114.out    |  1 +
tests/qemu-iotests/122        | 27 ++++++++++++--------
tests/qemu-iotests/122.out    |  8 +++---
tests/qemu-iotests/126        |  4 +--
tests/qemu-iotests/126.out    |  4 +--
tests/qemu-iotests/127        |  4 +--
tests/qemu-iotests/127.out    |  4 +--
tests/qemu-iotests/129        |  3 ++-
tests/qemu-iotests/133        |  2 +-
tests/qemu-iotests/133.out    |  2 +-
tests/qemu-iotests/139        |  2 +-
tests/qemu-iotests/141        |  4 +--
tests/qemu-iotests/141.out    |  4 +--
tests/qemu-iotests/142        |  2 +-
tests/qemu-iotests/142.out    |  2 +-
tests/qemu-iotests/153        | 14 +++++------
tests/qemu-iotests/153.out    | 35 ++++++++++++++------------
tests/qemu-iotests/154        | 42 +++++++++++++++----------------
tests/qemu-iotests/154.out    | 42 +++++++++++++++----------------
tests/qemu-iotests/155        | 12 ++++++---
tests/qemu-iotests/156        |  9 ++++---
tests/qemu-iotests/156.out    |  6 ++---
tests/qemu-iotests/158        |  2 +-
tests/qemu-iotests/158.out    |  2 +-
tests/qemu-iotests/161        |  8 +++---
tests/qemu-iotests/161.out    |  8 +++---
tests/qemu-iotests/176        |  4 +--
tests/qemu-iotests/176.out    | 32 ++++++++++++------------
tests/qemu-iotests/177        |  2 +-
tests/qemu-iotests/177.out    |  2 +-
tests/qemu-iotests/179        |  2 +-
tests/qemu-iotests/179.out    |  2 +-
tests/qemu-iotests/189        |  2 +-
tests/qemu-iotests/189.out    |  2 +-
tests/qemu-iotests/191        | 12 ++++-----
tests/qemu-iotests/191.out    | 12 ++++-----
tests/qemu-iotests/195        |  6 ++---
tests/qemu-iotests/195.out    |  6 ++---
tests/qemu-iotests/198        |  2 +-
tests/qemu-iotests/198.out    |  3 ++-
tests/qemu-iotests/204        |  2 +-
tests/qemu-iotests/204.out    |  2 +-
tests/qemu-iotests/216        |  2 +-
tests/qemu-iotests/224        |  4 +--
tests/qemu-iotests/228        |  5 ++--
tests/qemu-iotests/245        |  3 ++-
tests/qemu-iotests/249        |  4 +--
tests/qemu-iotests/249.out    |  4 +--
tests/qemu-iotests/252        |  2 +-
tests/qemu-iotests/257        |  3 ++-
tests/qemu-iotests/267        |  4 +--
tests/qemu-iotests/267.out    |  6 ++---
tests/qemu-iotests/270        |  2 +-
tests/qemu-iotests/270.out    |  2 +-
tests/qemu-iotests/273        |  4 +--
tests/qemu-iotests/273.out    |  4 +--
tests/qemu-iotests/279        |  4 +--
tests/qemu-iotests/279.out    |  4 +--
121 files changed, 466 insertions(+), 348 deletions(-)
[PATCH 0/3] Tighten qemu-img rules on missing backing format
Posted by Eric Blake 4 years, 2 months ago
In the past, we have had CVEs caused by qemu probing one image type
when an image started out as another but the guest was able to modify
content.  The solution to those CVEs was to encode backing format
information into qcow2, to ensure that once we make a decision, we
don't have to probe any further.  However, we failed to enforce this
at the time.  And now that libvirt is switching to -blockdev, it has
come back to bite us: with -block, libvirt had no easy way (other than
json:{} pseudoprotocol) to force a backing file, but with -blockdev,
libvirt HAS to use blockdev-open on the backing chain and supply a
backing format there, and thus has to probe images.  If libvirt ever
probes differently than qemu, we are back to the potential
guest-visible data corruption or potential host CVEs.

It's time to deprecate images without backing formats.  This patch
series does two things: 1. record an implicit backing format where one
is learned (although sadly, not all qemu-img commands are able to
learn a format), 2. warn to the user any time a probe had ambiguous
results or a backing format is omitted from an image.  All previous
images without a backing format are still usable, but hopefully the
warnings (along with libvirt's complaints about images without a
backing format) help us pinpoint remaining applications that are
creating images on their own without recording a backing format.

Perhaps I need to amend patch 3 and/or add a followup patch 4 that
adds further iotest coverage of all the new warnings (patch 1 touched
all the './check -qcow2' tests that were affected by the new warnings,
except for 114 which actually wanted to trigger the warning, if you
want to apply the series out of order to see the impact of the
warnings).

Eric Blake (3):
  iotests: Specify explicit backing format where sensible
  block: Add support to warn on backing file change without format
  qemu-img: Deprecate use of -b without -F

 block.c                       | 31 ++++++++++++++++++++---
 block/qcow2.c                 |  2 +-
 block/stream.c                |  2 +-
 blockdev.c                    |  3 ++-
 include/block/block.h         |  4 +--
 qemu-deprecated.texi          | 12 +++++++++
 qemu-img.c                    | 10 ++++++--
 tests/qemu-iotests/017        |  2 +-
 tests/qemu-iotests/017.out    |  2 +-
 tests/qemu-iotests/018        |  2 +-
 tests/qemu-iotests/018.out    |  2 +-
 tests/qemu-iotests/019        |  5 ++--
 tests/qemu-iotests/019.out    |  2 +-
 tests/qemu-iotests/020        |  2 +-
 tests/qemu-iotests/020.out    |  2 +-
 tests/qemu-iotests/024        |  8 +++---
 tests/qemu-iotests/024.out    |  5 ++--
 tests/qemu-iotests/028        |  4 +--
 tests/qemu-iotests/028.out    |  2 +-
 tests/qemu-iotests/030        | 26 +++++++++++++------
 tests/qemu-iotests/034        |  2 +-
 tests/qemu-iotests/034.out    |  2 +-
 tests/qemu-iotests/037        |  2 +-
 tests/qemu-iotests/037.out    |  2 +-
 tests/qemu-iotests/038        |  2 +-
 tests/qemu-iotests/038.out    |  2 +-
 tests/qemu-iotests/039        |  3 ++-
 tests/qemu-iotests/039.out    |  2 +-
 tests/qemu-iotests/040        | 47 +++++++++++++++++++++++++----------
 tests/qemu-iotests/041        | 37 ++++++++++++++++++---------
 tests/qemu-iotests/042        |  4 +--
 tests/qemu-iotests/043        | 18 +++++++-------
 tests/qemu-iotests/043.out    | 16 +++++++-----
 tests/qemu-iotests/046        |  2 +-
 tests/qemu-iotests/046.out    |  2 +-
 tests/qemu-iotests/050        |  4 +--
 tests/qemu-iotests/050.out    |  2 +-
 tests/qemu-iotests/051        |  2 +-
 tests/qemu-iotests/051.out    |  2 +-
 tests/qemu-iotests/051.pc.out |  2 +-
 tests/qemu-iotests/060        |  2 +-
 tests/qemu-iotests/060.out    |  2 +-
 tests/qemu-iotests/061        | 10 ++++----
 tests/qemu-iotests/061.out    | 10 ++++----
 tests/qemu-iotests/069        |  2 +-
 tests/qemu-iotests/069.out    |  2 +-
 tests/qemu-iotests/073        |  2 +-
 tests/qemu-iotests/073.out    |  2 +-
 tests/qemu-iotests/082        | 16 +++++++-----
 tests/qemu-iotests/082.out    | 16 ++++++------
 tests/qemu-iotests/085        |  4 +--
 tests/qemu-iotests/085.out    |  6 ++---
 tests/qemu-iotests/089        |  2 +-
 tests/qemu-iotests/089.out    |  2 +-
 tests/qemu-iotests/095        |  4 +--
 tests/qemu-iotests/095.out    |  4 +--
 tests/qemu-iotests/097        |  4 +--
 tests/qemu-iotests/097.out    | 16 ++++++------
 tests/qemu-iotests/098        |  2 +-
 tests/qemu-iotests/098.out    |  8 +++---
 tests/qemu-iotests/110        |  4 +--
 tests/qemu-iotests/110.out    |  4 +--
 tests/qemu-iotests/114        |  4 +--
 tests/qemu-iotests/114.out    |  1 +
 tests/qemu-iotests/122        | 27 ++++++++++++--------
 tests/qemu-iotests/122.out    |  8 +++---
 tests/qemu-iotests/126        |  4 +--
 tests/qemu-iotests/126.out    |  4 +--
 tests/qemu-iotests/127        |  4 +--
 tests/qemu-iotests/127.out    |  4 +--
 tests/qemu-iotests/129        |  3 ++-
 tests/qemu-iotests/133        |  2 +-
 tests/qemu-iotests/133.out    |  2 +-
 tests/qemu-iotests/139        |  2 +-
 tests/qemu-iotests/141        |  4 +--
 tests/qemu-iotests/141.out    |  4 +--
 tests/qemu-iotests/142        |  2 +-
 tests/qemu-iotests/142.out    |  2 +-
 tests/qemu-iotests/153        | 14 +++++------
 tests/qemu-iotests/153.out    | 35 ++++++++++++++------------
 tests/qemu-iotests/154        | 42 +++++++++++++++----------------
 tests/qemu-iotests/154.out    | 42 +++++++++++++++----------------
 tests/qemu-iotests/155        | 12 ++++++---
 tests/qemu-iotests/156        |  9 ++++---
 tests/qemu-iotests/156.out    |  6 ++---
 tests/qemu-iotests/158        |  2 +-
 tests/qemu-iotests/158.out    |  2 +-
 tests/qemu-iotests/161        |  8 +++---
 tests/qemu-iotests/161.out    |  8 +++---
 tests/qemu-iotests/176        |  4 +--
 tests/qemu-iotests/176.out    | 32 ++++++++++++------------
 tests/qemu-iotests/177        |  2 +-
 tests/qemu-iotests/177.out    |  2 +-
 tests/qemu-iotests/179        |  2 +-
 tests/qemu-iotests/179.out    |  2 +-
 tests/qemu-iotests/189        |  2 +-
 tests/qemu-iotests/189.out    |  2 +-
 tests/qemu-iotests/191        | 12 ++++-----
 tests/qemu-iotests/191.out    | 12 ++++-----
 tests/qemu-iotests/195        |  6 ++---
 tests/qemu-iotests/195.out    |  6 ++---
 tests/qemu-iotests/198        |  2 +-
 tests/qemu-iotests/198.out    |  3 ++-
 tests/qemu-iotests/204        |  2 +-
 tests/qemu-iotests/204.out    |  2 +-
 tests/qemu-iotests/216        |  2 +-
 tests/qemu-iotests/224        |  4 +--
 tests/qemu-iotests/228        |  5 ++--
 tests/qemu-iotests/245        |  3 ++-
 tests/qemu-iotests/249        |  4 +--
 tests/qemu-iotests/249.out    |  4 +--
 tests/qemu-iotests/252        |  2 +-
 tests/qemu-iotests/257        |  3 ++-
 tests/qemu-iotests/267        |  4 +--
 tests/qemu-iotests/267.out    |  6 ++---
 tests/qemu-iotests/270        |  2 +-
 tests/qemu-iotests/270.out    |  2 +-
 tests/qemu-iotests/273        |  4 +--
 tests/qemu-iotests/273.out    |  4 +--
 tests/qemu-iotests/279        |  4 +--
 tests/qemu-iotests/279.out    |  4 +--
 121 files changed, 466 insertions(+), 348 deletions(-)

-- 
2.24.1


Re: [PATCH 0/3] Tighten qemu-img rules on missing backing format
Posted by Peter Krempa 4 years, 2 months ago
On Sat, Feb 22, 2020 at 05:23:38 -0600, Eric Blake wrote:
> In the past, we have had CVEs caused by qemu probing one image type
> when an image started out as another but the guest was able to modify
> content.  The solution to those CVEs was to encode backing format
> information into qcow2, to ensure that once we make a decision, we
> don't have to probe any further.  However, we failed to enforce this
> at the time.  And now that libvirt is switching to -blockdev, it has
> come back to bite us: with -block, libvirt had no easy way (other than

s/-block/-drive/ ?

> json:{} pseudoprotocol) to force a backing file, but with -blockdev,

"json:{}" is basically -blockdev with extra steps. Old -drive usage
didn't have any way to do that apart from rewriting the image. Which is
basically the same since json:{} also needs to be recorded in the image
to take effect.

> libvirt HAS to use blockdev-open on the backing chain and supply a
> backing format there, and thus has to probe images.  If libvirt ever
> probes differently than qemu, we are back to the potential
> guest-visible data corruption or potential host CVEs.

As I've elaborated in [1] I disagree with the host CVE part. The
insecure part is not probing the format itself, but probing format AND
using the backing file of the image if we probed format.

I agree that mis-probing format leads to data corruption though.

> It's time to deprecate images without backing formats.  This patch
> series does two things: 1. record an implicit backing format where one
> is learned (although sadly, not all qemu-img commands are able to
> learn a format), 2. warn to the user any time a probe had ambiguous
> results or a backing format is omitted from an image.  All previous
> images without a backing format are still usable, but hopefully the
> warnings (along with libvirt's complaints about images without a
> backing format) help us pinpoint remaining applications that are

It is not a warning in libvirt though. We just refuse it now because we
don't do probing. Previously we allowed qemu to probe the format and the
only thing that prevented host CVEs was if the host used selinux or any
other security approach which would prevent opening the backing file.

> creating images on their own without recording a backing format.

Re: [PATCH 0/3] Tighten qemu-img rules on missing backing format
Posted by Peter Krempa 4 years, 2 months ago
On Mon, Feb 24, 2020 at 12:01:45 +0100, Peter Krempa wrote:
> On Sat, Feb 22, 2020 at 05:23:38 -0600, Eric Blake wrote:

[...]

> > libvirt HAS to use blockdev-open on the backing chain and supply a
> > backing format there, and thus has to probe images.  If libvirt ever
> > probes differently than qemu, we are back to the potential
> > guest-visible data corruption or potential host CVEs.
> 
> As I've elaborated in [1] I disagree with the host CVE part. The

[1] https://www.redhat.com/archives/libvir-list/2020-February/msg00624.html

> insecure part is not probing the format itself, but probing format AND
> using the backing file of the image if we probed format.

Re: [PATCH 0/3] Tighten qemu-img rules on missing backing format
Posted by Eric Blake 4 years, 2 months ago
On 2/24/20 5:01 AM, Peter Krempa wrote:
> On Sat, Feb 22, 2020 at 05:23:38 -0600, Eric Blake wrote:
>> In the past, we have had CVEs caused by qemu probing one image type
>> when an image started out as another but the guest was able to modify
>> content.  The solution to those CVEs was to encode backing format
>> information into qcow2, to ensure that once we make a decision, we
>> don't have to probe any further.  However, we failed to enforce this
>> at the time.  And now that libvirt is switching to -blockdev, it has
>> come back to bite us: with -block, libvirt had no easy way (other than
> 
> s/-block/-drive/ ?

Whoops, yes.

> 
>> json:{} pseudoprotocol) to force a backing file, but with -blockdev,
> 
> "json:{}" is basically -blockdev with extra steps. Old -drive usage
> didn't have any way to do that apart from rewriting the image. Which is
> basically the same since json:{} also needs to be recorded in the image
> to take effect.

Discussed in my other reply (it looks like I'll need to distinguish 
between json: pointing to just a protocol as the backing layer, vs. the 
more typical json: pointing to a format as the backing layer).

> 
>> libvirt HAS to use blockdev-open on the backing chain and supply a
>> backing format there, and thus has to probe images.  If libvirt ever
>> probes differently than qemu, we are back to the potential
>> guest-visible data corruption or potential host CVEs.
> 
> As I've elaborated in [1] I disagree with the host CVE part. The
> insecure part is not probing the format itself, but probing format AND
> using the backing file of the image if we probed format.
> 
> I agree that mis-probing format leads to data corruption though.

Your argument that there are other means at hand to prevent CVE when 
probing does occur is valid, however, my point is that CVEs due to 
probing were historically possible if the rest of the toolchain is not 
careful.  It is more of a burden-shifting problem: when qemu is not 
preventing probing, then other applications like libvirt have to take on 
extra measures to ensure libvirt does not have a CVE (the fact that we 
haven't had any in a few years is a good sign that we are at least aware 
of the problem and have worked hard to remain safe, even if it has 
required duplicated effort across multiple tools); whereas if qemu takes 
a hard-line stance on probing (which is the goal of the deprecation 
introduced in this series), now qemu has ensured safety whether or not 
the other layers also take measures to avoid any CVE.

> 
>> It's time to deprecate images without backing formats.  This patch
>> series does two things: 1. record an implicit backing format where one
>> is learned (although sadly, not all qemu-img commands are able to
>> learn a format), 2. warn to the user any time a probe had ambiguous
>> results or a backing format is omitted from an image.  All previous
>> images without a backing format are still usable, but hopefully the
>> warnings (along with libvirt's complaints about images without a
>> backing format) help us pinpoint remaining applications that are
> 
> It is not a warning in libvirt though. We just refuse it now because we
> don't do probing. Previously we allowed qemu to probe the format and the
> only thing that prevented host CVEs was if the host used selinux or any
> other security approach which would prevent opening the backing file.
> 
>> creating images on their own without recording a backing format.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org