200 lines
8.1 KiB
Diff
200 lines
8.1 KiB
Diff
From 78da5faccb3e065116b75b3ff87ff55381da6c76 Mon Sep 17 00:00:00 2001
|
||
From: Philip Withnall <pwithnall@endlessos.org>
|
||
Date: Thu, 15 Dec 2022 13:00:39 +0000
|
||
Subject: [PATCH 1/2] =?UTF-8?q?gvariant:=20Check=20offset=20table=20doesn?=
|
||
=?UTF-8?q?=E2=80=99t=20fall=20outside=20variant=20bounds?=
|
||
MIME-Version: 1.0
|
||
Content-Type: text/plain; charset=UTF-8
|
||
Content-Transfer-Encoding: 8bit
|
||
|
||
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
|
||
---
|
||
glib/gvariant-serialiser.c | 12 ++++++--
|
||
glib/tests/gvariant.c | 63 ++++++++++++++++++++++++++++++++++++++
|
||
2 files changed, 72 insertions(+), 3 deletions(-)
|
||
|
||
diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c
|
||
index f443c2eb85..4e4a73ad17 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 b360888e4d..98c51a1d75 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",
|
||
--
|
||
GitLab
|
||
|
||
|
||
From 21a204147b16539b3eda3143b32844c49e29f4d4 Mon Sep 17 00:00:00 2001
|
||
From: Philip Withnall <pwithnall@endlessos.org>
|
||
Date: Thu, 15 Dec 2022 16:49:28 +0000
|
||
Subject: [PATCH 2/2] gvariant: Propagate trust when getting a child of a
|
||
serialised variant
|
||
|
||
If a variant is trusted, that means all its children are trusted, so
|
||
ensure that their checked offsets are set as such.
|
||
|
||
This allows a lot of the offset table checks to be avoided when getting
|
||
children from trusted serialised tuples, which speeds things up.
|
||
|
||
No unit test is included because this is just a performance fix. If
|
||
there are other slownesses, or regressions, in serialised `GVariant`
|
||
performance, the fuzzing setup will catch them like it did this one.
|
||
|
||
This change does reduce the time to run the oss-fuzz reproducer from 80s
|
||
to about 0.7s on my machine.
|
||
|
||
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
|
||
|
||
Fixes: #2841
|
||
oss-fuzz#54314
|
||
---
|
||
glib/gvariant-core.c | 4 ++--
|
||
1 file changed, 2 insertions(+), 2 deletions(-)
|
||
|
||
diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c
|
||
index f441c4757e..4778022829 100644
|
||
--- a/glib/gvariant-core.c
|
||
+++ b/glib/gvariant-core.c
|
||
@@ -1198,8 +1198,8 @@ 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;
|
||
- child->contents.serialised.checked_offsets_up_to = s_child.checked_offsets_up_to;
|
||
+ child->contents.serialised.ordered_offsets_up_to = (value->state & STATE_TRUSTED) ? G_MAXSIZE : s_child.ordered_offsets_up_to;
|
||
+ child->contents.serialised.checked_offsets_up_to = (value->state & STATE_TRUSTED) ? G_MAXSIZE : s_child.checked_offsets_up_to;
|
||
|
||
return child;
|
||
}
|
||
--
|
||
GitLab
|
||
|