summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorHavoc Pennington <hp@pobox.com>2010-12-13 02:08:43 (GMT)
committerWill Thompson <will.thompson@collabora.co.uk>2010-12-20 21:39:00 (GMT)
commit7d65a3a6ed8815e34a99c680ac3869fde49dbbd4 (patch)
tree230fe4f9beaa1be2eb5137b3103fde668cf6f8df
parentf2905def7b65970724e57c2d57202de8d7138e98 (diff)
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>
-rw-r--r--dbus/dbus-marshal-validate.c30
-rw-r--r--dbus/dbus-marshal-validate.h1
-rw-r--r--dbus/dbus-message-factory.c65
-rw-r--r--doc/dbus-specification.xml14
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)