From 7cf6f5b69146d20948d42f0c476688fe17fef787 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 16 Aug 2023 12:09:06 +0000 Subject: [PATCH] gvariant: Don't allow child elements of a tuple to overlap each other 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 CVE: CVE-2023-32665 Upstream-Status: Backport [https://gitlab.gnome.org/GNOME/glib/-/commit/7cf6f5b69146d20948d42f0c476688fe17fef787] Signed-off-by: Soumya Sambu --- 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 4e0b2b5..c57ee77 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 * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -179,7 +180,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. * @@ -1139,6 +1140,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 2493e76..d46d05c 100644 --- a/glib/gvariant-serialiser.c +++ b/glib/gvariant-serialiser.c @@ -942,6 +942,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_, @@ -1028,6 +1032,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 3ab83b3..03826f9 100644 --- a/glib/gvariant-serialiser.h +++ b/glib/gvariant-serialiser.h @@ -34,8 +34,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 42ffc9a..f645e05 100644 --- a/glib/gvariant.c +++ b/glib/gvariant.c @@ -5997,6 +5997,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 291f796..3ddff96 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 * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -1447,6 +1448,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, @@ -1570,6 +1572,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); @@ -1734,6 +1737,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); @@ -1830,6 +1834,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); @@ -5090,6 +5095,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 @@ -5236,6 +5411,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); -- 2.40.0