10:21 a.m.
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(a)redhat.com>
---
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