[Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Christophe Fergeau]

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

Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Christophe Fergeau]

[-- Attachment #1: Type: text/plain, Size: 2172 bytes --]

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
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 915 bytes --]

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, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1309 bytes --]

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.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Christophe Fergeau]

[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Markus Armbruster]

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?

Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3320 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Markus Armbruster]

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 %-/

Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 4366 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Daniel P. Berrangé]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 5969 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Markus Armbruster]

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 ;)

Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 5037 bytes --]

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 ;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Daniel P. Berrangé]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Richard W.M. Jones]

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.

-- 
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

[Richard W.M. Jones]

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.

-- 
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

[Markus Armbruster]

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?

Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Christophe Fergeau]

[-- Attachment #1: Type: text/plain, Size: 3421 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Markus Armbruster]

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!

Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1182 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Markus Armbruster]

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:"'?

Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 817 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]