[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 43/56] qjson: Fix qobject_from_json() & friends

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH 43/56] qjson: Fix qobject_from_json() & friends for multiple values
Date: Tue, 14 Aug 2018 08:26:00 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0

On 08/08/2018 07:03 AM, Markus Armbruster wrote:
qobject_from_json() & friends use the consume_json() callback to
receive either a value or an error from the parser.

When they are fed a string that contains more than either one JSON
value or one JSON syntax error, consume_json() gets called multiple

When the last call receives a value, qobject_from_json() returns that
value.  Any other values are leaked.

When any call receives an error, qobject_from_json() sets the first
error received.  Any other errors are thrown away.

When values follow errors, qobject_from_json() returns both a value
and sets an error.  That's bad.  Impact:

* block.c's parse_json_protocol() ignores and leaks the value.  It's
   used to to parse pseudo-filenames starting with "json:".  The
   pseudo-filenames can come from the user or from image meta-data such
   as a QCOW2 image's backing file name.

Fortunately, I don't think this falls in the category of a denial-of-service attack worthy of a CVE (memory leaks in long-running processes are bad, but to be an escalation attack, you'd have to convince someone with more rights to repeatedly reload your malicious image to cause them to suffer from the leak - but why would they load your image more than once? You can reload it yourself, but then you are only killing your own qemu so there is no escalation of privilege).

* vl.c's parse_display_qapi() ignores and leaks the error.  It's used
   to parse the argument of command line option -display.

* vl.c's main() case QEMU_OPTION_blockdev ignores the error and leaves
   it in @err.  main() will then pass a pointer to a non-null Error *
   to net_init_clients(), which is forbidden.  It can lead to assertion
   failure or other misbehavior.

* check-qjson.c's multiple_values() demonstrates the badness.

* The other callers are not affected since they only pass strings with
   exactly one JSON value or, in the case of negative tests, one

The impact on the _nofail() functions is relatively harmless.  They
abort when any call receives an error.  Else they return the last
value, and leak the others, if any.

Fix consume_json() as follows.  On the first call, save value and
error as before.  On subsequent calls, if any, don't save them.  If
the first call saved a value, the next call, if any, replaces the
value by an "Expecting at most one JSON value" error.  Take care not
to leak values or errors that aren't saved.

Signed-off-by: Markus Armbruster <address@hidden>
  qobject/qjson.c     | 15 ++++++++++++++-
  tests/check-qjson.c | 10 +++-------
  2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/qobject/qjson.c b/qobject/qjson.c
index 7395556069..7f69036487 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -33,8 +33,21 @@ static void consume_json(void *opaque, QObject *json, Error 
      JSONParsingState *s = opaque;
+ assert(!json != !err);
+    assert(!s->result || !s->err);
+    if (s->result) {

Reached whether the second item encountered was also an object (json != NULL) or an error (err != NULL), but seems reasonable.

+        qobject_unref(s->result);
+        s->result = NULL;
+        error_setg(&s->err, "Expecting at most one JSON value");
+    }
+    if (s->err) {
+        qobject_unref(json);
+        error_free(err);
+        return;
+    }

Worth spelling this clause as error_propagate(&s->err, err)? Oh, I see why you can't - you have to do the early return in the case that (json != NULL) so that you don't assign s->result again if this was a parse of a second object.

      s->result = json;
-    error_propagate(&s->err, err);
+    s->err = err;

Took me a couple of reads to verify the logic makes sense, but it looks right.

Reviewed-by: Eric Blake <address@hidden>

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]