gvariant: Allow g_variant_byteswap() to operate on tree-form variantsmcatanzaro/glib-2-68-rhel9

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 <pwithnall@endlessos.org>

Fixes: #2797

2 files changed, 115 insertions, 29 deletions

diff --git a/glib/gvariant.c b/glib/gvariant.c
index 9388c529d..1f0eccb73 100644
--- a/glib/gvariant.c
+++ b/glib/gvariant.c
@@ -5795,7 +5795,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))
     {
@@ -5812,7 +5813,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);
           }
 
@@ -5863,7 +5864,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)
               {
@@ -5872,7 +5873,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);
               }
 
@@ -5891,28 +5892,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));
@@ -5969,7 +6005,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);
@@ -5989,6 +6025,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
@@ -6006,22 +6047,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);
 
@@ -6029,6 +6069,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 871a6dab0..143057de7 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);
-
-  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