gvariant: Check offset table doesn’t fall outside variant bounds

When dereferencing the first entry in the offset table for a tuple,
check that it doesn’t fall outside the bounds of the variant first.

This prevents an out-of-bounds read from some non-normal tuples.

This bug was introduced in commit 73d0aa81c2575a5c9ae77d.

Includes a unit test, although the test will likely only catch the
original bug if run with asan enabled.

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

Fixes: #2840
oss-fuzz#54302

2 files changed, 72 insertions, 3 deletions

diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c
index f443c2eb8..4e4a73ad1 100644
--- a/glib/gvariant-serialiser.c
+++ b/glib/gvariant-serialiser.c
@@ -984,7 +984,8 @@ gvs_tuple_get_member_bounds (GVariantSerialised  value,
 
   member_info = g_variant_type_info_member_info (value.type_info, index_);
 
-  if (member_info->i + 1)
+  if (member_info->i + 1 &&
+      offset_size * (member_info->i + 1) <= value.size)
     member_start = gvs_read_unaligned_le (value.data + value.size -
                                           offset_size * (member_info->i + 1),
                                           offset_size);
@@ -995,7 +996,8 @@ gvs_tuple_get_member_bounds (GVariantSerialised  value,
   member_start &= member_info->b;
   member_start |= member_info->c;
 
-  if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_LAST)
+  if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_LAST &&
+      offset_size * (member_info->i + 1) <= value.size)
     member_end = value.size - offset_size * (member_info->i + 1);
 
   else if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_FIXED)
@@ -1006,11 +1008,15 @@ gvs_tuple_get_member_bounds (GVariantSerialised  value,
       member_end = member_start + fixed_size;
     }
 
-  else /* G_VARIANT_MEMBER_ENDING_OFFSET */
+  else if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_OFFSET &&
+           offset_size * (member_info->i + 2) <= value.size)
     member_end = gvs_read_unaligned_le (value.data + value.size -
                                         offset_size * (member_info->i + 2),
                                         offset_size);
 
+  else  /* invalid */
+    member_end = G_MAXSIZE;
+
   if (out_member_start != NULL)
     *out_member_start = member_start;
   if (out_member_end != NULL)
diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c
index b360888e4..98c51a1d7 100644
--- a/glib/tests/gvariant.c
+++ b/glib/tests/gvariant.c
@@ -5576,6 +5576,67 @@ test_normal_checking_tuple_offsets4 (void)
   g_variant_unref (variant);
 }
 
+/* This is a regression test that dereferencing the first element in the offset
+ * table doesn’t dereference memory before the start of the GVariant. The first
+ * element in the offset table gives the offset of the final member in the
+ * tuple (the offset table is stored in reverse), and the position of this final
+ * member is needed to check that none of the tuple members overlap with the
+ * offset table
+ *
+ * See https://gitlab.gnome.org/GNOME/glib/-/issues/2840 */
+static void
+test_normal_checking_tuple_offsets5 (void)
+{
+  /* A tuple of type (sss) in normal form would have an offset table with two
+   * entries:
+   *  - The first entry (lowest index in the table) gives the offset of the
+   *    third `s` in the tuple, as the offset table is reversed compared to the
+   *    tuple members.
+   *  - The second entry (highest index in the table) gives the offset of the
+   *    second `s` in the tuple.
+   *  - The offset of the first `s` in the tuple is always 0.
+   *
+   * See §2.5.4 (Structures) of the GVariant specification for details, noting
+   * that the table is only layed out this way because all three members of the
+   * tuple have non-fixed sizes.
+   *
+   * It’s not clear whether the 0xaa data of this variant is part of the strings
+   * in the tuple, or part of the offset table. It doesn’t really matter. This
+   * is a regression test to check that the code to validate the offset table
+   * doesn’t unconditionally try to access the first entry in the offset table
+   * by subtracting the table size from the end of the GVariant data.
+   *
+   * In this non-normal case, that would result in an address off the start of
+   * the GVariant data, and an out-of-bounds read, because the GVariant is one
+   * byte long, but the offset table is calculated as two bytes long (with 1B
+   * sized entries) from the tuple’s type.
+   */
+  const GVariantType *data_type = G_VARIANT_TYPE ("(sss)");
+  const guint8 data[] = { 0xaa };
+  gsize size = sizeof (data);
+  GVariant *variant = NULL;
+  GVariant *normal_variant = NULL;
+  GVariant *expected = NULL;
+
+  g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/2840");
+
+  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);
+
+  expected = g_variant_new_parsed ("('', '', '')");
+  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 otherwise-valid serialised GVariant is considered non-normal if
  * its offset table entries are too wide.
  *
@@ -5827,6 +5888,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-offsets5",
+                   test_normal_checking_tuple_offsets5);
   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",