From 446e69f5edd72deb2196dee36bbaf8056caf6948 Mon Sep 17 00:00:00 2001 From: William Manley Date: Wed, 9 Aug 2023 10:39:34 +0000 Subject: [PATCH] gvariant-serialiser: Factor out functions for dealing with framing offsets This introduces no functional changes. Helps: #2121 CVE: CVE-2023-32665 Upstream-Status: Backport from [https://gitlab.gnome.org/GNOME/glib/-/commit/446e69f5edd72deb2196dee36bbaf8056caf6948] Signed-off-by: Siddharth Doshi --- glib/gvariant.c | 81 +++++++++++++++++++++++++++++++++---------- glib/tests/gvariant.c | 57 ++++++++++++++++++++++++++---- 2 files changed, 112 insertions(+), 26 deletions(-) diff --git a/glib/gvariant.c b/glib/gvariant.c index 4dbd9e8..a80c2c9 100644 --- a/glib/gvariant.c +++ b/glib/gvariant.c @@ -5788,7 +5788,8 @@ g_variant_iter_loop (GVariantIter *iter, /* Serialised data {{{1 */ static GVariant * -g_variant_deep_copy (GVariant *value) +g_variant_deep_copy (GVariant *value, + gboolean byteswap) { switch (g_variant_classify (value)) { @@ -5806,7 +5807,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); } @@ -5820,28 +5821,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)); @@ -5896,7 +5932,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); @@ -5916,6 +5952,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. * * Returns: (transfer full): the byteswapped form of @value @@ -5933,21 +5974,20 @@ 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.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); @@ -5955,6 +5995,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 3dda08e..679dd40 100644 --- a/glib/tests/gvariant.c +++ b/glib/tests/gvariant.c @@ -2284,24 +2284,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); - - g_assert_cmpmem (one.data, one.size, two.data, two.size); - g_assert_cmpuint (one.depth, ==, two.depth); - + /* 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); + + /* 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 -- 2.24.4