From a70a16b28bf78403dc86ae798c291ba167573d4a Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 27 Oct 2022 22:53:13 +0100 Subject: [PATCH] gvariant: Allow g_variant_byteswap() to operate on tree-form variants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This avoids needing to always serialise a variant before byteswapping it. With variants in non-normal forms, serialisation can result in a large increase in size of the variant, and a lot of allocations for leaf `GVariant`s. This can lead to a denial of service attack. Avoid that by changing byteswapping so that it happens on the tree form of the variant if the input is in non-normal form. If the input is in normal form (either serialised or in tree form), continue using the existing code as byteswapping an already-serialised normal variant is about 3× faster than byteswapping on the equivalent tree form. The existing unit tests cover byteswapping well, but need some adaptation so that they operate on tree form variants too. I considered dropping the serialised byteswapping code and doing all byteswapping on tree-form variants, as that would make maintenance simpler (avoiding having two parallel implementations of byteswapping). However, most inputs to `g_variant_byteswap()` are likely to be serialised variants (coming from a byte array of input from some foreign source) and most of them are going to be in normal form (as corruption and malicious action are rare). So getting rid of the serialised byteswapping code would impose quite a performance penalty on the common case. Signed-off-by: Philip Withnall Fixes: #2797 --- glib/gvariant.c | 87 ++++++++++++++++++++++++++++++++----------- glib/tests/gvariant.c | 51 +++++++++++++++++++++++-- 2 files changed, 112 insertions(+), 26 deletions(-) diff --git a/glib/gvariant.c b/glib/gvariant.c index f7b2e9703..5fef88d58 100644 --- a/glib/gvariant.c +++ b/glib/gvariant.c @@ -5852,7 +5852,8 @@ g_variant_iter_loop (GVariantIter *iter, /* Serialized data {{{1 */ static GVariant * -g_variant_deep_copy (GVariant *value) +g_variant_deep_copy (GVariant *value, + gboolean byteswap) { switch (g_variant_classify (value)) { @@ -5869,7 +5870,7 @@ g_variant_deep_copy (GVariant *value) for (i = 0, n_children = g_variant_n_children (value); i < n_children; i++) { GVariant *child = g_variant_get_child_value (value, i); - g_variant_builder_add_value (&builder, g_variant_deep_copy (child)); + g_variant_builder_add_value (&builder, g_variant_deep_copy (child, byteswap)); g_variant_unref (child); } @@ -5920,7 +5921,7 @@ g_variant_deep_copy (GVariant *value) * be non-normal for reasons other than invalid offset table * entries. As they are all the same type, they will all have * the same default value though, so keep that around. */ - g_variant_builder_add_value (&builder, g_variant_deep_copy (child)); + g_variant_builder_add_value (&builder, g_variant_deep_copy (child, byteswap)); } else if (child == NULL && first_invalid_child_deep_copy != NULL) { @@ -5929,7 +5930,7 @@ g_variant_deep_copy (GVariant *value) else if (child == NULL) { child = g_variant_get_child_value (value, i); - first_invalid_child_deep_copy = g_variant_ref_sink (g_variant_deep_copy (child)); + first_invalid_child_deep_copy = g_variant_ref_sink (g_variant_deep_copy (child, byteswap)); g_variant_builder_add_value (&builder, first_invalid_child_deep_copy); } @@ -5948,28 +5949,63 @@ g_variant_deep_copy (GVariant *value) return g_variant_new_byte (g_variant_get_byte (value)); case G_VARIANT_CLASS_INT16: - return g_variant_new_int16 (g_variant_get_int16 (value)); + if (byteswap) + return g_variant_new_int16 (GUINT16_SWAP_LE_BE (g_variant_get_int16 (value))); + else + return g_variant_new_int16 (g_variant_get_int16 (value)); case G_VARIANT_CLASS_UINT16: - return g_variant_new_uint16 (g_variant_get_uint16 (value)); + if (byteswap) + return g_variant_new_uint16 (GUINT16_SWAP_LE_BE (g_variant_get_uint16 (value))); + else + return g_variant_new_uint16 (g_variant_get_uint16 (value)); case G_VARIANT_CLASS_INT32: - return g_variant_new_int32 (g_variant_get_int32 (value)); + if (byteswap) + return g_variant_new_int32 (GUINT32_SWAP_LE_BE (g_variant_get_int32 (value))); + else + return g_variant_new_int32 (g_variant_get_int32 (value)); case G_VARIANT_CLASS_UINT32: - return g_variant_new_uint32 (g_variant_get_uint32 (value)); + if (byteswap) + return g_variant_new_uint32 (GUINT32_SWAP_LE_BE (g_variant_get_uint32 (value))); + else + return g_variant_new_uint32 (g_variant_get_uint32 (value)); case G_VARIANT_CLASS_INT64: - return g_variant_new_int64 (g_variant_get_int64 (value)); + if (byteswap) + return g_variant_new_int64 (GUINT64_SWAP_LE_BE (g_variant_get_int64 (value))); + else + return g_variant_new_int64 (g_variant_get_int64 (value)); case G_VARIANT_CLASS_UINT64: - return g_variant_new_uint64 (g_variant_get_uint64 (value)); + if (byteswap) + return g_variant_new_uint64 (GUINT64_SWAP_LE_BE (g_variant_get_uint64 (value))); + else + return g_variant_new_uint64 (g_variant_get_uint64 (value)); case G_VARIANT_CLASS_HANDLE: - return g_variant_new_handle (g_variant_get_handle (value)); + if (byteswap) + return g_variant_new_handle (GUINT32_SWAP_LE_BE (g_variant_get_handle (value))); + else + return g_variant_new_handle (g_variant_get_handle (value)); case G_VARIANT_CLASS_DOUBLE: - return g_variant_new_double (g_variant_get_double (value)); + if (byteswap) + { + /* We have to convert the double to a uint64 here using a union, + * because a cast will round it numerically. */ + union + { + guint64 u64; + gdouble dbl; + } u1, u2; + u1.dbl = g_variant_get_double (value); + u2.u64 = GUINT64_SWAP_LE_BE (u1.u64); + return g_variant_new_double (u2.dbl); + } + else + return g_variant_new_double (g_variant_get_double (value)); case G_VARIANT_CLASS_STRING: return g_variant_new_string (g_variant_get_string (value, NULL)); @@ -6026,7 +6062,7 @@ g_variant_get_normal_form (GVariant *value) if (g_variant_is_normal_form (value)) return g_variant_ref (value); - trusted = g_variant_deep_copy (value); + trusted = g_variant_deep_copy (value, FALSE); g_assert (g_variant_is_trusted (trusted)); return g_variant_ref_sink (trusted); @@ -6046,6 +6082,11 @@ g_variant_get_normal_form (GVariant *value) * contain multi-byte numeric data. That include strings, booleans, * bytes and containers containing only these things (recursively). * + * While this function can safely handle untrusted, non-normal data, it is + * recommended to check whether the input is in normal form beforehand, using + * g_variant_is_normal_form(), and to reject non-normal inputs if your + * application can be strict about what inputs it rejects. + * * The returned value is always in normal form and is marked as trusted. * A full, not floating, reference is returned. * @@ -6064,22 +6105,21 @@ g_variant_byteswap (GVariant *value) g_variant_type_info_query (type_info, &alignment, NULL); - if (alignment) - /* (potentially) contains multi-byte numeric data */ + if (alignment && g_variant_is_normal_form (value)) { + /* (potentially) contains multi-byte numeric data, but is also already in + * normal form so we can use a faster byteswapping codepath on the + * serialised data */ GVariantSerialised serialised = { 0, }; - GVariant *trusted; GBytes *bytes; - trusted = g_variant_get_normal_form (value); - serialised.type_info = g_variant_get_type_info (trusted); - serialised.size = g_variant_get_size (trusted); + serialised.type_info = g_variant_get_type_info (value); + serialised.size = g_variant_get_size (value); serialised.data = g_malloc (serialised.size); - serialised.depth = g_variant_get_depth (trusted); + serialised.depth = g_variant_get_depth (value); serialised.ordered_offsets_up_to = G_MAXSIZE; /* operating on the normal form */ serialised.checked_offsets_up_to = G_MAXSIZE; - g_variant_store (trusted, serialised.data); - g_variant_unref (trusted); + g_variant_store (value, serialised.data); g_variant_serialised_byteswap (serialised); @@ -6087,6 +6127,9 @@ g_variant_byteswap (GVariant *value) new = g_variant_ref_sink (g_variant_new_from_bytes (g_variant_get_type (value), bytes, TRUE)); g_bytes_unref (bytes); } + else if (alignment) + /* (potentially) contains multi-byte numeric data */ + new = g_variant_ref_sink (g_variant_deep_copy (value, TRUE)); else /* contains no multi-byte data */ new = g_variant_get_normal_form (value); diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c index 1e7ed16cc..3357488a4 100644 --- a/glib/tests/gvariant.c +++ b/glib/tests/gvariant.c @@ -2288,24 +2288,67 @@ serialise_tree (TreeInstance *tree, static void test_byteswap (void) { - GVariantSerialised one = { 0, }, two = { 0, }; + GVariantSerialised one = { 0, }, two = { 0, }, three = { 0, }; TreeInstance *tree; + GVariant *one_variant = NULL; + GVariant *two_variant = NULL; + GVariant *two_byteswapped = NULL; + GVariant *three_variant = NULL; + GVariant *three_byteswapped = NULL; + guint8 *three_data_copy = NULL; + gsize three_size_copy = 0; + /* Write a tree out twice, once normally and once byteswapped. */ tree = tree_instance_new (NULL, 3); serialise_tree (tree, &one); + one_variant = g_variant_new_from_data (G_VARIANT_TYPE (g_variant_type_info_get_type_string (one.type_info)), + one.data, one.size, FALSE, NULL, NULL); + i_am_writing_byteswapped = TRUE; serialise_tree (tree, &two); + serialise_tree (tree, &three); i_am_writing_byteswapped = FALSE; - g_variant_serialised_byteswap (two); + /* Swap the first byteswapped one back using the function we want to test. */ + two_variant = g_variant_new_from_data (G_VARIANT_TYPE (g_variant_type_info_get_type_string (two.type_info)), + two.data, two.size, FALSE, NULL, NULL); + two_byteswapped = g_variant_byteswap (two_variant); - g_assert_cmpmem (one.data, one.size, two.data, two.size); - g_assert_cmpuint (one.depth, ==, two.depth); + /* Make the second byteswapped one non-normal (hopefully), and then byteswap + * it back using the function we want to test in its non-normal mode. + * This might not work because it’s not necessarily possible to make an + * arbitrary random variant non-normal. Adding a single zero byte to the end + * often makes something non-normal but still readable. */ + three_size_copy = three.size + 1; + three_data_copy = g_malloc (three_size_copy); + memcpy (three_data_copy, three.data, three.size); + three_data_copy[three.size] = '\0'; + three_variant = g_variant_new_from_data (G_VARIANT_TYPE (g_variant_type_info_get_type_string (three.type_info)), + three_data_copy, three_size_copy, FALSE, NULL, NULL); + three_byteswapped = g_variant_byteswap (three_variant); + + /* Check they’re the same. We can always compare @one_variant and + * @two_byteswapped. We can only compare @two_byteswapped and + * @three_byteswapped if @two_variant and @three_variant are equal: in that + * case, the corruption to @three_variant was enough to make it non-normal but + * not enough to change its value. */ + g_assert_cmpvariant (one_variant, two_byteswapped); + + if (g_variant_equal (two_variant, three_variant)) + g_assert_cmpvariant (two_byteswapped, three_byteswapped); + + g_variant_unref (three_byteswapped); + g_variant_unref (three_variant); + g_variant_unref (two_byteswapped); + g_variant_unref (two_variant); + g_variant_unref (one_variant); tree_instance_free (tree); g_free (one.data); g_free (two.data); + g_free (three.data); + g_free (three_data_copy); } static void