1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] qemu: qapi: Limit traversal depth for QAPI schema queries

Peter Krempa posted 1 patch 4 years, 7 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/64307faba8f8ff0a611a1f621914f85ac13e2f4d.1567758795.git.pkrempa@redhat.com
src/qemu/qemu_qapi.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
[libvirt] [PATCH] qemu: qapi: Limit traversal depth for QAPI schema queries
Posted by Peter Krempa 4 years, 7 months ago 
Implicitly the query depth is limited by the length of the QAPI schema
query, but 'alternate' and 'array' QAPI meta-types don't consume a part
of the query string thus a loop on such types would get our traversal
code stuck in an infinite loop. Prevent this from happening by limiting
the nesting depth to 1000.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_qapi.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/src/qemu/qemu_qapi.c b/src/qemu/qemu_qapi.c
index 0226d6c659..93fcae0d44 100644
--- a/src/qemu/qemu_qapi.c
+++ b/src/qemu/qemu_qapi.c
@@ -74,9 +74,23 @@ struct virQEMUQAPISchemaTraverseContext {
     virHashTablePtr schema;
     char **queries;
     virJSONValuePtr returnType;
+    size_t depth;
 };


+static int
+virQEMUQAPISchemaTraverseContextValidateDepth(struct virQEMUQAPISchemaTraverseContext *ctxt)
+{
+    if (ctxt->depth++ > 1000) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("possible loop in QMP schema"));
+        return -1;
+    }
+
+    return 0;
+}
+
+
 static void
 virQEMUQAPISchemaTraverseContextInit(struct virQEMUQAPISchemaTraverseContext *ctxt,
                                      char **queries,
@@ -329,6 +343,9 @@ virQEMUQAPISchemaTraverse(const char *baseName,
     const char *metatype;
     size_t i;

+    if (virQEMUQAPISchemaTraverseContextValidateDepth(ctxt) < 0)
+        return -2;
+
     if (!(cur = virHashLookup(ctxt->schema, baseName)))
         return -2;

-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: qapi: Limit traversal depth for QAPI schema queries
Posted by Daniel P. Berrangé 4 years, 7 months ago 
On Fri, Sep 06, 2019 at 10:33:15AM +0200, Peter Krempa wrote:
> Implicitly the query depth is limited by the length of the QAPI schema
> query, but 'alternate' and 'array' QAPI meta-types don't consume a part
> of the query string thus a loop on such types would get our traversal
> code stuck in an infinite loop. Prevent this from happening by limiting
> the nesting depth to 1000.

I'm not too clear on what 'depth' is applying to here ? Is this
the level of nesting in the JSON compound types we're following,
or is it something else ?

I ask because YAJL limits JSON nesting to only 128. So 1000 is
almost an order of magnitude larger.

> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/qemu/qemu_qapi.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/src/qemu/qemu_qapi.c b/src/qemu/qemu_qapi.c
> index 0226d6c659..93fcae0d44 100644
> --- a/src/qemu/qemu_qapi.c
> +++ b/src/qemu/qemu_qapi.c
> @@ -74,9 +74,23 @@ struct virQEMUQAPISchemaTraverseContext {
>      virHashTablePtr schema;
>      char **queries;
>      virJSONValuePtr returnType;
> +    size_t depth;
>  };
> 
> 
> +static int
> +virQEMUQAPISchemaTraverseContextValidateDepth(struct virQEMUQAPISchemaTraverseContext *ctxt)
> +{
> +    if (ctxt->depth++ > 1000) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("possible loop in QMP schema"));
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +
>  static void
>  virQEMUQAPISchemaTraverseContextInit(struct virQEMUQAPISchemaTraverseContext *ctxt,
>                                       char **queries,
> @@ -329,6 +343,9 @@ virQEMUQAPISchemaTraverse(const char *baseName,
>      const char *metatype;
>      size_t i;
> 
> +    if (virQEMUQAPISchemaTraverseContextValidateDepth(ctxt) < 0)
> +        return -2;
> +
>      if (!(cur = virHashLookup(ctxt->schema, baseName)))
>          return -2;
> 
> -- 
> 2.21.0
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: qapi: Limit traversal depth for QAPI schema queries
Posted by Peter Krempa 4 years, 7 months ago 
On Fri, Sep 06, 2019 at 09:50:41 +0100, Daniel Berrange wrote:
> On Fri, Sep 06, 2019 at 10:33:15AM +0200, Peter Krempa wrote:
> > Implicitly the query depth is limited by the length of the QAPI schema
> > query, but 'alternate' and 'array' QAPI meta-types don't consume a part
> > of the query string thus a loop on such types would get our traversal
> > code stuck in an infinite loop. Prevent this from happening by limiting
> > the nesting depth to 1000.
> 
> I'm not too clear on what 'depth' is applying to here ? Is this
> the level of nesting in the JSON compound types we're following,
> or is it something else ?
> 
> I ask because YAJL limits JSON nesting to only 128. So 1000 is
> almost an order of magnitude larger.

This is not about JSON/YAJL limits. The QAPI schema is flattened and
cross-referenced by type names ('name' field in the schema). Our
code for 'alternate' and 'array' looks up the corresponding type and
moves to it when processing the query string without processing any part
of input.

This means that if you create a mallicious or broken QAPI
schema where the array member type (element-type)  would be exactly the
same as the type name of the array [1] our query string evaluator
would be stuck in an infinite loop. The number is crazy big, because it
only needs to prevent from loops of 'alternate' and 'arrays'. All other
cases consume an element from the query string and thus the bounds are
limited by what you are attempting to query.

[1]

query-qmp-schema:

Normal case:
    {
      "name": "[14]",
      "element-type": "14",
      "meta-type": "array"
    },

broken looping case:
    {
      "name": "[14]",
      "element-type": "[14]",
      "meta-type": "array"
    },

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: qapi: Limit traversal depth for QAPI schema queries
Posted by Daniel P. Berrangé 4 years, 7 months ago 
On Fri, Sep 06, 2019 at 10:33:15AM +0200, Peter Krempa wrote:
> Implicitly the query depth is limited by the length of the QAPI schema
> query, but 'alternate' and 'array' QAPI meta-types don't consume a part
> of the query string thus a loop on such types would get our traversal
> code stuck in an infinite loop. Prevent this from happening by limiting
> the nesting depth to 1000.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/qemu/qemu_qapi.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.