From 590f7a6b76b5a3695aa562c9c3ab244433b6624b Mon Sep 17 00:00:00 2001
From: William Manley <will@stb-tester.com>
Date: Tue, 23 Jun 2020 22:59:58 +0100
Subject: [PATCH 01/18] gvariant-core: Consolidate construction of
 `GVariantSerialised`

So I only need to change it in one place.

This introduces no functional changes.

Helps: #2121
---
 glib/gvariant-core.c | 49 ++++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 24 deletions(-)

diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c
index ad0bab5b62..70c712ddc0 100644
--- a/glib/gvariant-core.c
+++ b/glib/gvariant-core.c
@@ -351,6 +351,27 @@ g_variant_ensure_size (GVariant *value)
     }
 }
 
+/* < private >
+ * g_variant_to_serialised:
+ * @value: a #GVariant
+ *
+ * Gets a GVariantSerialised for a GVariant in state STATE_SERIALISED.
+ */
+inline static GVariantSerialised
+g_variant_to_serialised (GVariant *value)
+{
+  g_assert (value->state & STATE_SERIALISED);
+  {
+    GVariantSerialised serialised = {
+      value->type_info,
+      (gpointer) value->contents.serialised.data,
+      value->size,
+      value->depth,
+    };
+    return serialised;
+  }
+}
+
 /* < private >
  * g_variant_serialise:
  * @value: a #GVariant
@@ -1009,16 +1030,8 @@ g_variant_n_children (GVariant *value)
   g_variant_lock (value);
 
   if (value->state & STATE_SERIALISED)
-    {
-      GVariantSerialised serialised = {
-        value->type_info,
-        (gpointer) value->contents.serialised.data,
-        value->size,
-        value->depth,
-      };
-
-      n_children = g_variant_serialised_n_children (serialised);
-    }
+    n_children = g_variant_serialised_n_children (
+        g_variant_to_serialised (value));
   else
     n_children = value->contents.tree.n_children;
 
@@ -1085,12 +1098,7 @@ g_variant_get_child_value (GVariant *value,
     }
 
   {
-    GVariantSerialised serialised = {
-      value->type_info,
-      (gpointer) value->contents.serialised.data,
-      value->size,
-      value->depth,
-    };
+    GVariantSerialised serialised = g_variant_to_serialised (value);
     GVariantSerialised s_child;
     GVariant *child;
 
@@ -1203,14 +1211,7 @@ g_variant_is_normal_form (GVariant *value)
 
   if (value->state & STATE_SERIALISED)
     {
-      GVariantSerialised serialised = {
-        value->type_info,
-        (gpointer) value->contents.serialised.data,
-        value->size,
-        value->depth
-      };
-
-      if (g_variant_serialised_is_normal (serialised))
+      if (g_variant_serialised_is_normal (g_variant_to_serialised (value)))
         value->state |= STATE_TRUSTED;
     }
   else
-- 
GitLab


From f8f5d8eefa06008aa8fe684069193dc3b1ae1b58 Mon Sep 17 00:00:00 2001
From: William Manley <will@stb-tester.com>
Date: Thu, 25 Jun 2020 17:08:21 +0100
Subject: [PATCH 02/18] gvariant-serialiser: Factor out functions for dealing
 with framing offsets

This introduces no functional changes.

Helps: #2121
---
 glib/gvariant-serialiser.c | 108 +++++++++++++++++++------------------
 1 file changed, 57 insertions(+), 51 deletions(-)

diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c
index 3af68b79b2..09492e4526 100644
--- a/glib/gvariant-serialiser.c
+++ b/glib/gvariant-serialiser.c
@@ -635,30 +635,62 @@ gvs_calculate_total_size (gsize body_size,
   return body_size + 8 * offsets;
 }
 
+struct Offsets
+{
+  gsize     data_size;
+
+  guchar   *array;
+  gsize     length;
+  guint     offset_size;
+
+  gboolean  is_normal;
+};
+
 static gsize
-gvs_variable_sized_array_n_children (GVariantSerialised value)
+gvs_offsets_get_offset_n (struct Offsets *offsets,
+                          gsize           n)
+{
+  return gvs_read_unaligned_le (
+      offsets->array + (offsets->offset_size * n), offsets->offset_size);
+}
+
+static struct Offsets
+gvs_variable_sized_array_get_frame_offsets (GVariantSerialised value)
 {
+  struct Offsets out = { 0, };
   gsize offsets_array_size;
-  gsize offset_size;
   gsize last_end;
 
   if (value.size == 0)
-    return 0;
-
-  offset_size = gvs_get_offset_size (value.size);
+    {
+      out.is_normal = TRUE;
+      return out;
+    }
 
-  last_end = gvs_read_unaligned_le (value.data + value.size -
-                                    offset_size, offset_size);
+  out.offset_size = gvs_get_offset_size (value.size);
+  last_end = gvs_read_unaligned_le (value.data + value.size - out.offset_size,
+                                    out.offset_size);
 
   if (last_end > value.size)
-    return 0;
+    return out;  /* offsets not normal */
 
   offsets_array_size = value.size - last_end;
 
-  if (offsets_array_size % offset_size)
-    return 0;
+  if (offsets_array_size % out.offset_size)
+    return out;  /* offsets not normal */
+
+  out.data_size = last_end;
+  out.array = value.data + last_end;
+  out.length = offsets_array_size / out.offset_size;
+  out.is_normal = TRUE;
 
-  return offsets_array_size / offset_size;
+  return out;
+}
+
+static gsize
+gvs_variable_sized_array_n_children (GVariantSerialised value)
+{
+  return gvs_variable_sized_array_get_frame_offsets (value).length;
 }
 
 static GVariantSerialised
