1From 78da5faccb3e065116b75b3ff87ff55381da6c76 Mon Sep 17 00:00:00 2001 2From: Philip Withnall <pwithnall@endlessos.org> 3Date: Thu, 15 Dec 2022 13:00:39 +0000 4Subject: [PATCH 1/2] =?UTF-8?q?gvariant:=20Check=20offset=20table=20doesn?= 5 =?UTF-8?q?=E2=80=99t=20fall=20outside=20variant=20bounds?= 6MIME-Version: 1.0 7Content-Type: text/plain; charset=UTF-8 8Content-Transfer-Encoding: 8bit 9 10When dereferencing the first entry in the offset table for a tuple, 11check that it doesn’t fall outside the bounds of the variant first. 12 13This prevents an out-of-bounds read from some non-normal tuples. 14 15This bug was introduced in commit 73d0aa81c2575a5c9ae77d. 16 17Includes a unit test, although the test will likely only catch the 18original bug if run with asan enabled. 19 20Signed-off-by: Philip Withnall <pwithnall@endlessos.org> 21 22Fixes: #2840 23oss-fuzz#54302 24--- 25 glib/gvariant-serialiser.c | 12 ++++++-- 26 glib/tests/gvariant.c | 63 ++++++++++++++++++++++++++++++++++++++ 27 2 files changed, 72 insertions(+), 3 deletions(-) 28 29diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c 30index f443c2eb85..4e4a73ad17 100644 31--- a/glib/gvariant-serialiser.c 32+++ b/glib/gvariant-serialiser.c 33@@ -984,7 +984,8 @@ gvs_tuple_get_member_bounds (GVariantSerialised value, 34 35 member_info = g_variant_type_info_member_info (value.type_info, index_); 36 37- if (member_info->i + 1) 38+ if (member_info->i + 1 && 39+ offset_size * (member_info->i + 1) <= value.size) 40 member_start = gvs_read_unaligned_le (value.data + value.size - 41 offset_size * (member_info->i + 1), 42 offset_size); 43@@ -995,7 +996,8 @@ gvs_tuple_get_member_bounds (GVariantSerialised value, 44 member_start &= member_info->b; 45 member_start |= member_info->c; 46 47- if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_LAST) 48+ if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_LAST && 49+ offset_size * (member_info->i + 1) <= value.size) 50 member_end = value.size - offset_size * (member_info->i + 1); 51 52 else if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_FIXED) 53@@ -1006,11 +1008,15 @@ gvs_tuple_get_member_bounds (GVariantSerialised value, 54 member_end = member_start + fixed_size; 55 } 56 57- else /* G_VARIANT_MEMBER_ENDING_OFFSET */ 58+ else if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_OFFSET && 59+ offset_size * (member_info->i + 2) <= value.size) 60 member_end = gvs_read_unaligned_le (value.data + value.size - 61 offset_size * (member_info->i + 2), 62 offset_size); 63 64+ else /* invalid */ 65+ member_end = G_MAXSIZE; 66+ 67 if (out_member_start != NULL) 68 *out_member_start = member_start; 69 if (out_member_end != NULL) 70diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c 71index b360888e4d..98c51a1d75 100644 72--- a/glib/tests/gvariant.c 73+++ b/glib/tests/gvariant.c 74@@ -5576,6 +5576,67 @@ test_normal_checking_tuple_offsets4 (void) 75 g_variant_unref (variant); 76 } 77 78+/* This is a regression test that dereferencing the first element in the offset 79+ * table doesn’t dereference memory before the start of the GVariant. The first 80+ * element in the offset table gives the offset of the final member in the 81+ * tuple (the offset table is stored in reverse), and the position of this final 82+ * member is needed to check that none of the tuple members overlap with the 83+ * offset table 84+ * 85+ * See https://gitlab.gnome.org/GNOME/glib/-/issues/2840 */ 86+static void 87+test_normal_checking_tuple_offsets5 (void) 88+{ 89+ /* A tuple of type (sss) in normal form would have an offset table with two 90+ * entries: 91+ * - The first entry (lowest index in the table) gives the offset of the 92+ * third `s` in the tuple, as the offset table is reversed compared to the 93+ * tuple members. 94+ * - The second entry (highest index in the table) gives the offset of the 95+ * second `s` in the tuple. 96+ * - The offset of the first `s` in the tuple is always 0. 97+ * 98+ * See §2.5.4 (Structures) of the GVariant specification for details, noting 99+ * that the table is only layed out this way because all three members of the 100+ * tuple have non-fixed sizes. 101+ * 102+ * It’s not clear whether the 0xaa data of this variant is part of the strings 103+ * in the tuple, or part of the offset table. It doesn’t really matter. This 104+ * is a regression test to check that the code to validate the offset table 105+ * doesn’t unconditionally try to access the first entry in the offset table 106+ * by subtracting the table size from the end of the GVariant data. 107+ * 108+ * In this non-normal case, that would result in an address off the start of 109+ * the GVariant data, and an out-of-bounds read, because the GVariant is one 110+ * byte long, but the offset table is calculated as two bytes long (with 1B 111+ * sized entries) from the tuple’s type. 112+ */ 113+ const GVariantType *data_type = G_VARIANT_TYPE ("(sss)"); 114+ const guint8 data[] = { 0xaa }; 115+ gsize size = sizeof (data); 116+ GVariant *variant = NULL; 117+ GVariant *normal_variant = NULL; 118+ GVariant *expected = NULL; 119+ 120+ g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/2840"); 121+ 122+ variant = g_variant_new_from_data (data_type, data, size, FALSE, NULL, NULL); 123+ g_assert_nonnull (variant); 124+ 125+ g_assert_false (g_variant_is_normal_form (variant)); 126+ 127+ normal_variant = g_variant_get_normal_form (variant); 128+ g_assert_nonnull (normal_variant); 129+ 130+ expected = g_variant_new_parsed ("('', '', '')"); 131+ g_assert_cmpvariant (expected, variant); 132+ g_assert_cmpvariant (expected, normal_variant); 133+ 134+ g_variant_unref (expected); 135+ g_variant_unref (normal_variant); 136+ g_variant_unref (variant); 137+} 138+ 139 /* Test that an otherwise-valid serialised GVariant is considered non-normal if 140 * its offset table entries are too wide. 141 * 142@@ -5827,6 +5888,8 @@ main (int argc, char **argv) 143 test_normal_checking_tuple_offsets3); 144 g_test_add_func ("/gvariant/normal-checking/tuple-offsets4", 145 test_normal_checking_tuple_offsets4); 146+ g_test_add_func ("/gvariant/normal-checking/tuple-offsets5", 147+ test_normal_checking_tuple_offsets5); 148 g_test_add_func ("/gvariant/normal-checking/tuple-offsets/minimal-sized", 149 test_normal_checking_tuple_offsets_minimal_sized); 150 g_test_add_func ("/gvariant/normal-checking/empty-object-path", 151-- 152GitLab 153 154 155From 21a204147b16539b3eda3143b32844c49e29f4d4 Mon Sep 17 00:00:00 2001 156From: Philip Withnall <pwithnall@endlessos.org> 157Date: Thu, 15 Dec 2022 16:49:28 +0000 158Subject: [PATCH 2/2] gvariant: Propagate trust when getting a child of a 159 serialised variant 160 161If a variant is trusted, that means all its children are trusted, so 162ensure that their checked offsets are set as such. 163 164This allows a lot of the offset table checks to be avoided when getting 165children from trusted serialised tuples, which speeds things up. 166 167No unit test is included because this is just a performance fix. If 168there are other slownesses, or regressions, in serialised `GVariant` 169performance, the fuzzing setup will catch them like it did this one. 170 171This change does reduce the time to run the oss-fuzz reproducer from 80s 172to about 0.7s on my machine. 173 174Signed-off-by: Philip Withnall <pwithnall@endlessos.org> 175 176Fixes: #2841 177oss-fuzz#54314 178--- 179 glib/gvariant-core.c | 4 ++-- 180 1 file changed, 2 insertions(+), 2 deletions(-) 181 182diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c 183index f441c4757e..4778022829 100644 184--- a/glib/gvariant-core.c 185+++ b/glib/gvariant-core.c 186@@ -1198,8 +1198,8 @@ g_variant_get_child_value (GVariant *value, 187 child->contents.serialised.bytes = 188 g_bytes_ref (value->contents.serialised.bytes); 189 child->contents.serialised.data = s_child.data; 190- child->contents.serialised.ordered_offsets_up_to = s_child.ordered_offsets_up_to; 191- child->contents.serialised.checked_offsets_up_to = s_child.checked_offsets_up_to; 192+ child->contents.serialised.ordered_offsets_up_to = (value->state & STATE_TRUSTED) ? G_MAXSIZE : s_child.ordered_offsets_up_to; 193+ child->contents.serialised.checked_offsets_up_to = (value->state & STATE_TRUSTED) ? G_MAXSIZE : s_child.checked_offsets_up_to; 194 195 return child; 196 } 197-- 198GitLab 199 200