Attachment 41245 Details for Bug 32321 – squashed patch

[patch] squashed patch

0001-CVE-2010-4352-Reject-deeply-nested-variants.patch (text/plain), 8.81 KB, created by Colin Walters on 2010-12-18 14:59:19 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Colin Walters

Created: 2010-12-18 14:59:19 UTC

Size: 8.81 KB

patch

obsolete

>From 7fab4e9e0674828ef3aaaddf2a6dc7f16408dec8 Mon Sep 17 00:00:00 2001
>From: Havoc Pennington <hp@pobox.com>
>Date: Sun, 12 Dec 2010 21:08:43 -0500
>Subject: [PATCH] CVE 2010-4352: Reject deeply nested variants
>
>Add DBUS_INVALID_NESTED_TOO_DEEPLY validity problem and a test that
>should generate it.
>
>Previously, we rejected deep nesting in the signature, but
>variants allow dynamic message nesting, conditional only
>on the depth of the message body.
>
>The nesting limit is 64, which was also the limit in static
>signatures.  Empirically, dynamic nesting depth observed on my
>Fedora 14 system doesn't exceed 2; 64 is really a huge limit.
>
>Signed-Off-By: Colin Walters <walters@verbum.org>
>
>https://bugs.freedesktop.org/show_bug.cgi?id=32321
>---
> dbus/dbus-marshal-validate.c |   30 ++++++++++++++++---
> dbus/dbus-marshal-validate.h |    1 +
> dbus/dbus-message-factory.c  |   65 ++++++++++++++++++++++++++++++++++++++++++
> doc/dbus-specification.xml   |   14 +++++----
> 4 files changed, 99 insertions(+), 11 deletions(-)
>
>diff --git a/dbus/dbus-marshal-validate.c b/dbus/dbus-marshal-validate.c
>index aa470fc..b457997 100644
>--- a/dbus/dbus-marshal-validate.c
>+++ b/dbus/dbus-marshal-validate.c
>@@ -291,16 +291,30 @@ out:
>   return result;
> }
> 
>+/* note: this function is also used to validate the header's values,
>+ * since the header is a valid body with a particular signature.
>+ */
> static DBusValidity
> validate_body_helper (DBusTypeReader       *reader,
>                       int                   byte_order,
>                       dbus_bool_t           walk_reader_to_end,
>+                      int                   total_depth,
>                       const unsigned char  *p,
>                       const unsigned char  *end,
>                       const unsigned char **new_p)
> {
>   int current_type;
> 
>+  /* The spec allows arrays and structs to each nest 32, for total
>+   * nesting of 2*32. We want to impose the same limit on "dynamic"
>+   * value nesting (not visible in the signature) which is introduced
>+   * by DBUS_TYPE_VARIANT.
>+   */
>+  if (total_depth > (DBUS_MAXIMUM_TYPE_RECURSION_DEPTH * 2))
>+    {
>+      return DBUS_INVALID_NESTED_TOO_DEEPLY;
>+    }
>+
>   while ((current_type = _dbus_type_reader_get_current_type (reader)) != DBUS_TYPE_INVALID)
>     {
>       const unsigned char *a;
>@@ -477,7 +491,9 @@ validate_body_helper (DBusTypeReader       *reader,
>                   {
>                     while (p < array_end)
>                       {
>-                        validity = validate_body_helper (&sub, byte_order, FALSE, p, end, &p);
>+                        validity = validate_body_helper (&sub, byte_order, FALSE,
>+                                                         total_depth + 1,
>+                                                         p, end, &p);
>                         if (validity != DBUS_VALID)
>                           return validity;
>                       }
>@@ -594,7 +610,9 @@ validate_body_helper (DBusTypeReader       *reader,
> 
>             _dbus_assert (_dbus_type_reader_get_current_type (&sub) != DBUS_TYPE_INVALID);
> 
>-            validity = validate_body_helper (&sub, byte_order, FALSE, p, end, &p);
>+            validity = validate_body_helper (&sub, byte_order, FALSE,
>+                                             total_depth + 1,
>+                                             p, end, &p);
>             if (validity != DBUS_VALID)
>               return validity;
> 
>@@ -623,7 +641,9 @@ validate_body_helper (DBusTypeReader       *reader,
> 
>             _dbus_type_reader_recurse (reader, &sub);
> 
>-            validity = validate_body_helper (&sub, byte_order, TRUE, p, end, &p);
>+            validity = validate_body_helper (&sub, byte_order, TRUE,
>+                                             total_depth + 1,
>+                                             p, end, &p);
>             if (validity != DBUS_VALID)
>               return validity;
>           }
>@@ -708,7 +728,7 @@ _dbus_validate_body_with_reason (const DBusString *expected_signature,
>   p = _dbus_string_get_const_data_len (value_str, value_pos, len);
>   end = p + len;
> 
>-  validity = validate_body_helper (&reader, byte_order, TRUE, p, end, &p);
>+  validity = validate_body_helper (&reader, byte_order, TRUE, 0, p, end, &p);
>   if (validity != DBUS_VALID)
>     return validity;
>   
>@@ -878,7 +898,7 @@ _dbus_validity_to_error_message (DBusValidity validity)
>     case DBUS_INVALID_DICT_ENTRY_HAS_TOO_MANY_FIELDS:              return "Dict entry has too many fields";
>     case DBUS_INVALID_DICT_ENTRY_NOT_INSIDE_ARRAY:                 return "Dict entry not inside array";
>     case DBUS_INVALID_DICT_KEY_MUST_BE_BASIC_TYPE:                 return "Dict key must be basic type";
>-
>+    case DBUS_INVALID_NESTED_TOO_DEEPLY:                           return "Variants cannot be used to create a hugely recursive tree of values";
>     default:
>       return "Invalid";
>     }
>diff --git a/dbus/dbus-marshal-validate.h b/dbus/dbus-marshal-validate.h
>index 5817de3..8947a2a 100644
>--- a/dbus/dbus-marshal-validate.h
>+++ b/dbus/dbus-marshal-validate.h
>@@ -112,6 +112,7 @@ typedef enum
>   DBUS_INVALID_DICT_ENTRY_NOT_INSIDE_ARRAY = 54,
>   DBUS_INVALID_DICT_KEY_MUST_BE_BASIC_TYPE = 55,
>   DBUS_INVALID_MISSING_UNIX_FDS = 56,
>+  DBUS_INVALID_NESTED_TOO_DEEPLY = 57,
>   DBUS_VALIDITY_LAST
> } DBusValidity;
> 
>diff --git a/dbus/dbus-message-factory.c b/dbus/dbus-message-factory.c
>index 7432cf2..7ecf827 100644
>--- a/dbus/dbus-message-factory.c
>+++ b/dbus/dbus-message-factory.c
>@@ -333,6 +333,53 @@ simple_error (void)
>   return message;
> }
> 
>+static DBusMessage*
>+message_with_nesting_levels (int levels)
>+{
>+  DBusMessage *message;
>+  dbus_int32_t v_INT32;
>+  DBusMessageIter *parents;
>+  DBusMessageIter *children;
>+  int i;
>+
>+  /* If levels is higher it breaks sig_refcount in DBusMessageRealIter
>+   * in dbus-message.c, this assert is just to help you know you need
>+   * to fix that if you hit it
>+   */
>+  _dbus_assert (levels < 256);
>+
>+  parents = dbus_new(DBusMessageIter, levels + 1);
>+  children = dbus_new(DBusMessageIter, levels + 1);
>+
>+  v_INT32 = 42;
>+  message = simple_method_call ();
>+
>+  i = 0;
>+  dbus_message_iter_init_append (message, &parents[i]);
>+  while (i < levels)
>+    {
>+      dbus_message_iter_open_container (&parents[i], DBUS_TYPE_VARIANT,
>+                                        i == (levels - 1) ?
>+                                        DBUS_TYPE_INT32_AS_STRING :
>+                                        DBUS_TYPE_VARIANT_AS_STRING,
>+                                        &children[i]);
>+      ++i;
>+      parents[i] = children[i-1];
>+    }
>+  --i;
>+  dbus_message_iter_append_basic (&children[i], DBUS_TYPE_INT32, &v_INT32);
>+  while (i >= 0)
>+    {
>+      dbus_message_iter_close_container (&parents[i], &children[i]);
>+      --i;
>+    }
>+
>+  dbus_free(parents);
>+  dbus_free(children);
>+
>+  return message;
>+}
>+
> static dbus_bool_t
> generate_special (DBusMessageDataIter   *iter,
>                   DBusString            *data,
>@@ -735,6 +782,24 @@ generate_special (DBusMessageDataIter   *iter,
>       
>       *expected_validity = DBUS_INVALID_DICT_ENTRY_HAS_NO_FIELDS;
>     }
>+  else if (item_seq == 20)
>+    {
>+      /* 64 levels of nesting is OK */
>+      message = message_with_nesting_levels(64);
>+
>+      generate_from_message (data, expected_validity, message);
>+
>+      *expected_validity = DBUS_VALID;
>+    }
>+  else if (item_seq == 21)
>+    {
>+      /* 65 levels of nesting is not OK */
>+      message = message_with_nesting_levels(65);
>+
>+      generate_from_message (data, expected_validity, message);
>+
>+      *expected_validity = DBUS_INVALID_NESTED_TOO_DEEPLY;
>+    }
>   else
>     {
>       return FALSE;
>diff --git a/doc/dbus-specification.xml b/doc/dbus-specification.xml
>index 9a5d70c..ee5aac5 100644
>--- a/doc/dbus-specification.xml
>+++ b/doc/dbus-specification.xml
>@@ -620,12 +620,14 @@
> 	      </row><row>
>                 <entry><literal>VARIANT</literal></entry>
>                 <entry>
>-                  A variant type has a marshaled <literal>SIGNATURE</literal>
>-                  followed by a marshaled value with the type
>-                  given in the signature.
>-                  Unlike a message signature, the variant signature 
>-                  can contain only a single complete type.
>-                  So "i", "ai" or "(ii)" is OK, but "ii" is not.
>+                  A variant type has a marshaled
>+                  <literal>SIGNATURE</literal> followed by a marshaled
>+                  value with the type given in the signature.  Unlike
>+                  a message signature, the variant signature can
>+                  contain only a single complete type.  So "i", "ai"
>+                  or "(ii)" is OK, but "ii" is not.  Use of variants may not
>+                  cause a total message depth to be larger than 64, including
>+		  other container types such as structures.
>                 </entry>
>                 <entry>
>                   1 (alignment of the signature)
>-- 
>1.7.3.3
>

Actions: View | Splinter Review

Attachments on bug 32321: 41018 | 41049 | 41050 | 41245

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.