From 2d55b3b74b1bc256e91a9d0d59120570376e6acc Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 7 Jan 2022 16:42:14 +0000 Subject: [PATCH] =?UTF-8?q?gvariant:=20Don=E2=80=99t=20allow=20child=20ele?= =?UTF-8?q?ments=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 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 a75e7db9e..c0e1931c3 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 d32e25548..fda59d684 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 a1bccb834..661bd8fd1 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 6912eebe0..16af7a567 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 c759e8d70..0cb69d4b7 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);