| Message ID | 20190102140535.11512-1-cfergeau@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | json: Fix % handling when not interpolating | expand |
Adding Markus to cc: list, I forgot to do it when sending the patch. Christophe On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote: > commit 8bca4613 added support for %% in json strings when interpolating, > but in doing so, this broke handling of % when not interpolating as the > '%' is skipped in both cases. > This commit ensures we only try to handle %% when interpolating. > > Signed-off-by: Christophe Fergeau <cfergeau@redhat.com> > --- > qobject/json-parser.c | 10 ++++++---- > tests/check-qjson.c | 5 +++++ > 2 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/qobject/json-parser.c b/qobject/json-parser.c > index 7a7ae9e8d1..d8eb210c0c 100644 > --- a/qobject/json-parser.c > +++ b/qobject/json-parser.c > @@ -208,11 +208,13 @@ static QString *parse_string(JSONParserContext *ctxt, JSONToken *token) > } > break; > case '%': > - if (ctxt->ap && ptr[1] != '%') { > - parse_error(ctxt, token, "can't interpolate into string"); > - goto out; > + if (ctxt->ap) { > + if (ptr[1] != '%') { > + parse_error(ctxt, token, "can't interpolate into string"); > + goto out; > + } > + ptr++; > } > - ptr++; > /* fall through */ > default: > cp = mod_utf8_codepoint(ptr, 6, &end); > diff --git a/tests/check-qjson.c b/tests/check-qjson.c > index d876a7a96e..fa2afccb0a 100644 > --- a/tests/check-qjson.c > +++ b/tests/check-qjson.c > @@ -175,6 +175,11 @@ static void utf8_string(void) > "\xCE\xBA\xE1\xBD\xB9\xCF\x83\xCE\xBC\xCE\xB5", > "\xCE\xBA\xE1\xBD\xB9\xCF\x83\xCE\xBC\xCE\xB5", > "\\u03BA\\u1F79\\u03C3\\u03BC\\u03B5", > + }, > + /* '%' character when not interpolating */ > + { > + "100%", > + "100%", > }, > /* 2 Boundary condition test cases */ > /* 2.1 First possible sequence of a certain length */ > -- > 2.20.1 > >
On 1/2/19 12:01 PM, Christophe Fergeau wrote: > Adding Markus to cc: list, I forgot to do it when sending the patch. Also worth backporting via qemu-stable, now in cc. > > Christophe > > On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote: >> commit 8bca4613 added support for %% in json strings when interpolating, >> but in doing so, this broke handling of % when not interpolating as the >> '%' is skipped in both cases. >> This commit ensures we only try to handle %% when interpolating. >> >> Signed-off-by: Christophe Fergeau <cfergeau@redhat.com> >> --- >> qobject/json-parser.c | 10 ++++++---- >> tests/check-qjson.c | 5 +++++ >> 2 files changed, 11 insertions(+), 4 deletions(-) >> Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake <eblake@redhat.com> writes: > On 1/2/19 12:01 PM, Christophe Fergeau wrote: >> Adding Markus to cc: list, I forgot to do it when sending the patch. > > Also worth backporting via qemu-stable, now in cc. > >> >> Christophe >> >> On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote: >>> commit 8bca4613 added support for %% in json strings when interpolating, >>> but in doing so, this broke handling of % when not interpolating as the >>> '%' is skipped in both cases. >>> This commit ensures we only try to handle %% when interpolating. Impact? If you're unable to assess, could you give us at least a reproducer? >>> Signed-off-by: Christophe Fergeau <cfergeau@redhat.com> >>> --- >>> qobject/json-parser.c | 10 ++++++---- >>> tests/check-qjson.c | 5 +++++ >>> 2 files changed, 11 insertions(+), 4 deletions(-) >>> > > Reviewed-by: Eric Blake <eblake@redhat.com> Patch looks good to me, but I'd like us to improve the commit message.
On 1/7/19 9:47 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 1/2/19 12:01 PM, Christophe Fergeau wrote: >>> Adding Markus to cc: list, I forgot to do it when sending the patch. >> >> Also worth backporting via qemu-stable, now in cc. >> >>> >>> Christophe >>> >>> On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote: >>>> commit 8bca4613 added support for %% in json strings when interpolating, >>>> but in doing so, this broke handling of % when not interpolating as the >>>> '%' is skipped in both cases. >>>> This commit ensures we only try to handle %% when interpolating. > > Impact? > > If you're unable to assess, could you give us at least a reproducer? Another thread pointed out that Spice passwords involving % get corrupted. > >>>> Signed-off-by: Christophe Fergeau <cfergeau@redhat.com> >>>> --- >>>> qobject/json-parser.c | 10 ++++++---- >>>> tests/check-qjson.c | 5 +++++ >>>> 2 files changed, 11 insertions(+), 4 deletions(-) >>>> >> >> Reviewed-by: Eric Blake <eblake@redhat.com> > > Patch looks good to me, but I'd like us to improve the commit message. Indeed.
On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > > > On 1/2/19 12:01 PM, Christophe Fergeau wrote: > >> Adding Markus to cc: list, I forgot to do it when sending the patch. > > > > Also worth backporting via qemu-stable, now in cc. > > > >> > >> Christophe > >> > >> On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote: > >>> commit 8bca4613 added support for %% in json strings when interpolating, > >>> but in doing so, this broke handling of % when not interpolating as the > >>> '%' is skipped in both cases. > >>> This commit ensures we only try to handle %% when interpolating. > > Impact? > > If you're unable to assess, could you give us at least a reproducer? This all came from https://lists.freedesktop.org/archives/spice-devel/2018-December/046644.html Setting up a VM with libvirt with <graphics type='spice' autoport='yes' passwd='password%'/> fails to start with: qemu-system-x86_64: qobject/json-parser.c:146: parse_string: Assertion `*ptr' failed. If you use 'password%%' as the password instead, when trying to connect to the VM, you type 'password%' as the password instead of 'password%%' as configured in the domain XML. Christophe
Copying block maintainers for help with assessing the bug's (non-)impact on security. Christophe Fergeau <cfergeau@redhat.com> writes: > On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >> > On 1/2/19 12:01 PM, Christophe Fergeau wrote: >> >> Adding Markus to cc: list, I forgot to do it when sending the patch. >> > >> > Also worth backporting via qemu-stable, now in cc. >> > >> >> >> >> Christophe >> >> >> >> On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote: >> >>> commit 8bca4613 added support for %% in json strings when interpolating, >> >>> but in doing so, this broke handling of % when not interpolating as the >> >>> '%' is skipped in both cases. >> >>> This commit ensures we only try to handle %% when interpolating. >> >> Impact? >> >> If you're unable to assess, could you give us at least a reproducer? > > This all came from https://lists.freedesktop.org/archives/spice-devel/2018-December/046644.html > Setting up a VM with libvirt with <graphics type='spice' autoport='yes' passwd='password%'/> > fails to start with: > qemu-system-x86_64: qobject/json-parser.c:146: parse_string: Assertion `*ptr' failed. > > If you use 'password%%' as the password instead, when trying to connect > to the VM, you type 'password%' as the password instead of 'password%%' > as configured in the domain XML. Thanks. As the commit message says, the bug bites when we parse a string containing '%s' with !ctxt->ap. The parser then swallows a character. If it swallows the terminating '"', it fails the assertion. We parse with !ctxt->ap in the following cases: * Tests (tests/check-qjson.c, tests/test-qobject-input-visitor.c, tests/test-visitor-serialization.c) Plenty of tests, but we still failed to cover the buggy case :( * QMP input (monitor.c) * QGA input (qga/main.c) * qobject_from_json() - JSON pseudo-filenames (block.c) These are pseudo-filenames starting with "json:". - JSON key pairs (block/rbd.c) As far as I can tell, these can come only from pseudo-filenames starting with "rbd:". - JSON command line option arguments of -display and -blockdev (qobject-input-visitor.c) Reproducer: -blockdev '{"%"}' Command line, QMP and QGA input are trusted. Filenames are trusted when they come from command line, QMP or HMP. They are untrusted when they come from from image file headers. Example: QCOW2 backing file name. Note that this is *not* the security boundary between host and guest. It's the boundary between host and an image file from an untrusted source. I can't see how the bug could be exploited. Neither failing an assertion nor skipping a character in a filename of your choice is interesting. We don't support compiling with NDEBUG. Kevin, Max, do you agree?
On 08.01.19 11:36, Markus Armbruster wrote: > Copying block maintainers for help with assessing the bug's (non-)impact > on security. > > Christophe Fergeau <cfergeau@redhat.com> writes: > >> On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote: >>> Eric Blake <eblake@redhat.com> writes: >>> >>>> On 1/2/19 12:01 PM, Christophe Fergeau wrote: >>>>> Adding Markus to cc: list, I forgot to do it when sending the patch. >>>> >>>> Also worth backporting via qemu-stable, now in cc. >>>> >>>>> >>>>> Christophe >>>>> >>>>> On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote: >>>>>> commit 8bca4613 added support for %% in json strings when interpolating, >>>>>> but in doing so, this broke handling of % when not interpolating as the >>>>>> '%' is skipped in both cases. >>>>>> This commit ensures we only try to handle %% when interpolating. >>> >>> Impact? >>> >>> If you're unable to assess, could you give us at least a reproducer? >> >> This all came from https://lists.freedesktop.org/archives/spice-devel/2018-December/046644.html >> Setting up a VM with libvirt with <graphics type='spice' autoport='yes' passwd='password%'/> >> fails to start with: >> qemu-system-x86_64: qobject/json-parser.c:146: parse_string: Assertion `*ptr' failed. >> >> If you use 'password%%' as the password instead, when trying to connect >> to the VM, you type 'password%' as the password instead of 'password%%' >> as configured in the domain XML. > > Thanks. > > As the commit message says, the bug bites when we parse a string > containing '%s' with !ctxt->ap. The parser then swallows a character. > If it swallows the terminating '"', it fails the assertion. > > We parse with !ctxt->ap in the following cases: > > * Tests (tests/check-qjson.c, tests/test-qobject-input-visitor.c, > tests/test-visitor-serialization.c) > > Plenty of tests, but we still failed to cover the buggy case :( > > * QMP input (monitor.c) > > * QGA input (qga/main.c) > > * qobject_from_json() > > - JSON pseudo-filenames (block.c) > > These are pseudo-filenames starting with "json:". > > - JSON key pairs (block/rbd.c) > > As far as I can tell, these can come only from pseudo-filenames > starting with "rbd:". > > - JSON command line option arguments of -display and -blockdev > (qobject-input-visitor.c) > > Reproducer: -blockdev '{"%"}' > > Command line, QMP and QGA input are trusted. > > Filenames are trusted when they come from command line, QMP or HMP. > They are untrusted when they come from from image file headers. > Example: QCOW2 backing file name. Note that this is *not* the security > boundary between host and guest. It's the boundary between host and an > image file from an untrusted source. > > I can't see how the bug could be exploited. Neither failing an > assertion nor skipping a character in a filename of your choice is > interesting. We don't support compiling with NDEBUG. > > Kevin, Max, do you agree? I wouldn't call it "not interesting" if adding an image to your VM at runtime can crash the whole thing. (qemu-img create -f qcow2 -u -b 'json:{"%"}' foo.qcow2 64M) Whether this is a security issue... I don't know, but it is a DoS. Max
Max Reitz <mreitz@redhat.com> writes: > On 08.01.19 11:36, Markus Armbruster wrote: >> Copying block maintainers for help with assessing the bug's (non-)impact >> on security. >> >> Christophe Fergeau <cfergeau@redhat.com> writes: >> >>> On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote: >>>> Eric Blake <eblake@redhat.com> writes: >>>> >>>>> On 1/2/19 12:01 PM, Christophe Fergeau wrote: >>>>>> Adding Markus to cc: list, I forgot to do it when sending the patch. >>>>> >>>>> Also worth backporting via qemu-stable, now in cc. >>>>> >>>>>> >>>>>> Christophe >>>>>> >>>>>> On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote: >>>>>>> commit 8bca4613 added support for %% in json strings when interpolating, >>>>>>> but in doing so, this broke handling of % when not interpolating as the >>>>>>> '%' is skipped in both cases. >>>>>>> This commit ensures we only try to handle %% when interpolating. >>>> >>>> Impact? >>>> >>>> If you're unable to assess, could you give us at least a reproducer? >>> >>> This all came from https://lists.freedesktop.org/archives/spice-devel/2018-December/046644.html >>> Setting up a VM with libvirt with <graphics type='spice' autoport='yes' passwd='password%'/> >>> fails to start with: >>> qemu-system-x86_64: qobject/json-parser.c:146: parse_string: Assertion `*ptr' failed. >>> >>> If you use 'password%%' as the password instead, when trying to connect >>> to the VM, you type 'password%' as the password instead of 'password%%' >>> as configured in the domain XML. >> >> Thanks. >> >> As the commit message says, the bug bites when we parse a string >> containing '%s' with !ctxt->ap. The parser then swallows a character. >> If it swallows the terminating '"', it fails the assertion. >> >> We parse with !ctxt->ap in the following cases: >> >> * Tests (tests/check-qjson.c, tests/test-qobject-input-visitor.c, >> tests/test-visitor-serialization.c) >> >> Plenty of tests, but we still failed to cover the buggy case :( >> >> * QMP input (monitor.c) >> >> * QGA input (qga/main.c) >> >> * qobject_from_json() >> >> - JSON pseudo-filenames (block.c) >> >> These are pseudo-filenames starting with "json:". >> >> - JSON key pairs (block/rbd.c) >> >> As far as I can tell, these can come only from pseudo-filenames >> starting with "rbd:". >> >> - JSON command line option arguments of -display and -blockdev >> (qobject-input-visitor.c) >> >> Reproducer: -blockdev '{"%"}' >> >> Command line, QMP and QGA input are trusted. >> >> Filenames are trusted when they come from command line, QMP or HMP. >> They are untrusted when they come from from image file headers. >> Example: QCOW2 backing file name. Note that this is *not* the security >> boundary between host and guest. It's the boundary between host and an >> image file from an untrusted source. >> >> I can't see how the bug could be exploited. Neither failing an >> assertion nor skipping a character in a filename of your choice is >> interesting. We don't support compiling with NDEBUG. >> >> Kevin, Max, do you agree? > > I wouldn't call it "not interesting" if adding an image to your VM at > runtime can crash the whole thing. > > (qemu-img create -f qcow2 -u -b 'json:{"%"}' foo.qcow2 64M) "Not interesting" strictly from the point of view of exploiting the bug to penetrate trust boundaries. > Whether this is a security issue... I don't know, but it is a DoS. I'm not sure whether feeding untrusted images to QEMU is a good idea in general --- there's so much that could go wrong. How hardened against abuse are out block drivers? I figure what distinguishes this case is how utterly trivial creating a "bad" image is. Anyway, you are the block layer maintainers, so you get to decide whether to give this the full security bug treatment. I'm merely the clown who broke it %-/
On 09.01.19 15:32, Markus Armbruster wrote: > Max Reitz <mreitz@redhat.com> writes: > >> On 08.01.19 11:36, Markus Armbruster wrote: >>> Copying block maintainers for help with assessing the bug's (non-)impact >>> on security. >>> >>> Christophe Fergeau <cfergeau@redhat.com> writes: >>> >>>> On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote: >>>>> Eric Blake <eblake@redhat.com> writes: >>>>> >>>>>> On 1/2/19 12:01 PM, Christophe Fergeau wrote: >>>>>>> Adding Markus to cc: list, I forgot to do it when sending the patch. >>>>>> >>>>>> Also worth backporting via qemu-stable, now in cc. >>>>>> >>>>>>> >>>>>>> Christophe >>>>>>> >>>>>>> On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote: >>>>>>>> commit 8bca4613 added support for %% in json strings when interpolating, >>>>>>>> but in doing so, this broke handling of % when not interpolating as the >>>>>>>> '%' is skipped in both cases. >>>>>>>> This commit ensures we only try to handle %% when interpolating. >>>>> >>>>> Impact? >>>>> >>>>> If you're unable to assess, could you give us at least a reproducer? >>>> >>>> This all came from https://lists.freedesktop.org/archives/spice-devel/2018-December/046644.html >>>> Setting up a VM with libvirt with <graphics type='spice' autoport='yes' passwd='password%'/> >>>> fails to start with: >>>> qemu-system-x86_64: qobject/json-parser.c:146: parse_string: Assertion `*ptr' failed. >>>> >>>> If you use 'password%%' as the password instead, when trying to connect >>>> to the VM, you type 'password%' as the password instead of 'password%%' >>>> as configured in the domain XML. >>> >>> Thanks. >>> >>> As the commit message says, the bug bites when we parse a string >>> containing '%s' with !ctxt->ap. The parser then swallows a character. >>> If it swallows the terminating '"', it fails the assertion. >>> >>> We parse with !ctxt->ap in the following cases: >>> >>> * Tests (tests/check-qjson.c, tests/test-qobject-input-visitor.c, >>> tests/test-visitor-serialization.c) >>> >>> Plenty of tests, but we still failed to cover the buggy case :( >>> >>> * QMP input (monitor.c) >>> >>> * QGA input (qga/main.c) >>> >>> * qobject_from_json() >>> >>> - JSON pseudo-filenames (block.c) >>> >>> These are pseudo-filenames starting with "json:". >>> >>> - JSON key pairs (block/rbd.c) >>> >>> As far as I can tell, these can come only from pseudo-filenames >>> starting with "rbd:". >>> >>> - JSON command line option arguments of -display and -blockdev >>> (qobject-input-visitor.c) >>> >>> Reproducer: -blockdev '{"%"}' >>> >>> Command line, QMP and QGA input are trusted. >>> >>> Filenames are trusted when they come from command line, QMP or HMP. >>> They are untrusted when they come from from image file headers. >>> Example: QCOW2 backing file name. Note that this is *not* the security >>> boundary between host and guest. It's the boundary between host and an >>> image file from an untrusted source. >>> >>> I can't see how the bug could be exploited. Neither failing an >>> assertion nor skipping a character in a filename of your choice is >>> interesting. We don't support compiling with NDEBUG. >>> >>> Kevin, Max, do you agree? >> >> I wouldn't call it "not interesting" if adding an image to your VM at >> runtime can crash the whole thing. >> >> (qemu-img create -f qcow2 -u -b 'json:{"%"}' foo.qcow2 64M) > > "Not interesting" strictly from the point of view of exploiting the bug > to penetrate trust boundaries. > >> Whether this is a security issue... I don't know, but it is a DoS. > > I'm not sure whether feeding untrusted images to QEMU is a good idea in > general --- there's so much that could go wrong. How hardened against > abuse are out block drivers? They are supposed to handle such cases gracefully, that's for sure. At least for qcow2 we do care about it. > I figure what distinguishes this case is how utterly trivial creating a > "bad" image is. I don't think an untrusted image should be able to crash qemu. > Anyway, you are the block layer maintainers, so you get to decide > whether to give this the full security bug treatment. I'm merely the > clown who broke it %-/ Er, then I suppose it is no security bug? O:-) Max
On Wed, Jan 09, 2019 at 03:32:47PM +0100, Markus Armbruster wrote: > Max Reitz <mreitz@redhat.com> writes: > > > On 08.01.19 11:36, Markus Armbruster wrote: > >> Copying block maintainers for help with assessing the bug's (non-)impact > >> on security. > >> > >> Christophe Fergeau <cfergeau@redhat.com> writes: > >> > >>> On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote: > >>>> Eric Blake <eblake@redhat.com> writes: > >>>> > >>>>> On 1/2/19 12:01 PM, Christophe Fergeau wrote: > >>>>>> Adding Markus to cc: list, I forgot to do it when sending the patch. > >>>>> > >>>>> Also worth backporting via qemu-stable, now in cc. > >>>>> > >>>>>> > >>>>>> Christophe > >>>>>> > >>>>>> On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote: > >>>>>>> commit 8bca4613 added support for %% in json strings when interpolating, > >>>>>>> but in doing so, this broke handling of % when not interpolating as the > >>>>>>> '%' is skipped in both cases. > >>>>>>> This commit ensures we only try to handle %% when interpolating. > >>>> > >>>> Impact? > >>>> > >>>> If you're unable to assess, could you give us at least a reproducer? > >>> > >>> This all came from https://lists.freedesktop.org/archives/spice-devel/2018-December/046644.html > >>> Setting up a VM with libvirt with <graphics type='spice' autoport='yes' passwd='password%'/> > >>> fails to start with: > >>> qemu-system-x86_64: qobject/json-parser.c:146: parse_string: Assertion `*ptr' failed. > >>> > >>> If you use 'password%%' as the password instead, when trying to connect > >>> to the VM, you type 'password%' as the password instead of 'password%%' > >>> as configured in the domain XML. > >> > >> Thanks. > >> > >> As the commit message says, the bug bites when we parse a string > >> containing '%s' with !ctxt->ap. The parser then swallows a character. > >> If it swallows the terminating '"', it fails the assertion. > >> > >> We parse with !ctxt->ap in the following cases: > >> > >> * Tests (tests/check-qjson.c, tests/test-qobject-input-visitor.c, > >> tests/test-visitor-serialization.c) > >> > >> Plenty of tests, but we still failed to cover the buggy case :( > >> > >> * QMP input (monitor.c) > >> > >> * QGA input (qga/main.c) > >> > >> * qobject_from_json() > >> > >> - JSON pseudo-filenames (block.c) > >> > >> These are pseudo-filenames starting with "json:". > >> > >> - JSON key pairs (block/rbd.c) > >> > >> As far as I can tell, these can come only from pseudo-filenames > >> starting with "rbd:". > >> > >> - JSON command line option arguments of -display and -blockdev > >> (qobject-input-visitor.c) > >> > >> Reproducer: -blockdev '{"%"}' > >> > >> Command line, QMP and QGA input are trusted. > >> > >> Filenames are trusted when they come from command line, QMP or HMP. > >> They are untrusted when they come from from image file headers. > >> Example: QCOW2 backing file name. Note that this is *not* the security > >> boundary between host and guest. It's the boundary between host and an > >> image file from an untrusted source. > >> > >> I can't see how the bug could be exploited. Neither failing an > >> assertion nor skipping a character in a filename of your choice is > >> interesting. We don't support compiling with NDEBUG. > >> > >> Kevin, Max, do you agree? > > > > I wouldn't call it "not interesting" if adding an image to your VM at > > runtime can crash the whole thing. > > > > (qemu-img create -f qcow2 -u -b 'json:{"%"}' foo.qcow2 64M) > > "Not interesting" strictly from the point of view of exploiting the bug > to penetrate trust boundaries. > > > Whether this is a security issue... I don't know, but it is a DoS. > > I'm not sure whether feeding untrusted images to QEMU is a good idea in > general --- there's so much that could go wrong. How hardened against > abuse are out block drivers? > > I figure what distinguishes this case is how utterly trivial creating a > "bad" image is. Consider that you can already create a qcow2 image with a backing file of /etc/shadow. Or create a qcow2 image many EB in size that causes QEMU to allocate massive amounts of RAM and/or burn CPU time, and so on. IOW, mgmt apps should never pass untrusted images to QEMU. Crashing is just one of many bad things, and probably not the worst that can happen. They need to do validation upfront in some manner if receiving an untrustworthy image. Openstack does this by running qemu-img, with limits set on virutal memory size, CPU time, and then rejecting any image with a backing file from being used at all. > Anyway, you are the block layer maintainers, so you get to decide > whether to give this the full security bug treatment. I'm merely the > clown who broke it %-/ Accepting an image with any backing file at all from an untrusted user would be a flaw in the layered management app itself, not QEMU. So I think it would only be considered a security bug in QEMU if there was a way for an unprivileged user to trick QEMU into writing malformed JSON into an otherwise trusted image. Regards, Daniel
On 09.01.19 15:49, Daniel P. Berrangé wrote: > On Wed, Jan 09, 2019 at 03:32:47PM +0100, Markus Armbruster wrote: >> Max Reitz <mreitz@redhat.com> writes: >> >>> On 08.01.19 11:36, Markus Armbruster wrote: >>>> Copying block maintainers for help with assessing the bug's (non-)impact >>>> on security. >>>> >>>> Christophe Fergeau <cfergeau@redhat.com> writes: >>>> >>>>> On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote: >>>>>> Eric Blake <eblake@redhat.com> writes: >>>>>> >>>>>>> On 1/2/19 12:01 PM, Christophe Fergeau wrote: >>>>>>>> Adding Markus to cc: list, I forgot to do it when sending the patch. >>>>>>> >>>>>>> Also worth backporting via qemu-stable, now in cc. >>>>>>> >>>>>>>> >>>>>>>> Christophe >>>>>>>> >>>>>>>> On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote: >>>>>>>>> commit 8bca4613 added support for %% in json strings when interpolating, >>>>>>>>> but in doing so, this broke handling of % when not interpolating as the >>>>>>>>> '%' is skipped in both cases. >>>>>>>>> This commit ensures we only try to handle %% when interpolating. >>>>>> >>>>>> Impact? >>>>>> >>>>>> If you're unable to assess, could you give us at least a reproducer? >>>>> >>>>> This all came from https://lists.freedesktop.org/archives/spice-devel/2018-December/046644.html >>>>> Setting up a VM with libvirt with <graphics type='spice' autoport='yes' passwd='password%'/> >>>>> fails to start with: >>>>> qemu-system-x86_64: qobject/json-parser.c:146: parse_string: Assertion `*ptr' failed. >>>>> >>>>> If you use 'password%%' as the password instead, when trying to connect >>>>> to the VM, you type 'password%' as the password instead of 'password%%' >>>>> as configured in the domain XML. >>>> >>>> Thanks. >>>> >>>> As the commit message says, the bug bites when we parse a string >>>> containing '%s' with !ctxt->ap. The parser then swallows a character. >>>> If it swallows the terminating '"', it fails the assertion. >>>> >>>> We parse with !ctxt->ap in the following cases: >>>> >>>> * Tests (tests/check-qjson.c, tests/test-qobject-input-visitor.c, >>>> tests/test-visitor-serialization.c) >>>> >>>> Plenty of tests, but we still failed to cover the buggy case :( >>>> >>>> * QMP input (monitor.c) >>>> >>>> * QGA input (qga/main.c) >>>> >>>> * qobject_from_json() >>>> >>>> - JSON pseudo-filenames (block.c) >>>> >>>> These are pseudo-filenames starting with "json:". >>>> >>>> - JSON key pairs (block/rbd.c) >>>> >>>> As far as I can tell, these can come only from pseudo-filenames >>>> starting with "rbd:". >>>> >>>> - JSON command line option arguments of -display and -blockdev >>>> (qobject-input-visitor.c) >>>> >>>> Reproducer: -blockdev '{"%"}' >>>> >>>> Command line, QMP and QGA input are trusted. >>>> >>>> Filenames are trusted when they come from command line, QMP or HMP. >>>> They are untrusted when they come from from image file headers. >>>> Example: QCOW2 backing file name. Note that this is *not* the security >>>> boundary between host and guest. It's the boundary between host and an >>>> image file from an untrusted source. >>>> >>>> I can't see how the bug could be exploited. Neither failing an >>>> assertion nor skipping a character in a filename of your choice is >>>> interesting. We don't support compiling with NDEBUG. >>>> >>>> Kevin, Max, do you agree? >>> >>> I wouldn't call it "not interesting" if adding an image to your VM at >>> runtime can crash the whole thing. >>> >>> (qemu-img create -f qcow2 -u -b 'json:{"%"}' foo.qcow2 64M) >> >> "Not interesting" strictly from the point of view of exploiting the bug >> to penetrate trust boundaries. >> >>> Whether this is a security issue... I don't know, but it is a DoS. >> >> I'm not sure whether feeding untrusted images to QEMU is a good idea in >> general --- there's so much that could go wrong. How hardened against >> abuse are out block drivers? >> >> I figure what distinguishes this case is how utterly trivial creating a >> "bad" image is. > > Consider that you can already create a qcow2 image with a backing file > of /etc/shadow. If you cannot access this file, then it should just be an error and not crash qemu. If you can access this file, that's your own fault for bad permissions. > Or create a qcow2 image many EB in size that causes QEMU > to allocate massive amounts of RAM and/or burn CPU time, and so on. That would be the qcow2 driver's fault. We do try to open only images which are sane. And memory allocation failures should be handled gracefully, so the VM shouldn't crash. Well, at least qcow2 does its best, what Linux makes of it, who knows. (e.g. it may assign the memory to qemu and then the OOM killer may crash it later) > IOW, mgmt apps should never pass untrusted images to QEMU. Crashing is > just one of many bad things, and probably not the worst that can happen. > > They need to do validation upfront in some manner if receiving an > untrustworthy image. Openstack does this by running qemu-img, with > limits set on virutal memory size, CPU time, and then rejecting any > image with a backing file from being used at all. > >> Anyway, you are the block layer maintainers, so you get to decide >> whether to give this the full security bug treatment. I'm merely the >> clown who broke it %-/ > > Accepting an image with any backing file at all from an untrusted user > would be a flaw in the layered management app itself, not QEMU. > > So I think it would only be considered a security bug in QEMU if there was > a way for an unprivileged user to trick QEMU into writing malformed JSON > into an otherwise trusted image. Not making it a security bug makes me happy, of course, but I don't quite agree that qemu is not to blame if you pass it some image which makes it crash. Max
Max Reitz <mreitz@redhat.com> writes: > On 09.01.19 15:32, Markus Armbruster wrote: >> Max Reitz <mreitz@redhat.com> writes: >> >>> On 08.01.19 11:36, Markus Armbruster wrote: >>>> Copying block maintainers for help with assessing the bug's (non-)impact >>>> on security. >>>> >>>> Christophe Fergeau <cfergeau@redhat.com> writes: >>>> >>>>> On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote: >>>>>> Eric Blake <eblake@redhat.com> writes: >>>>>> >>>>>>> On 1/2/19 12:01 PM, Christophe Fergeau wrote: >>>>>>>> Adding Markus to cc: list, I forgot to do it when sending the patch. >>>>>>> >>>>>>> Also worth backporting via qemu-stable, now in cc. >>>>>>> >>>>>>>> >>>>>>>> Christophe >>>>>>>> >>>>>>>> On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote: >>>>>>>>> commit 8bca4613 added support for %% in json strings when interpolating, >>>>>>>>> but in doing so, this broke handling of % when not interpolating as the >>>>>>>>> '%' is skipped in both cases. >>>>>>>>> This commit ensures we only try to handle %% when interpolating. >>>>>> >>>>>> Impact? >>>>>> >>>>>> If you're unable to assess, could you give us at least a reproducer? >>>>> >>>>> This all came from https://lists.freedesktop.org/archives/spice-devel/2018-December/046644.html >>>>> Setting up a VM with libvirt with <graphics type='spice' autoport='yes' passwd='password%'/> >>>>> fails to start with: >>>>> qemu-system-x86_64: qobject/json-parser.c:146: parse_string: Assertion `*ptr' failed. >>>>> >>>>> If you use 'password%%' as the password instead, when trying to connect >>>>> to the VM, you type 'password%' as the password instead of 'password%%' >>>>> as configured in the domain XML. >>>> >>>> Thanks. >>>> >>>> As the commit message says, the bug bites when we parse a string >>>> containing '%s' with !ctxt->ap. The parser then swallows a character. >>>> If it swallows the terminating '"', it fails the assertion. >>>> >>>> We parse with !ctxt->ap in the following cases: >>>> >>>> * Tests (tests/check-qjson.c, tests/test-qobject-input-visitor.c, >>>> tests/test-visitor-serialization.c) >>>> >>>> Plenty of tests, but we still failed to cover the buggy case :( >>>> >>>> * QMP input (monitor.c) >>>> >>>> * QGA input (qga/main.c) >>>> >>>> * qobject_from_json() >>>> >>>> - JSON pseudo-filenames (block.c) >>>> >>>> These are pseudo-filenames starting with "json:". >>>> >>>> - JSON key pairs (block/rbd.c) >>>> >>>> As far as I can tell, these can come only from pseudo-filenames >>>> starting with "rbd:". >>>> >>>> - JSON command line option arguments of -display and -blockdev >>>> (qobject-input-visitor.c) >>>> >>>> Reproducer: -blockdev '{"%"}' >>>> >>>> Command line, QMP and QGA input are trusted. >>>> >>>> Filenames are trusted when they come from command line, QMP or HMP. >>>> They are untrusted when they come from from image file headers. >>>> Example: QCOW2 backing file name. Note that this is *not* the security >>>> boundary between host and guest. It's the boundary between host and an >>>> image file from an untrusted source. >>>> >>>> I can't see how the bug could be exploited. Neither failing an >>>> assertion nor skipping a character in a filename of your choice is >>>> interesting. We don't support compiling with NDEBUG. >>>> >>>> Kevin, Max, do you agree? >>> >>> I wouldn't call it "not interesting" if adding an image to your VM at >>> runtime can crash the whole thing. >>> >>> (qemu-img create -f qcow2 -u -b 'json:{"%"}' foo.qcow2 64M) >> >> "Not interesting" strictly from the point of view of exploiting the bug >> to penetrate trust boundaries. >> >>> Whether this is a security issue... I don't know, but it is a DoS. >> >> I'm not sure whether feeding untrusted images to QEMU is a good idea in >> general --- there's so much that could go wrong. How hardened against >> abuse are out block drivers? > > They are supposed to handle such cases gracefully, that's for sure. At > least for qcow2 we do care about it. > >> I figure what distinguishes this case is how utterly trivial creating a >> "bad" image is. > > I don't think an untrusted image should be able to crash qemu. "Should" in the sense of "if they don't, it's a bug, and we'll do what we can to fix it", or "if they don't, I'll be surprised"? >> Anyway, you are the block layer maintainers, so you get to decide >> whether to give this the full security bug treatment. I'm merely the >> clown who broke it %-/ > > Er, then I suppose it is no security bug? O:-) I'm not charging toll for the bridge I built for you ;)
On 09.01.19 17:20, Markus Armbruster wrote: > Max Reitz <mreitz@redhat.com> writes: > >> On 09.01.19 15:32, Markus Armbruster wrote: >>> Max Reitz <mreitz@redhat.com> writes: >>> >>>> On 08.01.19 11:36, Markus Armbruster wrote: >>>>> Copying block maintainers for help with assessing the bug's (non-)impact >>>>> on security. >>>>> >>>>> Christophe Fergeau <cfergeau@redhat.com> writes: >>>>> >>>>>> On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote: >>>>>>> Eric Blake <eblake@redhat.com> writes: >>>>>>> >>>>>>>> On 1/2/19 12:01 PM, Christophe Fergeau wrote: >>>>>>>>> Adding Markus to cc: list, I forgot to do it when sending the patch. >>>>>>>> >>>>>>>> Also worth backporting via qemu-stable, now in cc. >>>>>>>> >>>>>>>>> >>>>>>>>> Christophe >>>>>>>>> >>>>>>>>> On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote: >>>>>>>>>> commit 8bca4613 added support for %% in json strings when interpolating, >>>>>>>>>> but in doing so, this broke handling of % when not interpolating as the >>>>>>>>>> '%' is skipped in both cases. >>>>>>>>>> This commit ensures we only try to handle %% when interpolating. >>>>>>> >>>>>>> Impact? >>>>>>> >>>>>>> If you're unable to assess, could you give us at least a reproducer? >>>>>> >>>>>> This all came from https://lists.freedesktop.org/archives/spice-devel/2018-December/046644.html >>>>>> Setting up a VM with libvirt with <graphics type='spice' autoport='yes' passwd='password%'/> >>>>>> fails to start with: >>>>>> qemu-system-x86_64: qobject/json-parser.c:146: parse_string: Assertion `*ptr' failed. >>>>>> >>>>>> If you use 'password%%' as the password instead, when trying to connect >>>>>> to the VM, you type 'password%' as the password instead of 'password%%' >>>>>> as configured in the domain XML. >>>>> >>>>> Thanks. >>>>> >>>>> As the commit message says, the bug bites when we parse a string >>>>> containing '%s' with !ctxt->ap. The parser then swallows a character. >>>>> If it swallows the terminating '"', it fails the assertion. >>>>> >>>>> We parse with !ctxt->ap in the following cases: >>>>> >>>>> * Tests (tests/check-qjson.c, tests/test-qobject-input-visitor.c, >>>>> tests/test-visitor-serialization.c) >>>>> >>>>> Plenty of tests, but we still failed to cover the buggy case :( >>>>> >>>>> * QMP input (monitor.c) >>>>> >>>>> * QGA input (qga/main.c) >>>>> >>>>> * qobject_from_json() >>>>> >>>>> - JSON pseudo-filenames (block.c) >>>>> >>>>> These are pseudo-filenames starting with "json:". >>>>> >>>>> - JSON key pairs (block/rbd.c) >>>>> >>>>> As far as I can tell, these can come only from pseudo-filenames >>>>> starting with "rbd:". >>>>> >>>>> - JSON command line option arguments of -display and -blockdev >>>>> (qobject-input-visitor.c) >>>>> >>>>> Reproducer: -blockdev '{"%"}' >>>>> >>>>> Command line, QMP and QGA input are trusted. >>>>> >>>>> Filenames are trusted when they come from command line, QMP or HMP. >>>>> They are untrusted when they come from from image file headers. >>>>> Example: QCOW2 backing file name. Note that this is *not* the security >>>>> boundary between host and guest. It's the boundary between host and an >>>>> image file from an untrusted source. >>>>> >>>>> I can't see how the bug could be exploited. Neither failing an >>>>> assertion nor skipping a character in a filename of your choice is >>>>> interesting. We don't support compiling with NDEBUG. >>>>> >>>>> Kevin, Max, do you agree? >>>> >>>> I wouldn't call it "not interesting" if adding an image to your VM at >>>> runtime can crash the whole thing. >>>> >>>> (qemu-img create -f qcow2 -u -b 'json:{"%"}' foo.qcow2 64M) >>> >>> "Not interesting" strictly from the point of view of exploiting the bug >>> to penetrate trust boundaries. >>> >>>> Whether this is a security issue... I don't know, but it is a DoS. >>> >>> I'm not sure whether feeding untrusted images to QEMU is a good idea in >>> general --- there's so much that could go wrong. How hardened against >>> abuse are out block drivers? >> >> They are supposed to handle such cases gracefully, that's for sure. At >> least for qcow2 we do care about it. >> >>> I figure what distinguishes this case is how utterly trivial creating a >>> "bad" image is. >> >> I don't think an untrusted image should be able to crash qemu. > > "Should" in the sense of "if they don't, it's a bug, and we'll do what > we can to fix it", or "if they don't, I'll be surprised"? Depends. If it's Linux's VMM design (lazy allocation + OOM killer), I don't care. If there is something we can do to fix it, I do think it's a bug. Max >>> Anyway, you are the block layer maintainers, so you get to decide >>> whether to give this the full security bug treatment. I'm merely the >>> clown who broke it %-/ >> >> Er, then I suppose it is no security bug? O:-) > > I'm not charging toll for the bridge I built for you ;
On Wed, Jan 09, 2019 at 04:02:28PM +0100, Max Reitz wrote: > On 09.01.19 15:49, Daniel P. Berrangé wrote: > > On Wed, Jan 09, 2019 at 03:32:47PM +0100, Markus Armbruster wrote: > >> Max Reitz <mreitz@redhat.com> writes: > >> > >>> On 08.01.19 11:36, Markus Armbruster wrote: > >>>> Copying block maintainers for help with assessing the bug's (non-)impact > >>>> on security. > >>>> > >>>> Christophe Fergeau <cfergeau@redhat.com> writes: > >>>> > >>>>> On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote: > >>>>>> Eric Blake <eblake@redhat.com> writes: > >>>>>> > >>>>>>> On 1/2/19 12:01 PM, Christophe Fergeau wrote: > >>>>>>>> Adding Markus to cc: list, I forgot to do it when sending the patch. > >>>>>>> > >>>>>>> Also worth backporting via qemu-stable, now in cc. > >>>>>>> > >>>>>>>> > >>>>>>>> Christophe > >>>>>>>> > >>>>>>>> On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote: > >>>>>>>>> commit 8bca4613 added support for %% in json strings when interpolating, > >>>>>>>>> but in doing so, this broke handling of % when not interpolating as the > >>>>>>>>> '%' is skipped in both cases. > >>>>>>>>> This commit ensures we only try to handle %% when interpolating. > >>>>>> > >>>>>> Impact? > >>>>>> > >>>>>> If you're unable to assess, could you give us at least a reproducer? > >>>>> > >>>>> This all came from https://lists.freedesktop.org/archives/spice-devel/2018-December/046644.html > >>>>> Setting up a VM with libvirt with <graphics type='spice' autoport='yes' passwd='password%'/> > >>>>> fails to start with: > >>>>> qemu-system-x86_64: qobject/json-parser.c:146: parse_string: Assertion `*ptr' failed. > >>>>> > >>>>> If you use 'password%%' as the password instead, when trying to connect > >>>>> to the VM, you type 'password%' as the password instead of 'password%%' > >>>>> as configured in the domain XML. > >>>> > >>>> Thanks. > >>>> > >>>> As the commit message says, the bug bites when we parse a string > >>>> containing '%s' with !ctxt->ap. The parser then swallows a character. > >>>> If it swallows the terminating '"', it fails the assertion. > >>>> > >>>> We parse with !ctxt->ap in the following cases: > >>>> > >>>> * Tests (tests/check-qjson.c, tests/test-qobject-input-visitor.c, > >>>> tests/test-visitor-serialization.c) > >>>> > >>>> Plenty of tests, but we still failed to cover the buggy case :( > >>>> > >>>> * QMP input (monitor.c) > >>>> > >>>> * QGA input (qga/main.c) > >>>> > >>>> * qobject_from_json() > >>>> > >>>> - JSON pseudo-filenames (block.c) > >>>> > >>>> These are pseudo-filenames starting with "json:". > >>>> > >>>> - JSON key pairs (block/rbd.c) > >>>> > >>>> As far as I can tell, these can come only from pseudo-filenames > >>>> starting with "rbd:". > >>>> > >>>> - JSON command line option arguments of -display and -blockdev > >>>> (qobject-input-visitor.c) > >>>> > >>>> Reproducer: -blockdev '{"%"}' > >>>> > >>>> Command line, QMP and QGA input are trusted. > >>>> > >>>> Filenames are trusted when they come from command line, QMP or HMP. > >>>> They are untrusted when they come from from image file headers. > >>>> Example: QCOW2 backing file name. Note that this is *not* the security > >>>> boundary between host and guest. It's the boundary between host and an > >>>> image file from an untrusted source. > >>>> > >>>> I can't see how the bug could be exploited. Neither failing an > >>>> assertion nor skipping a character in a filename of your choice is > >>>> interesting. We don't support compiling with NDEBUG. > >>>> > >>>> Kevin, Max, do you agree? > >>> > >>> I wouldn't call it "not interesting" if adding an image to your VM at > >>> runtime can crash the whole thing. > >>> > >>> (qemu-img create -f qcow2 -u -b 'json:{"%"}' foo.qcow2 64M) > >> > >> "Not interesting" strictly from the point of view of exploiting the bug > >> to penetrate trust boundaries. > >> > >>> Whether this is a security issue... I don't know, but it is a DoS. > >> > >> I'm not sure whether feeding untrusted images to QEMU is a good idea in > >> general --- there's so much that could go wrong. How hardened against > >> abuse are out block drivers? > >> > >> I figure what distinguishes this case is how utterly trivial creating a > >> "bad" image is. > > > > Consider that you can already create a qcow2 image with a backing file > > of /etc/shadow. > > If you cannot access this file, then it should just be an error and not > crash qemu. > > If you can access this file, that's your own fault for bad permissions. > > > Or create a qcow2 image many EB in size that causes QEMU > > to allocate massive amounts of RAM and/or burn CPU time, and so on. > > That would be the qcow2 driver's fault. We do try to open only images > which are sane. The defintion of "sane" is quite hard though, as its contextual. There are things that are sane when viewed from QEMU level, which can none the less be considered a security bug from the mgmt app level. CPU/memory usage associated with huge images is in this bucket I think, given how enourmous some disk images can genuinely be. > And memory allocation failures should be handled gracefully, so the VM > shouldn't crash. Well, at least qcow2 does its best, what Linux makes > of it, who knows. (e.g. it may assign the memory to qemu and then the > OOM killer may crash it later) Yep, you'll quite likely succeed and trigger the OOM killer, or alternatively succeed and push other running VMs out to swap effectively inflicting a denial of service on them. > > IOW, mgmt apps should never pass untrusted images to QEMU. Crashing is > > just one of many bad things, and probably not the worst that can happen. > > > > They need to do validation upfront in some manner if receiving an > > untrustworthy image. Openstack does this by running qemu-img, with > > limits set on virutal memory size, CPU time, and then rejecting any > > image with a backing file from being used at all. > > > >> Anyway, you are the block layer maintainers, so you get to decide > >> whether to give this the full security bug treatment. I'm merely the > >> clown who broke it %-/ > > > > Accepting an image with any backing file at all from an untrusted user > > would be a flaw in the layered management app itself, not QEMU. > > > > So I think it would only be considered a security bug in QEMU if there was > > a way for an unprivileged user to trick QEMU into writing malformed JSON > > into an otherwise trusted image. > > Not making it a security bug makes me happy, of course, but I don't > quite agree that qemu is not to blame if you pass it some image which > makes it crash. Certainly QEMU should never crash on any input. I just think that wrt untrustworthy disk images, you need security protection well before you get to a live QEMU VM process, so I think this can be just a "normal" bug. Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Wed, Jan 09, 2019 at 04:02:28PM +0100, Max Reitz wrote: >> On 09.01.19 15:49, Daniel P. Berrangé wrote: >> > On Wed, Jan 09, 2019 at 03:32:47PM +0100, Markus Armbruster wrote: >> >> Max Reitz <mreitz@redhat.com> writes: >> >> >> >>> On 08.01.19 11:36, Markus Armbruster wrote: >> >>>> Copying block maintainers for help with assessing the bug's (non-)impact >> >>>> on security. >> >>>> >> >>>> Christophe Fergeau <cfergeau@redhat.com> writes: >> >>>> >> >>>>> On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote: >> >>>>>> Eric Blake <eblake@redhat.com> writes: >> >>>>>> >> >>>>>>> On 1/2/19 12:01 PM, Christophe Fergeau wrote: >> >>>>>>>> Adding Markus to cc: list, I forgot to do it when sending the patch. >> >>>>>>> >> >>>>>>> Also worth backporting via qemu-stable, now in cc. >> >>>>>>> >> >>>>>>>> >> >>>>>>>> Christophe >> >>>>>>>> >> >>>>>>>> On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote: >> >>>>>>>>> commit 8bca4613 added support for %% in json strings when interpolating, >> >>>>>>>>> but in doing so, this broke handling of % when not interpolating as the >> >>>>>>>>> '%' is skipped in both cases. >> >>>>>>>>> This commit ensures we only try to handle %% when interpolating. >> >>>>>> >> >>>>>> Impact? >> >>>>>> >> >>>>>> If you're unable to assess, could you give us at least a reproducer? >> >>>>> >> >>>>> This all came from https://lists.freedesktop.org/archives/spice-devel/2018-December/046644.html >> >>>>> Setting up a VM with libvirt with <graphics type='spice' autoport='yes' passwd='password%'/> >> >>>>> fails to start with: >> >>>>> qemu-system-x86_64: qobject/json-parser.c:146: parse_string: Assertion `*ptr' failed. >> >>>>> >> >>>>> If you use 'password%%' as the password instead, when trying to connect >> >>>>> to the VM, you type 'password%' as the password instead of 'password%%' >> >>>>> as configured in the domain XML. >> >>>> >> >>>> Thanks. >> >>>> >> >>>> As the commit message says, the bug bites when we parse a string >> >>>> containing '%s' with !ctxt->ap. The parser then swallows a character. >> >>>> If it swallows the terminating '"', it fails the assertion. >> >>>> >> >>>> We parse with !ctxt->ap in the following cases: >> >>>> >> >>>> * Tests (tests/check-qjson.c, tests/test-qobject-input-visitor.c, >> >>>> tests/test-visitor-serialization.c) >> >>>> >> >>>> Plenty of tests, but we still failed to cover the buggy case :( >> >>>> >> >>>> * QMP input (monitor.c) >> >>>> >> >>>> * QGA input (qga/main.c) >> >>>> >> >>>> * qobject_from_json() >> >>>> >> >>>> - JSON pseudo-filenames (block.c) >> >>>> >> >>>> These are pseudo-filenames starting with "json:". >> >>>> >> >>>> - JSON key pairs (block/rbd.c) >> >>>> >> >>>> As far as I can tell, these can come only from pseudo-filenames >> >>>> starting with "rbd:". >> >>>> >> >>>> - JSON command line option arguments of -display and -blockdev >> >>>> (qobject-input-visitor.c) >> >>>> >> >>>> Reproducer: -blockdev '{"%"}' >> >>>> >> >>>> Command line, QMP and QGA input are trusted. >> >>>> >> >>>> Filenames are trusted when they come from command line, QMP or HMP. >> >>>> They are untrusted when they come from from image file headers. >> >>>> Example: QCOW2 backing file name. Note that this is *not* the security >> >>>> boundary between host and guest. It's the boundary between host and an >> >>>> image file from an untrusted source. >> >>>> >> >>>> I can't see how the bug could be exploited. Neither failing an >> >>>> assertion nor skipping a character in a filename of your choice is >> >>>> interesting. We don't support compiling with NDEBUG. >> >>>> >> >>>> Kevin, Max, do you agree? >> >>> >> >>> I wouldn't call it "not interesting" if adding an image to your VM at >> >>> runtime can crash the whole thing. >> >>> >> >>> (qemu-img create -f qcow2 -u -b 'json:{"%"}' foo.qcow2 64M) >> >> >> >> "Not interesting" strictly from the point of view of exploiting the bug >> >> to penetrate trust boundaries. >> >> >> >>> Whether this is a security issue... I don't know, but it is a DoS. >> >> >> >> I'm not sure whether feeding untrusted images to QEMU is a good idea in >> >> general --- there's so much that could go wrong. How hardened against >> >> abuse are out block drivers? >> >> >> >> I figure what distinguishes this case is how utterly trivial creating a >> >> "bad" image is. >> > >> > Consider that you can already create a qcow2 image with a backing file >> > of /etc/shadow. >> >> If you cannot access this file, then it should just be an error and not >> crash qemu. >> >> If you can access this file, that's your own fault for bad permissions. >> >> > Or create a qcow2 image many EB in size that causes QEMU >> > to allocate massive amounts of RAM and/or burn CPU time, and so on. >> >> That would be the qcow2 driver's fault. We do try to open only images >> which are sane. > > The defintion of "sane" is quite hard though, as its contextual. There are > things that are sane when viewed from QEMU level, which can none the less > be considered a security bug from the mgmt app level. CPU/memory usage > associated with huge images is in this bucket I think, given how enourmous > some disk images can genuinely be. > >> And memory allocation failures should be handled gracefully, so the VM >> shouldn't crash. Well, at least qcow2 does its best, what Linux makes >> of it, who knows. (e.g. it may assign the memory to qemu and then the >> OOM killer may crash it later) > > Yep, you'll quite likely succeed and trigger the OOM killer, or > alternatively succeed and push other running VMs out to swap > effectively inflicting a denial of service on them. > >> > IOW, mgmt apps should never pass untrusted images to QEMU. Crashing is >> > just one of many bad things, and probably not the worst that can happen. >> > >> > They need to do validation upfront in some manner if receiving an >> > untrustworthy image. Openstack does this by running qemu-img, with >> > limits set on virutal memory size, CPU time, and then rejecting any >> > image with a backing file from being used at all. >> > >> >> Anyway, you are the block layer maintainers, so you get to decide >> >> whether to give this the full security bug treatment. I'm merely the >> >> clown who broke it %-/ >> > >> > Accepting an image with any backing file at all from an untrusted user >> > would be a flaw in the layered management app itself, not QEMU. >> > >> > So I think it would only be considered a security bug in QEMU if there was >> > a way for an unprivileged user to trick QEMU into writing malformed JSON >> > into an otherwise trusted image. >> >> Not making it a security bug makes me happy, of course, but I don't >> quite agree that qemu is not to blame if you pass it some image which >> makes it crash. No argument, it's definitely a bug. What we've been debating (as far as I'm concerned) is whether it's an exploitable security hole. I don't think it is, and it sounds like nobody really disagrees. The ability to run random crap downloaded from the internet as root with SELinux turned off is not a security hole. It's to be filed under "don't do that". Likewise, the ability to make a virtual machine abort() by feeding it untrusted images is not a security hole, it's a "don't do that". > Certainly QEMU should never crash on any input. I just think that wrt > untrustworthy disk images, you need security protection well before you > get to a live QEMU VM process, so I think this can be just a "normal" bug. Concur.
On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > > > On 1/2/19 12:01 PM, Christophe Fergeau wrote: > >> Adding Markus to cc: list, I forgot to do it when sending the patch. > > > > Also worth backporting via qemu-stable, now in cc. > > > >> > >> Christophe > >> > >> On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote: > >>> commit 8bca4613 added support for %% in json strings when interpolating, > >>> but in doing so, this broke handling of % when not interpolating as the > >>> '%' is skipped in both cases. > >>> This commit ensures we only try to handle %% when interpolating. > > Impact? > > If you're unable to assess, could you give us at least a reproducer? This affects the block/curl.c driver. Here's a simple reproducer: https://bugzilla.redhat.com/show_bug.cgi?id=1668244#c3 Rich.
On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote: > commit 8bca4613 added support for %% in json strings when interpolating, > but in doing so, this broke handling of % when not interpolating as the > '%' is skipped in both cases. > This commit ensures we only try to handle %% when interpolating. > > Signed-off-by: Christophe Fergeau <cfergeau@redhat.com> > --- > qobject/json-parser.c | 10 ++++++---- > tests/check-qjson.c | 5 +++++ > 2 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/qobject/json-parser.c b/qobject/json-parser.c > index 7a7ae9e8d1..d8eb210c0c 100644 > --- a/qobject/json-parser.c > +++ b/qobject/json-parser.c > @@ -208,11 +208,13 @@ static QString *parse_string(JSONParserContext *ctxt, JSONToken *token) > } > break; > case '%': > - if (ctxt->ap && ptr[1] != '%') { > - parse_error(ctxt, token, "can't interpolate into string"); > - goto out; > + if (ctxt->ap) { > + if (ptr[1] != '%') { > + parse_error(ctxt, token, "can't interpolate into string"); > + goto out; > + } > + ptr++; > } > - ptr++; > /* fall through */ > default: > cp = mod_utf8_codepoint(ptr, 6, &end); > diff --git a/tests/check-qjson.c b/tests/check-qjson.c > index d876a7a96e..fa2afccb0a 100644 > --- a/tests/check-qjson.c > +++ b/tests/check-qjson.c > @@ -175,6 +175,11 @@ static void utf8_string(void) > "\xCE\xBA\xE1\xBD\xB9\xCF\x83\xCE\xBC\xCE\xB5", > "\xCE\xBA\xE1\xBD\xB9\xCF\x83\xCE\xBC\xCE\xB5", > "\\u03BA\\u1F79\\u03C3\\u03BC\\u03B5", > + }, > + /* '%' character when not interpolating */ > + { > + "100%", > + "100%", > }, > /* 2 Boundary condition test cases */ > /* 2.1 First possible sequence of a certain length */ Tested-by: Richard W.M. Jones <rjones@redhat.com> as a fix for: https://bugzilla.redhat.com/show_bug.cgi?id=1668244#c3 Rich.
Markus Armbruster <armbru@redhat.com> writes: > Eric Blake <eblake@redhat.com> writes: > >> On 1/2/19 12:01 PM, Christophe Fergeau wrote: >>> Adding Markus to cc: list, I forgot to do it when sending the patch. >> >> Also worth backporting via qemu-stable, now in cc. >> >>> >>> Christophe >>> >>> On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote: >>>> commit 8bca4613 added support for %% in json strings when interpolating, >>>> but in doing so, this broke handling of % when not interpolating as the >>>> '%' is skipped in both cases. >>>> This commit ensures we only try to handle %% when interpolating. > > Impact? > > If you're unable to assess, could you give us at least a reproducer? > >>>> Signed-off-by: Christophe Fergeau <cfergeau@redhat.com> >>>> --- >>>> qobject/json-parser.c | 10 ++++++---- >>>> tests/check-qjson.c | 5 +++++ >>>> 2 files changed, 11 insertions(+), 4 deletions(-) >>>> >> >> Reviewed-by: Eric Blake <eblake@redhat.com> > > Patch looks good to me, but I'd like us to improve the commit message. Let me try: json: Fix % handling when not interpolating Commit 8bca4613 added support for %% in json strings when interpolating, but in doing so broke handling of % when not interpolating. When parse_string() is fed a string token containing '%', it skips the '%' regardless of ctxt->ap, i.e. even it's not interpolating. If the '%' is the string's last character, it fails an assertion. Else, it "merely" swallows the '%'. Fix parse_string() to handle '%' specially only when interpolating. To gauge the bug's impact, let's review non-interpolating users of this parser, i.e. code passing NULL context to json_message_parser_init(): * tests/check-qjson.c, tests/test-qobject-input-visitor.c, tests/test-visitor-serialization.c Plenty of tests, but we still failed to cover the buggy case. * monitor.c: QMP input * qga/main.c: QGA input * qobject_from_json(): - qobject-input-visitor.c: JSON command line option arguments of -display and -blockdev Reproducer: -blockdev '{"%"}' - block.c: JSON pseudo-filenames starting with "json:" Reproducer: https://bugzilla.redhat.com/show_bug.cgi?id=1668244#c3 - block/rbd.c: JSON key pairs Pseudo-filenames starting with "rbd:". Command line, QMP and QGA input are trusted. Filenames are trusted when they come from command line, QMP or HMP. They are untrusted when they come from from image file headers. Example: QCOW2 backing file name. Note that this is *not* the security boundary between host and guest. It's the boundary between host and an image file from an untrusted source. Neither failing an assertion nor skipping a character in a filename of your choice looks exploitable. Note that we don't support compiling with NDEBUG. Fixes: 8bca4613e6cddd948895b8db3def05950463495b Cc: qemu-stable@nongnu.org Comments?
Hey, On Thu, Jan 24, 2019 at 10:35:52AM +0100, Markus Armbruster wrote: > Markus Armbruster <armbru@redhat.com> writes: > > > Eric Blake <eblake@redhat.com> writes: > > > >> On 1/2/19 12:01 PM, Christophe Fergeau wrote: > >>> Adding Markus to cc: list, I forgot to do it when sending the patch. > >> > >> Also worth backporting via qemu-stable, now in cc. > >> > >>> > >>> Christophe > >>> > >>> On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote: > >>>> commit 8bca4613 added support for %% in json strings when interpolating, > >>>> but in doing so, this broke handling of % when not interpolating as the > >>>> '%' is skipped in both cases. > >>>> This commit ensures we only try to handle %% when interpolating. > > > > Impact? > > > > If you're unable to assess, could you give us at least a reproducer? > > > >>>> Signed-off-by: Christophe Fergeau <cfergeau@redhat.com> > >>>> --- > >>>> qobject/json-parser.c | 10 ++++++---- > >>>> tests/check-qjson.c | 5 +++++ > >>>> 2 files changed, 11 insertions(+), 4 deletions(-) > >>>> > >> > >> Reviewed-by: Eric Blake <eblake@redhat.com> > > > > Patch looks good to me, but I'd like us to improve the commit message. > > Let me try: > > json: Fix % handling when not interpolating > > Commit 8bca4613 added support for %% in json strings when interpolating, > but in doing so broke handling of % when not interpolating. > > When parse_string() is fed a string token containing '%', it skips the > '%' regardless of ctxt->ap, i.e. even it's not interpolating. If the > '%' is the string's last character, it fails an assertion. Else, it > "merely" swallows the '%'. > > Fix parse_string() to handle '%' specially only when interpolating. > > To gauge the bug's impact, let's review non-interpolating users of this > parser, i.e. code passing NULL context to json_message_parser_init(): > > * tests/check-qjson.c, tests/test-qobject-input-visitor.c, > tests/test-visitor-serialization.c > > Plenty of tests, but we still failed to cover the buggy case. > > * monitor.c: QMP input > > * qga/main.c: QGA input > > * qobject_from_json(): > > - qobject-input-visitor.c: JSON command line option arguments of > -display and -blockdev > > Reproducer: -blockdev '{"%"}' > > - block.c: JSON pseudo-filenames starting with "json:" > > Reproducer: https://bugzilla.redhat.com/show_bug.cgi?id=1668244#c3 > > - block/rbd.c: JSON key pairs > > Pseudo-filenames starting with "rbd:". > > Command line, QMP and QGA input are trusted. > > Filenames are trusted when they come from command line, QMP or HMP. > They are untrusted when they come from from image file headers. > Example: QCOW2 backing file name. Note that this is *not* the security > boundary between host and guest. It's the boundary between host and an > image file from an untrusted source. > > Neither failing an assertion nor skipping a character in a filename of > your choice looks exploitable. Note that we don't support compiling > with NDEBUG. > > Fixes: 8bca4613e6cddd948895b8db3def05950463495b > Cc: qemu-stable@nongnu.org > > Comments? This looks good to me, thanks for the expanded log! Christophe
Christophe Fergeau <cfergeau@redhat.com> writes: > commit 8bca4613 added support for %% in json strings when interpolating, > but in doing so, this broke handling of % when not interpolating as the > '%' is skipped in both cases. > This commit ensures we only try to handle %% when interpolating. > > Signed-off-by: Christophe Fergeau <cfergeau@redhat.com> Queued with commit message improvements, thanks!
On 1/24/19 3:35 AM, Markus Armbruster wrote: > To gauge the bug's impact, let's review non-interpolating users of this > parser, i.e. code passing NULL context to json_message_parser_init(): > > * tests/check-qjson.c, tests/test-qobject-input-visitor.c, > tests/test-visitor-serialization.c > > Plenty of tests, but we still failed to cover the buggy case. > > * monitor.c: QMP input > > * qga/main.c: QGA input > > * qobject_from_json(): > > - qobject-input-visitor.c: JSON command line option arguments of > -display and -blockdev > > Reproducer: -blockdev '{"%"}' > > - block.c: JSON pseudo-filenames starting with "json:" > > Reproducer: https://bugzilla.redhat.com/show_bug.cgi?id=1668244#c3 > > - block/rbd.c: JSON key pairs > > Pseudo-filenames starting with "rbd:". > Missed curl as being impacted. You'd have to do a v2 pull request to mention it now... https://bugzilla.redhat.com/show_bug.cgi?id=1668244
Eric Blake <eblake@redhat.com> writes: > On 1/24/19 3:35 AM, Markus Armbruster wrote: > >> To gauge the bug's impact, let's review non-interpolating users of this >> parser, i.e. code passing NULL context to json_message_parser_init(): >> >> * tests/check-qjson.c, tests/test-qobject-input-visitor.c, >> tests/test-visitor-serialization.c >> >> Plenty of tests, but we still failed to cover the buggy case. >> >> * monitor.c: QMP input >> >> * qga/main.c: QGA input >> >> * qobject_from_json(): >> >> - qobject-input-visitor.c: JSON command line option arguments of >> -display and -blockdev >> >> Reproducer: -blockdev '{"%"}' >> >> - block.c: JSON pseudo-filenames starting with "json:" >> >> Reproducer: https://bugzilla.redhat.com/show_bug.cgi?id=1668244#c3 >> >> - block/rbd.c: JSON key pairs >> >> Pseudo-filenames starting with "rbd:". >> > > Missed curl as being impacted. You'd have to do a v2 pull request to > mention it now... > > https://bugzilla.redhat.com/show_bug.cgi?id=1668244 Isn't that an instance of 'JSON pseudo-filenames starting with "json:"'?
On 1/24/19 12:29 PM, Markus Armbruster wrote: >>> - block.c: JSON pseudo-filenames starting with "json:" >>> >>> Reproducer: https://bugzilla.redhat.com/show_bug.cgi?id=1668244#c3 >>> >>> - block/rbd.c: JSON key pairs >>> >>> Pseudo-filenames starting with "rbd:". >>> >> >> Missed curl as being impacted. You'd have to do a v2 pull request to >> mention it now... >> >> https://bugzilla.redhat.com/show_bug.cgi?id=1668244 > > Isn't that an instance of 'JSON pseudo-filenames starting with "json:"'? Indeed - and I even linked to the same BZ without realizing it. Nothing further to see here, I'll go back to hiding in the corner...
diff --git a/qobject/json-parser.c b/qobject/json-parser.c index 7a7ae9e8d1..d8eb210c0c 100644 --- a/qobject/json-parser.c +++ b/qobject/json-parser.c @@ -208,11 +208,13 @@ static QString *parse_string(JSONParserContext *ctxt, JSONToken *token) } break; case '%': - if (ctxt->ap && ptr[1] != '%') { - parse_error(ctxt, token, "can't interpolate into string"); - goto out; + if (ctxt->ap) { + if (ptr[1] != '%') { + parse_error(ctxt, token, "can't interpolate into string"); + goto out; + } + ptr++; } - ptr++; /* fall through */ default: cp = mod_utf8_codepoint(ptr, 6, &end); diff --git a/tests/check-qjson.c b/tests/check-qjson.c index d876a7a96e..fa2afccb0a 100644 --- a/tests/check-qjson.c +++ b/tests/check-qjson.c @@ -175,6 +175,11 @@ static void utf8_string(void) "\xCE\xBA\xE1\xBD\xB9\xCF\x83\xCE\xBC\xCE\xB5", "\xCE\xBA\xE1\xBD\xB9\xCF\x83\xCE\xBC\xCE\xB5", "\\u03BA\\u1F79\\u03C3\\u03BC\\u03B5", + }, + /* '%' character when not interpolating */ + { + "100%", + "100%", }, /* 2 Boundary condition test cases */ /* 2.1 First possible sequence of a certain length */
commit 8bca4613 added support for %% in json strings when interpolating, but in doing so, this broke handling of % when not interpolating as the '%' is skipped in both cases. This commit ensures we only try to handle %% when interpolating. Signed-off-by: Christophe Fergeau <cfergeau@redhat.com> --- qobject/json-parser.c | 10 ++++++---- tests/check-qjson.c | 5 +++++ 2 files changed, 11 insertions(+), 4 deletions(-)