Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
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... -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
Eric Blake 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:"'?
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
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, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
Christophe Fergeau 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 Queued with commit message improvements, thanks!
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
Hey,
On Thu, Jan 24, 2019 at 10:35:52AM +0100, Markus Armbruster wrote:
> Markus Armbruster writes:
>
> > Eric Blake 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
> ---
> qobject/json-parser.c | 10 ++
> tests/check-qjson.c | 5 +
> 2 files changed, 11 insertions(+), 4 deletions(-)
>
> >>
> >> Reviewed-by: Eric Blake
> >
> > 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-sta...@nongnu.org
>
> Comments?
This looks good to me, thanks for the expanded log!
Christophe
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
Markus Armbruster writes:
> Eric Blake 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
---
qobject/json-parser.c | 10 ++
tests/check-qjson.c | 5 +
2 files changed, 11 insertions(+), 4 deletions(-)
>>
>> Reviewed-by: Eric Blake
>
> 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-sta...@nongnu.org
Comments?
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote: > Eric Blake 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. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
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
> ---
> 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
as a fix for: https://bugzilla.redhat.com/show_bug.cgi?id=1668244#c3
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
Daniel P. Berrangé 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 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 writes:
>>
>> > On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote:
>> >> Eric Blake 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 > > 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 th
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
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 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 writes:
>
> > On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote:
> >> Eric Blake 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 > 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 thi
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
On 09.01.19 17:20, Markus Armbruster wrote:
> Max Reitz writes:
>
>> On 09.01.19 15:32, Markus Armbruster wrote:
>>> Max Reitz 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 writes:
>
>> On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote:
>>> Eric Blake 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 > 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 ;
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
Max Reitz writes:
> On 09.01.19 15:32, Markus Armbruster wrote:
>> Max Reitz 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 writes:
> On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote:
>> Eric Blake 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 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 ;)
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
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 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 writes:
> On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote:
>> Eric Blake 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 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
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
On Wed, Jan 09, 2019 at 03:32:47PM +0100, Markus Armbruster wrote:
> Max Reitz 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 writes:
> >>
> >>> On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote:
> Eric Blake 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 >>> 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
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
On 09.01.19 15:32, Markus Armbruster wrote:
> Max Reitz 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 writes:
>>>
On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote:
> Eric Blake 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 >>> 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
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
Max Reitz 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 writes:
>>
>>> On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote:
Eric Blake 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 >> 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 %-/
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
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 writes:
>
>> On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote:
>>> Eric Blake 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 > 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
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
Copying block maintainers for help with assessing the bug's (non-)impact
on security.
Christophe Fergeau writes:
> On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote:
>> Eric Blake 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 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?
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote: > Eric Blake 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 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 signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
On 1/7/19 9:47 AM, Markus Armbruster wrote: > Eric Blake 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 --- qobject/json-parser.c | 10 ++ tests/check-qjson.c | 5 + 2 files changed, 11 insertions(+), 4 deletions(-) >> >> Reviewed-by: Eric Blake > > Patch looks good to me, but I'd like us to improve the commit message. Indeed. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
Eric Blake 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 >>> --- >>> qobject/json-parser.c | 10 ++ >>> tests/check-qjson.c | 5 + >>> 2 files changed, 11 insertions(+), 4 deletions(-) >>> > > Reviewed-by: Eric Blake Patch looks good to me, but I'd like us to improve the commit message.
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
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 >> --- >> qobject/json-parser.c | 10 ++ >> tests/check-qjson.c | 5 + >> 2 files changed, 11 insertions(+), 4 deletions(-) >> Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating
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
> ---
> 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
>
>
signature.asc
Description: PGP signature
[Qemu-devel] [PATCH] json: Fix % handling when not interpolating
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
---
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
