From 0ff5e5cd32c9373d62668f94bb44be39378c2d60 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 14 Nov 2018 14:23:19 +0000 Subject: [PATCH 1/2] gdbusmessage: Gracefully handle message signatures with invalid types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With the changes to limit GVariant type nesting (commit 7c4e6e9fbe47), it’s now possible to have a valid type signature which is not a valid GVariant type when enclosed in parentheses (to make it a tuple). Check for that when parsing the signature field in a D-Bus message. Includes a unit test. oss-fuzz#11120 Signed-off-by: Philip Withnall --- gio/gdbusmessage.c | 8 +++-- gio/tests/gdbus-serialization.c | 60 +++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/gio/gdbusmessage.c b/gio/gdbusmessage.c index 169e6fd15..b9f32e921 100644 --- a/gio/gdbusmessage.c +++ b/gio/gdbusmessage.c @@ -2148,18 +2148,20 @@ g_dbus_message_new_from_blob (guchar *blob, else if (signature_str_len > 0) { GVariantType *variant_type; - gchar *tupled_signature_str; + gchar *tupled_signature_str = g_strdup_printf ("(%s)", signature_str); - if (!g_variant_is_signature (signature_str)) + if (!g_variant_is_signature (signature_str) || + !g_variant_type_string_is_valid (tupled_signature_str)) { g_set_error (error, G_IO_ERROR, G_IO_ERROR_INVALID_ARGUMENT, _("Parsed value “%s” is not a valid D-Bus signature (for body)"), signature_str); + g_free (tupled_signature_str); goto out; } - tupled_signature_str = g_strdup_printf ("(%s)", signature_str); + variant_type = g_variant_type_new (tupled_signature_str); g_free (tupled_signature_str); #ifdef DEBUG_SERIALIZER diff --git a/gio/tests/gdbus-serialization.c b/gio/tests/gdbus-serialization.c index e7c402e70..6446d940a 100644 --- a/gio/tests/gdbus-serialization.c +++ b/gio/tests/gdbus-serialization.c @@ -1205,6 +1205,64 @@ test_message_parse_multiple_signature_header (void) /* ---------------------------------------------------------------------------------------------------- */ +/* Test that an invalid header in a D-Bus message (specifically, containing a + * variant with a valid type signature that is too long to be a valid + * #GVariantType due to exceeding the array nesting limits) is gracefully + * handled with an error rather than a crash. The set of bytes here come + * directly from fuzzer output. */ +static void +test_message_parse_over_long_signature_header (void) +{ + const guint8 data[] = { + 'l', /* little-endian byte order */ + 0x20, /* message type */ + 0x20, /* message flags */ + 0x01, /* major protocol version */ + 0x20, 0x20, 0x20, 0x01, /* body length (invalid) */ + 0x20, 0x20, 0x20, 0x20, /* message serial */ + /* a{yv} of header fields: + * (things start to be even more invalid below here) */ + 0x20, 0x00, 0x00, 0x00, /* array length (in bytes) */ + 0x08, /* array key */ + /* Variant array value: */ + 0x04, /* signature length */ + 'g', 0x00, 0x20, 0x20, /* one complete type plus some rubbish */ + 0x00, /* nul terminator */ + /* (Variant array value payload) */ + /* Critically, this contains 128 nested ‘a’s, which exceeds + * %G_VARIANT_MAX_RECURSION_DEPTH. */ + 0xec, + 'a', 'b', 'g', 'd', 'u', 'd', 'd', 'd', 'd', 'd', 'd', 'd', + 'd', 'd', 'd', + 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', + 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', + 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', + 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', + 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', + 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', + 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', + 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', + 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', + 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', + 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', + 'v' + /* (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); +} + +/* ---------------------------------------------------------------------------------------------------- */ + int main (int argc, char *argv[]) @@ -1230,6 +1288,8 @@ main (int argc, test_message_parse_empty_signature_header); g_test_add_func ("/gdbus/message-parse/multiple-signature-header", test_message_parse_multiple_signature_header); + g_test_add_func ("/gdbus/message-parse/over-long-signature-header", + test_message_parse_over_long_signature_header); return g_test_run(); } From c2607ab321702a6d08adcb05d8370907d06dd7c5 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 15 Nov 2018 09:27:24 +0000 Subject: [PATCH 2/2] glib.supp: Make a suppression less specific MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sometimes valgrind doesn’t count the calloc() call at the innermost stack frame, and counts it as g_type_create_instance() instead. Signed-off-by: Philip Withnall --- glib.supp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/glib.supp b/glib.supp index 9236b3d16..9163ee3e1 100644 --- a/glib.supp +++ b/glib.supp @@ -203,7 +203,7 @@ { g-type-class-init Memcheck:Leak - fun:calloc + fun:g_type_create_instance ... fun:type_class_init_Wm }