@@ -666,8 +698,9 @@ gvs_variable_sized_array_get_child (GVariantSerialised value,
                                     gsize              index_)
 {
   GVariantSerialised child = { 0, };
-  gsize offset_size;
-  gsize last_end;
+
+  struct Offsets offsets = gvs_variable_sized_array_get_frame_offsets (value);
+
   gsize start;
   gsize end;
 
@@ -675,18 +708,11 @@ gvs_variable_sized_array_get_child (GVariantSerialised value,
   g_variant_type_info_ref (child.type_info);
   child.depth = value.depth + 1;
 
-  offset_size = gvs_get_offset_size (value.size);
-
-  last_end = gvs_read_unaligned_le (value.data + value.size -
-                                    offset_size, offset_size);
-
   if (index_ > 0)
     {
       guint alignment;
 
-      start = gvs_read_unaligned_le (value.data + last_end +
-                                     (offset_size * (index_ - 1)),
-                                     offset_size);
+      start = gvs_offsets_get_offset_n (&offsets, index_ - 1);
 
       g_variant_type_info_query (child.type_info, &alignment, NULL);
       start += (-start) & alignment;
@@ -694,11 +720,9 @@ gvs_variable_sized_array_get_child (GVariantSerialised value,
   else
     start = 0;
 
-  end = gvs_read_unaligned_le (value.data + last_end +
-                               (offset_size * index_),
-                               offset_size);
+  end = gvs_offsets_get_offset_n (&offsets, index_);
 
-  if (start < end && end <= value.size && end <= last_end)
+  if (start < end && end <= value.size && end <= offsets.data_size)
     {
       child.data = value.data + start;
       child.size = end - start;
@@ -770,34 +794,16 @@ static gboolean
 gvs_variable_sized_array_is_normal (GVariantSerialised value)
 {
   GVariantSerialised child = { 0, };
-  gsize offsets_array_size;
-  guchar *offsets_array;
-  guint offset_size;
   guint alignment;
-  gsize last_end;
-  gsize length;
   gsize offset;
   gsize i;
 
-  if (value.size == 0)
-    return TRUE;
-
-  offset_size = gvs_get_offset_size (value.size);
-  last_end = gvs_read_unaligned_le (value.data + value.size -
-                                    offset_size, offset_size);
+  struct Offsets offsets = gvs_variable_sized_array_get_frame_offsets (value);
 
-  if (last_end > value.size)
+  if (!offsets.is_normal)
     return FALSE;
 
-  offsets_array_size = value.size - last_end;
-
-  if (offsets_array_size % offset_size)
-    return FALSE;
-
-  offsets_array = value.data + value.size - offsets_array_size;
-  length = offsets_array_size / offset_size;
-
-  if (length == 0)
+  if (value.size != 0 && offsets.length == 0)
     return FALSE;
 
   child.type_info = g_variant_type_info_element (value.type_info);
@@ -805,14 +811,14 @@ gvs_variable_sized_array_is_normal (GVariantSerialised value)
   child.depth = value.depth + 1;
   offset = 0;
 
-  for (i = 0; i < length; i++)
+  for (i = 0; i < offsets.length; i++)
     {
       gsize this_end;
 
-      this_end = gvs_read_unaligned_le (offsets_array + offset_size * i,
-                                        offset_size);
+      this_end = gvs_read_unaligned_le (offsets.array + offsets.offset_size * i,
+                                        offsets.offset_size);
 
-      if (this_end < offset || this_end > last_end)
+      if (this_end < offset || this_end > offsets.data_size)
         return FALSE;
 
       while (offset & alignment)
@@ -834,7 +840,7 @@ gvs_variable_sized_array_is_normal (GVariantSerialised value)
       offset = this_end;
     }
 
-  g_assert (offset == last_end);
+  g_assert (offset == offsets.data_size);
 
   return TRUE;
 }
-- 
GitLab


From 5c27f22aff636766fbc5e49927fd28213629e840 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Tue, 25 Oct 2022 18:05:52 +0100
Subject: [PATCH 03/18] gvariant: Zero-initialise various GVariantSerialised
 objects

The following few commits will add a couple of new fields to
`GVariantSerialised`, and they should be zero-filled by default.

Try and pre-empt that a bit by zero-filling `GVariantSerialised` by
default in a few places.

Signed-off-by: Philip Withnall <pwithnall@endlessos.org>

Helps: #2121
---
 glib/gvariant.c       |  2 +-
 glib/tests/gvariant.c | 12 ++++++------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/glib/gvariant.c b/glib/gvariant.c
index 93eb6cb6bc..6912eebe05 100644
--- a/glib/gvariant.c
+++ b/glib/gvariant.c
@@ -6001,7 +6001,7 @@ g_variant_byteswap (GVariant *value)
   if (alignment)
     /* (potentially) contains multi-byte numeric data */
     {
-      GVariantSerialised serialised;
+      GVariantSerialised serialised = { 0, };
       GVariant *trusted;
       GBytes *bytes;
 
diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c
index 29110e4e24..1a57dc6ef7 100644
--- a/glib/tests/gvariant.c
+++ b/glib/tests/gvariant.c
@@ -1440,7 +1440,7 @@ test_maybe (void)
 
     for (flavour = 0; flavour < 8; flavour += alignment)
       {
-        GVariantSerialised serialised;
+        GVariantSerialised serialised = { 0, };
         GVariantSerialised child;
 
         serialised.type_info = type_info;
@@ -1564,7 +1564,7 @@ test_array (void)
 
     for (flavour = 0; flavour < 8; flavour += alignment)
       {
-        GVariantSerialised serialised;
+        GVariantSerialised serialised = { 0, };
 
         serialised.type_info = array_info;
         serialised.data = flavoured_malloc (needed_size, flavour);
@@ -1728,7 +1728,7 @@ test_tuple (void)
 
     for (flavour = 0; flavour < 8; flavour += alignment)
       {
-        GVariantSerialised serialised;
+        GVariantSerialised serialised = { 0, };
 
         serialised.type_info = type_info;
         serialised.data = flavoured_malloc (needed_size, flavour);
@@ -1823,7 +1823,7 @@ test_variant (void)
 
     for (flavour = 0; flavour < 8; flavour += alignment)
       {
-        GVariantSerialised serialised;
+        GVariantSerialised serialised = { 0, };
         GVariantSerialised child;
 
         serialised.type_info = type_info;
@@ -2270,7 +2270,7 @@ serialise_tree (TreeInstance       *tree,
 static void
 test_byteswap (void)
 {
-  GVariantSerialised one, two;
+  GVariantSerialised one = { 0, }, two = { 0, };
   TreeInstance *tree;
 
   tree = tree_instance_new (NULL, 3);
@@ -2344,7 +2344,7 @@ test_serialiser_children (void)
 static void
 test_fuzz (gdouble *fuzziness)
 {
-  GVariantSerialised serialised;
+  GVariantSerialised serialised = { 0, };
   TreeInstance *tree;
 
   /* make an instance */
-- 
GitLab


From c8067857f7c8fb369ecb30bb534b018b2e2b8f87 Mon Sep 17 00:00:00 2001
From: William Manley <will@stb-tester.com>
Date: Mon, 29 Jun 2020 16:59:44 +0100
Subject: [PATCH 04/18] =?UTF-8?q?gvariant:=20Don=E2=80=99t=20allow=20child?=
 =?UTF-8?q?=20elements=20to=20overlap=20with=20each=20other?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If different elements of a variable sized array can overlap with each
other then we can cause a `GVariant` to normalise to a much larger type.

This commit changes the behaviour of `GVariant` with non-normal form data. If
an invalid frame offset is found all subsequent elements are given their
default value.

When retrieving an element at index `n` we scan the frame offsets up to index
`n` and if they are not in order we return an element with the default value
for that type.  This guarantees that elements don't overlap with each
other.  We remember the offset we've scanned up to so we don't need to
repeat this work on subsequent accesses.  We skip these checks for trusted
data.

Unfortunately this makes random access of untrusted data O(n) — at least
on first access.  It doesn't affect the algorithmic complexity of accessing
elements in order, such as when using the `GVariantIter` interface.  Also:
the cost of validation will be amortised as the `GVariant` instance is
continued to be used.

I've implemented this with 4 different functions, 1 for each element size,
rather than looping calling `gvs_read_unaligned_le` in the hope that the
compiler will find it easy to optimise and should produce fairly tight
code.

Fixes: #2121
---
 glib/gvariant-core.c       | 35 ++++++++++++++++
 glib/gvariant-serialiser.c | 86 ++++++++++++++++++++++++++++++++++++--
 glib/gvariant-serialiser.h |  9 ++++
 glib/tests/gvariant.c      | 45 ++++++++++++++++++++
 4 files changed, 172 insertions(+), 3 deletions(-)

diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c
index 70c712ddc0..a75e7db9ea 100644
--- a/glib/gvariant-core.c
+++ b/glib/gvariant-core.c
@@ -67,6 +67,7 @@ struct _GVariant
     {
       GBytes *bytes;
       gconstpointer data;
+      gsize ordered_offsets_up_to;
     } serialised;
 
     struct
@@ -164,6 +165,24 @@ struct _GVariant
  *                if .data pointed to the appropriate number of nul
  *                bytes.
  *
+ *     .ordered_offsets_up_to: If ordered_offsets_up_to == n this means that all
+ *                             the frame offsets up to and including the frame
+ *                             offset determining the end of element n are in
+ *                             order. This guarantees that the bytes of element
+ *                             n don't overlap with any previous element.
+ *
+ *                             For trusted data this is set to G_MAXSIZE and we
+ *                             don't check that the frame offsets are in order.
+ *
+ *                             Note: This doesn't imply the offsets are good in
+ *                             any way apart from their ordering.  In particular
+ *                             offsets may be out of bounds for this value or
+ *                             may imply that the data overlaps the frame
+ *                             offsets themselves.
+ *
+ *                             This field is only relevant for arrays of non
+ *                             fixed width types.
+ *
  *   .tree: Only valid when the instance is in tree form.
  *
  *          Note that accesses from other threads could result in
@@ -367,6 +386,7 @@ g_variant_to_serialised (GVariant *value)
       (gpointer) value->contents.serialised.data,
       value->size,
       value->depth,
+      value->contents.serialised.ordered_offsets_up_to,
     };
     return serialised;
   }
@@ -398,6 +418,7 @@ g_variant_serialise (GVariant *value,
   serialised.size = value->size;
   serialised.data = data;
   serialised.depth = value->depth;
+  serialised.ordered_offsets_up_to = 0;
 
   children = (gpointer *) value->contents.tree.children;
   n_children = value->contents.tree.n_children;
@@ -441,6 +462,15 @@ g_variant_fill_gvs (GVariantSerialised *serialised,
   g_assert (serialised->size == value->size);
   serialised->depth = value->depth;
 
+  if (value->state & STATE_SERIALISED)
+    {
+      serialised->ordered_offsets_up_to = value->contents.serialised.ordered_offsets_up_to;
+    }
+  else
+    {
+      serialised->ordered_offsets_up_to = 0;
+    }
+
   if (serialised->data)
     /* g_variant_store() is a public API, so it
      * it will reacquire the lock if it needs to.
@@ -483,6 +513,7 @@ g_variant_ensure_serialised (GVariant *value)
       bytes = g_bytes_new_take (data, value->size);
       value->contents.serialised.data = g_bytes_get_data (bytes, NULL);
       value->contents.serialised.bytes = bytes;
+      value->contents.serialised.ordered_offsets_up_to = G_MAXSIZE;
       value->state |= STATE_SERIALISED;
     }
 }
@@ -563,6 +594,7 @@ g_variant_new_from_bytes (const GVariantType *type,
   serialised.type_info = value->type_info;
   serialised.data = (guchar *) g_bytes_get_data (bytes, &serialised.size);
   serialised.depth = 0;
+  serialised.ordered_offsets_up_to = trusted ? G_MAXSIZE : 0;
 
   if (!g_variant_serialised_check (serialised))
     {
@@ -613,6 +645,8 @@ g_variant_new_from_bytes (const GVariantType *type,
       value->contents.serialised.data = g_bytes_get_data (bytes, &value->size);
     }
 
+  value->contents.serialised.ordered_offsets_up_to = trusted ? G_MAXSIZE : 0;
+
   g_clear_pointer (&owned_bytes, g_bytes_unref);
 
   return value;
@@ -1132,6 +1166,7 @@ g_variant_get_child_value (GVariant *value,
     child->contents.serialised.bytes =
       g_bytes_ref (value->contents.serialised.bytes);
     child->contents.serialised.data = s_child.data;
+    child->contents.serialised.ordered_offsets_up_to = s_child.ordered_offsets_up_to;
 
     return child;
   }
diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c
index 09492e4526..1aa2b8fa9b 100644
--- a/glib/gvariant-serialiser.c
+++ b/glib/gvariant-serialiser.c
@@ -1,6 +1,7 @@
 /*
  * Copyright © 2007, 2008 Ryan Lortie
  * Copyright © 2010 Codethink Limited
+ * Copyright © 2020 William Manley
  *
  * SPDX-License-Identifier: LGPL-2.1-or-later
  *
@@ -266,6 +267,7 @@ gvs_fixed_sized_maybe_get_child (GVariantSerialised value,
   value.type_info = g_variant_type_info_element (value.type_info);
   g_variant_type_info_ref (value.type_info);
   value.depth++;
+  value.ordered_offsets_up_to = 0;
 
   return value;
 }
@@ -297,7 +299,7 @@ gvs_fixed_sized_maybe_serialise (GVariantSerialised        value,
 {
   if (n_children)
     {
-      GVariantSerialised child = { NULL, value.data, value.size, value.depth + 1 };
+      GVariantSerialised child = { NULL, value.data, value.size, value.depth + 1, 0 };
 
       gvs_filler (&child, children[0]);
     }
@@ -319,6 +321,7 @@ gvs_fixed_sized_maybe_is_normal (GVariantSerialised value)
       /* proper element size: "Just".  recurse to the child. */
       value.type_info = g_variant_type_info_element (value.type_info);
       value.depth++;
+      value.ordered_offsets_up_to = 0;
 
       return g_variant_serialised_is_normal (value);
     }
@@ -360,6 +363,7 @@ gvs_variable_sized_maybe_get_child (GVariantSerialised value,
     value.data = NULL;
 
   value.depth++;
+  value.ordered_offsets_up_to = 0;
 
   return value;
 }
@@ -390,7 +394,7 @@ gvs_variable_sized_maybe_serialise (GVariantSerialised        value,
 {
   if (n_children)
     {
-      GVariantSerialised child = { NULL, value.data, value.size - 1, value.depth + 1 };
+      GVariantSerialised child = { NULL, value.data, value.size - 1, value.depth + 1, 0 };
 
       /* write the data for the child.  */
       gvs_filler (&child, children[0]);
@@ -410,6 +414,7 @@ gvs_variable_sized_maybe_is_normal (GVariantSerialised value)
   value.type_info = g_variant_type_info_element (value.type_info);
   value.size--;
   value.depth++;
+  value.ordered_offsets_up_to = 0;
 
   return g_variant_serialised_is_normal (value);
 }
@@ -693,6 +698,32 @@ gvs_variable_sized_array_n_children (GVariantSerialised value)
   return gvs_variable_sized_array_get_frame_offsets (value).length;
 }
 
+/* Find the index of the first out-of-order element in @data, assuming that
+ * @data is an array of elements of given @type, starting at index @start and
+ * containing a further @len-@start elements. */
+#define DEFINE_FIND_UNORDERED(type) \
+  static gsize \
+  find_unordered_##type (const guint8 *data, gsize start, gsize len) \
+  { \
+    gsize off; \
+    type current, previous; \
+    \
+    memcpy (&previous, data + start * sizeof (current), sizeof (current)); \
+    for (off = (start + 1) * sizeof (current); off < len * sizeof (current); off += sizeof (current)) \
+      { \
+        memcpy (&current, data + off, sizeof (current)); \
+        if (current < previous) \
+          break; \
+        previous = current; \
+      } \
+    return off / sizeof (current) - 1; \
+  }
+
+DEFINE_FIND_UNORDERED (guint8);
+DEFINE_FIND_UNORDERED (guint16);
+DEFINE_FIND_UNORDERED (guint32);
+DEFINE_FIND_UNORDERED (guint64);
+
 static GVariantSerialised
 gvs_variable_sized_array_get_child (GVariantSerialised value,
                                     gsize              index_)
@@ -708,6 +739,49 @@ gvs_variable_sized_array_get_child (GVariantSerialised value,
   g_variant_type_info_ref (child.type_info);
   child.depth = value.depth + 1;
 
+  /* If the requested @index_ is beyond the set of indices whose framing offsets
+   * have been checked, check the remaining offsets to see whether they’re
+   * normal (in order, no overlapping array elements). */
+  if (index_ > value.ordered_offsets_up_to)
+    {
+      switch (offsets.offset_size)
+        {
+        case 1:
+          {
+            value.ordered_offsets_up_to = find_unordered_guint8 (
+                offsets.array, value.ordered_offsets_up_to, index_ + 1);
+            break;
+          }
+        case 2:
+          {
+            value.ordered_offsets_up_to = find_unordered_guint16 (
+                offsets.array, value.ordered_offsets_up_to, index_ + 1);
+            break;
+          }
+        case 4:
+          {
+            value.ordered_offsets_up_to = find_unordered_guint32 (
+                offsets.array, value.ordered_offsets_up_to, index_ + 1);
+            break;
+          }
+        case 8:
+          {
+            value.ordered_offsets_up_to = find_unordered_guint64 (
+                offsets.array, value.ordered_offsets_up_to, index_ + 1);
+            break;
+          }
+        default:
+          /* gvs_get_offset_size() only returns maximum 8 */
+          g_assert_not_reached ();
+        }
+    }
+
+  if (index_ > value.ordered_offsets_up_to)
+    {
+      /* Offsets are invalid somewhere, so return an empty child. */
+      return child;
+    }
+
   if (index_ > 0)
     {
       guint alignment;
@@ -842,6 +916,9 @@ gvs_variable_sized_array_is_normal (GVariantSerialised value)
 
   g_assert (offset == offsets.data_size);
 
+  /* All offsets have now been checked. */
+  value.ordered_offsets_up_to = G_MAXSIZE;
+
   return TRUE;
 }
 
@@ -1074,7 +1151,7 @@ gvs_tuple_is_normal (GVariantSerialised value)
   for (i = 0; i < length; i++)
     {
       const GVariantMemberInfo *member_info;
-      GVariantSerialised child;
+      GVariantSerialised child = { 0, };
       gsize fixed_size;
       guint alignment;
       gsize end;
@@ -1134,6 +1211,9 @@ gvs_tuple_is_normal (GVariantSerialised value)
       offset = end;
     }
 
+  /* All element bounds have been checked above. */
+  value.ordered_offsets_up_to = G_MAXSIZE;
+
   {
     gsize fixed_size;
     guint alignment;
diff --git a/glib/gvariant-serialiser.h b/glib/gvariant-serialiser.h
index 6ced7e3d6c..a1bccb834b 100644
--- a/glib/gvariant-serialiser.h
+++ b/glib/gvariant-serialiser.h
@@ -31,6 +31,15 @@ typedef struct
   guchar           *data;
   gsize             size;
   gsize             depth;  /* same semantics as GVariant.depth */
+
+  /* If ordered_offsets_up_to == n this means that all the frame offsets up to and
+   * including the frame offset determining the end of element n are in order.
+   * This guarantees that the bytes of element n don't overlap with any previous
+   * element.
+   *
+   * This is both read and set by g_variant_serialised_get_child for arrays of
+   * non-fixed-width types */
+  gsize             ordered_offsets_up_to;
 } GVariantSerialised;
 
 /* deserialization */
diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c
index 1a57dc6ef7..c759e8d701 100644
--- a/glib/tests/gvariant.c
+++ b/glib/tests/gvariant.c
@@ -1,5 +1,6 @@
 /*
  * Copyright © 2010 Codethink Limited
+ * Copyright © 2020 William Manley
  *
  * SPDX-License-Identifier: LGPL-2.1-or-later
  *
@@ -1281,6 +1282,7 @@ random_instance_filler (GVariantSerialised *serialised,
     serialised->size = instance->size;
 
   serialised->depth = 0;
+  serialised->ordered_offsets_up_to = 0;
 
   g_assert_true (serialised->type_info == instance->type_info);
   g_assert_cmpuint (serialised->size, ==, instance->size);
@@ -5131,6 +5133,47 @@ test_normal_checking_array_offsets (void)
   g_variant_unref (variant);
 }
 
+/* This is a regression test that we can't have non-normal values that take up
+ * significantly more space than the normal equivalent, by specifying the
+ * offset table entries so that array elements overlap.
+ *
+ * See https://gitlab.gnome.org/GNOME/glib/-/issues/2121#note_832242 */
+static void
+test_normal_checking_array_offsets2 (void)
+{
+  const guint8 data[] = {
+    'h', 'i', '\0',
+    0x03, 0x00, 0x03,
+    0x06, 0x00, 0x06,
+    0x09, 0x00, 0x09,
+    0x0c, 0x00, 0x0c,
+    0x0f, 0x00, 0x0f,
+    0x12, 0x00, 0x12,
+    0x15, 0x00, 0x15,
+  };
+  gsize size = sizeof (data);
+  const GVariantType *aaaaaaas = G_VARIANT_TYPE ("aaaaaaas");
+  GVariant *variant = NULL;
+  GVariant *normal_variant = NULL;
+  GVariant *expected = NULL;
+
+  variant = g_variant_new_from_data (aaaaaaas, data, size, FALSE, NULL, NULL);
+  g_assert_nonnull (variant);
+
+  normal_variant = g_variant_get_normal_form (variant);
+  g_assert_nonnull (normal_variant);
+  g_assert_cmpuint (g_variant_get_size (normal_variant), <=, size * 2);
+
+  expected = g_variant_new_parsed (
+      "[[[[[[['hi', '', ''], [], []], [], []], [], []], [], []], [], []], [], []]");
+  g_assert_cmpvariant (expected, variant);
+  g_assert_cmpvariant (expected, normal_variant);
+
+  g_variant_unref (expected);
+  g_variant_unref (normal_variant);
+  g_variant_unref (variant);
+}
+
 /* Test that a tuple with invalidly large values in its offset table is
  * normalised successfully without looping infinitely. */
 static void
@@ -5299,6 +5342,8 @@ main (int argc, char **argv)
                    test_normal_checking_tuples);
   g_test_add_func ("/gvariant/normal-checking/array-offsets",
                    test_normal_checking_array_offsets);
+  g_test_add_func ("/gvariant/normal-checking/array-offsets2",
+                   test_normal_checking_array_offsets2);
   g_test_add_func ("/gvariant/normal-checking/tuple-offsets",
                    test_normal_checking_tuple_offsets);
   g_test_add_func ("/gvariant/normal-checking/empty-object-path",
-- 
GitLab


From 66e7c10aa1ec3102c49186f83671a5f0461ffbc0 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Fri, 7 Jan 2022 15:03:52 +0000
Subject: [PATCH 05/18] gvariant-serialiser: Factor out code to get bounds of a
 tuple member

This introduces no functional changes.

Signed-off-by: Philip Withnall <pwithnall@endlessos.org>

Helps: #2121
---
 glib/gvariant-serialiser.c | 73 ++++++++++++++++++++++++--------------
 1 file changed, 46 insertions(+), 27 deletions(-)

diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c
index 1aa2b8fa9b..a518117e3c 100644
--- a/glib/gvariant-serialiser.c
+++ b/glib/gvariant-serialiser.c
@@ -944,6 +944,51 @@ gvs_variable_sized_array_is_normal (GVariantSerialised value)
  * for the tuple.  See the notes in gvarianttypeinfo.h.
  */
 
+static void
+gvs_tuple_get_member_bounds (GVariantSerialised  value,
+                             gsize               index_,
+                             gsize               offset_size,
+                             gsize              *out_member_start,
+                             gsize              *out_member_end)
+{
+  const GVariantMemberInfo *member_info;
+  gsize member_start, member_end;
+
+  member_info = g_variant_type_info_member_info (value.type_info, index_);
+
+  if (member_info->i + 1)
+    member_start = gvs_read_unaligned_le (value.data + value.size -
+                                          offset_size * (member_info->i + 1),
+                                          offset_size);
+  else
+    member_start = 0;
+
+  member_start += member_info->a;
+  member_start &= member_info->b;
+  member_start |= member_info->c;
+
+  if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_LAST)
+    member_end = value.size - offset_size * (member_info->i + 1);
+
+  else if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_FIXED)
+    {
+      gsize fixed_size;
+
+      g_variant_type_info_query (member_info->type_info, NULL, &fixed_size);
+      member_end = member_start + fixed_size;
+    }
+
+  else /* G_VARIANT_MEMBER_ENDING_OFFSET */
+    member_end = gvs_read_unaligned_le (value.data + value.size -
+                                        offset_size * (member_info->i + 2),
+                                        offset_size);
+
+  if (out_member_start != NULL)
+    *out_member_start = member_start;
+  if (out_member_end != NULL)
+    *out_member_end = member_end;
+}
+
 static gsize
 gvs_tuple_n_children (GVariantSerialised value)
 {
@@ -999,33 +1044,7 @@ gvs_tuple_get_child (GVariantSerialised value,
         }
     }
 
-  if (member_info->i + 1)
-    start = gvs_read_unaligned_le (value.data + value.size -
-                                   offset_size * (member_info->i + 1),
-                                   offset_size);
-  else
-    start = 0;
-
-  start += member_info->a;
-  start &= member_info->b;
-  start |= member_info->c;
-
-  if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_LAST)
-    end = value.size - offset_size * (member_info->i + 1);
-
-  else if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_FIXED)
-    {
-      gsize fixed_size;
-
-      g_variant_type_info_query (child.type_info, NULL, &fixed_size);
-      end = start + fixed_size;
-      child.size = fixed_size;
-    }
-
-  else /* G_VARIANT_MEMBER_ENDING_OFFSET */
-    end = gvs_read_unaligned_le (value.data + value.size -
-                                 offset_size * (member_info->i + 2),
-                                 offset_size);
+  gvs_tuple_get_member_bounds (value, index_, offset_size, &start, &end);
 
   /* The child should not extend into the offset table. */
   if (index_ != g_variant_type_info_n_members (value.type_info) - 1)
-- 
GitLab


From a62a6b5d3e53b30a4628db2a077ab9ed03605748 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Fri, 7 Jan 2022 16:37:29 +0000
Subject: [PATCH 06/18] gvariant-serialiser: Rework child size calculation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This reduces a few duplicate calls to `g_variant_type_info_query()` and
explains why they’re needed.

Signed-off-by: Philip Withnall <pwithnall@endlessos.org>

Helps: #2121
---
 glib/gvariant-serialiser.c | 31 +++++++++----------------------
 1 file changed, 9 insertions(+), 22 deletions(-)

diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c
index a518117e3c..d32e255480 100644
--- a/glib/gvariant-serialiser.c
+++ b/glib/gvariant-serialiser.c
@@ -1009,14 +1009,18 @@ gvs_tuple_get_child (GVariantSerialised value,
   child.depth = value.depth + 1;
   offset_size = gvs_get_offset_size (value.size);
 
+  /* Ensure the size is set for fixed-sized children, or
+   * g_variant_serialised_check() will fail, even if we return
+   * (child.data == NULL) to indicate an error. */
+  if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_FIXED)
+    g_variant_type_info_query (child.type_info, NULL, &child.size);
+
   /* tuples are the only (potentially) fixed-sized containers, so the
    * only ones that have to deal with the possibility of having %NULL
    * data with a non-zero %size if errors occurred elsewhere.
    */
   if G_UNLIKELY (value.data == NULL && value.size != 0)
     {
-      g_variant_type_info_query (child.type_info, NULL, &child.size);
-
       /* this can only happen in fixed-sized tuples,
        * so the child must also be fixed sized.
        */
@@ -1034,29 +1038,12 @@ gvs_tuple_get_child (GVariantSerialised value,
   else
     {
       if (offset_size * (member_info->i + 1) > value.size)
-        {
-          /* if the child is fixed size, return its size.
-           * if child is not fixed-sized, return size = 0.
-           */
-          g_variant_type_info_query (child.type_info, NULL, &child.size);
-
-          return child;
-        }
+        return child;
     }
 
-  gvs_tuple_get_member_bounds (value, index_, offset_size, &start, &end);
-
   /* The child should not extend into the offset table. */
-  if (index_ != g_variant_type_info_n_members (value.type_info) - 1)
-    {
-      GVariantSerialised last_child;
-      last_child = gvs_tuple_get_child (value,
-                                        g_variant_type_info_n_members (value.type_info) - 1);
-      last_end = last_child.data + last_child.size - value.data;
-      g_variant_type_info_unref (last_child.type_info);
-    }
-  else
-    last_end = end;
+  gvs_tuple_get_member_bounds (value, index_, offset_size, &start, &end);
+  gvs_tuple_get_member_bounds (value, g_variant_type_info_n_members (value.type_info) - 1, offset_size, NULL, &last_end);
 
   if (start < end && end <= value.size && end <= last_end)
     {
-- 
GitLab


From 2d55b3b74b1bc256e91a9d0d59120570376e6acc Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Fri, 7 Jan 2022 16:42:14 +0000
Subject: [PATCH 07/18] =?UTF-8?q?gvariant:=20Don=E2=80=99t=20allow=20child?=
 =?UTF-8?q?=20elements=20of=20a=20tuple=20to=20overlap=20each=20other?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This is similar to the earlier commit which prevents child elements of a
variable-sized array from overlapping each other, but this time for
tuples. It is based heavily on ideas by William Manley.

Tuples are slightly different from variable-sized arrays in that they
contain a mixture of fixed and variable sized elements. All but one of
the variable sized elements have an entry in the frame offsets table.
This means that if we were to just check the ordering of the frame
offsets table, the variable sized elements could still overlap
interleaving fixed sized elements, which would be bad.

Therefore we have to check the elements rather than the frame offsets.

The logic of checking the elements up to the index currently being
requested, and caching the result in `ordered_offsets_up_to`, means that
the algorithmic cost implications are the same for this commit as for
variable-sized arrays: an O(N) cost for these checks is amortised out
over N accesses to O(1) per access.

Signed-off-by: Philip Withnall <pwithnall@endlessos.org>

Fixes: #2121
---
 glib/gvariant-core.c       |   6 +-
 glib/gvariant-serialiser.c |  40 ++++++++
 glib/gvariant-serialiser.h |   7 +-
 glib/gvariant.c            |   1 +
 glib/tests/gvariant.c      | 181 +++++++++++++++++++++++++++++++++++++
 5 files changed, 232 insertions(+), 3 deletions(-)

diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c
index a75e7db9ea..c0e1931c39 100644
--- a/glib/gvariant-core.c
+++ b/glib/gvariant-core.c
@@ -1,6 +1,7 @@
 /*
  * Copyright © 2007, 2008 Ryan Lortie
  * Copyright © 2010 Codethink Limited
+ * Copyright © 2022 Endless OS Foundation, LLC
  *
  * SPDX-License-Identifier: LGPL-2.1-or-later
  *
@@ -181,7 +182,7 @@ struct _GVariant
  *                             offsets themselves.
  *
  *                             This field is only relevant for arrays of non
- *                             fixed width types.
+ *                             fixed width types and for tuples.
  *
  *   .tree: Only valid when the instance is in tree form.
  *
@@ -1141,6 +1142,9 @@ g_variant_get_child_value (GVariant *value,
      */
     s_child = g_variant_serialised_get_child (serialised, index_);
 
+    /* Update the cached ordered_offsets_up_to, since @serialised will be thrown away when this function exits */
+    value->contents.serialised.ordered_offsets_up_to = MAX (value->contents.serialised.ordered_offsets_up_to, serialised.ordered_offsets_up_to);
+
     /* Check whether this would cause nesting too deep. If so, return a fake
      * child. The only situation we expect this to happen in is with a variant,
      * as all other deeply-nested types have a static type, and hence should
diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c
index d32e255480..fda59d6843 100644
--- a/glib/gvariant-serialiser.c
+++ b/glib/gvariant-serialiser.c
@@ -944,6 +944,10 @@ gvs_variable_sized_array_is_normal (GVariantSerialised value)
  * for the tuple.  See the notes in gvarianttypeinfo.h.
  */
 
+/* Note: This doesn’t guarantee that @out_member_end >= @out_member_start; that
+ * condition may not hold true for invalid serialised variants. The caller is
+ * responsible for checking the returned values and handling invalid ones
+ * appropriately. */
 static void
 gvs_tuple_get_member_bounds (GVariantSerialised  value,
                              gsize               index_,
@@ -1030,6 +1034,42 @@ gvs_tuple_get_child (GVariantSerialised value,
       return child;
     }
 
+  /* If the requested @index_ is beyond the set of indices whose framing offsets
+   * have been checked, check the remaining offsets to see whether they’re
+   * normal (in order, no overlapping tuple elements).
+   *
+   * Unlike the checks in gvs_variable_sized_array_get_child(), we have to check
+   * all the tuple *elements* here, not just all the framing offsets, since
+   * tuples contain a mix of elements which use framing offsets and ones which
+   * don’t. None of them are allowed to overlap. */
+  if (index_ > value.ordered_offsets_up_to)
+    {
+      gsize i, prev_i_end = 0;
+
+      if (value.ordered_offsets_up_to > 0)
+        gvs_tuple_get_member_bounds (value, value.ordered_offsets_up_to - 1, offset_size, NULL, &prev_i_end);
+
+      for (i = value.ordered_offsets_up_to; i <= index_; i++)
+        {
+          gsize i_start, i_end;
+
+          gvs_tuple_get_member_bounds (value, i, offset_size, &i_start, &i_end);
+
+          if (i_start > i_end || i_start < prev_i_end || i_end > value.size)
+            break;
+
+          prev_i_end = i_end;
+        }
+
+      value.ordered_offsets_up_to = i - 1;
+    }
+
+  if (index_ > value.ordered_offsets_up_to)
+    {
+      /* Offsets are invalid somewhere, so return an empty child. */
+      return child;
+    }
+
   if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_OFFSET)
     {
       if (offset_size * (member_info->i + 2) > value.size)
diff --git a/glib/gvariant-serialiser.h b/glib/gvariant-serialiser.h
index a1bccb834b..661bd8fd1b 100644
--- a/glib/gvariant-serialiser.h
+++ b/glib/gvariant-serialiser.h
@@ -37,8 +37,11 @@ typedef struct
    * This guarantees that the bytes of element n don't overlap with any previous
    * element.
    *
-   * This is both read and set by g_variant_serialised_get_child for arrays of
-   * non-fixed-width types */
+   * This is both read and set by g_variant_serialised_get_child() for arrays of
+   * non-fixed-width types, and for tuples.
+   *
+   * Even when dealing with tuples, @ordered_offsets_up_to is an element index,
+   * rather than an index into the frame offsets. */
   gsize             ordered_offsets_up_to;
 } GVariantSerialised;
 
diff --git a/glib/gvariant.c b/glib/gvariant.c
index 6912eebe05..16af7a5678 100644
--- a/glib/gvariant.c
+++ b/glib/gvariant.c
@@ -6010,6 +6010,7 @@ g_variant_byteswap (GVariant *value)
       serialised.size = g_variant_get_size (trusted);
       serialised.data = g_malloc (serialised.size);
       serialised.depth = g_variant_get_depth (trusted);
+      serialised.ordered_offsets_up_to = G_MAXSIZE;  /* operating on the normal form */
       g_variant_store (trusted, serialised.data);
       g_variant_unref (trusted);
 
diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c
index c759e8d701..0cb69d4b70 100644
--- a/glib/tests/gvariant.c
+++ b/glib/tests/gvariant.c
@@ -1,6 +1,7 @@
 /*
  * Copyright © 2010 Codethink Limited
  * Copyright © 2020 William Manley
+ * Copyright © 2022 Endless OS Foundation, LLC
  *
  * SPDX-License-Identifier: LGPL-2.1-or-later
  *
@@ -1449,6 +1450,7 @@ test_maybe (void)
         serialised.data = flavoured_malloc (needed_size, flavour);
         serialised.size = needed_size;
         serialised.depth = 0;
+        serialised.ordered_offsets_up_to = 0;
 
         g_variant_serialiser_serialise (serialised,
                                         random_instance_filler,
@@ -1572,6 +1574,7 @@ test_array (void)
         serialised.data = flavoured_malloc (needed_size, flavour);
         serialised.size = needed_size;
         serialised.depth = 0;
+        serialised.ordered_offsets_up_to = 0;
 
         g_variant_serialiser_serialise (serialised, random_instance_filler,
                                         (gpointer *) instances, n_children);
@@ -1736,6 +1739,7 @@ test_tuple (void)
         serialised.data = flavoured_malloc (needed_size, flavour);
         serialised.size = needed_size;
         serialised.depth = 0;
+        serialised.ordered_offsets_up_to = 0;
 
         g_variant_serialiser_serialise (serialised, random_instance_filler,
                                         (gpointer *) instances, n_children);
@@ -1832,6 +1836,7 @@ test_variant (void)
         serialised.data = flavoured_malloc (needed_size, flavour);
         serialised.size = needed_size;
         serialised.depth = 0;
+        serialised.ordered_offsets_up_to = 0;
 
         g_variant_serialiser_serialise (serialised, random_instance_filler,
                                         (gpointer *) &instance, 1);
@@ -5198,6 +5203,176 @@ test_normal_checking_tuple_offsets (void)
   g_variant_unref (variant);
 }
 
+/* This is a regression test that we can't have non-normal values that take up
+ * significantly more space than the normal equivalent, by specifying the
+ * offset table entries so that tuple elements overlap.
+ *
+ * See https://gitlab.gnome.org/GNOME/glib/-/issues/2121#note_838503 and
+ * https://gitlab.gnome.org/GNOME/glib/-/issues/2121#note_838513 */
+static void
+test_normal_checking_tuple_offsets2 (void)
+{
+  const GVariantType *data_type = G_VARIANT_TYPE ("(yyaiyyaiyy)");
+  const guint8 data[] = {
+    0x12, 0x34, 0x56, 0x78, 0x01,
+    /*
+         ^───────────────────┘
+
+    ^^^^^^^^^^                   1st yy
+          ^^^^^^^^^^             2nd yy
+                ^^^^^^^^^^       3rd yy
+                            ^^^^ Framing offsets
+     */
+
+  /* If this variant was encoded normally, it would be something like this:
+   * 0x12, 0x34,  pad,  pad, [array bytes], 0x56, 0x78,  pad,  pad, [array bytes], 0x9A, 0xBC, 0xXX
+   *                                      ^─────────────────────────────────────────────────────┘
+   *
+   * ^^^^^^^^^^                                                                                     1st yy
+   *                                        ^^^^^^^^^^                                              2nd yy
+   *                                                                               ^^^^^^^^^^       3rd yy
+   *                                                                                           ^^^^ Framing offsets
+   */
+  };
+  gsize size = sizeof (data);
+  GVariant *variant = NULL;
+  GVariant *normal_variant = NULL;
+  GVariant *expected = NULL;
+
+  variant = g_variant_new_from_data (data_type, data, size, FALSE, NULL, NULL);
+  g_assert_nonnull (variant);
+
+  normal_variant = g_variant_get_normal_form (variant);
+  g_assert_nonnull (normal_variant);
+  g_assert_cmpuint (g_variant_get_size (normal_variant), <=, size * 3);
+
+  expected = g_variant_new_parsed (
+      "@(yyaiyyaiyy) (0x12, 0x34, [], 0x00, 0x00, [], 0x00, 0x00)");
+  g_assert_cmpvariant (expected, variant);
+  g_assert_cmpvariant (expected, normal_variant);
+
+  g_variant_unref (expected);
+  g_variant_unref (normal_variant);
+  g_variant_unref (variant);
+}
+
+/* This is a regression test that overlapping entries in the offset table are
+ * decoded consistently, even though they’re non-normal.
+ *
+ * See https://gitlab.gnome.org/GNOME/glib/-/issues/2121#note_910935 */
+static void
+test_normal_checking_tuple_offsets3 (void)
+{
+  /* The expected decoding of this non-normal byte stream is complex. See
+   * section 2.7.3 (Handling Non-Normal Serialised Data) of the GVariant
+   * specification.
+   *
+   * The rule “Child Values Overlapping Framing Offsets” from the specification
+   * says that the first `ay` must be decoded as `[0x01]` even though it
+   * overlaps the first byte of the offset table. However, since commit
+   * 7eedcd76f7d5b8c98fa60013e1fe6e960bf19df3, GLib explicitly doesn’t allow
+   * this as it’s exploitable. So the first `ay` must be given a default value.
+   *
+   * The second and third `ay`s must be given default values because of rule
+   * “End Boundary Precedes Start Boundary”.
+   *
+   * The `i` must be given a default value because of rule “Start or End
+   * Boundary of a Child Falls Outside the Container”.
+   */
+  const GVariantType *data_type = G_VARIANT_TYPE ("(ayayiay)");
+  const guint8 data[] = {
+    0x01, 0x00, 0x02,
+    /*
+               ^──┘
+
+    ^^^^^^^^^^                   1st ay, bytes 0-2 (but given a default value anyway, see above)
+                                 2nd ay, bytes 2-0
+                                     i,  bytes 0-4
+                                 3rd ay, bytes 4-1
+          ^^^^^^^^^^ Framing offsets
+     */
+  };
+  gsize size = sizeof (data);
+  GVariant *variant = NULL;
+  GVariant *normal_variant = NULL;
+  GVariant *expected = NULL;
+
+  variant = g_variant_new_from_data (data_type, data, size, FALSE, NULL, NULL);
+  g_assert_nonnull (variant);
+
+  g_assert_false (g_variant_is_normal_form (variant));
+
+  normal_variant = g_variant_get_normal_form (variant);
+  g_assert_nonnull (normal_variant);
+  g_assert_cmpuint (g_variant_get_size (normal_variant), <=, size * 3);
+
+  expected = g_variant_new_parsed ("@(ayayiay) ([], [], 0, [])");
+  g_assert_cmpvariant (expected, variant);
+  g_assert_cmpvariant (expected, normal_variant);
+
+  g_variant_unref (expected);
+  g_variant_unref (normal_variant);
+  g_variant_unref (variant);
+}
+
+/* This is a regression test that overlapping entries in the offset table are
+ * decoded consistently, even though they’re non-normal.
+ *
+ * See https://gitlab.gnome.org/GNOME/glib/-/issues/2121#note_910935 */
+static void
+test_normal_checking_tuple_offsets4 (void)
+{
+  /* The expected decoding of this non-normal byte stream is complex. See
+   * section 2.7.3 (Handling Non-Normal Serialised Data) of the GVariant
+   * specification.
+   *
+   * The rule “Child Values Overlapping Framing Offsets” from the specification
+   * says that the first `ay` must be decoded as `[0x01]` even though it
+   * overlaps the first byte of the offset table. However, since commit
+   * 7eedcd76f7d5b8c98fa60013e1fe6e960bf19df3, GLib explicitly doesn’t allow
+   * this as it’s exploitable. So the first `ay` must be given a default value.
+   *
+   * The second `ay` must be given a default value because of rule “End Boundary
+   * Precedes Start Boundary”.
+   *
+   * The third `ay` must be given a default value because its framing offsets
+   * overlap that of the first `ay`.
+   */
+  const GVariantType *data_type = G_VARIANT_TYPE ("(ayayay)");
+  const guint8 data[] = {
+    0x01, 0x00, 0x02,
+    /*
+               ^──┘
+
+    ^^^^^^^^^^                   1st ay, bytes 0-2 (but given a default value anyway, see above)
+                                 2nd ay, bytes 2-0
+                                 3rd ay, bytes 0-1
+          ^^^^^^^^^^ Framing offsets
+     */
+  };
+  gsize size = sizeof (data);
+  GVariant *variant = NULL;
+  GVariant *normal_variant = NULL;
+  GVariant *expected = NULL;
+
+  variant = g_variant_new_from_data (data_type, data, size, FALSE, NULL, NULL);
+  g_assert_nonnull (variant);
+
+  g_assert_false (g_variant_is_normal_form (variant));
+
+  normal_variant = g_variant_get_normal_form (variant);
+  g_assert_nonnull (normal_variant);
+  g_assert_cmpuint (g_variant_get_size (normal_variant), <=, size * 3);
+
+  expected = g_variant_new_parsed ("@(ayayay) ([], [], [])");
+  g_assert_cmpvariant (expected, variant);
+  g_assert_cmpvariant (expected, normal_variant);
+
+  g_variant_unref (expected);
+  g_variant_unref (normal_variant);
+  g_variant_unref (variant);
+}
+
 /* Test that an empty object path is normalised successfully to the base object
  * path, ‘/’. */
 static void
@@ -5346,6 +5521,12 @@ main (int argc, char **argv)
                    test_normal_checking_array_offsets2);
   g_test_add_func ("/gvariant/normal-checking/tuple-offsets",
                    test_normal_checking_tuple_offsets);
+  g_test_add_func ("/gvariant/normal-checking/tuple-offsets2",
+                   test_normal_checking_tuple_offsets2);
+  g_test_add_func ("/gvariant/normal-checking/tuple-offsets3",
+                   test_normal_checking_tuple_offsets3);
+  g_test_add_func ("/gvariant/normal-checking/tuple-offsets4",
+                   test_normal_checking_tuple_offsets4);
   g_test_add_func ("/gvariant/normal-checking/empty-object-path",
                    test_normal_checking_empty_object_path);
 
-- 
GitLab


From a6cb880af0a0932493ba096f01990e694e2c5b72 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Tue, 25 Oct 2022 15:14:14 +0100
Subject: [PATCH 08/18] gvariant: Track checked and ordered offsets
 independently

The past few commits introduced the concept of known-good offsets in the
offset table (which is used for variable-width arrays and tuples).
Good offsets are ones which are non-overlapping with all the previous
offsets in the table.

If a bad offset is encountered when indexing into the array or tuple,
the cached known-good offset index will not be increased. In this way,
all child variants at and beyond the first bad offset can be returned as
default values rather than dereferencing potentially invalid data.

In this case, there was no information about the fact that the indexes
between the highest known-good index and the requested one had been
checked already. That could lead to a pathological case where an offset
table with an invalid first offset is repeatedly checked in full when
trying to access higher-indexed children.

Avoid that by storing the index of the highest checked offset in the
table, as well as the index of the highest good/ordered offset.

Signed-off-by: Philip Withnall <pwithnall@endlessos.org>

Helps: #2121
---
 glib/gvariant-core.c       | 28 ++++++++++++++++++++++++
 glib/gvariant-serialiser.c | 44 +++++++++++++++++++++++++++-----------
 glib/gvariant-serialiser.h |  9 ++++++++
 glib/gvariant.c            |  1 +
 glib/tests/gvariant.c      |  5 +++++
 5 files changed, 75 insertions(+), 12 deletions(-)

diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c
index c0e1931c39..e61242f763 100644
--- a/glib/gvariant-core.c
+++ b/glib/gvariant-core.c
@@ -69,6 +69,7 @@ struct _GVariant
       GBytes *bytes;
       gconstpointer data;
       gsize ordered_offsets_up_to;
+      gsize checked_offsets_up_to;
     } serialised;
 
     struct
@@ -184,6 +185,24 @@ struct _GVariant
  *                             This field is only relevant for arrays of non
  *                             fixed width types and for tuples.
  *
+ *     .checked_offsets_up_to: Similarly to .ordered_offsets_up_to, this stores
+ *                             the index of the highest element, n, whose frame
+ *                             offsets (and all the preceding frame offsets)
+ *                             have been checked for validity.
+ *
+ *                             It is always the case that
+ *                             .checked_offsets_up_to ≥ .ordered_offsets_up_to.
+ *
+ *                             If .checked_offsets_up_to == .ordered_offsets_up_to,
+ *                             then a bad offset has not been found so far.
+ *
+ *                             If .checked_offsets_up_to > .ordered_offsets_up_to,
+ *                             then a bad offset has been found at
+ *                             (.ordered_offsets_up_to + 1).
+ *
+ *                             This field is only relevant for arrays of non
+ *                             fixed width types and for tuples.
+ *
  *   .tree: Only valid when the instance is in tree form.
  *
  *          Note that accesses from other threads could result in
@@ -388,6 +407,7 @@ g_variant_to_serialised (GVariant *value)
       value->size,
       value->depth,
       value->contents.serialised.ordered_offsets_up_to,
+      value->contents.serialised.checked_offsets_up_to,
     };
     return serialised;
   }
@@ -420,6 +440,7 @@ g_variant_serialise (GVariant *value,
   serialised.data = data;
   serialised.depth = value->depth;
   serialised.ordered_offsets_up_to = 0;
+  serialised.checked_offsets_up_to = 0;
 
   children = (gpointer *) value->contents.tree.children;
   n_children = value->contents.tree.n_children;
@@ -466,10 +487,12 @@ g_variant_fill_gvs (GVariantSerialised *serialised,
   if (value->state & STATE_SERIALISED)
     {
       serialised->ordered_offsets_up_to = value->contents.serialised.ordered_offsets_up_to;
+      serialised->checked_offsets_up_to = value->contents.serialised.checked_offsets_up_to;
     }
   else
     {
       serialised->ordered_offsets_up_to = 0;
+      serialised->checked_offsets_up_to = 0;
     }
 
   if (serialised->data)
@@ -515,6 +538,7 @@ g_variant_ensure_serialised (GVariant *value)
       value->contents.serialised.data = g_bytes_get_data (bytes, NULL);
       value->contents.serialised.bytes = bytes;
       value->contents.serialised.ordered_offsets_up_to = G_MAXSIZE;
+      value->contents.serialised.checked_offsets_up_to = G_MAXSIZE;
       value->state |= STATE_SERIALISED;
     }
 }
@@ -596,6 +620,7 @@ g_variant_new_from_bytes (const GVariantType *type,
   serialised.data = (guchar *) g_bytes_get_data (bytes, &serialised.size);
   serialised.depth = 0;
   serialised.ordered_offsets_up_to = trusted ? G_MAXSIZE : 0;
+  serialised.checked_offsets_up_to = trusted ? G_MAXSIZE : 0;
 
   if (!g_variant_serialised_check (serialised))
     {
@@ -647,6 +672,7 @@ g_variant_new_from_bytes (const GVariantType *type,
     }
 
   value->contents.serialised.ordered_offsets_up_to = trusted ? G_MAXSIZE : 0;
+  value->contents.serialised.checked_offsets_up_to = trusted ? G_MAXSIZE : 0;
 
   g_clear_pointer (&owned_bytes, g_bytes_unref);
 
@@ -1144,6 +1170,7 @@ g_variant_get_child_value (GVariant *value,
 
     /* Update the cached ordered_offsets_up_to, since @serialised will be thrown away when this function exits */
     value->contents.serialised.ordered_offsets_up_to = MAX (value->contents.serialised.ordered_offsets_up_to, serialised.ordered_offsets_up_to);
+    value->contents.serialised.checked_offsets_up_to = MAX (value->contents.serialised.checked_offsets_up_to, serialised.checked_offsets_up_to);
 
     /* Check whether this would cause nesting too deep. If so, return a fake
      * child. The only situation we expect this to happen in is with a variant,
@@ -1171,6 +1198,7 @@ g_variant_get_child_value (GVariant *value,
       g_bytes_ref (value->contents.serialised.bytes);
     child->contents.serialised.data = s_child.data;
     child->contents.serialised.ordered_offsets_up_to = s_child.ordered_offsets_up_to;
+    child->contents.serialised.checked_offsets_up_to = s_child.checked_offsets_up_to;
 
     return child;
   }
diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c
index fda59d6843..591522a2e1 100644
--- a/glib/gvariant-serialiser.c
+++ b/glib/gvariant-serialiser.c
@@ -122,6 +122,8 @@
  *
  * @depth has no restrictions; the depth of a top-level serialized #GVariant is
  * zero, and it increases for each level of nested child.
+ *
+ * @checked_offsets_up_to is always ≥ @ordered_offsets_up_to
  */
 
 /* < private >
@@ -149,6 +151,9 @@ g_variant_serialised_check (GVariantSerialised serialised)
            !(serialised.size == 0 || serialised.data != NULL))
     return FALSE;
 
+  if (serialised.ordered_offsets_up_to > serialised.checked_offsets_up_to)
+    return FALSE;
+
   /* Depending on the native alignment requirements of the machine, the
    * compiler will insert either 3 or 7 padding bytes after the char.
    * This will result in the sizeof() the struct being 12 or 16.
@@ -268,6 +273,7 @@ gvs_fixed_sized_maybe_get_child (GVariantSerialised value,
   g_variant_type_info_ref (value.type_info);
   value.depth++;
   value.ordered_offsets_up_to = 0;
+  value.checked_offsets_up_to = 0;
 
   return value;
 }
@@ -299,7 +305,7 @@ gvs_fixed_sized_maybe_serialise (GVariantSerialised        value,
 {
   if (n_children)
     {
-      GVariantSerialised child = { NULL, value.data, value.size, value.depth + 1, 0 };
+      GVariantSerialised child = { NULL, value.data, value.size, value.depth + 1, 0, 0 };
 
       gvs_filler (&child, children[0]);
     }
@@ -322,6 +328,7 @@ gvs_fixed_sized_maybe_is_normal (GVariantSerialised value)
       value.type_info = g_variant_type_info_element (value.type_info);
       value.depth++;
       value.ordered_offsets_up_to = 0;
+      value.checked_offsets_up_to = 0;
 
       return g_variant_serialised_is_normal (value);
     }
@@ -364,6 +371,7 @@ gvs_variable_sized_maybe_get_child (GVariantSerialised value,
 
   value.depth++;
   value.ordered_offsets_up_to = 0;
+  value.checked_offsets_up_to = 0;
 
   return value;
 }
@@ -394,7 +402,7 @@ gvs_variable_sized_maybe_serialise (GVariantSerialised        value,
 {
   if (n_children)
     {
-      GVariantSerialised child = { NULL, value.data, value.size - 1, value.depth + 1, 0 };
+      GVariantSerialised child = { NULL, value.data, value.size - 1, value.depth + 1, 0, 0 };
 
       /* write the data for the child.  */
       gvs_filler (&child, children[0]);
@@ -415,6 +423,7 @@ gvs_variable_sized_maybe_is_normal (GVariantSerialised value)
   value.size--;
   value.depth++;
   value.ordered_offsets_up_to = 0;
+  value.checked_offsets_up_to = 0;
 
   return g_variant_serialised_is_normal (value);
 }
@@ -741,39 +750,46 @@ gvs_variable_sized_array_get_child (GVariantSerialised value,
 
   /* If the requested @index_ is beyond the set of indices whose framing offsets
    * have been checked, check the remaining offsets to see whether they’re
-   * normal (in order, no overlapping array elements). */
-  if (index_ > value.ordered_offsets_up_to)
+   * normal (in order, no overlapping array elements).
+   *
+   * Don’t bother checking if the highest known-good offset is lower than the
+   * highest checked offset, as that means there’s an invalid element at that
+   * index, so there’s no need to check further. */
+  if (index_ > value.checked_offsets_up_to &&
+      value.ordered_offsets_up_to == value.checked_offsets_up_to)
     {
       switch (offsets.offset_size)
         {
         case 1:
           {
             value.ordered_offsets_up_to = find_unordered_guint8 (
-                offsets.array, value.ordered_offsets_up_to, index_ + 1);
+                offsets.array, value.checked_offsets_up_to, index_ + 1);
             break;
           }
         case 2:
           {
             value.ordered_offsets_up_to = find_unordered_guint16 (
-                offsets.array, value.ordered_offsets_up_to, index_ + 1);
+                offsets.array, value.checked_offsets_up_to, index_ + 1);
             break;
           }
         case 4:
           {
             value.ordered_offsets_up_to = find_unordered_guint32 (
-                offsets.array, value.ordered_offsets_up_to, index_ + 1);
+                offsets.array, value.checked_offsets_up_to, index_ + 1);
             break;
           }
         case 8:
           {
             value.ordered_offsets_up_to = find_unordered_guint64 (
-                offsets.array, value.ordered_offsets_up_to, index_ + 1);
+                offsets.array, value.checked_offsets_up_to, index_ + 1);
             break;
           }
         default:
           /* gvs_get_offset_size() only returns maximum 8 */
           g_assert_not_reached ();
         }
+
+      value.checked_offsets_up_to = index_;
     }
 
   if (index_ > value.ordered_offsets_up_to)
@@ -918,6 +934,7 @@ gvs_variable_sized_array_is_normal (GVariantSerialised value)
 
   /* All offsets have now been checked. */
   value.ordered_offsets_up_to = G_MAXSIZE;
+  value.checked_offsets_up_to = G_MAXSIZE;
 
   return TRUE;
 }
@@ -1042,14 +1059,15 @@ gvs_tuple_get_child (GVariantSerialised value,
    * all the tuple *elements* here, not just all the framing offsets, since
    * tuples contain a mix of elements which use framing offsets and ones which
    * don’t. None of them are allowed to overlap. */
-  if (index_ > value.ordered_offsets_up_to)
+  if (index_ > value.checked_offsets_up_to &&
+      value.ordered_offsets_up_to == value.checked_offsets_up_to)
     {
       gsize i, prev_i_end = 0;
 
-      if (value.ordered_offsets_up_to > 0)
-        gvs_tuple_get_member_bounds (value, value.ordered_offsets_up_to - 1, offset_size, NULL, &prev_i_end);
+      if (value.checked_offsets_up_to > 0)
+        gvs_tuple_get_member_bounds (value, value.checked_offsets_up_to - 1, offset_size, NULL, &prev_i_end);
 
-      for (i = value.ordered_offsets_up_to; i <= index_; i++)
+      for (i = value.checked_offsets_up_to; i <= index_; i++)
         {
           gsize i_start, i_end;
 
@@ -1062,6 +1080,7 @@ gvs_tuple_get_child (GVariantSerialised value,
         }
 
       value.ordered_offsets_up_to = i - 1;
+      value.checked_offsets_up_to = index_;
     }
 
   if (index_ > value.ordered_offsets_up_to)
@@ -1259,6 +1278,7 @@ gvs_tuple_is_normal (GVariantSerialised value)
 
   /* All element bounds have been checked above. */
   value.ordered_offsets_up_to = G_MAXSIZE;
+  value.checked_offsets_up_to = G_MAXSIZE;
 
   {
     gsize fixed_size;
diff --git a/glib/gvariant-serialiser.h b/glib/gvariant-serialiser.h
index 661bd8fd1b..eb74fe7804 100644
--- a/glib/gvariant-serialiser.h
+++ b/glib/gvariant-serialiser.h
@@ -43,6 +43,15 @@ typedef struct
    * Even when dealing with tuples, @ordered_offsets_up_to is an element index,
    * rather than an index into the frame offsets. */
   gsize             ordered_offsets_up_to;
+
+  /* Similar to @ordered_offsets_up_to. This gives the index of the child element
+   * whose frame offset is the highest in the offset table which has been
+   * checked so far.
+   *
+   * This is always ≥ @ordered_offsets_up_to. It is always an element index.
+   *
+   * See documentation in gvariant-core.c for `struct GVariant` for details. */
+  gsize             checked_offsets_up_to;
 } GVariantSerialised;
 
 /* deserialization */
diff --git a/glib/gvariant.c b/glib/gvariant.c
index 16af7a5678..48e49490ba 100644
--- a/glib/gvariant.c
+++ b/glib/gvariant.c
@@ -6011,6 +6011,7 @@ g_variant_byteswap (GVariant *value)
       serialised.data = g_malloc (serialised.size);
       serialised.depth = g_variant_get_depth (trusted);
       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);
 
diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c
index 0cb69d4b70..941e58f1bc 100644
--- a/glib/tests/gvariant.c
+++ b/glib/tests/gvariant.c
@@ -1284,6 +1284,7 @@ random_instance_filler (GVariantSerialised *serialised,
 
   serialised->depth = 0;
   serialised->ordered_offsets_up_to = 0;
+  serialised->checked_offsets_up_to = 0;
 
   g_assert_true (serialised->type_info == instance->type_info);
   g_assert_cmpuint (serialised->size, ==, instance->size);
@@ -1451,6 +1452,7 @@ test_maybe (void)
         serialised.size = needed_size;
         serialised.depth = 0;
         serialised.ordered_offsets_up_to = 0;
+        serialised.checked_offsets_up_to = 0;
 
         g_variant_serialiser_serialise (serialised,
                                         random_instance_filler,
@@ -1575,6 +1577,7 @@ test_array (void)
         serialised.size = needed_size;
         serialised.depth = 0;
         serialised.ordered_offsets_up_to = 0;
+        serialised.checked_offsets_up_to = 0;
 
         g_variant_serialiser_serialise (serialised, random_instance_filler,
                                         (gpointer *) instances, n_children);
@@ -1740,6 +1743,7 @@ test_tuple (void)
         serialised.size = needed_size;
         serialised.depth = 0;
         serialised.ordered_offsets_up_to = 0;
+        serialised.checked_offsets_up_to = 0;
 
         g_variant_serialiser_serialise (serialised, random_instance_filler,
                                         (gpointer *) instances, n_children);
@@ -1837,6 +1841,7 @@ test_variant (void)
         serialised.size = needed_size;
         serialised.depth = 0;
         serialised.ordered_offsets_up_to = 0;
+        serialised.checked_offsets_up_to = 0;
 
         g_variant_serialiser_serialise (serialised, random_instance_filler,
                                         (gpointer *) &instance, 1);
-- 
GitLab


From 8c1a7815e7e6695c120cdedff48395c1222af6d1 Mon Sep 17 00:00:00 2001
From: Philip Withnall <withnall@endlessm.com>
Date: Fri, 12 Jun 2020 18:01:13 +0100
Subject: [PATCH 09/18] tests: Add another test for overlapping offsets in
 GVariant

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Helps: #2121
---
 glib/tests/gvariant.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c
index 941e58f1bc..867cb5b9b4 100644
--- a/glib/tests/gvariant.c
+++ b/glib/tests/gvariant.c
@@ -5119,6 +5119,38 @@ test_recursion_limits_array_in_variant (void)
   g_variant_unref (wrapper_variant);
 }
 
+/* Test that a nested array with invalid values in its offset table (which point
+ * from the inner to the outer array) is normalised successfully without
+ * looping infinitely. */
+static void
+test_normal_checking_array_offsets_overlapped (void)
+{
+  const guint8 data[] = {
+    0x01, 0x00,
+  };
+  gsize size = sizeof (data);
+  GVariant *variant = NULL;
+  GVariant *normal_variant = NULL;
+  GVariant *expected_variant = NULL;
+
+  variant = g_variant_new_from_data (G_VARIANT_TYPE ("aay"), data, size,
+                                     FALSE, NULL, NULL);
+  g_assert_nonnull (variant);
+
+  normal_variant = g_variant_get_normal_form (variant);
+  g_assert_nonnull (normal_variant);
+
+  expected_variant = g_variant_new_parsed ("[@ay [], []]");
+  g_assert_cmpvariant (normal_variant, expected_variant);
+
+  g_assert_cmpmem (g_variant_get_data (normal_variant), g_variant_get_size (normal_variant),
+                   g_variant_get_data (expected_variant), g_variant_get_size (expected_variant));
+
+  g_variant_unref (expected_variant);
+  g_variant_unref (normal_variant);
+  g_variant_unref (variant);
+}
+
 /* Test that an array with invalidly large values in its offset table is
  * normalised successfully without looping infinitely. */
 static void
@@ -5520,6 +5552,8 @@ main (int argc, char **argv)
 
   g_test_add_func ("/gvariant/normal-checking/tuples",
                    test_normal_checking_tuples);
+  g_test_add_func ("/gvariant/normal-checking/array-offsets/overlapped",
+                   test_normal_checking_array_offsets_overlapped);
   g_test_add_func ("/gvariant/normal-checking/array-offsets",
                    test_normal_checking_array_offsets);
   g_test_add_func ("/gvariant/normal-checking/array-offsets2",
-- 
GitLab


From 019505a7ccc32d0afa06e104dc0ac2e63e6f7189 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Mon, 24 Oct 2022 16:43:23 +0100
Subject: [PATCH 10/18] tests: Disable some random instance tests of GVariants
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Building a `GVariant` using entirely random data may result in a
non-normally-formed `GVariant`. It’s always possible to read these
`GVariant`s, but the API might return default values for some or all of
their components.

In particular, this can easily happen when randomly generating the
offset tables for non-fixed-width container types.

If it does happen, bytewise comparison of the parsed `GVariant` with the
original bytes will not always match. So skip those checks.

Signed-off-by: Philip Withnall <pwithnall@endlessos.org>

Helps: #2121
---
 glib/tests/gvariant.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c
index 867cb5b9b4..9739993e66 100644
--- a/glib/tests/gvariant.c
+++ b/glib/tests/gvariant.c
@@ -1231,6 +1231,7 @@ random_instance_assert (RandomInstance *instance,
   GRand *rand;
   gsize i;
 
+  g_assert_true (size == 0 || buffer != NULL);
   g_assert_cmpint ((gsize) buffer & ALIGN_BITS & instance->alignment, ==, 0);
   g_assert_cmpint (size, ==, instance->size);
 
@@ -1457,10 +1458,13 @@ test_maybe (void)
         g_variant_serialiser_serialise (serialised,
                                         random_instance_filler,
                                         (gpointer *) &instance, 1);
+
         child = g_variant_serialised_get_child (serialised, 0);
         g_assert_true (child.type_info == instance->type_info);
-        random_instance_assert (instance, child.data, child.size);
+        if (child.data != NULL)  /* could be NULL if element is non-normal */
+          random_instance_assert (instance, child.data, child.size);
         g_variant_type_info_unref (child.type_info);
+
         flavoured_free (serialised.data, flavour);
       }
   }
@@ -1593,7 +1597,8 @@ test_array (void)
 
             child = g_variant_serialised_get_child (serialised, i);
             g_assert_true (child.type_info == instances[i]->type_info);
-            random_instance_assert (instances[i], child.data, child.size);
+            if (child.data != NULL)  /* could be NULL if element is non-normal */
+              random_instance_assert (instances[i], child.data, child.size);
             g_variant_type_info_unref (child.type_info);
           }
 
@@ -1759,7 +1764,8 @@ test_tuple (void)
 
             child = g_variant_serialised_get_child (serialised, i);
             g_assert_true (child.type_info == instances[i]->type_info);
-            random_instance_assert (instances[i], child.data, child.size);
+            if (child.data != NULL)  /* could be NULL if element is non-normal */
+              random_instance_assert (instances[i], child.data, child.size);
             g_variant_type_info_unref (child.type_info);
           }
 
-- 
GitLab


From 9d2a142807806212a23436d0332b0209733810f2 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Mon, 24 Oct 2022 18:14:57 +0100
Subject: [PATCH 11/18] gvariant: Clarify the docs for
 g_variant_get_normal_form()

Document how non-normal parts of the `GVariant` are handled.

Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
---
 glib/gvariant.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/glib/gvariant.c b/glib/gvariant.c
index 48e49490ba..e92564eb64 100644
--- a/glib/gvariant.c
+++ b/glib/gvariant.c
@@ -5936,7 +5936,9 @@ g_variant_deep_copy (GVariant *value)
  * marked as trusted and a new reference to it is returned.
  *
  * If @value is found not to be in normal form then a new trusted
- * #GVariant is created with the same value as @value.
+ * #GVariant is created with the same value as @value. The non-normal parts of
+ * @value will be replaced with default values which are guaranteed to be in
+ * normal form.
  *
  * It makes sense to call this function if you've received #GVariant
  * data from untrusted sources and you want to ensure your serialized
-- 
GitLab


From b0ccb1616688290088e49dea2dc0d7fe723136e4 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Mon, 24 Oct 2022 18:43:55 +0100
Subject: [PATCH 12/18] gvariant: Port g_variant_deep_copy() to count its
 iterations directly

This is equivalent to what `GVariantIter` does, but it means that
`g_variant_deep_copy()` is making its own `g_variant_get_child_value()`
calls.

This will be useful in an upcoming commit, where those child values will
be inspected a little more deeply.

Signed-off-by: Philip Withnall <pwithnall@endlessos.org>

Helps: #2121
---
 glib/gvariant.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/glib/gvariant.c b/glib/gvariant.c
index e92564eb64..82a921a89e 100644
--- a/glib/gvariant.c
+++ b/glib/gvariant.c
@@ -5863,14 +5863,13 @@ g_variant_deep_copy (GVariant *value)
     case G_VARIANT_CLASS_VARIANT:
       {
         GVariantBuilder builder;
-        GVariantIter iter;
-        GVariant *child;
+        gsize i, n_children;
 
         g_variant_builder_init (&builder, g_variant_get_type (value));
-        g_variant_iter_init (&iter, value);
 
-        while ((child = g_variant_iter_next_value (&iter)))
+        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_unref (child);
           }
-- 
GitLab


From 1770e255ae6cc3f0bf5312322432bbc6524a3632 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Tue, 25 Oct 2022 13:03:22 +0100
Subject: [PATCH 13/18] gvariant: Add internal
 g_variant_maybe_get_child_value()

This will be used in a following commit.

Signed-off-by: Philip Withnall <pwithnall@endlessos.org>

Helps: #2540
---
 glib/gvariant-core.c | 68 ++++++++++++++++++++++++++++++++++++++++++++
 glib/gvariant-core.h |  3 ++
 2 files changed, 71 insertions(+)

diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c
index e61242f763..3f4ca0e6dc 100644
--- a/glib/gvariant-core.c
+++ b/glib/gvariant-core.c
@@ -1204,6 +1204,74 @@ g_variant_get_child_value (GVariant *value,
   }
 }
 
+/**
+ * g_variant_maybe_get_child_value:
+ * @value: a container #GVariant
+ * @index_: the index of the child to fetch
+ *
+ * Reads a child item out of a container #GVariant instance, if it is in normal
+ * form. If it is not in normal form, return %NULL.
+ *
+ * This function behaves the same as g_variant_get_child_value(), except that it
+ * returns %NULL if the child is not in normal form. g_variant_get_child_value()
+ * would instead return a new default value of the correct type.
+ *
+ * This is intended to be used internally to avoid unnecessary #GVariant
+ * allocations.
+ *
+ * The returned value is never floating.  You should free it with
+ * g_variant_unref() when you're done with it.
+ *
+ * This function is O(1).
+ *
+ * Returns: (transfer full): the child at the specified index
+ *
+ * Since: 2.74
+ */
+GVariant *
+g_variant_maybe_get_child_value (GVariant *value,
+                                 gsize     index_)
+{
+  g_return_val_if_fail (index_ < g_variant_n_children (value), NULL);
+  g_return_val_if_fail (value->depth < G_MAXSIZE, NULL);
+
+  if (~g_atomic_int_get (&value->state) & STATE_SERIALISED)
+    {
+      g_variant_lock (value);
+
+      if (~value->state & STATE_SERIALISED)
+        {
+          GVariant *child;
+
+          child = g_variant_ref (value->contents.tree.children[index_]);
+          g_variant_unlock (value);
+
+          return child;
+        }
+
+      g_variant_unlock (value);
+    }
+
+  {
+    GVariantSerialised serialised = g_variant_to_serialised (value);
+    GVariantSerialised s_child;
+
+    /* get the serializer to extract the serialized data for the child
+     * from the serialized data for the container
+     */
+    s_child = g_variant_serialised_get_child (serialised, index_);
+
+    if (!(value->state & STATE_TRUSTED) && s_child.data == NULL)
+      {
+        g_variant_type_info_unref (s_child.type_info);
+        return NULL;
+      }
+
+    g_variant_type_info_unref (s_child.type_info);
+    return g_variant_get_child_value (value, index_);
+  }
+}
+
 /**
  * g_variant_store:
  * @value: the #GVariant to store
diff --git a/glib/gvariant-core.h b/glib/gvariant-core.h
index a1c34739a3..fb5f4bff45 100644
--- a/glib/gvariant-core.h
+++ b/glib/gvariant-core.h
@@ -38,4 +38,7 @@ GVariantTypeInfo *      g_variant_get_type_info                         (GVarian
 
 gsize                   g_variant_get_depth                             (GVariant            *value);
 
+GVariant *              g_variant_maybe_get_child_value                 (GVariant            *value,
+                                                                         gsize                index_);
+
 #endif /* __G_VARIANT_CORE_H__ */
-- 
GitLab


From 82fc15af4c84a2645343c51b18ab3528c51790ab Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Tue, 25 Oct 2022 13:03:45 +0100
Subject: [PATCH 14/18] gvariant: Cut allocs of default values for children of
 non-normal arrays
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This improves a slow case in `g_variant_get_normal_form()` where
allocating many identical default values for the children of a
variable-sized array which has a malformed offset table would take a lot
of time.

The fix is to make all child values after the first invalid one be
references to the default value emitted for the first invalid one,
rather than identical new `GVariant`s.

In particular, this fixes a case where an attacker could create an array
of length L of very large tuples of size T each, corrupt the offset table
so they don’t have to specify the array content, and then induce
`g_variant_get_normal_form()` into allocating L×T default values from an
input which is significantly smaller than L×T in length.

A pre-existing workaround for this issue is for code to call
`g_variant_is_normal_form()` before calling
`g_variant_get_normal_form()`, and to skip the latter call if the former
returns false. This commit improves the behaviour in the case that
`g_variant_get_normal_form()` is called anyway.

This fix changes the time to run the `fuzz_variant_binary` test on the
testcase from oss-fuzz#19777 from >60s (before being terminated) with
2.3GB of memory usage and 580k page faults; to 32s, 8.3MB of memory
usage and 1500 page faults (as measured by `time -v`).

Signed-off-by: Philip Withnall <pwithnall@endlessos.org>

Fixes: #2540
oss-fuzz#19777
---
 glib/gvariant.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/glib/gvariant.c b/glib/gvariant.c
index 82a921a89e..bb6be363e0 100644
--- a/glib/gvariant.c
+++ b/glib/gvariant.c
@@ -5857,7 +5857,6 @@ g_variant_deep_copy (GVariant *value)
   switch (g_variant_classify (value))
     {
     case G_VARIANT_CLASS_MAYBE:
-    case G_VARIANT_CLASS_ARRAY:
     case G_VARIANT_CLASS_TUPLE:
     case G_VARIANT_CLASS_DICT_ENTRY:
     case G_VARIANT_CLASS_VARIANT:
@@ -5877,6 +5876,71 @@ g_variant_deep_copy (GVariant *value)
         return g_variant_builder_end (&builder);
       }
 
+    case G_VARIANT_CLASS_ARRAY:
+      {
+        GVariantBuilder builder;
+        gsize i, n_children;
+        GVariant *first_invalid_child_deep_copy = NULL;
+
+        /* Arrays are in theory treated the same as maybes, tuples, dict entries
+         * and variants, and could be another case in the above block of code.
+         *
+         * However, they have the property that when dealing with non-normal
+         * data (which is the only time g_variant_deep_copy() is currently
+         * called) in a variable-sized array, the code above can easily end up
+         * creating many default child values in order to return an array which
+         * is of the right length and type, but without containing non-normal
+         * data. This can happen if the offset table for the array is malformed.
+         *
+         * In this case, the code above would end up allocating the same default
+         * value for each one of the child indexes beyond the first malformed
+         * entry in the offset table. This can end up being a lot of identical
+         * allocations of default values, particularly if the non-normal array
+         * is crafted maliciously.
+         *
+         * Avoid that problem by returning a new reference to the same default
+         * value for every child after the first invalid one. This results in
+         * returning an equivalent array, in normal form and trusted — but with
+         * significantly fewer memory allocations.
+         *
+         * See https://gitlab.gnome.org/GNOME/glib/-/issues/2540 */
+
+        g_variant_builder_init (&builder, g_variant_get_type (value));
+
+        for (i = 0, n_children = g_variant_n_children (value); i < n_children; i++)
+          {
+            /* Try maybe_get_child_value() first; if it returns NULL, this child
+             * is non-normal. get_child_value() would have constructed and
+             * returned a default value in that case. */
+            GVariant *child = g_variant_maybe_get_child_value (value, i);
+
+            if (child != NULL)
+              {
+                /* Non-normal children may not always be contiguous, as they may
+                 * 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));
+              }
+            else if (child == NULL && first_invalid_child_deep_copy != NULL)
+              {
+                g_variant_builder_add_value (&builder, first_invalid_child_deep_copy);
+              }
+            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));
+                g_variant_builder_add_value (&builder, first_invalid_child_deep_copy);
+              }
+
+            g_clear_pointer (&child, g_variant_unref);
+          }
+
+        g_clear_pointer (&first_invalid_child_deep_copy, g_variant_unref);
+
+        return g_variant_builder_end (&builder);
+      }
+
     case G_VARIANT_CLASS_BOOLEAN:
       return g_variant_new_boolean (g_variant_get_boolean (value));
 
-- 
GitLab


From 935f1c200789c76ad5b51b1f403f611e3cc75318 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Tue, 25 Oct 2022 18:03:56 +0100
Subject: [PATCH 15/18] gvariant: Fix a leak of a GVariantTypeInfo on an error
 handling path

Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
---
 glib/gvariant-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c
index 3f4ca0e6dc..75d23f39af 100644
--- a/glib/gvariant-core.c
+++ b/glib/gvariant-core.c
@@ -1183,6 +1183,7 @@ g_variant_get_child_value (GVariant *value,
         G_VARIANT_MAX_RECURSION_DEPTH - value->depth)
       {
         g_assert (g_variant_is_of_type (value, G_VARIANT_TYPE_VARIANT));
+        g_variant_type_info_unref (s_child.type_info);
         return g_variant_new_tuple (NULL, 0);
       }
 
-- 
GitLab


From f1dfc5d0c5c0486b5fccb2ceddf1db1162c7033c Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Thu, 27 Oct 2022 12:00:04 +0100
Subject: [PATCH 16/18] gvariant-serialiser: Check offset table entry size is
 minimal

The entries in an offset table (which is used for variable sized arrays
and tuples containing variable sized members) are sized so that they can
address every byte in the overall variant.

The specification requires that for a variant to be in normal form, its
offset table entries must be the minimum width such that they can
address every byte in the variant.

That minimality requirement was not checked in
`g_variant_is_normal_form()`, leading to two different byte arrays being
interpreted as the normal form of a given variant tree. That kind of
confusion could potentially be exploited, and is certainly a bug.

Fix it by adding the necessary checks on offset table entry width, and
unit tests.

Spotted by William Manley.

Signed-off-by: Philip Withnall <pwithnall@endlessos.org>

Fixes: #2794
---
 glib/gvariant-serialiser.c |  19 +++-
 glib/tests/gvariant.c      | 176 +++++++++++++++++++++++++++++++++++++
 2 files changed, 194 insertions(+), 1 deletion(-)

diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c
index 591522a2e1..25c85b30b1 100644
--- a/glib/gvariant-serialiser.c
+++ b/glib/gvariant-serialiser.c
@@ -696,6 +696,10 @@ gvs_variable_sized_array_get_frame_offsets (GVariantSerialised value)
   out.data_size = last_end;
   out.array = value.data + last_end;
   out.length = offsets_array_size / out.offset_size;
+
+  if (out.length > 0 && gvs_calculate_total_size (last_end, out.length) != value.size)
+    return out;  /* offset size not minimal */
+
   out.is_normal = TRUE;
 
   return out;
@@ -1203,6 +1207,7 @@ gvs_tuple_is_normal (GVariantSerialised value)
   gsize length;
   gsize offset;
   gsize i;
+  gsize offset_table_size;
 
   /* as per the comment in gvs_tuple_get_child() */
   if G_UNLIKELY (value.data == NULL && value.size != 0)
@@ -1307,7 +1312,19 @@ gvs_tuple_is_normal (GVariantSerialised value)
       }
   }
 
-  return offset_ptr == offset;
+  /* @offset_ptr has been counting backwards from the end of the variant, to
+   * find the beginning of the offset table. @offset has been counting forwards
+   * from the beginning of the variant to find the end of the data. They should
+   * have met in the middle. */
+  if (offset_ptr != offset)
+    return FALSE;
+
+  offset_table_size = value.size - offset_ptr;
+  if (value.size > 0 &&
+      gvs_calculate_total_size (offset, offset_table_size / offset_size) != value.size)
+    return FALSE;  /* offset size not minimal */
+
+  return TRUE;
 }
 
 /* Variants {{{2
diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c
index 9739993e66..339edc4e04 100644
--- a/glib/tests/gvariant.c
+++ b/glib/tests/gvariant.c
@@ -5222,6 +5222,86 @@ test_normal_checking_array_offsets2 (void)
   g_variant_unref (variant);
 }
 
+/* Test that an otherwise-valid serialised GVariant is considered non-normal if
+ * its offset table entries are too wide.
+ *
+ * See §2.3.6 (Framing Offsets) of the GVariant specification. */
+static void
+test_normal_checking_array_offsets_minimal_sized (void)
+{
+  GVariantBuilder builder;
+  gsize i;
+  GVariant *aay_constructed = NULL;
+  const guint8 *data = NULL;
+  guint8 *data_owned = NULL;
+  GVariant *aay_deserialised = NULL;
+  GVariant *aay_normalised = NULL;
+
+  /* Construct an array of type aay, consisting of 128 elements which are each
+   * an empty array, i.e. `[[] * 128]`. This is chosen because the inner
+   * elements are variable sized (making the outer array variable sized, so it
+   * must have an offset table), but they are also zero-sized when serialised.
+   * So the serialised representation of @aay_constructed consists entirely of
+   * its offset table, which is entirely zeroes.
+   *
+   * The array is chosen to be 128 elements long because that means offset
+   * table entries which are 1 byte long. If the elements in the array were
+   * non-zero-sized (to the extent that the overall array is ≥256 bytes long),
+   * the offset table entries would end up being 2 bytes long. */
+  g_variant_builder_init (&builder, G_VARIANT_TYPE ("aay"));
+
+  for (i = 0; i < 128; i++)
+    g_variant_builder_add_value (&builder, g_variant_new_array (G_VARIANT_TYPE_BYTE, NULL, 0));
+
+  aay_constructed = g_variant_builder_end (&builder);
+
+  /* Verify that the constructed array is in normal form, and its serialised
+   * form is `b'\0' * 128`. */
+  g_assert_true (g_variant_is_normal_form (aay_constructed));
+  g_assert_cmpuint (g_variant_n_children (aay_constructed), ==, 128);
+  g_assert_cmpuint (g_variant_get_size (aay_constructed), ==, 128);
+
+  data = g_variant_get_data (aay_constructed);
+  for (i = 0; i < g_variant_get_size (aay_constructed); i++)
+    g_assert_cmpuint (data[i], ==, 0);
+
+  /* Construct a serialised `aay` GVariant which is `b'\0' * 256`. This has to
+   * be a non-normal form of `[[] * 128]`, with 2-byte-long offset table
+   * entries, because each offset table entry has to be able to reference all of
+   * the byte boundaries in the container. All the entries in the offset table
+   * are zero, so all the elements of the array are zero-sized. */
+  data = data_owned = g_malloc0 (256);
+  aay_deserialised = g_variant_new_from_data (G_VARIANT_TYPE ("aay"),
+                                              data,
+                                              256,
+                                              FALSE,
+                                              g_free,
+                                              g_steal_pointer (&data_owned));
+
+  g_assert_false (g_variant_is_normal_form (aay_deserialised));
+  g_assert_cmpuint (g_variant_n_children (aay_deserialised), ==, 128);
+  g_assert_cmpuint (g_variant_get_size (aay_deserialised), ==, 256);
+
+  data = g_variant_get_data (aay_deserialised);
+  for (i = 0; i < g_variant_get_size (aay_deserialised); i++)
+    g_assert_cmpuint (data[i], ==, 0);
+
+  /* Get its normal form. That should change the serialised size. */
+  aay_normalised = g_variant_get_normal_form (aay_deserialised);
+
+  g_assert_true (g_variant_is_normal_form (aay_normalised));
+  g_assert_cmpuint (g_variant_n_children (aay_normalised), ==, 128);
+  g_assert_cmpuint (g_variant_get_size (aay_normalised), ==, 128);
+
+  data = g_variant_get_data (aay_normalised);
+  for (i = 0; i < g_variant_get_size (aay_normalised); i++)
+    g_assert_cmpuint (data[i], ==, 0);
+
+  g_variant_unref (aay_normalised);
+  g_variant_unref (aay_deserialised);
+  g_variant_unref (aay_constructed);
+}
+
 /* Test that a tuple with invalidly large values in its offset table is
  * normalised successfully without looping infinitely. */
 static void
@@ -5416,6 +5496,98 @@ test_normal_checking_tuple_offsets4 (void)
   g_variant_unref (variant);
 }
 
+/* Test that an otherwise-valid serialised GVariant is considered non-normal if
+ * its offset table entries are too wide.
+ *
+ * See §2.3.6 (Framing Offsets) of the GVariant specification. */
+static void
+test_normal_checking_tuple_offsets_minimal_sized (void)
+{
+  GString *type_string = NULL;
+  GVariantBuilder builder;
+  gsize i;
+  GVariant *ray_constructed = NULL;
+  const guint8 *data = NULL;
+  guint8 *data_owned = NULL;
+  GVariant *ray_deserialised = NULL;
+  GVariant *ray_normalised = NULL;
+
+  /* Construct a tuple of type (ay…ay), consisting of 129 members which are each
+   * an empty array, i.e. `([] * 129)`. This is chosen because the inner
+   * members are variable sized, so the outer tuple must have an offset table,
+   * but they are also zero-sized when serialised. So the serialised
+   * representation of @ray_constructed consists entirely of its offset table,
+   * which is entirely zeroes.
+   *
+   * The tuple is chosen to be 129 members long because that means it has 128
+   * offset table entries which are 1 byte long each. If the members in the
+   * tuple were non-zero-sized (to the extent that the overall tuple is ≥256
+   * bytes long), the offset table entries would end up being 2 bytes long.
+   *
+   * 129 members are used unlike 128 array elements in
+   * test_normal_checking_array_offsets_minimal_sized(), because the last member
+   * in a tuple never needs an offset table entry. */
+  type_string = g_string_new ("");
+  g_string_append_c (type_string, '(');
+  for (i = 0; i < 129; i++)
+    g_string_append (type_string, "ay");
+  g_string_append_c (type_string, ')');
+
+  g_variant_builder_init (&builder, G_VARIANT_TYPE (type_string->str));
+
+  for (i = 0; i < 129; i++)
+    g_variant_builder_add_value (&builder, g_variant_new_array (G_VARIANT_TYPE_BYTE, NULL, 0));
+
+  ray_constructed = g_variant_builder_end (&builder);
+
+  /* Verify that the constructed tuple is in normal form, and its serialised
+   * form is `b'\0' * 128`. */
+  g_assert_true (g_variant_is_normal_form (ray_constructed));
+  g_assert_cmpuint (g_variant_n_children (ray_constructed), ==, 129);
+  g_assert_cmpuint (g_variant_get_size (ray_constructed), ==, 128);
+
+  data = g_variant_get_data (ray_constructed);
+  for (i = 0; i < g_variant_get_size (ray_constructed); i++)
+    g_assert_cmpuint (data[i], ==, 0);
+
+  /* Construct a serialised `(ay…ay)` GVariant which is `b'\0' * 256`. This has
+   * to be a non-normal form of `([] * 129)`, with 2-byte-long offset table
+   * entries, because each offset table entry has to be able to reference all of
+   * the byte boundaries in the container. All the entries in the offset table
+   * are zero, so all the members of the tuple are zero-sized. */
+  data = data_owned = g_malloc0 (256);
+  ray_deserialised = g_variant_new_from_data (G_VARIANT_TYPE (type_string->str),
+                                              data,
+                                              256,
+                                              FALSE,
+                                              g_free,
+                                              g_steal_pointer (&data_owned));
+
+  g_assert_false (g_variant_is_normal_form (ray_deserialised));
+  g_assert_cmpuint (g_variant_n_children (ray_deserialised), ==, 129);
+  g_assert_cmpuint (g_variant_get_size (ray_deserialised), ==, 256);
+
+  data = g_variant_get_data (ray_deserialised);
+  for (i = 0; i < g_variant_get_size (ray_deserialised); i++)
+    g_assert_cmpuint (data[i], ==, 0);
+
+  /* Get its normal form. That should change the serialised size. */
+  ray_normalised = g_variant_get_normal_form (ray_deserialised);
+
+  g_assert_true (g_variant_is_normal_form (ray_normalised));
+  g_assert_cmpuint (g_variant_n_children (ray_normalised), ==, 129);
+  g_assert_cmpuint (g_variant_get_size (ray_normalised), ==, 128);
+
+  data = g_variant_get_data (ray_normalised);
+  for (i = 0; i < g_variant_get_size (ray_normalised); i++)
+    g_assert_cmpuint (data[i], ==, 0);
+
+  g_variant_unref (ray_normalised);
+  g_variant_unref (ray_deserialised);
+  g_variant_unref (ray_constructed);
+  g_string_free (type_string, TRUE);
+}
+
 /* Test that an empty object path is normalised successfully to the base object
  * path, ‘/’. */
 static void
@@ -5564,6 +5736,8 @@ main (int argc, char **argv)
                    test_normal_checking_array_offsets);
   g_test_add_func ("/gvariant/normal-checking/array-offsets2",
                    test_normal_checking_array_offsets2);
+  g_test_add_func ("/gvariant/normal-checking/array-offsets/minimal-sized",
+                   test_normal_checking_array_offsets_minimal_sized);
   g_test_add_func ("/gvariant/normal-checking/tuple-offsets",
                    test_normal_checking_tuple_offsets);
   g_test_add_func ("/gvariant/normal-checking/tuple-offsets2",
@@ -5572,6 +5746,8 @@ main (int argc, char **argv)
                    test_normal_checking_tuple_offsets3);
   g_test_add_func ("/gvariant/normal-checking/tuple-offsets4",
                    test_normal_checking_tuple_offsets4);
+  g_test_add_func ("/gvariant/normal-checking/tuple-offsets/minimal-sized",
+                   test_normal_checking_tuple_offsets_minimal_sized);
   g_test_add_func ("/gvariant/normal-checking/empty-object-path",
                    test_normal_checking_empty_object_path);
 
-- 
GitLab


From 781f05a22ef11d8a2177b4e9078978decec36dd0 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Thu, 27 Oct 2022 16:13:54 +0100
Subject: [PATCH 17/18] gvariant: Fix g_variant_byteswap() returning non-normal
 data sometimes
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If `g_variant_byteswap()` was called on a non-normal variant of a type
which doesn’t need byteswapping, it would return a non-normal output.

That contradicts the documentation, which says that the return value is
always in normal form.

Fix the code so it matches the documentation.

Includes a unit test.

Signed-off-by: Philip Withnall <pwithnall@endlessos.org>

Helps: #2797
---
 glib/gvariant.c       |  8 +++++---
 glib/tests/gvariant.c | 24 ++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/glib/gvariant.c b/glib/gvariant.c
index bb6be363e0..b3495d5cab 100644
--- a/glib/gvariant.c
+++ b/glib/gvariant.c
@@ -6083,14 +6083,16 @@ g_variant_byteswap (GVariant *value)
       g_variant_serialised_byteswap (serialised);
 
       bytes = g_bytes_new_take (serialised.data, serialised.size);
-      new = g_variant_new_from_bytes (g_variant_get_type (value), bytes, TRUE);
+      new = g_variant_ref_sink (g_variant_new_from_bytes (g_variant_get_type (value), bytes, TRUE));
       g_bytes_unref (bytes);
     }
   else
     /* contains no multi-byte data */
-    new = value;
+    new = g_variant_get_normal_form (value);
 
-  return g_variant_ref_sink (new);
+  g_assert (g_variant_is_trusted (new));
+
+  return g_steal_pointer (&new);
 }
 
 /**
diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c
index 339edc4e04..bea16b5e58 100644
--- a/glib/tests/gvariant.c
+++ b/glib/tests/gvariant.c
@@ -3826,6 +3826,29 @@ test_gv_byteswap (void)
   g_free (string);
 }
 
+static void
+test_gv_byteswap_non_normal_non_aligned (void)
+{
+  const guint8 data[] = { 0x02 };
+  GVariant *v = NULL;
+  GVariant *v_byteswapped = NULL;
+
+  g_test_summary ("Test that calling g_variant_byteswap() on a variant which "
+                  "is in non-normal form and doesn’t need byteswapping returns "
+                  "the same variant in normal form.");
+
+  v = g_variant_new_from_data (G_VARIANT_TYPE_BOOLEAN, data, sizeof (data), FALSE, NULL, NULL);
+  g_assert_false (g_variant_is_normal_form (v));
+
+  v_byteswapped = g_variant_byteswap (v);
+  g_assert_true (g_variant_is_normal_form (v_byteswapped));
+
+  g_assert_cmpvariant (v, v_byteswapped);
+
+  g_variant_unref (v);
+  g_variant_unref (v_byteswapped);
+}
+
 static void
 test_parser (void)
 {
@@ -5699,6 +5722,7 @@ main (int argc, char **argv)
   g_test_add_func ("/gvariant/builder-memory", test_builder_memory);
   g_test_add_func ("/gvariant/hashing", test_hashing);
   g_test_add_func ("/gvariant/byteswap", test_gv_byteswap);
+  g_test_add_func ("/gvariant/byteswap/non-normal-non-aligned", test_gv_byteswap_non_normal_non_aligned);
   g_test_add_func ("/gvariant/parser", test_parses);
   g_test_add_func ("/gvariant/parser/integer-bounds", test_parser_integer_bounds);
   g_test_add_func ("/gvariant/parser/recursion", test_parser_recursion);
-- 
GitLab


From 7d7efce1d9c379fdd7d2ff58caea88f8806fdd2e Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Thu, 27 Oct 2022 22:53:13 +0100
Subject: [PATCH 18/18] 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 <pwithnall@endlessos.org>

Fixes: #2797
---
 glib/gvariant.c       | 87 ++++++++++++++++++++++++++++++++-----------
 glib/tests/gvariant.c | 57 ++++++++++++++++++++++++----
 2 files changed, 115 insertions(+), 29 deletions(-)

diff --git a/glib/gvariant.c b/glib/gvariant.c
index b3495d5cab..95904bd382 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.
  *
  * Returns: (transfer full): the byteswapped form of @value
@@ -6063,22 +6104,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);
 
@@ -6086,6 +6126,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 bea16b5e58..ba837af48d 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
-- 
GitLab