| Message ID | 20200227023928.1021959-4-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Tighten qemu-img rules on missing backing format | expand |
On Wed, Feb 26, 2020 at 20:39:28 -0600, Eric Blake wrote: > Creating an image that requires format probing of the backing image is > inherently unsafe (we've had several CVEs over the years based on > probes leaking information to the guest on a subsequent boot). If our > probing algorithm ever changes, or if other tools like libvirt > determine a different probe result than we do, then subsequent use of > that backing file under a different format will present corrupted data > to the guest. Start a deprecation clock so that future qemu-img can > refuse to create unsafe backing chains that would rely on probing. > > However, there is one time where probing is safe: if we probe raw, > then it is safe to record that implicitly in the image (but we still > warn, as it's better to teach the user to supply -F always than to > make them guess when it is safe). > > iotest 114 specifically wants to create an unsafe image for later > amendment rather than defaulting to our new default of recording a > probed format, so it needs an update. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > qemu-deprecated.texi | 15 +++++++++++++++ > block.c | 21 ++++++++++++++++++++- > qemu-img.c | 8 +++++++- > tests/qemu-iotests/114 | 4 ++-- > tests/qemu-iotests/114.out | 1 + > 5 files changed, 45 insertions(+), 4 deletions(-) > > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi > index 66eca3a1dede..f99b49addccc 100644 > --- a/qemu-deprecated.texi > +++ b/qemu-deprecated.texi > @@ -309,6 +309,21 @@ The above, converted to the current supported format: > > @section Related binaries > > +@subsection qemu-img backing file without format (since 5.0.0) > + > +The use of @command{qemu-img create}, @command{qemu-img rebase}, > +@command{qemu-img convert}, or @command{qemu-img ament} to create or s/ament/amend/ > +modify an image that depends on a backing file now recommends that an > +explicit backing format be provided. This is for safety - if qemu > +probes a different format than what you thought, the data presented to > +the guest will be corrupt; similarly, presenting a raw image to a > +guest allows the guest a potential security exploit if a future probe > +sees non-raw. To avoid warning messages, or even future refusal to > +create an unsafe image, you must pass @option{-o backing_fmt=} (or > +shorthand @option{-F}) to specify the intended backing format. You > +may use @command{qemu-img rebase -u} to retroactively add a backing > +format to an existing image. I'd add a note for users who wish to fix existing images (and I need to add it to libvirt's knowledge base too) that when the user is unsure of the backing image format and desn't trust that the image was handled in a trusted way, they must not use the format detected by qemu-img info if the image specifies a backing file, unless they also trust the backing file. (Sorry as a non-native English speaker I can't express this in a more elegant way.). > @subsection qemu-img convert -n -o (since 4.2.0) > > All options specified in @option{-o} are image creation options, so [...] > diff --git a/qemu-img.c b/qemu-img.c > index b9375427404d..e75ec1bdb555 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -3637,7 +3637,13 @@ static int img_rebase(int argc, char **argv) > * doesn't change when we switch the backing file. > */ > if (out_baseimg && *out_baseimg) { > - ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt, false); > + if (blk_new_backing && !out_basefmt) { > + out_basefmt = blk_bs(blk_new_backing)->drv->format_name; > + warn_report("Deprecated use of backing file " > + "without explicit backing format, using " > + "detected format of %s", out_basefmt); Isn't this a similar instance to what I've reported in the previous version? You warn that the format is missing, but set out_basefmt to the detected format , thus bdrv_change_backing_file will then write the detected format into the overlay even when it previously did not? I think this has to have the same check for raw-only as above. > + } > + ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt, true); or just the above line. > } else { > ret = bdrv_change_backing_file(bs, NULL, NULL, false); > } I feel that this deprecation will be at least partially controversial, but in my opinion it's the correct thing to do. Reviewed-by: Peter Krempa <pkrempa@redhat.com> after you address the issue above.
On a Wednesday in 2020, Eric Blake wrote: >Creating an image that requires format probing of the backing image is >inherently unsafe (we've had several CVEs over the years based on >probes leaking information to the guest on a subsequent boot). If our >probing algorithm ever changes, or if other tools like libvirt >determine a different probe result than we do, then subsequent use of >that backing file under a different format will present corrupted data >to the guest. Start a deprecation clock so that future qemu-img can >refuse to create unsafe backing chains that would rely on probing. > >However, there is one time where probing is safe: if we probe raw, >then it is safe to record that implicitly in the image (but we still >warn, as it's better to teach the user to supply -F always than to >make them guess when it is safe). > >iotest 114 specifically wants to create an unsafe image for later >amendment rather than defaulting to our new default of recording a >probed format, so it needs an update. > >Signed-off-by: Eric Blake <eblake@redhat.com> >--- > qemu-deprecated.texi | 15 +++++++++++++++ > block.c | 21 ++++++++++++++++++++- > qemu-img.c | 8 +++++++- > tests/qemu-iotests/114 | 4 ++-- > tests/qemu-iotests/114.out | 1 + > 5 files changed, 45 insertions(+), 4 deletions(-) > This seems to affect code paths that are used even outside of qemu-img, should the commit message mention it? Jano
On 2/27/20 1:09 AM, Peter Krempa wrote: > On Wed, Feb 26, 2020 at 20:39:28 -0600, Eric Blake wrote: >> Creating an image that requires format probing of the backing image is >> inherently unsafe (we've had several CVEs over the years based on >> probes leaking information to the guest on a subsequent boot). If our >> probing algorithm ever changes, or if other tools like libvirt >> determine a different probe result than we do, then subsequent use of >> that backing file under a different format will present corrupted data >> to the guest. Start a deprecation clock so that future qemu-img can >> refuse to create unsafe backing chains that would rely on probing. >> >> However, there is one time where probing is safe: if we probe raw, >> then it is safe to record that implicitly in the image (but we still >> warn, as it's better to teach the user to supply -F always than to >> make them guess when it is safe). >> >> iotest 114 specifically wants to create an unsafe image for later >> amendment rather than defaulting to our new default of recording a >> probed format, so it needs an update. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> qemu-deprecated.texi | 15 +++++++++++++++ >> block.c | 21 ++++++++++++++++++++- >> qemu-img.c | 8 +++++++- >> tests/qemu-iotests/114 | 4 ++-- >> tests/qemu-iotests/114.out | 1 + >> 5 files changed, 45 insertions(+), 4 deletions(-) >> >> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi >> index 66eca3a1dede..f99b49addccc 100644 >> --- a/qemu-deprecated.texi >> +++ b/qemu-deprecated.texi >> @@ -309,6 +309,21 @@ The above, converted to the current supported format: >> >> @section Related binaries >> >> +@subsection qemu-img backing file without format (since 5.0.0) >> + >> +The use of @command{qemu-img create}, @command{qemu-img rebase}, >> +@command{qemu-img convert}, or @command{qemu-img ament} to create or > > s/ament/amend/ > Fixing. (Do you ever wonder if I leave a typo in, just to check that reviewers were actually reviewing? But in this case, it was a symptom of me posting a series late at night to start the review process, even though I _really_ need to post a v3 that adds even more iotest coverage of every new deprecation warning made possible in this patch) >> +modify an image that depends on a backing file now recommends that an >> +explicit backing format be provided. This is for safety - if qemu >> +probes a different format than what you thought, the data presented to >> +the guest will be corrupt; similarly, presenting a raw image to a >> +guest allows the guest a potential security exploit if a future probe >> +sees non-raw. To avoid warning messages, or even future refusal to >> +create an unsafe image, you must pass @option{-o backing_fmt=} (or >> +shorthand @option{-F}) to specify the intended backing format. You >> +may use @command{qemu-img rebase -u} to retroactively add a backing >> +format to an existing image. > > I'd add a note for users who wish to fix existing images (and I need > to add it to libvirt's knowledge base too) that when the user is unsure > of the backing image format and desn't trust that the image was handled > in a trusted way, they must not use the format detected by qemu-img info > if the image specifies a backing file, unless they also trust the > backing file. (Sorry as a non-native English speaker I can't express > this in a more elegant way.). Good idea. How about: ...to retroactively add a backing format to an existing image. However, be aware that there are already potential security risks to using 'qemu-img info' to probe the format of an untrusted backing image, when deciding what format to write into an image with 'qemu-img rebase -u'. > >> @subsection qemu-img convert -n -o (since 4.2.0) >> >> All options specified in @option{-o} are image creation options, so > > [...] > >> diff --git a/qemu-img.c b/qemu-img.c >> index b9375427404d..e75ec1bdb555 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -3637,7 +3637,13 @@ static int img_rebase(int argc, char **argv) >> * doesn't change when we switch the backing file. >> */ >> if (out_baseimg && *out_baseimg) { >> - ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt, false); >> + if (blk_new_backing && !out_basefmt) { >> + out_basefmt = blk_bs(blk_new_backing)->drv->format_name; >> + warn_report("Deprecated use of backing file " >> + "without explicit backing format, using " >> + "detected format of %s", out_basefmt); > > Isn't this a similar instance to what I've reported in the previous > version? You warn that the format is missing, but set out_basefmt to the > detected format , thus bdrv_change_backing_file will then write the > detected format into the overlay even when it previously did not? > > I think this has to have the same check for raw-only as above. Yes, I think I missed a spot (blame the late-night push to get the patch out the door). I'll wait to see if I get review from the qemu side before posting v3, though. > >> + } >> + ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt, true); > > or just the above line. > >> } else { >> ret = bdrv_change_backing_file(bs, NULL, NULL, false); >> } > > I feel that this deprecation will be at least partially controversial, > but in my opinion it's the correct thing to do. > > Reviewed-by: Peter Krempa <pkrempa@redhat.com> > > after you address the issue above. >
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi index 66eca3a1dede..f99b49addccc 100644 --- a/qemu-deprecated.texi +++ b/qemu-deprecated.texi @@ -309,6 +309,21 @@ The above, converted to the current supported format: @section Related binaries +@subsection qemu-img backing file without format (since 5.0.0) + +The use of @command{qemu-img create}, @command{qemu-img rebase}, +@command{qemu-img convert}, or @command{qemu-img ament} to create or +modify an image that depends on a backing file now recommends that an +explicit backing format be provided. This is for safety - if qemu +probes a different format than what you thought, the data presented to +the guest will be corrupt; similarly, presenting a raw image to a +guest allows the guest a potential security exploit if a future probe +sees non-raw. To avoid warning messages, or even future refusal to +create an unsafe image, you must pass @option{-o backing_fmt=} (or +shorthand @option{-F}) to specify the intended backing format. You +may use @command{qemu-img rebase -u} to retroactively add a backing +format to an existing image. + @subsection qemu-img convert -n -o (since 4.2.0) All options specified in @option{-o} are image creation options, so diff --git a/block.c b/block.c index 10c2a34e7c00..9907cf1e3c78 100644 --- a/block.c +++ b/block.c @@ -6009,6 +6009,20 @@ void bdrv_img_create(const char *filename, const char *fmt, "Could not open backing image to determine size.\n"); goto out; } else { + if (!backing_fmt) { + warn_report("Deprecated use of backing file without explicit " + "backing format (detected format of %s)", + bs->drv->format_name); + if (bs->drv == &bdrv_raw) { + /* + * A probe of raw is always correct, so in this one + * case, we can write that into the image. + */ + backing_fmt = bs->drv->format_name; + qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, backing_fmt, + NULL); + } + } if (size == -1) { /* Opened BS, have no size */ size = bdrv_getlength(bs); @@ -6022,7 +6036,12 @@ void bdrv_img_create(const char *filename, const char *fmt, } bdrv_unref(bs); } - } /* (backing_file && !(flags & BDRV_O_NO_BACKING)) */ + /* (backing_file && !(flags & BDRV_O_NO_BACKING)) */ + } else if (backing_file && !backing_fmt) { + warn_report("Deprecated use of unopened backing file without " + "explicit backing format, use of this image requires " + "potentially unsafe format probing"); + } if (size == -1) { error_setg(errp, "Image creation needs a size parameter"); diff --git a/qemu-img.c b/qemu-img.c index b9375427404d..e75ec1bdb555 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -3637,7 +3637,13 @@ static int img_rebase(int argc, char **argv) * doesn't change when we switch the backing file. */ if (out_baseimg && *out_baseimg) { - ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt, false); + if (blk_new_backing && !out_basefmt) { + out_basefmt = blk_bs(blk_new_backing)->drv->format_name; + warn_report("Deprecated use of backing file " + "without explicit backing format, using " + "detected format of %s", out_basefmt); + } + ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt, true); } else { ret = bdrv_change_backing_file(bs, NULL, NULL, false); } diff --git a/tests/qemu-iotests/114 b/tests/qemu-iotests/114 index 26104fff6c67..727e06e283a5 100755 --- a/tests/qemu-iotests/114 +++ b/tests/qemu-iotests/114 @@ -42,9 +42,9 @@ _unsupported_proto vxhs # qcow2.py does not work too well with external data files _unsupported_imgopts data_file - +# Intentionally specify backing file without backing format TEST_IMG="$TEST_IMG.base" _make_test_img 64M -_make_test_img -b "$TEST_IMG.base" 64M +_make_test_img -u -b "$TEST_IMG.base" 64M # Set an invalid backing file format $PYTHON qcow2.py "$TEST_IMG" add-header-ext 0xE2792ACA "foo" diff --git a/tests/qemu-iotests/114.out b/tests/qemu-iotests/114.out index 67adef37a4f6..81d5a8e0ad03 100644 --- a/tests/qemu-iotests/114.out +++ b/tests/qemu-iotests/114.out @@ -1,5 +1,6 @@ QA output created by 114 Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864 +qemu-img: warning: Deprecated use of unopened backing file without explicit backing format, use of this image requires potentially unsafe format probing Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base image: TEST_DIR/t.IMGFMT file format: IMGFMT
Creating an image that requires format probing of the backing image is inherently unsafe (we've had several CVEs over the years based on probes leaking information to the guest on a subsequent boot). If our probing algorithm ever changes, or if other tools like libvirt determine a different probe result than we do, then subsequent use of that backing file under a different format will present corrupted data to the guest. Start a deprecation clock so that future qemu-img can refuse to create unsafe backing chains that would rely on probing. However, there is one time where probing is safe: if we probe raw, then it is safe to record that implicitly in the image (but we still warn, as it's better to teach the user to supply -F always than to make them guess when it is safe). iotest 114 specifically wants to create an unsafe image for later amendment rather than defaulting to our new default of recording a probed format, so it needs an update. Signed-off-by: Eric Blake <eblake@redhat.com> --- qemu-deprecated.texi | 15 +++++++++++++++ block.c | 21 ++++++++++++++++++++- qemu-img.c | 8 +++++++- tests/qemu-iotests/114 | 4 ++-- tests/qemu-iotests/114.out | 1 + 5 files changed, 45 insertions(+), 4 deletions(-)