From 96d792197a34e22b0f40c659c482f119018d7cfd Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 29 Oct 2019 18:46:39 +0000 Subject: [PATCH 1/3] gdbusmessage: Move variable initialisation to declaration time Tidies up the code a bit, but introduces no functional changes. Signed-off-by: Philip Withnall --- gio/gdbusmessage.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/gio/gdbusmessage.c b/gio/gdbusmessage.c index 852a70430..9914caa51 100644 --- a/gio/gdbusmessage.c +++ b/gio/gdbusmessage.c @@ -1443,8 +1443,8 @@ parse_value_from_blob (GMemoryBuffer *buf, guint indent, GError **error) { - GVariant *ret; - GError *local_error; + GVariant *ret = NULL; + GError *local_error = NULL; #ifdef DEBUG_SERIALIZER gboolean is_leaf; #endif /* DEBUG_SERIALIZER */ @@ -1465,12 +1465,9 @@ parse_value_from_blob (GMemoryBuffer *buf, } #endif /* DEBUG_SERIALIZER */ - ret = NULL; - #ifdef DEBUG_SERIALIZER is_leaf = TRUE; #endif /* DEBUG_SERIALIZER */ - local_error = NULL; switch (type_string[0]) { case 'b': /* G_VARIANT_TYPE_BOOLEAN */ From de2236584ddeb2a902bc087765c2fa8e43282612 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 30 Oct 2019 15:48:57 +0000 Subject: [PATCH 2/3] tests: Tidy up test case naming in gdbus-serialization test Signed-off-by: Philip Withnall --- gio/tests/gdbus-serialization.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/gio/tests/gdbus-serialization.c b/gio/tests/gdbus-serialization.c index 89a2e97d5..ae6514b0f 100644 --- a/gio/tests/gdbus-serialization.c +++ b/gio/tests/gdbus-serialization.c @@ -627,7 +627,7 @@ check_serialization (GVariant *value, } static void -message_serialize_basic (void) +test_message_serialize_basic (void) { check_serialization (NULL, ""); @@ -661,7 +661,7 @@ message_serialize_basic (void) /* ---------------------------------------------------------------------------------------------------- */ static void -message_serialize_complex (void) +test_message_serialize_complex (void) { GError *error; GVariant *value; @@ -749,7 +749,7 @@ replace (char *blob, } static void -message_serialize_invalid (void) +test_message_serialize_invalid (void) { guint n; @@ -842,7 +842,7 @@ message_serialize_invalid (void) /* ---------------------------------------------------------------------------------------------------- */ static void -message_serialize_header_checks (void) +test_message_serialize_header_checks (void) { GDBusMessage *message; GDBusMessage *reply; @@ -996,7 +996,7 @@ message_serialize_header_checks (void) /* ---------------------------------------------------------------------------------------------------- */ static void -message_parse_empty_arrays_of_arrays (void) +test_message_parse_empty_arrays_of_arrays (void) { GVariant *body; GError *error = NULL; @@ -1052,7 +1052,7 @@ message_parse_empty_arrays_of_arrays (void) /* ---------------------------------------------------------------------------------------------------- */ static void -test_double_array (void) +test_message_serialize_double_array (void) { GVariantBuilder builder; GVariant *body; @@ -1262,15 +1262,19 @@ main (int argc, g_test_init (&argc, &argv, NULL); g_test_bug_base ("https://bugzilla.gnome.org/show_bug.cgi?id="); - g_test_add_func ("/gdbus/message-serialize-basic", message_serialize_basic); - g_test_add_func ("/gdbus/message-serialize-complex", message_serialize_complex); - g_test_add_func ("/gdbus/message-serialize-invalid", message_serialize_invalid); - g_test_add_func ("/gdbus/message-serialize-header-checks", message_serialize_header_checks); + g_test_add_func ("/gdbus/message-serialize/basic", + test_message_serialize_basic); + g_test_add_func ("/gdbus/message-serialize/complex", + test_message_serialize_complex); + g_test_add_func ("/gdbus/message-serialize/invalid", + test_message_serialize_invalid); + g_test_add_func ("/gdbus/message-serialize/header-checks", + test_message_serialize_header_checks); + g_test_add_func ("/gdbus/message-serialize/double-array", + test_message_serialize_double_array); - g_test_add_func ("/gdbus/message-parse-empty-arrays-of-arrays", - message_parse_empty_arrays_of_arrays); - - g_test_add_func ("/gdbus/message-serialize/double-array", test_double_array); + g_test_add_func ("/gdbus/message-parse/empty-arrays-of-arrays", + test_message_parse_empty_arrays_of_arrays); g_test_add_func ("/gdbus/message-parse/non-signature-header", test_message_parse_non_signature_header); g_test_add_func ("/gdbus/message-parse/empty-signature-header", From 5054b48b7ccf7e040a0770427645d67bf2b44f2d Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 29 Oct 2019 18:47:02 +0000 Subject: [PATCH 3/3] gdbusmessage: Limit recursion of variants in D-Bus messages This is the analogue of commit 7c4e6e9fbe, but applied to the `GDBusMessage` parser, which does its own top-level parsing of the variant format in D-Bus messages. Previously, this code allowed arbitrary recursion of variant containers, which could lead to a stack overflow. Now, that recursion is limited to 64 levels, as per the D-Bus specification: https://dbus.freedesktop.org/doc/dbus-specification.html#message-protocol-marshaling-signature This includes a new unit test. oss-fuzz#14870 Signed-off-by: Philip Withnall --- gio/gdbusmessage.c | 49 +++++++++ gio/tests/gdbus-serialization.c | 179 ++++++++++++++++++++++++++++++-- 2 files changed, 221 insertions(+), 7 deletions(-) diff --git a/gio/gdbusmessage.c b/gio/gdbusmessage.c index 9914caa51..6e3bd8bf4 100644 --- a/gio/gdbusmessage.c +++ b/gio/gdbusmessage.c @@ -58,6 +58,10 @@ #include "glibintl.h" +/* See https://dbus.freedesktop.org/doc/dbus-specification.html#message-protocol-marshaling-signature + * This is 64 containers plus 1 value within them. */ +#define G_DBUS_MAX_TYPE_DEPTH (64 + 1) + typedef struct _GMemoryBuffer GMemoryBuffer; struct _GMemoryBuffer { @@ -1439,6 +1443,7 @@ read_bytes (GMemoryBuffer *mbuf, static GVariant * parse_value_from_blob (GMemoryBuffer *buf, const GVariantType *type, + guint max_depth, gboolean just_align, guint indent, GError **error) @@ -1450,6 +1455,15 @@ parse_value_from_blob (GMemoryBuffer *buf, #endif /* DEBUG_SERIALIZER */ const gchar *type_string; + if (max_depth == 0) + { + g_set_error_literal (&local_error, + G_IO_ERROR, + G_IO_ERROR_INVALID_ARGUMENT, + _("Value nested too deeply")); + goto fail; + } + type_string = g_variant_type_peek_string (type); #ifdef DEBUG_SERIALIZER @@ -1687,6 +1701,17 @@ parse_value_from_blob (GMemoryBuffer *buf, goto fail; } + if (max_depth == 1) + { + /* If we had recursed into parse_value_from_blob() again to + * parse the array values, this would have been emitted. */ + g_set_error_literal (&local_error, + G_IO_ERROR, + G_IO_ERROR_INVALID_ARGUMENT, + _("Value nested too deeply")); + goto fail; + } + ensure_input_padding (buf, fixed_size); array_data = read_bytes (buf, array_len, &local_error); if (array_data == NULL) @@ -1714,6 +1739,7 @@ parse_value_from_blob (GMemoryBuffer *buf, GVariant *item G_GNUC_UNUSED /* when compiling with G_DISABLE_ASSERT */; item = parse_value_from_blob (buf, element_type, + max_depth - 1, TRUE, indent + 2, NULL); @@ -1728,6 +1754,7 @@ parse_value_from_blob (GMemoryBuffer *buf, GVariant *item; item = parse_value_from_blob (buf, element_type, + max_depth - 1, FALSE, indent + 2, &local_error); @@ -1767,6 +1794,7 @@ parse_value_from_blob (GMemoryBuffer *buf, key_type = g_variant_type_key (type); key = parse_value_from_blob (buf, key_type, + max_depth - 1, FALSE, indent + 2, &local_error); @@ -1775,6 +1803,7 @@ parse_value_from_blob (GMemoryBuffer *buf, value_type = g_variant_type_value (type); value = parse_value_from_blob (buf, value_type, + max_depth - 1, FALSE, indent + 2, &local_error); @@ -1809,6 +1838,7 @@ parse_value_from_blob (GMemoryBuffer *buf, GVariant *item; item = parse_value_from_blob (buf, element_type, + max_depth - 1, FALSE, indent + 2, &local_error); @@ -1855,9 +1885,26 @@ parse_value_from_blob (GMemoryBuffer *buf, sig); goto fail; } + + if (max_depth <= g_variant_type_string_get_depth_ (sig)) + { + /* Catch the type nesting being too deep without having to + * parse the data. We don’t have to check this for static + * container types (like arrays and tuples, above) because + * the g_variant_type_string_is_valid() check performed before + * the initial parse_value_from_blob() call should check the + * static type nesting. */ + g_set_error_literal (&local_error, + G_IO_ERROR, + G_IO_ERROR_INVALID_ARGUMENT, + _("Value nested too deeply")); + goto fail; + } + variant_type = g_variant_type_new (sig); value = parse_value_from_blob (buf, variant_type, + max_depth - 1, FALSE, indent + 2, &local_error); @@ -2095,6 +2142,7 @@ g_dbus_message_new_from_blob (guchar *blob, #endif /* DEBUG_SERIALIZER */ headers = parse_value_from_blob (&mbuf, G_VARIANT_TYPE ("a{yv}"), + G_DBUS_MAX_TYPE_DEPTH + 2 /* for the a{yv} */, FALSE, 2, error); @@ -2166,6 +2214,7 @@ g_dbus_message_new_from_blob (guchar *blob, #endif /* DEBUG_SERIALIZER */ message->body = parse_value_from_blob (&mbuf, variant_type, + G_DBUS_MAX_TYPE_DEPTH + 1 /* for the surrounding tuple */, FALSE, 2, error); diff --git a/gio/tests/gdbus-serialization.c b/gio/tests/gdbus-serialization.c index ae6514b0f..2ca28d99b 100644 --- a/gio/tests/gdbus-serialization.c +++ b/gio/tests/gdbus-serialization.c @@ -514,9 +514,8 @@ get_body_signature (GVariant *value) } /* If @value is floating, this assumes ownership. */ -static void -check_serialization (GVariant *value, - const gchar *expected_dbus_1_output) +static gchar * +get_and_check_serialization (GVariant *value) { guchar *blob; gsize blob_size; @@ -525,7 +524,7 @@ check_serialization (GVariant *value, GDBusMessage *recovered_message; GError *error; DBusError dbus_error; - gchar *s; + gchar *s = NULL; guint n; message = g_dbus_message_new (); @@ -597,9 +596,6 @@ check_serialization (GVariant *value, s = dbus_1_message_print (dbus_1_message); dbus_message_unref (dbus_1_message); - g_assert_cmpstr (s, ==, expected_dbus_1_output); - g_free (s); - /* Then serialize back and check that the body is identical */ error = NULL; @@ -624,6 +620,18 @@ check_serialization (GVariant *value, } g_object_unref (message); + + return g_steal_pointer (&s); +} + +/* If @value is floating, this assumes ownership. */ +static void +check_serialization (GVariant *value, + const gchar *expected_dbus_1_output) +{ + gchar *s = get_and_check_serialization (value); + g_assert_cmpstr (s, ==, expected_dbus_1_output); + g_free (s); } static void @@ -665,6 +673,8 @@ test_message_serialize_complex (void) { GError *error; GVariant *value; + guint i; + gchar *serialization = NULL; error = NULL; @@ -724,6 +734,37 @@ test_message_serialize_complex (void) " unix-fd: (not extracted)\n"); g_variant_unref (value); #endif + + /* Deep nesting of variants (just below the recursion limit). */ + value = g_variant_new_string ("buried"); + for (i = 0; i < 64; i++) + value = g_variant_new_variant (value); + value = g_variant_new_tuple (&value, 1); + + serialization = get_and_check_serialization (value); + g_assert_nonnull (serialization); + g_assert_true (g_str_has_prefix (serialization, + "value 0: variant:\n" + " variant:\n" + " variant:\n")); + g_free (serialization); + + /* Deep nesting of arrays and structs (just below the recursion limit). + * See https://dbus.freedesktop.org/doc/dbus-specification.html#message-protocol-marshaling-signature */ + value = g_variant_new_string ("hello"); + for (i = 0; i < 32; i++) + value = g_variant_new_tuple (&value, 1); + for (i = 0; i < 32; i++) + value = g_variant_new_array (NULL, &value, 1); + value = g_variant_new_tuple (&value, 1); + + serialization = get_and_check_serialization (value); + g_assert_nonnull (serialization); + g_assert_true (g_str_has_prefix (serialization, + "value 0: array:\n" + " array:\n" + " array:\n")); + g_free (serialization); } @@ -1252,6 +1293,126 @@ test_message_parse_over_long_signature_header (void) /* ---------------------------------------------------------------------------------------------------- */ +/* Test that an invalid header in a D-Bus message (specifically, containing too + * many levels of nested variant) is gracefully handled with an error rather + * than a crash. The set of bytes here come almost directly from fuzzer output. */ +static void +test_message_parse_deep_header_nesting (void) +{ + const guint8 data[] = { + 'l', /* little-endian byte order */ + 0x20, /* message type */ + 0x20, /* message flags */ + 0x01, /* major protocol version */ + 0x20, 0x20, 0x20, 0x00, /* body length (invalid) */ + 0x20, 0x20, 0x20, 0x20, /* message serial */ + /* a{yv} of header fields: + * (things start to be even more invalid below here) */ + 0x20, 0x20, 0x20, 0x00, /* array length (in bytes) */ + 0x20, /* array key (this is not currently a valid header field) */ + /* Variant array value: */ + 0x01, /* signature length */ + 'v', /* one complete type */ + 0x00, /* nul terminator */ + /* (Variant array value payload) */ + /* Critically, this contains 64 nested variants (minus two for the + * ‘arbitrary valid content’ below, but ignoring two for the `a{yv}` + * above), which in total exceeds %G_DBUS_MAX_TYPE_DEPTH. */ + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, + /* Some arbitrary valid content inside the innermost variant: */ + 0x01, 'y', 0x00, 0xcc, + /* (message body length missing) */ + }; + gsize size = sizeof (data); + GDBusMessage *message = NULL; + GError *local_error = NULL; + + message = g_dbus_message_new_from_blob ((guchar *) data, size, + G_DBUS_CAPABILITY_FLAGS_NONE, + &local_error); + g_assert_error (local_error, G_IO_ERROR, G_IO_ERROR_INVALID_ARGUMENT); + g_assert_null (message); + + g_clear_error (&local_error); +} + +/* ---------------------------------------------------------------------------------------------------- */ + +/* Test that an invalid body in a D-Bus message (specifically, containing too + * many levels of nested variant) is gracefully handled with an error rather + * than a crash. The set of bytes here are a modified version of the bytes from + * test_message_parse_deep_header_nesting(). */ +static void +test_message_parse_deep_body_nesting (void) +{ + const guint8 data[] = { + 'l', /* little-endian byte order */ + 0x20, /* message type */ + 0x20, /* message flags */ + 0x01, /* major protocol version */ + 0x20, 0x20, 0x20, 0x00, /* body length (invalid) */ + 0x20, 0x20, 0x20, 0x20, /* message serial */ + /* a{yv} of header fields: */ + 0x07, 0x00, 0x00, 0x00, /* array length (in bytes) */ + 0x08, /* array key (signature field) */ + /* Variant array value: */ + 0x01, /* signature length */ + 'g', /* one complete type */ + 0x00, /* nul terminator */ + /* (Variant array value payload) */ + 0x01, 'v', 0x00, + /* End-of-header padding to reach an 8-byte boundary: */ + 0x00, + /* Message body: over 64 levels of nested variant, which is not valid: */ + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, 0x01, 'v', 0x00, + /* Some arbitrary valid content inside the innermost variant: */ + 0x01, 'y', 0x00, 0xcc, + }; + gsize size = sizeof (data); + GDBusMessage *message = NULL; + GError *local_error = NULL; + + message = g_dbus_message_new_from_blob ((guchar *) data, size, + G_DBUS_CAPABILITY_FLAGS_NONE, + &local_error); + g_assert_error (local_error, G_IO_ERROR, G_IO_ERROR_INVALID_ARGUMENT); + g_assert_null (message); + + g_clear_error (&local_error); +} + +/* ---------------------------------------------------------------------------------------------------- */ + int main (int argc, char *argv[]) @@ -1283,6 +1444,10 @@ main (int argc, test_message_parse_multiple_signature_header); g_test_add_func ("/gdbus/message-parse/over-long-signature-header", test_message_parse_over_long_signature_header); + g_test_add_func ("/gdbus/message-parse/deep-header-nesting", + test_message_parse_deep_header_nesting); + g_test_add_func ("/gdbus/message-parse/deep-body-nesting", + test_message_parse_deep_body_nesting); return g_test_run(); }