From 7d65a3a6ed8815e34a99c680ac3869fde49dbbd4 Mon Sep 17 00:00:00 2001
From: Havoc Pennington <hp@pobox.com>
Date: Mon, 13 Dec 2010 02:08:43 +0000
Subject: 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.

https://bugs.freedesktop.org/show_bug.cgi?id=32321

Signed-Off-By: Colin Walters <walters@verbum.org>
Signed-off-by: Will Thompson <will.thompson@collabora.co.uk>
---
Index: dbus-1.2.10/dbus/dbus-marshal-validate.c
===================================================================
--- dbus-1.2.10.orig/dbus/dbus-marshal-validate.c
+++ dbus-1.2.10/dbus/dbus-marshal-validate.c
@@ -288,16 +288,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;
@@ -425,7 +439,7 @@ validate_body_helper (DBusTypeReader
                      * big blocks of ints/bytes/etc.
                      */                     
                     
-                    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;
                   }
@@ -541,7 +555,9 @@ validate_body_helper (DBusTypeReader
 
             _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;
 
@@ -570,7 +586,9 @@ validate_body_helper (DBusTypeReader
 
             _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;
           }
@@ -655,7 +673,7 @@ _dbus_validate_body_with_reason (const D
   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;
   
Index: dbus-1.2.10/dbus/dbus-marshal-validate.h
===================================================================
--- dbus-1.2.10.orig/dbus/dbus-marshal-validate.h
+++ dbus-1.2.10/dbus/dbus-marshal-validate.h
@@ -117,6 +117,7 @@ typedef enum
   DBUS_INVALID_DICT_ENTRY_HAS_TOO_MANY_FIELDS = 53,
   DBUS_INVALID_DICT_ENTRY_NOT_INSIDE_ARRAY = 54,
   DBUS_INVALID_DICT_KEY_MUST_BE_BASIC_TYPE = 55,
+  DBUS_INVALID_NESTED_TOO_DEEPLY = 56,
   DBUS_VALIDITY_LAST
 } DBusValidity;
 
Index: dbus-1.2.10/dbus/dbus-message-factory.c
===================================================================
--- dbus-1.2.10.orig/dbus/dbus-message-factory.c
+++ dbus-1.2.10/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
       
       *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;
Index: dbus-1.2.10/doc/dbus-specification.xml
===================================================================
--- dbus-1.2.10.orig/doc/dbus-specification.xml
+++ dbus-1.2.10/doc/dbus-specification.xml
@@ -561,12 +561,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)
