Re: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances

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

 



On 2/17/20 11:13 AM, Peter Krempa wrote:
Allow format probing to work around lazy clients which did not specify
their format in the overlay. Format probing will be allowed only, if we

s/only, if/only if/

are able to probe the image, the probing result was successful and the
probed image does not have any backing or data file.

This relaxes the restrictions which were imposed in commit 3615e8b39bad
in cases when we know that the image probing will not result in security
issues or data corruption.

It took me a few minutes of thinking about this.

Scenario 1:

base.raw <- wrap.qcow2

where wrap.qcow2 specifies backing of base.raw but not the format. If we probe, we can have a couple of outcomes:

1a: base.raw probes as raw (the probed image has no backing or data file), using it as raw is safe (it matches what wrap.qcow2 should have specified but didn't, and we aren't changing the data the guest would read nor are we opening unexpected files)

1b: base.raw probes as qcow2 (because of whatever the guest wrote there), using it as qcow2 is wrong - the guest will see corrupted data. What's more, if the probe sees it as qcow2 with backing file, and we open the backing file, it also has security implications.

Scenario 2:

base.qcow2 <- wrap.qcow2

where wrap.qcow2 specifies backing of base.qcow2 but not the format. If we probe, we will always have just one outcome:

2a: base.qcow2 probes as qcow2. Using it as qcow2 is correct, but if base.qcow2 has a further backing image, the backing chain is now dependent on a probe.

Since 1b and 2a have the same probe result, but massively different data corruption and/or security concerns, it is NOT sufficient to claim that a probe was safe merely because "the probed image does not have any backing or data file". It is ONLY safe if the probe turns up raw. Any other probed format is inherently unsafe.


We perform the image format detection and in the case that we were able
to probe the format and the format does not specify a backing store (or
doesn't support backing store) we can use this format.

Wrong.  The condition needs to be:

If we probe the format, and the probe returns raw, then it is safe to use raw as the format.


With pre-blockdev configurations this will restore the previous
behaviour for the images mentioned above as qemu would probe the format
anyways. It also improves error reporting compared to the old state as
we now report that the backing chain will be broken in case when there
is a backing file.

Improved error reporting because the probe returned qcow2 that would have required us to chase a backing file is fine; but while blindly accepting a qcow2 probe result when there is no backing file might avoid the security issue of chasing a backing file under guest control, it does not solve the data corruption issue of interpreting data as qcow2 that should have been interpreted as raw.


In blockdev configurations this ensures that libvirt will not cause data
corruption by ending the chain prematurely without notifying the user,
but still allows the old semantics when the users forgot to specify the
format.

The only time where it is safe to imply a forgotten format is if the probed format is still raw.


The price for this is that libvirt will need to keep the image format
detector still current and working or replace it by invocation of
qemu-img.

Maybe. Any format that qemu recognizes but libvirt does not risks a case where libvirt probes the image as raw but lets qemu re-probe the image and then qemu exposes different data. But as long as libvirt always passes explicit format to qemu (including explicit raw format of a backing file whose format was forgotten but probing said it was raw), then it doesn't matter what other formats libvirt can probe for. The only benefit for libvirt probing formats is then for better error messages for non-raw.


Signed-off-by: Peter Krempa <pkrempa@xxxxxxxxxx>
---
  src/util/virstoragefile.c | 52 ++++++++++++++++++++++-----------------
  1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index b984204b93..bbdf7be094 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -5010,6 +5010,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
                                   virHashTablePtr cycle,
                                   unsigned int depth)
  {
+    virStorageFileFormat orig_format = src->format;
      size_t headerLen;
      int backingFormat;
      int rv;
@@ -5020,10 +5021,17 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
                NULLSTR(src->path), src->format,
                (unsigned int)uid, (unsigned int)gid);

+    if (src->format == VIR_STORAGE_FILE_AUTO_SAFE)
+        src->format = VIR_STORAGE_FILE_AUTO;
+
      /* exit if we can't load information about the current image */
      rv = virStorageFileSupportsBackingChainTraversal(src);
-    if (rv <= 0)
+    if (rv <= 0) {
+        if (orig_format == VIR_STORAGE_FILE_AUTO)
+            return -2;
+
          return rv;
+    }

      if (virStorageFileGetMetadataRecurseReadHeader(src, parent, uid, gid,
                                                     &buf, &headerLen, cycle) < 0)
@@ -5032,6 +5040,18 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
      if (virStorageFileGetMetadataInternal(src, buf, headerLen, &backingFormat) < 0)
          return -1;

+    /* If we probed the format we MUST ensure that nothing else than the current
+     * image (this includes both backing files and external data store) is
+     * considered for security labelling and/or recursion. */

Grammar:

If we probed the format, we MUST ensure that nothing besides the current image (including both backing files and external data store) will be considered for security labelling and/or recursion.

+    if (orig_format == VIR_STORAGE_FILE_AUTO) {
+        if (src->backingStoreRaw || src->externalDataStoreRaw) {
+            src->format = VIR_STORAGE_FILE_RAW;
+            VIR_FREE(src->backingStoreRaw);
+            VIR_FREE(src->externalDataStoreRaw);
+            return -2;
+        }
+    }
+
      if (src->backingStoreRaw) {
          if ((rv = virStorageSourceNewFromBacking(src, &backingStore)) < 0)
              return -1;
@@ -5042,33 +5062,21 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,

          backingStore->format = backingFormat;

-        if (backingStore->format == VIR_STORAGE_FILE_AUTO) {
-            /* Assuming the backing store to be raw can lead to failures. We do
-             * it only when we must not report an error to prevent losing VMs.
-             * Otherwise report an error.
-             */
-            if (report_broken) {
+        if ((rv = virStorageFileGetMetadataRecurse(backingStore, parent,
+                                                   uid, gid,
+                                                   report_broken,
+                                                   cycle, depth + 1)) < 0) {
+            if (!report_broken)
+                return 0;
+
+            if (rv == -2) {
                  virReportError(VIR_ERR_OPERATION_INVALID,
                                 _("format of backing image '%s' of image '%s' was not specified in the image metadata "
                                   "(See https://libvirt.org/kbase/backing_chains.html for troubleshooting)"),
                                 src->backingStoreRaw, NULLSTR(src->path));

I disagree with the logic here.  What we really need is:

If the backing format was not specified, we probe to see what is there. If the result of that probe is raw, it is safe to treat the image as raw. If the result is anything else, we must report an error stating that what we probed could not be trusted unless the user adds an explicit backing format (either confirming that our probe was correct, or with the correct format overriding what we mis-probed).

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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux