diff --git a/SOURCES/2826.patch b/SOURCES/2826.patch new file mode 100644 index 0000000..83d527a --- /dev/null +++ b/SOURCES/2826.patch @@ -0,0 +1,278 @@ +From 764f071909df70622e79ee71323973c18c055c8c Mon Sep 17 00:00:00 2001 +From: Giuseppe Scrivano +Date: Mon, 14 Sep 2020 16:28:10 +0200 +Subject: [PATCH 1/5] gdbusauth: empty DATA does not need a trailing space + +This is an interoperability fix. If the line is exactly "DATA\r\n", +the reference implementation of D-Bus treats this as equivalent to +"DATA \r\n", meaning the data block consists of zero hex-encoded bytes. +In practice, D-Bus clients send empty data blocks as "DATA\r\n", and +in fact sd-bus only accepts that, rejecting "DATA \r\n". + +[Originally part of a larger commit; commit message added by smcv] + +Signed-off-by: Giuseppe Scrivano +Co-authored-by: Simon McVittie +Signed-off-by: Simon McVittie +--- + gio/gdbusauth.c | 8 ++++---- + 1 file changed, 4 insertions(+), 4 deletions(-) + +diff --git a/gio/gdbusauth.c b/gio/gdbusauth.c +index ede21c8514..d2ca41a201 100644 +--- a/gio/gdbusauth.c ++++ b/gio/gdbusauth.c +@@ -783,13 +783,13 @@ _g_dbus_auth_run_client (GDBusAuth *auth, + if (line == NULL) + goto out; + debug_print ("CLIENT: WaitingForData, read='%s'", line); +- if (g_str_has_prefix (line, "DATA ")) ++ if (g_str_equal (line, "DATA") || g_str_has_prefix (line, "DATA ")) + { + gchar *encoded; + gchar *decoded_data; + gsize decoded_data_len = 0; + +- encoded = g_strdup (line + 5); ++ encoded = g_strdup (line + 4); + g_free (line); + g_strstrip (encoded); + decoded_data = hexdecode (encoded, &decoded_data_len, error); +@@ -1255,13 +1255,13 @@ _g_dbus_auth_run_server (GDBusAuth *auth, + debug_print ("SERVER: WaitingForData, read '%s'", line); + if (line == NULL) + goto out; +- if (g_str_has_prefix (line, "DATA ")) ++ if (g_str_equal (line, "DATA") || g_str_has_prefix (line, "DATA ")) + { + gchar *encoded; + gchar *decoded_data; + gsize decoded_data_len = 0; + +- encoded = g_strdup (line + 5); ++ encoded = g_strdup (line + 4); + g_free (line); + g_strstrip (encoded); + decoded_data = hexdecode (encoded, &decoded_data_len, error); +-- +GitLab + + +From a7d2e727eefcf883bb463ad559f5632e8e448757 Mon Sep 17 00:00:00 2001 +From: Giuseppe Scrivano +Date: Mon, 14 Sep 2020 16:28:10 +0200 +Subject: [PATCH 2/5] GDBusServer: If no initial response for EXTERNAL, send a + challenge + +Sending an "initial response" along with the AUTH command is meant +to be an optional optimization, and clients are allowed to omit it. +We must reply with our initial challenge, which in the case of EXTERNAL +is an empty string: the client responds to that with the authorization +identity. + +If we do not reply to the AUTH command, then the client will wait +forever for our reply, while we wait forever for the reply that we +expect the client to send, resulting in deadlock. + +D-Bus does not have a way to distinguish between an empty initial +response and the absence of an initial response, so clients that want +to use an empty authorization identity, such as systed's sd-bus, +cannot use the initial-response optimization and will fail to connect +to a GDBusServer that does not have this change. + +[Originally part of a larger commit; commit message added by smcv.] + +Signed-off-by: Simon McVittie +--- + gio/gdbusauthmechanismexternal.c | 23 ++++++++++++++++++----- + 1 file changed, 18 insertions(+), 5 deletions(-) + +diff --git a/gio/gdbusauthmechanismexternal.c b/gio/gdbusauthmechanismexternal.c +index 617fe1d0e5..ddd06cbd5e 100644 +--- a/gio/gdbusauthmechanismexternal.c ++++ b/gio/gdbusauthmechanismexternal.c +@@ -40,6 +40,7 @@ struct _GDBusAuthMechanismExternalPrivate + gboolean is_client; + gboolean is_server; + GDBusAuthMechanismState state; ++ gboolean empty_data_sent; + }; + + static gint mechanism_get_priority (void); +@@ -253,7 +254,9 @@ mechanism_server_initiate (GDBusAuthMechanism *mechanism, + } + else + { +- m->priv->state = G_DBUS_AUTH_MECHANISM_STATE_WAITING_FOR_DATA; ++ /* The initial-response optimization was not used, so we need to ++ * send an empty challenge to prompt the client to respond. */ ++ m->priv->state = G_DBUS_AUTH_MECHANISM_STATE_HAVE_DATA_TO_SEND; + } + } + +@@ -288,12 +291,22 @@ mechanism_server_data_send (GDBusAuthMechanism *mechanism, + + g_return_val_if_fail (G_IS_DBUS_AUTH_MECHANISM_EXTERNAL (mechanism), NULL); + g_return_val_if_fail (m->priv->is_server && !m->priv->is_client, NULL); +- g_return_val_if_fail (m->priv->state == G_DBUS_AUTH_MECHANISM_STATE_HAVE_DATA_TO_SEND, NULL); + +- /* can never end up here because we are never in the HAVE_DATA_TO_SEND state */ +- g_assert_not_reached (); ++ if (out_data_len) ++ *out_data_len = 0; + +- return NULL; ++ if (m->priv->empty_data_sent) ++ { ++ /* We have already sent an empty data response. ++ Reject the connection. */ ++ m->priv->state = G_DBUS_AUTH_MECHANISM_STATE_REJECTED; ++ return NULL; ++ } ++ ++ m->priv->state = G_DBUS_AUTH_MECHANISM_STATE_WAITING_FOR_DATA; ++ m->priv->empty_data_sent = TRUE; ++ ++ return g_strdup (""); + } + + static gchar * +-- +GitLab + + +From b51e3ab09e39c590c65a7be6228ecfa48a6189f6 Mon Sep 17 00:00:00 2001 +From: Giuseppe Scrivano +Date: Mon, 14 Sep 2020 16:28:10 +0200 +Subject: [PATCH 3/5] GDBusServer: Accept empty authorization identity for + EXTERNAL mechanism + +RFC 4422 appendix A defines the empty authorization identity to mean +the identity that the server associated with its authentication +credentials. In this case, this means whatever uid is in the +GCredentials object. + +In particular, this means that clients in a different Linux user +namespace can authenticate against our server and will be authorized +as the version of their uid that is visible in the server's namespace, +even if the corresponding numeric uid returned by geteuid() in the +client's namespace was different. systemd's sd-bus has relied on this +since commit +https://github.com/systemd/systemd/commit/1ed4723d38cd0d1423c8fe650f90fa86007ddf55. + +[Originally part of a larger commit; commit message added by smcv] + +Signed-off-by: Simon McVittie +--- + gio/gdbusauthmechanismexternal.c | 16 +++++++++++++--- + 1 file changed, 13 insertions(+), 3 deletions(-) + +diff --git a/gio/gdbusauthmechanismexternal.c b/gio/gdbusauthmechanismexternal.c +index ddd06cbd5e..a465862d12 100644 +--- a/gio/gdbusauthmechanismexternal.c ++++ b/gio/gdbusauthmechanismexternal.c +@@ -201,14 +201,24 @@ data_matches_credentials (const gchar *data, + if (credentials == NULL) + goto out; + +- if (data == NULL || data_len == 0) +- goto out; +- + #if defined(G_OS_UNIX) + { + gint64 alleged_uid; + gchar *endp; + ++ /* If we were unable to find out the uid, then nothing ++ * can possibly match it. */ ++ if (g_credentials_get_unix_user (credentials, NULL) == (uid_t) -1) ++ goto out; ++ ++ /* An empty authorization identity means we want to be ++ * whatever identity the out-of-band credentials say we have ++ * (RFC 4422 appendix A.1). This effectively matches any uid. */ ++ if (data == NULL || data_len == 0) ++ { ++ match = TRUE; ++ goto out; ++ } + /* on UNIX, this is the uid as a string in base 10 */ + alleged_uid = g_ascii_strtoll (data, &endp, 10); + if (*endp == '\0') +-- +GitLab + + +From 3f532af65c98e4ba8426c53f26c9ee15d3692f9c Mon Sep 17 00:00:00 2001 +From: Simon McVittie +Date: Mon, 18 Jul 2022 17:14:44 +0100 +Subject: [PATCH 4/5] gdbusauth: Represent empty data block as DATA\r\n, with + no space + +This is an interoperability fix. The reference implementation of D-Bus +treats "DATA\r\n" as equivalent to "DATA \r\n", but sd-bus does not, +and only accepts the former. + +Signed-off-by: Simon McVittie +--- + gio/gdbusauth.c | 34 ++++++++++++++++++++++++++-------- + 1 file changed, 26 insertions(+), 8 deletions(-) + +diff --git a/gio/gdbusauth.c b/gio/gdbusauth.c +index d2ca41a201..89cbbf67c6 100644 +--- a/gio/gdbusauth.c ++++ b/gio/gdbusauth.c +@@ -807,11 +807,21 @@ _g_dbus_auth_run_client (GDBusAuth *auth, + { + gchar *data; + gsize data_len; +- gchar *encoded_data; ++ + data = _g_dbus_auth_mechanism_client_data_send (mech, &data_len); +- encoded_data = _g_dbus_hexencode (data, data_len); +- s = g_strdup_printf ("DATA %s\r\n", encoded_data); +- g_free (encoded_data); ++ ++ if (data_len == 0) ++ { ++ s = g_strdup ("DATA\r\n"); ++ } ++ else ++ { ++ gchar *encoded_data = _g_dbus_hexencode (data, data_len); ++ ++ s = g_strdup_printf ("DATA %s\r\n", encoded_data); ++ g_free (encoded_data); ++ } ++ + g_free (data); + debug_print ("CLIENT: writing '%s'", s); + if (!g_data_output_stream_put_string (dos, s, cancellable, error)) +@@ -1209,13 +1219,21 @@ _g_dbus_auth_run_server (GDBusAuth *auth, + gsize data_len; + + data = _g_dbus_auth_mechanism_server_data_send (mech, &data_len); ++ + if (data != NULL) + { +- gchar *encoded_data; ++ if (data_len == 0) ++ { ++ s = g_strdup ("DATA\r\n"); ++ } ++ else ++ { ++ gchar *encoded_data = _g_dbus_hexencode (data, data_len); ++ ++ s = g_strdup_printf ("DATA %s\r\n", encoded_data); ++ g_free (encoded_data); ++ } + +- encoded_data = _g_dbus_hexencode (data, data_len); +- s = g_strdup_printf ("DATA %s\r\n", encoded_data); +- g_free (encoded_data); + g_free (data); + + debug_print ("SERVER: writing '%s'", s); +-- +GitLab diff --git a/SOURCES/3126.patch b/SOURCES/3126.patch new file mode 100644 index 0000000..4bd6a17 --- /dev/null +++ b/SOURCES/3126.patch @@ -0,0 +1,3021 @@ +From b4ae1179cbcb91992adc0d5d4fa13341bbcd6c28 Mon Sep 17 00:00:00 2001 +From: William Manley +Date: Tue, 23 Jun 2020 22:59:58 +0100 +Subject: [PATCH 01/19] gvariant-core: Consolidate construction of + `GVariantSerialised` + +So I only need to change it in one place. + +This introduces no functional changes. + +Helps: #2121 +--- + glib/gvariant-core.c | 49 ++++++++++++++++++++++---------------------- + 1 file changed, 25 insertions(+), 24 deletions(-) + +diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c +index b34ba8d8e..cd6a88768 100644 +--- a/glib/gvariant-core.c ++++ b/glib/gvariant-core.c +@@ -349,6 +349,27 @@ g_variant_ensure_size (GVariant *value) + } + } + ++/* < private > ++ * g_variant_to_serialised: ++ * @value: a #GVariant ++ * ++ * Gets a GVariantSerialised for a GVariant in state STATE_SERIALISED. ++ */ ++inline static GVariantSerialised ++g_variant_to_serialised (GVariant *value) ++{ ++ g_assert (value->state & STATE_SERIALISED); ++ { ++ GVariantSerialised serialised = { ++ value->type_info, ++ (gpointer) value->contents.serialised.data, ++ value->size, ++ value->depth, ++ }; ++ return serialised; ++ } ++} ++ + /* < private > + * g_variant_serialise: + * @value: a #GVariant +@@ -1007,16 +1028,8 @@ g_variant_n_children (GVariant *value) + g_variant_lock (value); + + if (value->state & STATE_SERIALISED) +- { +- GVariantSerialised serialised = { +- value->type_info, +- (gpointer) value->contents.serialised.data, +- value->size, +- value->depth, +- }; +- +- n_children = g_variant_serialised_n_children (serialised); +- } ++ n_children = g_variant_serialised_n_children ( ++ g_variant_to_serialised (value)); + else + n_children = value->contents.tree.n_children; + +@@ -1083,12 +1096,7 @@ g_variant_get_child_value (GVariant *value, + } + + { +- GVariantSerialised serialised = { +- value->type_info, +- (gpointer) value->contents.serialised.data, +- value->size, +- value->depth, +- }; ++ GVariantSerialised serialised = g_variant_to_serialised (value); + GVariantSerialised s_child; + GVariant *child; + +@@ -1201,14 +1209,7 @@ g_variant_is_normal_form (GVariant *value) + + if (value->state & STATE_SERIALISED) + { +- GVariantSerialised serialised = { +- value->type_info, +- (gpointer) value->contents.serialised.data, +- value->size, +- value->depth +- }; +- +- if (g_variant_serialised_is_normal (serialised)) ++ if (g_variant_serialised_is_normal (g_variant_to_serialised (value))) + value->state |= STATE_TRUSTED; + } + else +-- +2.40.0 + +From d51c16a7c8c3a57f1bad4305363ad85914ecab2e Mon Sep 17 00:00:00 2001 +From: William Manley +Date: Thu, 25 Jun 2020 17:08:21 +0100 +Subject: [PATCH 02/19] gvariant-serialiser: Factor out functions for dealing + with framing offsets + +This introduces no functional changes. + +Helps: #2121 +--- + glib/gvariant-serialiser.c | 108 +++++++++++++++++++------------------ + 1 file changed, 57 insertions(+), 51 deletions(-) + +diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c +index 06f419fe4..ab6a56eb6 100644 +--- a/glib/gvariant-serialiser.c ++++ b/glib/gvariant-serialiser.c +@@ -633,30 +633,62 @@ gvs_calculate_total_size (gsize body_size, + return body_size + 8 * offsets; + } + ++struct Offsets ++{ ++ gsize data_size; ++ ++ guchar *array; ++ gsize length; ++ guint offset_size; ++ ++ gboolean is_normal; ++}; ++ + static gsize +-gvs_variable_sized_array_n_children (GVariantSerialised value) ++gvs_offsets_get_offset_n (struct Offsets *offsets, ++ gsize n) ++{ ++ return gvs_read_unaligned_le ( ++ offsets->array + (offsets->offset_size * n), offsets->offset_size); ++} ++ ++static struct Offsets ++gvs_variable_sized_array_get_frame_offsets (GVariantSerialised value) + { ++ struct Offsets out = { 0, }; + gsize offsets_array_size; +- gsize offset_size; + gsize last_end; + + if (value.size == 0) +- return 0; +- +- offset_size = gvs_get_offset_size (value.size); ++ { ++ out.is_normal = TRUE; ++ return out; ++ } + +- last_end = gvs_read_unaligned_le (value.data + value.size - +- offset_size, offset_size); ++ out.offset_size = gvs_get_offset_size (value.size); ++ last_end = gvs_read_unaligned_le (value.data + value.size - out.offset_size, ++ out.offset_size); + + if (last_end > value.size) +- return 0; ++ return out; /* offsets not normal */ + + offsets_array_size = value.size - last_end; + +- if (offsets_array_size % offset_size) +- return 0; ++ if (offsets_array_size % out.offset_size) ++ return out; /* offsets not normal */ ++ ++ out.data_size = last_end; ++ out.array = value.data + last_end; ++ out.length = offsets_array_size / out.offset_size; ++ out.is_normal = TRUE; + +- return offsets_array_size / offset_size; ++ return out; ++} ++ ++static gsize ++gvs_variable_sized_array_n_children (GVariantSerialised value) ++{ ++ return gvs_variable_sized_array_get_frame_offsets (value).length; + } + + static GVariantSerialised +@@ -664,8 +696,9 @@ gvs_variable_sized_array_get_child (GVariantSerialised value, + gsize index_) + { + GVariantSerialised child = { 0, }; +- gsize offset_size; +- gsize last_end; ++ ++ struct Offsets offsets = gvs_variable_sized_array_get_frame_offsets (value); ++ + gsize start; + gsize end; + +@@ -673,18 +706,11 @@ gvs_variable_sized_array_get_child (GVariantSerialised value, + g_variant_type_info_ref (child.type_info); + child.depth = value.depth + 1; + +- offset_size = gvs_get_offset_size (value.size); +- +- last_end = gvs_read_unaligned_le (value.data + value.size - +- offset_size, offset_size); +- + if (index_ > 0) + { + guint alignment; + +- start = gvs_read_unaligned_le (value.data + last_end + +- (offset_size * (index_ - 1)), +- offset_size); ++ start = gvs_offsets_get_offset_n (&offsets, index_ - 1); + + g_variant_type_info_query (child.type_info, &alignment, NULL); + start += (-start) & alignment; +@@ -692,11 +718,9 @@ gvs_variable_sized_array_get_child (GVariantSerialised value, + else + start = 0; + +- end = gvs_read_unaligned_le (value.data + last_end + +- (offset_size * index_), +- offset_size); ++ end = gvs_offsets_get_offset_n (&offsets, index_); + +- if (start < end && end <= value.size && end <= last_end) ++ if (start < end && end <= value.size && end <= offsets.data_size) + { + child.data = value.data + start; + child.size = end - start; +@@ -768,34 +792,16 @@ static gboolean + gvs_variable_sized_array_is_normal (GVariantSerialised value) + { + GVariantSerialised child = { 0, }; +- gsize offsets_array_size; +- guchar *offsets_array; +- guint offset_size; + guint alignment; +- gsize last_end; +- gsize length; + gsize offset; + gsize i; + +- if (value.size == 0) +- return TRUE; +- +- offset_size = gvs_get_offset_size (value.size); +- last_end = gvs_read_unaligned_le (value.data + value.size - +- offset_size, offset_size); ++ struct Offsets offsets = gvs_variable_sized_array_get_frame_offsets (value); + +- if (last_end > value.size) ++ if (!offsets.is_normal) + return FALSE; + +- offsets_array_size = value.size - last_end; +- +- if (offsets_array_size % offset_size) +- return FALSE; +- +- offsets_array = value.data + value.size - offsets_array_size; +- length = offsets_array_size / offset_size; +- +- if (length == 0) ++ if (value.size != 0 && offsets.length == 0) + return FALSE; + + child.type_info = g_variant_type_info_element (value.type_info); +@@ -803,14 +809,14 @@ gvs_variable_sized_array_is_normal (GVariantSerialised value) + child.depth = value.depth + 1; + offset = 0; + +- for (i = 0; i < length; i++) ++ for (i = 0; i < offsets.length; i++) + { + gsize this_end; + +- this_end = gvs_read_unaligned_le (offsets_array + offset_size * i, +- offset_size); ++ this_end = gvs_read_unaligned_le (offsets.array + offsets.offset_size * i, ++ offsets.offset_size); + +- if (this_end < offset || this_end > last_end) ++ if (this_end < offset || this_end > offsets.data_size) + return FALSE; + + while (offset & alignment) +@@ -832,7 +838,7 @@ gvs_variable_sized_array_is_normal (GVariantSerialised value) + offset = this_end; + } + +- g_assert (offset == last_end); ++ g_assert (offset == offsets.data_size); + + return TRUE; + } +-- +2.40.0 + +From fd215233ae37ce20cd84eb128c86b90c4dd610fa Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Tue, 25 Oct 2022 18:05:52 +0100 +Subject: [PATCH 03/19] gvariant: Zero-initialise various GVariantSerialised + objects + +The following few commits will add a couple of new fields to +`GVariantSerialised`, and they should be zero-filled by default. + +Try and pre-empt that a bit by zero-filling `GVariantSerialised` by +default in a few places. + +Signed-off-by: Philip Withnall + +Helps: #2121 +--- + glib/gvariant.c | 2 +- + glib/tests/gvariant.c | 12 ++++++------ + 2 files changed, 7 insertions(+), 7 deletions(-) + +diff --git a/glib/gvariant.c b/glib/gvariant.c +index e48dec1ad..8ebbaa662 100644 +--- a/glib/gvariant.c ++++ b/glib/gvariant.c +@@ -5944,7 +5944,7 @@ g_variant_byteswap (GVariant *value) + if (alignment) + /* (potentially) contains multi-byte numeric data */ + { +- GVariantSerialised serialised; ++ GVariantSerialised serialised = { 0, }; + GVariant *trusted; + GBytes *bytes; + +diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c +index 35434cae9..ebd617639 100644 +--- a/glib/tests/gvariant.c ++++ b/glib/tests/gvariant.c +@@ -1438,7 +1438,7 @@ test_maybe (void) + + for (flavour = 0; flavour < 8; flavour += alignment) + { +- GVariantSerialised serialised; ++ GVariantSerialised serialised = { 0, }; + GVariantSerialised child; + + serialised.type_info = type_info; +@@ -1562,7 +1562,7 @@ test_array (void) + + for (flavour = 0; flavour < 8; flavour += alignment) + { +- GVariantSerialised serialised; ++ GVariantSerialised serialised = { 0, }; + + serialised.type_info = array_info; + serialised.data = flavoured_malloc (needed_size, flavour); +@@ -1726,7 +1726,7 @@ test_tuple (void) + + for (flavour = 0; flavour < 8; flavour += alignment) + { +- GVariantSerialised serialised; ++ GVariantSerialised serialised = { 0, }; + + serialised.type_info = type_info; + serialised.data = flavoured_malloc (needed_size, flavour); +@@ -1821,7 +1821,7 @@ test_variant (void) + + for (flavour = 0; flavour < 8; flavour += alignment) + { +- GVariantSerialised serialised; ++ GVariantSerialised serialised = { 0, }; + GVariantSerialised child; + + serialised.type_info = type_info; +@@ -2268,7 +2268,7 @@ serialise_tree (TreeInstance *tree, + static void + test_byteswap (void) + { +- GVariantSerialised one, two; ++ GVariantSerialised one = { 0, }, two = { 0, }; + TreeInstance *tree; + + tree = tree_instance_new (NULL, 3); +@@ -2342,7 +2342,7 @@ test_serialiser_children (void) + static void + test_fuzz (gdouble *fuzziness) + { +- GVariantSerialised serialised; ++ GVariantSerialised serialised = { 0, }; + TreeInstance *tree; + + /* make an instance */ +-- +2.40.0 + +From 96e27afc7a1da0944d995ff0715679870ce8d503 Mon Sep 17 00:00:00 2001 +From: William Manley +Date: Mon, 29 Jun 2020 16:59:44 +0100 +Subject: [PATCH 04/19] =?UTF-8?q?gvariant:=20Don=E2=80=99t=20allow=20child?= + =?UTF-8?q?=20elements=20to=20overlap=20with=20each=20other?= +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +If different elements of a variable sized array can overlap with each +other then we can cause a `GVariant` to normalise to a much larger type. + +This commit changes the behaviour of `GVariant` with non-normal form data. If +an invalid frame offset is found all subsequent elements are given their +default value. + +When retrieving an element at index `n` we scan the frame offsets up to index +`n` and if they are not in order we return an element with the default value +for that type. This guarantees that elements don't overlap with each +other. We remember the offset we've scanned up to so we don't need to +repeat this work on subsequent accesses. We skip these checks for trusted +data. + +Unfortunately this makes random access of untrusted data O(n) — at least +on first access. It doesn't affect the algorithmic complexity of accessing +elements in order, such as when using the `GVariantIter` interface. Also: +the cost of validation will be amortised as the `GVariant` instance is +continued to be used. + +I've implemented this with 4 different functions, 1 for each element size, +rather than looping calling `gvs_read_unaligned_le` in the hope that the +compiler will find it easy to optimise and should produce fairly tight +code. + +Fixes: #2121 +--- + glib/gvariant-core.c | 35 ++++++++++++++++ + glib/gvariant-serialiser.c | 86 ++++++++++++++++++++++++++++++++++++-- + glib/gvariant-serialiser.h | 9 ++++ + glib/tests/gvariant.c | 45 ++++++++++++++++++++ + 4 files changed, 172 insertions(+), 3 deletions(-) + +diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c +index cd6a88768..4bd7d0537 100644 +--- a/glib/gvariant-core.c ++++ b/glib/gvariant-core.c +@@ -65,6 +65,7 @@ struct _GVariant + { + GBytes *bytes; + gconstpointer data; ++ gsize ordered_offsets_up_to; + } serialised; + + struct +@@ -162,6 +163,24 @@ struct _GVariant + * if .data pointed to the appropriate number of nul + * bytes. + * ++ * .ordered_offsets_up_to: If ordered_offsets_up_to == n this means that all ++ * the frame offsets up to and including the frame ++ * offset determining the end of element n are in ++ * order. This guarantees that the bytes of element ++ * n don't overlap with any previous element. ++ * ++ * For trusted data this is set to G_MAXSIZE and we ++ * don't check that the frame offsets are in order. ++ * ++ * Note: This doesn't imply the offsets are good in ++ * any way apart from their ordering. In particular ++ * offsets may be out of bounds for this value or ++ * may imply that the data overlaps the frame ++ * offsets themselves. ++ * ++ * This field is only relevant for arrays of non ++ * fixed width types. ++ * + * .tree: Only valid when the instance is in tree form. + * + * Note that accesses from other threads could result in +@@ -365,6 +384,7 @@ g_variant_to_serialised (GVariant *value) + (gpointer) value->contents.serialised.data, + value->size, + value->depth, ++ value->contents.serialised.ordered_offsets_up_to, + }; + return serialised; + } +@@ -396,6 +416,7 @@ g_variant_serialise (GVariant *value, + serialised.size = value->size; + serialised.data = data; + serialised.depth = value->depth; ++ serialised.ordered_offsets_up_to = 0; + + children = (gpointer *) value->contents.tree.children; + n_children = value->contents.tree.n_children; +@@ -439,6 +460,15 @@ g_variant_fill_gvs (GVariantSerialised *serialised, + g_assert (serialised->size == value->size); + serialised->depth = value->depth; + ++ if (value->state & STATE_SERIALISED) ++ { ++ serialised->ordered_offsets_up_to = value->contents.serialised.ordered_offsets_up_to; ++ } ++ else ++ { ++ serialised->ordered_offsets_up_to = 0; ++ } ++ + if (serialised->data) + /* g_variant_store() is a public API, so it + * it will reacquire the lock if it needs to. +@@ -481,6 +511,7 @@ g_variant_ensure_serialised (GVariant *value) + bytes = g_bytes_new_take (data, value->size); + value->contents.serialised.data = g_bytes_get_data (bytes, NULL); + value->contents.serialised.bytes = bytes; ++ value->contents.serialised.ordered_offsets_up_to = G_MAXSIZE; + value->state |= STATE_SERIALISED; + } + } +@@ -561,6 +592,7 @@ g_variant_new_from_bytes (const GVariantType *type, + serialised.type_info = value->type_info; + serialised.data = (guchar *) g_bytes_get_data (bytes, &serialised.size); + serialised.depth = 0; ++ serialised.ordered_offsets_up_to = trusted ? G_MAXSIZE : 0; + + if (!g_variant_serialised_check (serialised)) + { +@@ -611,6 +643,8 @@ g_variant_new_from_bytes (const GVariantType *type, + value->contents.serialised.data = g_bytes_get_data (bytes, &value->size); + } + ++ value->contents.serialised.ordered_offsets_up_to = trusted ? G_MAXSIZE : 0; ++ + g_clear_pointer (&owned_bytes, g_bytes_unref); + + return value; +@@ -1130,6 +1164,7 @@ 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; + + return child; + } +diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c +index ab6a56eb6..8e9364e85 100644 +--- a/glib/gvariant-serialiser.c ++++ b/glib/gvariant-serialiser.c +@@ -1,6 +1,7 @@ + /* + * Copyright © 2007, 2008 Ryan Lortie + * Copyright © 2010 Codethink Limited ++ * Copyright © 2020 William Manley + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public +@@ -264,6 +265,7 @@ gvs_fixed_sized_maybe_get_child (GVariantSerialised value, + value.type_info = g_variant_type_info_element (value.type_info); + g_variant_type_info_ref (value.type_info); + value.depth++; ++ value.ordered_offsets_up_to = 0; + + return value; + } +@@ -295,7 +297,7 @@ gvs_fixed_sized_maybe_serialise (GVariantSerialised value, + { + if (n_children) + { +- GVariantSerialised child = { NULL, value.data, value.size, value.depth + 1 }; ++ GVariantSerialised child = { NULL, value.data, value.size, value.depth + 1, 0 }; + + gvs_filler (&child, children[0]); + } +@@ -317,6 +319,7 @@ gvs_fixed_sized_maybe_is_normal (GVariantSerialised value) + /* proper element size: "Just". recurse to the child. */ + value.type_info = g_variant_type_info_element (value.type_info); + value.depth++; ++ value.ordered_offsets_up_to = 0; + + return g_variant_serialised_is_normal (value); + } +@@ -358,6 +361,7 @@ gvs_variable_sized_maybe_get_child (GVariantSerialised value, + value.data = NULL; + + value.depth++; ++ value.ordered_offsets_up_to = 0; + + return value; + } +@@ -388,7 +392,7 @@ gvs_variable_sized_maybe_serialise (GVariantSerialised value, + { + if (n_children) + { +- GVariantSerialised child = { NULL, value.data, value.size - 1, value.depth + 1 }; ++ GVariantSerialised child = { NULL, value.data, value.size - 1, value.depth + 1, 0 }; + + /* write the data for the child. */ + gvs_filler (&child, children[0]); +@@ -408,6 +412,7 @@ gvs_variable_sized_maybe_is_normal (GVariantSerialised value) + value.type_info = g_variant_type_info_element (value.type_info); + value.size--; + value.depth++; ++ value.ordered_offsets_up_to = 0; + + return g_variant_serialised_is_normal (value); + } +@@ -691,6 +696,32 @@ gvs_variable_sized_array_n_children (GVariantSerialised value) + return gvs_variable_sized_array_get_frame_offsets (value).length; + } + ++/* Find the index of the first out-of-order element in @data, assuming that ++ * @data is an array of elements of given @type, starting at index @start and ++ * containing a further @len-@start elements. */ ++#define DEFINE_FIND_UNORDERED(type) \ ++ static gsize \ ++ find_unordered_##type (const guint8 *data, gsize start, gsize len) \ ++ { \ ++ gsize off; \ ++ type current, previous; \ ++ \ ++ memcpy (&previous, data + start * sizeof (current), sizeof (current)); \ ++ for (off = (start + 1) * sizeof (current); off < len * sizeof (current); off += sizeof (current)) \ ++ { \ ++ memcpy (¤t, data + off, sizeof (current)); \ ++ if (current < previous) \ ++ break; \ ++ previous = current; \ ++ } \ ++ return off / sizeof (current) - 1; \ ++ } ++ ++DEFINE_FIND_UNORDERED (guint8); ++DEFINE_FIND_UNORDERED (guint16); ++DEFINE_FIND_UNORDERED (guint32); ++DEFINE_FIND_UNORDERED (guint64); ++ + static GVariantSerialised + gvs_variable_sized_array_get_child (GVariantSerialised value, + gsize index_) +@@ -706,6 +737,49 @@ gvs_variable_sized_array_get_child (GVariantSerialised value, + g_variant_type_info_ref (child.type_info); + child.depth = value.depth + 1; + ++ /* 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 array elements). */ ++ if (index_ > value.ordered_offsets_up_to) ++ { ++ switch (offsets.offset_size) ++ { ++ case 1: ++ { ++ value.ordered_offsets_up_to = find_unordered_guint8 ( ++ offsets.array, value.ordered_offsets_up_to, index_ + 1); ++ break; ++ } ++ case 2: ++ { ++ value.ordered_offsets_up_to = find_unordered_guint16 ( ++ offsets.array, value.ordered_offsets_up_to, index_ + 1); ++ break; ++ } ++ case 4: ++ { ++ value.ordered_offsets_up_to = find_unordered_guint32 ( ++ offsets.array, value.ordered_offsets_up_to, index_ + 1); ++ break; ++ } ++ case 8: ++ { ++ value.ordered_offsets_up_to = find_unordered_guint64 ( ++ offsets.array, value.ordered_offsets_up_to, index_ + 1); ++ break; ++ } ++ default: ++ /* gvs_get_offset_size() only returns maximum 8 */ ++ g_assert_not_reached (); ++ } ++ } ++ ++ if (index_ > value.ordered_offsets_up_to) ++ { ++ /* Offsets are invalid somewhere, so return an empty child. */ ++ return child; ++ } ++ + if (index_ > 0) + { + guint alignment; +@@ -840,6 +914,9 @@ gvs_variable_sized_array_is_normal (GVariantSerialised value) + + g_assert (offset == offsets.data_size); + ++ /* All offsets have now been checked. */ ++ value.ordered_offsets_up_to = G_MAXSIZE; ++ + return TRUE; + } + +@@ -1072,7 +1149,7 @@ gvs_tuple_is_normal (GVariantSerialised value) + for (i = 0; i < length; i++) + { + const GVariantMemberInfo *member_info; +- GVariantSerialised child; ++ GVariantSerialised child = { 0, }; + gsize fixed_size; + guint alignment; + gsize end; +@@ -1132,6 +1209,9 @@ gvs_tuple_is_normal (GVariantSerialised value) + offset = end; + } + ++ /* All element bounds have been checked above. */ ++ value.ordered_offsets_up_to = G_MAXSIZE; ++ + { + gsize fixed_size; + guint alignment; +diff --git a/glib/gvariant-serialiser.h b/glib/gvariant-serialiser.h +index 81343e9ca..bc636000c 100644 +--- a/glib/gvariant-serialiser.h ++++ b/glib/gvariant-serialiser.h +@@ -29,6 +29,15 @@ typedef struct + guchar *data; + gsize size; + gsize depth; /* same semantics as GVariant.depth */ ++ ++ /* If ordered_offsets_up_to == n this means that all the frame offsets up to and ++ * including the frame offset determining the end of element n are in order. ++ * 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 */ ++ gsize ordered_offsets_up_to; + } GVariantSerialised; + + /* deserialisation */ +diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c +index ebd617639..7e5664e65 100644 +--- a/glib/tests/gvariant.c ++++ b/glib/tests/gvariant.c +@@ -1,5 +1,6 @@ + /* + * Copyright © 2010 Codethink Limited ++ * Copyright © 2020 William Manley + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public +@@ -1279,6 +1280,7 @@ random_instance_filler (GVariantSerialised *serialised, + serialised->size = instance->size; + + serialised->depth = 0; ++ serialised->ordered_offsets_up_to = 0; + + g_assert_true (serialised->type_info == instance->type_info); + g_assert_cmpuint (serialised->size, ==, instance->size); +@@ -5023,6 +5025,47 @@ test_normal_checking_array_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 array elements overlap. ++ * ++ * See https://gitlab.gnome.org/GNOME/glib/-/issues/2121#note_832242 */ ++static void ++test_normal_checking_array_offsets2 (void) ++{ ++ const guint8 data[] = { ++ 'h', 'i', '\0', ++ 0x03, 0x00, 0x03, ++ 0x06, 0x00, 0x06, ++ 0x09, 0x00, 0x09, ++ 0x0c, 0x00, 0x0c, ++ 0x0f, 0x00, 0x0f, ++ 0x12, 0x00, 0x12, ++ 0x15, 0x00, 0x15, ++ }; ++ gsize size = sizeof (data); ++ const GVariantType *aaaaaaas = G_VARIANT_TYPE ("aaaaaaas"); ++ GVariant *variant = NULL; ++ GVariant *normal_variant = NULL; ++ GVariant *expected = NULL; ++ ++ variant = g_variant_new_from_data (aaaaaaas, 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 * 2); ++ ++ expected = g_variant_new_parsed ( ++ "[[[[[[['hi', '', ''], [], []], [], []], [], []], [], []], [], []], [], []]"); ++ 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 a tuple with invalidly large values in its offset table is + * normalised successfully without looping infinitely. */ + static void +@@ -5190,6 +5233,8 @@ main (int argc, char **argv) + test_normal_checking_tuples); + g_test_add_func ("/gvariant/normal-checking/array-offsets", + test_normal_checking_array_offsets); ++ g_test_add_func ("/gvariant/normal-checking/array-offsets2", ++ 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/empty-object-path", +-- +2.40.0 + +From 21fba6a534b35cc4fc245c6a8822b53c84b7fb48 Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Fri, 7 Jan 2022 15:03:52 +0000 +Subject: [PATCH 05/19] gvariant-serialiser: Factor out code to get bounds of a + tuple member + +This introduces no functional changes. + +Signed-off-by: Philip Withnall + +Helps: #2121 +--- + glib/gvariant-serialiser.c | 73 ++++++++++++++++++++++++-------------- + 1 file changed, 46 insertions(+), 27 deletions(-) + +diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c +index 8e9364e85..cf4d2a2a5 100644 +--- a/glib/gvariant-serialiser.c ++++ b/glib/gvariant-serialiser.c +@@ -942,6 +942,51 @@ gvs_variable_sized_array_is_normal (GVariantSerialised value) + * for the tuple. See the notes in gvarianttypeinfo.h. + */ + ++static void ++gvs_tuple_get_member_bounds (GVariantSerialised value, ++ gsize index_, ++ gsize offset_size, ++ gsize *out_member_start, ++ gsize *out_member_end) ++{ ++ const GVariantMemberInfo *member_info; ++ gsize member_start, member_end; ++ ++ member_info = g_variant_type_info_member_info (value.type_info, index_); ++ ++ if (member_info->i + 1) ++ member_start = gvs_read_unaligned_le (value.data + value.size - ++ offset_size * (member_info->i + 1), ++ offset_size); ++ else ++ member_start = 0; ++ ++ member_start += member_info->a; ++ member_start &= member_info->b; ++ member_start |= member_info->c; ++ ++ if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_LAST) ++ member_end = value.size - offset_size * (member_info->i + 1); ++ ++ else if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_FIXED) ++ { ++ gsize fixed_size; ++ ++ g_variant_type_info_query (member_info->type_info, NULL, &fixed_size); ++ member_end = member_start + fixed_size; ++ } ++ ++ else /* G_VARIANT_MEMBER_ENDING_OFFSET */ ++ member_end = gvs_read_unaligned_le (value.data + value.size - ++ offset_size * (member_info->i + 2), ++ offset_size); ++ ++ if (out_member_start != NULL) ++ *out_member_start = member_start; ++ if (out_member_end != NULL) ++ *out_member_end = member_end; ++} ++ + static gsize + gvs_tuple_n_children (GVariantSerialised value) + { +@@ -997,33 +1042,7 @@ gvs_tuple_get_child (GVariantSerialised value, + } + } + +- if (member_info->i + 1) +- start = gvs_read_unaligned_le (value.data + value.size - +- offset_size * (member_info->i + 1), +- offset_size); +- else +- start = 0; +- +- start += member_info->a; +- start &= member_info->b; +- start |= member_info->c; +- +- if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_LAST) +- end = value.size - offset_size * (member_info->i + 1); +- +- else if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_FIXED) +- { +- gsize fixed_size; +- +- g_variant_type_info_query (child.type_info, NULL, &fixed_size); +- end = start + fixed_size; +- child.size = fixed_size; +- } +- +- else /* G_VARIANT_MEMBER_ENDING_OFFSET */ +- end = gvs_read_unaligned_le (value.data + value.size - +- offset_size * (member_info->i + 2), +- offset_size); ++ gvs_tuple_get_member_bounds (value, index_, offset_size, &start, &end); + + /* The child should not extend into the offset table. */ + if (index_ != g_variant_type_info_n_members (value.type_info) - 1) +-- +2.40.0 + +From 44ae51d04615867fe3bcb2842b93c059e764f8f7 Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Fri, 7 Jan 2022 16:37:29 +0000 +Subject: [PATCH 06/19] gvariant-serialiser: Rework child size calculation +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +This reduces a few duplicate calls to `g_variant_type_info_query()` and +explains why they’re needed. + +Signed-off-by: Philip Withnall + +Helps: #2121 +--- + glib/gvariant-serialiser.c | 31 +++++++++---------------------- + 1 file changed, 9 insertions(+), 22 deletions(-) + +diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c +index cf4d2a2a5..7ba7aed1a 100644 +--- a/glib/gvariant-serialiser.c ++++ b/glib/gvariant-serialiser.c +@@ -1007,14 +1007,18 @@ gvs_tuple_get_child (GVariantSerialised value, + child.depth = value.depth + 1; + offset_size = gvs_get_offset_size (value.size); + ++ /* Ensure the size is set for fixed-sized children, or ++ * g_variant_serialised_check() will fail, even if we return ++ * (child.data == NULL) to indicate an error. */ ++ if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_FIXED) ++ g_variant_type_info_query (child.type_info, NULL, &child.size); ++ + /* tuples are the only (potentially) fixed-sized containers, so the + * only ones that have to deal with the possibility of having %NULL + * data with a non-zero %size if errors occurred elsewhere. + */ + if G_UNLIKELY (value.data == NULL && value.size != 0) + { +- g_variant_type_info_query (child.type_info, NULL, &child.size); +- + /* this can only happen in fixed-sized tuples, + * so the child must also be fixed sized. + */ +@@ -1032,29 +1036,12 @@ gvs_tuple_get_child (GVariantSerialised value, + else + { + if (offset_size * (member_info->i + 1) > value.size) +- { +- /* if the child is fixed size, return its size. +- * if child is not fixed-sized, return size = 0. +- */ +- g_variant_type_info_query (child.type_info, NULL, &child.size); +- +- return child; +- } ++ return child; + } + +- gvs_tuple_get_member_bounds (value, index_, offset_size, &start, &end); +- + /* The child should not extend into the offset table. */ +- if (index_ != g_variant_type_info_n_members (value.type_info) - 1) +- { +- GVariantSerialised last_child; +- last_child = gvs_tuple_get_child (value, +- g_variant_type_info_n_members (value.type_info) - 1); +- last_end = last_child.data + last_child.size - value.data; +- g_variant_type_info_unref (last_child.type_info); +- } +- else +- last_end = end; ++ gvs_tuple_get_member_bounds (value, index_, offset_size, &start, &end); ++ gvs_tuple_get_member_bounds (value, g_variant_type_info_n_members (value.type_info) - 1, offset_size, NULL, &last_end); + + if (start < end && end <= value.size && end <= last_end) + { +-- +2.40.0 + +From a4ddf8ea87a43c2ceffb5d806061d9c6297b9096 Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Fri, 7 Jan 2022 16:42:14 +0000 +Subject: [PATCH 07/19] =?UTF-8?q?gvariant:=20Don=E2=80=99t=20allow=20child?= + =?UTF-8?q?=20elements=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 4bd7d0537..9b268d977 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 7ba7aed1a..470e21919 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 bc636000c..765bd43b9 100644 +--- a/glib/gvariant-serialiser.h ++++ b/glib/gvariant-serialiser.h +@@ -35,8 +35,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 8ebbaa662..5f6eb63ee 100644 +--- a/glib/gvariant.c ++++ b/glib/gvariant.c +@@ -5953,6 +5953,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 7e5664e65..9ed56d66d 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 +@@ -5237,6 +5412,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 + +From 98d5b84c2cdff1431084909b47d1a7f6d0dd60b5 Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Fri, 7 Jan 2022 16:42:14 +0000 +Subject: [PATCH 08/19] =?UTF-8?q?gvariant:=20Don=E2=80=99t=20allow=20child?= + =?UTF-8?q?=20elements=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 | 2 ++ + glib/tests/gvariant.c | 2 ++ + 2 files changed, 4 insertions(+) + +diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c +index 9b268d977..c6c50a6ff 100644 +--- a/glib/gvariant-core.c ++++ b/glib/gvariant-core.c +@@ -3,6 +3,8 @@ + * Copyright © 2010 Codethink Limited + * Copyright © 2022 Endless OS Foundation, LLC + * ++ * SPDX-License-Identifier: LGPL-2.1-or-later ++ * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either +diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c +index 9ed56d66d..a565ea69a 100644 +--- a/glib/tests/gvariant.c ++++ b/glib/tests/gvariant.c +@@ -3,6 +3,8 @@ + * Copyright © 2020 William Manley + * Copyright © 2022 Endless OS Foundation, LLC + * ++ * SPDX-License-Identifier: LGPL-2.1-or-later ++ * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either +-- +2.40.0 + +From b53b26c5596a57d06bcc3947b9c8316526d9e70b Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Tue, 25 Oct 2022 15:14:14 +0100 +Subject: [PATCH 09/19] gvariant: Track checked and ordered offsets + independently + +The past few commits introduced the concept of known-good offsets in the +offset table (which is used for variable-width arrays and tuples). +Good offsets are ones which are non-overlapping with all the previous +offsets in the table. + +If a bad offset is encountered when indexing into the array or tuple, +the cached known-good offset index will not be increased. In this way, +all child variants at and beyond the first bad offset can be returned as +default values rather than dereferencing potentially invalid data. + +In this case, there was no information about the fact that the indexes +between the highest known-good index and the requested one had been +checked already. That could lead to a pathological case where an offset +table with an invalid first offset is repeatedly checked in full when +trying to access higher-indexed children. + +Avoid that by storing the index of the highest checked offset in the +table, as well as the index of the highest good/ordered offset. + +Signed-off-by: Philip Withnall + +Helps: #2121 +--- + glib/gvariant-core.c | 28 ++++++++++++++++++++++++ + glib/gvariant-serialiser.c | 44 +++++++++++++++++++++++++++----------- + glib/gvariant-serialiser.h | 9 ++++++++ + glib/gvariant.c | 1 + + glib/tests/gvariant.c | 5 +++++ + 5 files changed, 75 insertions(+), 12 deletions(-) + +diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c +index c6c50a6ff..1efcf200f 100644 +--- a/glib/gvariant-core.c ++++ b/glib/gvariant-core.c +@@ -69,6 +69,7 @@ struct _GVariant + GBytes *bytes; + gconstpointer data; + gsize ordered_offsets_up_to; ++ gsize checked_offsets_up_to; + } serialised; + + struct +@@ -184,6 +185,24 @@ struct _GVariant + * This field is only relevant for arrays of non + * fixed width types and for tuples. + * ++ * .checked_offsets_up_to: Similarly to .ordered_offsets_up_to, this stores ++ * the index of the highest element, n, whose frame ++ * offsets (and all the preceding frame offsets) ++ * have been checked for validity. ++ * ++ * It is always the case that ++ * .checked_offsets_up_to ≥ .ordered_offsets_up_to. ++ * ++ * If .checked_offsets_up_to == .ordered_offsets_up_to, ++ * then a bad offset has not been found so far. ++ * ++ * If .checked_offsets_up_to > .ordered_offsets_up_to, ++ * then a bad offset has been found at ++ * (.ordered_offsets_up_to + 1). ++ * ++ * This field is only relevant for arrays of non ++ * fixed width types and for tuples. ++ * + * .tree: Only valid when the instance is in tree form. + * + * Note that accesses from other threads could result in +@@ -388,6 +407,7 @@ g_variant_to_serialised (GVariant *value) + value->size, + value->depth, + value->contents.serialised.ordered_offsets_up_to, ++ value->contents.serialised.checked_offsets_up_to, + }; + return serialised; + } +@@ -420,6 +440,7 @@ g_variant_serialise (GVariant *value, + serialised.data = data; + serialised.depth = value->depth; + serialised.ordered_offsets_up_to = 0; ++ serialised.checked_offsets_up_to = 0; + + children = (gpointer *) value->contents.tree.children; + n_children = value->contents.tree.n_children; +@@ -466,10 +487,12 @@ g_variant_fill_gvs (GVariantSerialised *serialised, + if (value->state & STATE_SERIALISED) + { + serialised->ordered_offsets_up_to = value->contents.serialised.ordered_offsets_up_to; ++ serialised->checked_offsets_up_to = value->contents.serialised.checked_offsets_up_to; + } + else + { + serialised->ordered_offsets_up_to = 0; ++ serialised->checked_offsets_up_to = 0; + } + + if (serialised->data) +@@ -515,6 +538,7 @@ g_variant_ensure_serialised (GVariant *value) + value->contents.serialised.data = g_bytes_get_data (bytes, NULL); + value->contents.serialised.bytes = bytes; + value->contents.serialised.ordered_offsets_up_to = G_MAXSIZE; ++ value->contents.serialised.checked_offsets_up_to = G_MAXSIZE; + value->state |= STATE_SERIALISED; + } + } +@@ -596,6 +620,7 @@ g_variant_new_from_bytes (const GVariantType *type, + serialised.data = (guchar *) g_bytes_get_data (bytes, &serialised.size); + serialised.depth = 0; + serialised.ordered_offsets_up_to = trusted ? G_MAXSIZE : 0; ++ serialised.checked_offsets_up_to = trusted ? G_MAXSIZE : 0; + + if (!g_variant_serialised_check (serialised)) + { +@@ -647,6 +672,7 @@ g_variant_new_from_bytes (const GVariantType *type, + } + + value->contents.serialised.ordered_offsets_up_to = trusted ? G_MAXSIZE : 0; ++ value->contents.serialised.checked_offsets_up_to = trusted ? G_MAXSIZE : 0; + + g_clear_pointer (&owned_bytes, g_bytes_unref); + +@@ -1144,6 +1170,7 @@ g_variant_get_child_value (GVariant *value, + + /* 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); ++ value->contents.serialised.checked_offsets_up_to = MAX (value->contents.serialised.checked_offsets_up_to, serialised.checked_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, +@@ -1171,6 +1198,7 @@ g_variant_get_child_value (GVariant *value, + 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; + + return child; + } +diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c +index 470e21919..8e01ab53e 100644 +--- a/glib/gvariant-serialiser.c ++++ b/glib/gvariant-serialiser.c +@@ -120,6 +120,8 @@ + * + * @depth has no restrictions; the depth of a top-level serialised #GVariant is + * zero, and it increases for each level of nested child. ++ * ++ * @checked_offsets_up_to is always ≥ @ordered_offsets_up_to + */ + + /* < private > +@@ -147,6 +149,9 @@ g_variant_serialised_check (GVariantSerialised serialised) + !(serialised.size == 0 || serialised.data != NULL)) + return FALSE; + ++ if (serialised.ordered_offsets_up_to > serialised.checked_offsets_up_to) ++ return FALSE; ++ + /* Depending on the native alignment requirements of the machine, the + * compiler will insert either 3 or 7 padding bytes after the char. + * This will result in the sizeof() the struct being 12 or 16. +@@ -266,6 +271,7 @@ gvs_fixed_sized_maybe_get_child (GVariantSerialised value, + g_variant_type_info_ref (value.type_info); + value.depth++; + value.ordered_offsets_up_to = 0; ++ value.checked_offsets_up_to = 0; + + return value; + } +@@ -297,7 +303,7 @@ gvs_fixed_sized_maybe_serialise (GVariantSerialised value, + { + if (n_children) + { +- GVariantSerialised child = { NULL, value.data, value.size, value.depth + 1, 0 }; ++ GVariantSerialised child = { NULL, value.data, value.size, value.depth + 1, 0, 0 }; + + gvs_filler (&child, children[0]); + } +@@ -320,6 +326,7 @@ gvs_fixed_sized_maybe_is_normal (GVariantSerialised value) + value.type_info = g_variant_type_info_element (value.type_info); + value.depth++; + value.ordered_offsets_up_to = 0; ++ value.checked_offsets_up_to = 0; + + return g_variant_serialised_is_normal (value); + } +@@ -362,6 +369,7 @@ gvs_variable_sized_maybe_get_child (GVariantSerialised value, + + value.depth++; + value.ordered_offsets_up_to = 0; ++ value.checked_offsets_up_to = 0; + + return value; + } +@@ -392,7 +400,7 @@ gvs_variable_sized_maybe_serialise (GVariantSerialised value, + { + if (n_children) + { +- GVariantSerialised child = { NULL, value.data, value.size - 1, value.depth + 1, 0 }; ++ GVariantSerialised child = { NULL, value.data, value.size - 1, value.depth + 1, 0, 0 }; + + /* write the data for the child. */ + gvs_filler (&child, children[0]); +@@ -413,6 +421,7 @@ gvs_variable_sized_maybe_is_normal (GVariantSerialised value) + value.size--; + value.depth++; + value.ordered_offsets_up_to = 0; ++ value.checked_offsets_up_to = 0; + + return g_variant_serialised_is_normal (value); + } +@@ -739,39 +748,46 @@ gvs_variable_sized_array_get_child (GVariantSerialised value, + + /* 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 array elements). */ +- if (index_ > value.ordered_offsets_up_to) ++ * normal (in order, no overlapping array elements). ++ * ++ * Don’t bother checking if the highest known-good offset is lower than the ++ * highest checked offset, as that means there’s an invalid element at that ++ * index, so there’s no need to check further. */ ++ if (index_ > value.checked_offsets_up_to && ++ value.ordered_offsets_up_to == value.checked_offsets_up_to) + { + switch (offsets.offset_size) + { + case 1: + { + value.ordered_offsets_up_to = find_unordered_guint8 ( +- offsets.array, value.ordered_offsets_up_to, index_ + 1); ++ offsets.array, value.checked_offsets_up_to, index_ + 1); + break; + } + case 2: + { + value.ordered_offsets_up_to = find_unordered_guint16 ( +- offsets.array, value.ordered_offsets_up_to, index_ + 1); ++ offsets.array, value.checked_offsets_up_to, index_ + 1); + break; + } + case 4: + { + value.ordered_offsets_up_to = find_unordered_guint32 ( +- offsets.array, value.ordered_offsets_up_to, index_ + 1); ++ offsets.array, value.checked_offsets_up_to, index_ + 1); + break; + } + case 8: + { + value.ordered_offsets_up_to = find_unordered_guint64 ( +- offsets.array, value.ordered_offsets_up_to, index_ + 1); ++ offsets.array, value.checked_offsets_up_to, index_ + 1); + break; + } + default: + /* gvs_get_offset_size() only returns maximum 8 */ + g_assert_not_reached (); + } ++ ++ value.checked_offsets_up_to = index_; + } + + if (index_ > value.ordered_offsets_up_to) +@@ -916,6 +932,7 @@ gvs_variable_sized_array_is_normal (GVariantSerialised value) + + /* All offsets have now been checked. */ + value.ordered_offsets_up_to = G_MAXSIZE; ++ value.checked_offsets_up_to = G_MAXSIZE; + + return TRUE; + } +@@ -1040,14 +1057,15 @@ gvs_tuple_get_child (GVariantSerialised value, + * 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) ++ if (index_ > value.checked_offsets_up_to && ++ value.ordered_offsets_up_to == value.checked_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); ++ if (value.checked_offsets_up_to > 0) ++ gvs_tuple_get_member_bounds (value, value.checked_offsets_up_to - 1, offset_size, NULL, &prev_i_end); + +- for (i = value.ordered_offsets_up_to; i <= index_; i++) ++ for (i = value.checked_offsets_up_to; i <= index_; i++) + { + gsize i_start, i_end; + +@@ -1060,6 +1078,7 @@ gvs_tuple_get_child (GVariantSerialised value, + } + + value.ordered_offsets_up_to = i - 1; ++ value.checked_offsets_up_to = index_; + } + + if (index_ > value.ordered_offsets_up_to) +@@ -1257,6 +1276,7 @@ gvs_tuple_is_normal (GVariantSerialised value) + + /* All element bounds have been checked above. */ + value.ordered_offsets_up_to = G_MAXSIZE; ++ value.checked_offsets_up_to = G_MAXSIZE; + + { + gsize fixed_size; +diff --git a/glib/gvariant-serialiser.h b/glib/gvariant-serialiser.h +index 765bd43b9..113df2ccc 100644 +--- a/glib/gvariant-serialiser.h ++++ b/glib/gvariant-serialiser.h +@@ -41,6 +41,15 @@ typedef struct + * 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; ++ ++ /* Similar to @ordered_offsets_up_to. This gives the index of the child element ++ * whose frame offset is the highest in the offset table which has been ++ * checked so far. ++ * ++ * This is always ≥ @ordered_offsets_up_to. It is always an element index. ++ * ++ * See documentation in gvariant-core.c for `struct GVariant` for details. */ ++ gsize checked_offsets_up_to; + } GVariantSerialised; + + /* deserialisation */ +diff --git a/glib/gvariant.c b/glib/gvariant.c +index 5f6eb63ee..3e1adffaa 100644 +--- a/glib/gvariant.c ++++ b/glib/gvariant.c +@@ -5954,6 +5954,7 @@ g_variant_byteswap (GVariant *value) + 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 */ ++ serialised.checked_offsets_up_to = G_MAXSIZE; + g_variant_store (trusted, serialised.data); + g_variant_unref (trusted); + +diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c +index a565ea69a..a13655eef 100644 +--- a/glib/tests/gvariant.c ++++ b/glib/tests/gvariant.c +@@ -1284,6 +1284,7 @@ random_instance_filler (GVariantSerialised *serialised, + + serialised->depth = 0; + serialised->ordered_offsets_up_to = 0; ++ serialised->checked_offsets_up_to = 0; + + g_assert_true (serialised->type_info == instance->type_info); + g_assert_cmpuint (serialised->size, ==, instance->size); +@@ -1451,6 +1452,7 @@ test_maybe (void) + serialised.size = needed_size; + serialised.depth = 0; + serialised.ordered_offsets_up_to = 0; ++ serialised.checked_offsets_up_to = 0; + + g_variant_serialiser_serialise (serialised, + random_instance_filler, +@@ -1575,6 +1577,7 @@ test_array (void) + serialised.size = needed_size; + serialised.depth = 0; + serialised.ordered_offsets_up_to = 0; ++ serialised.checked_offsets_up_to = 0; + + g_variant_serialiser_serialise (serialised, random_instance_filler, + (gpointer *) instances, n_children); +@@ -1740,6 +1743,7 @@ test_tuple (void) + serialised.size = needed_size; + serialised.depth = 0; + serialised.ordered_offsets_up_to = 0; ++ serialised.checked_offsets_up_to = 0; + + g_variant_serialiser_serialise (serialised, random_instance_filler, + (gpointer *) instances, n_children); +@@ -1837,6 +1841,7 @@ test_variant (void) + serialised.size = needed_size; + serialised.depth = 0; + serialised.ordered_offsets_up_to = 0; ++ serialised.checked_offsets_up_to = 0; + + g_variant_serialiser_serialise (serialised, random_instance_filler, + (gpointer *) &instance, 1); +-- +2.40.0 + +From 319f859c4ad021f636df67d61fa674afdd10d3c2 Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Fri, 12 Jun 2020 18:01:13 +0100 +Subject: [PATCH 10/19] tests: Add another test for overlapping offsets in + GVariant + +Signed-off-by: Philip Withnall + +Helps: #2121 +--- + glib/tests/gvariant.c | 34 ++++++++++++++++++++++++++++++++++ + 1 file changed, 34 insertions(+) + +diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c +index a13655eef..965aa98af 100644 +--- a/glib/tests/gvariant.c ++++ b/glib/tests/gvariant.c +@@ -5013,6 +5013,38 @@ test_recursion_limits_array_in_variant (void) + g_variant_unref (wrapper_variant); + } + ++/* Test that a nested array with invalid values in its offset table (which point ++ * from the inner to the outer array) is normalised successfully without ++ * looping infinitely. */ ++static void ++test_normal_checking_array_offsets_overlapped (void) ++{ ++ const guint8 data[] = { ++ 0x01, 0x00, ++ }; ++ gsize size = sizeof (data); ++ GVariant *variant = NULL; ++ GVariant *normal_variant = NULL; ++ GVariant *expected_variant = NULL; ++ ++ variant = g_variant_new_from_data (G_VARIANT_TYPE ("aay"), data, size, ++ FALSE, NULL, NULL); ++ g_assert_nonnull (variant); ++ ++ normal_variant = g_variant_get_normal_form (variant); ++ g_assert_nonnull (normal_variant); ++ ++ expected_variant = g_variant_new_parsed ("[@ay [], []]"); ++ g_assert_cmpvariant (normal_variant, expected_variant); ++ ++ g_assert_cmpmem (g_variant_get_data (normal_variant), g_variant_get_size (normal_variant), ++ g_variant_get_data (expected_variant), g_variant_get_size (expected_variant)); ++ ++ g_variant_unref (expected_variant); ++ g_variant_unref (normal_variant); ++ g_variant_unref (variant); ++} ++ + /* Test that an array with invalidly large values in its offset table is + * normalised successfully without looping infinitely. */ + static void +@@ -5413,6 +5445,8 @@ main (int argc, char **argv) + + g_test_add_func ("/gvariant/normal-checking/tuples", + test_normal_checking_tuples); ++ g_test_add_func ("/gvariant/normal-checking/array-offsets/overlapped", ++ test_normal_checking_array_offsets_overlapped); + g_test_add_func ("/gvariant/normal-checking/array-offsets", + test_normal_checking_array_offsets); + g_test_add_func ("/gvariant/normal-checking/array-offsets2", +-- +2.40.0 + +From 24ac75fab0b869fbf7e2cb970974ba59bc1a8a10 Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Mon, 24 Oct 2022 16:43:23 +0100 +Subject: [PATCH 11/19] tests: Disable some random instance tests of GVariants +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +Building a `GVariant` using entirely random data may result in a +non-normally-formed `GVariant`. It’s always possible to read these +`GVariant`s, but the API might return default values for some or all of +their components. + +In particular, this can easily happen when randomly generating the +offset tables for non-fixed-width container types. + +If it does happen, bytewise comparison of the parsed `GVariant` with the +original bytes will not always match. So skip those checks. + +Signed-off-by: Philip Withnall + +Helps: #2121 +--- + glib/tests/gvariant.c | 12 +++++++++--- + 1 file changed, 9 insertions(+), 3 deletions(-) + +diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c +index 965aa98af..f3b975652 100644 +--- a/glib/tests/gvariant.c ++++ b/glib/tests/gvariant.c +@@ -1231,6 +1231,7 @@ random_instance_assert (RandomInstance *instance, + GRand *rand; + gsize i; + ++ g_assert_true (size == 0 || buffer != NULL); + g_assert_cmpint ((gsize) buffer & ALIGN_BITS & instance->alignment, ==, 0); + g_assert_cmpint (size, ==, instance->size); + +@@ -1457,10 +1458,13 @@ test_maybe (void) + g_variant_serialiser_serialise (serialised, + random_instance_filler, + (gpointer *) &instance, 1); ++ + child = g_variant_serialised_get_child (serialised, 0); + g_assert_true (child.type_info == instance->type_info); +- random_instance_assert (instance, child.data, child.size); ++ if (child.data != NULL) /* could be NULL if element is non-normal */ ++ random_instance_assert (instance, child.data, child.size); + g_variant_type_info_unref (child.type_info); ++ + flavoured_free (serialised.data, flavour); + } + } +@@ -1593,7 +1597,8 @@ test_array (void) + + child = g_variant_serialised_get_child (serialised, i); + g_assert_true (child.type_info == instances[i]->type_info); +- random_instance_assert (instances[i], child.data, child.size); ++ if (child.data != NULL) /* could be NULL if element is non-normal */ ++ random_instance_assert (instances[i], child.data, child.size); + g_variant_type_info_unref (child.type_info); + } + +@@ -1759,7 +1764,8 @@ test_tuple (void) + + child = g_variant_serialised_get_child (serialised, i); + g_assert_true (child.type_info == instances[i]->type_info); +- random_instance_assert (instances[i], child.data, child.size); ++ if (child.data != NULL) /* could be NULL if element is non-normal */ ++ random_instance_assert (instances[i], child.data, child.size); + g_variant_type_info_unref (child.type_info); + } + +-- +2.40.0 + +From 88bbf60b61e4aada5b8ebd7deb91fff1f25e60c2 Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Mon, 24 Oct 2022 18:14:57 +0100 +Subject: [PATCH 12/19] gvariant: Clarify the docs for + g_variant_get_normal_form() + +Document how non-normal parts of the `GVariant` are handled. + +Signed-off-by: Philip Withnall +--- + glib/gvariant.c | 4 +++- + 1 file changed, 3 insertions(+), 1 deletion(-) + +diff --git a/glib/gvariant.c b/glib/gvariant.c +index 3e1adffaa..2a6aaa1f4 100644 +--- a/glib/gvariant.c ++++ b/glib/gvariant.c +@@ -5879,7 +5879,9 @@ g_variant_deep_copy (GVariant *value) + * marked as trusted and a new reference to it is returned. + * + * If @value is found not to be in normal form then a new trusted +- * #GVariant is created with the same value as @value. ++ * #GVariant is created with the same value as @value. The non-normal parts of ++ * @value will be replaced with default values which are guaranteed to be in ++ * normal form. + * + * It makes sense to call this function if you've received #GVariant + * data from untrusted sources and you want to ensure your serialised +-- +2.40.0 + +From 2f3b16d3439e4843718428c21aaedd1ecba868d7 Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Mon, 24 Oct 2022 18:43:55 +0100 +Subject: [PATCH 13/19] gvariant: Port g_variant_deep_copy() to count its + iterations directly + +This is equivalent to what `GVariantIter` does, but it means that +`g_variant_deep_copy()` is making its own `g_variant_get_child_value()` +calls. + +This will be useful in an upcoming commit, where those child values will +be inspected a little more deeply. + +Signed-off-by: Philip Withnall + +Helps: #2121 +--- + glib/gvariant.c | 7 +++---- + 1 file changed, 3 insertions(+), 4 deletions(-) + +diff --git a/glib/gvariant.c b/glib/gvariant.c +index 2a6aaa1f4..40d3639bc 100644 +--- a/glib/gvariant.c ++++ b/glib/gvariant.c +@@ -5806,14 +5806,13 @@ g_variant_deep_copy (GVariant *value) + case G_VARIANT_CLASS_VARIANT: + { + GVariantBuilder builder; +- GVariantIter iter; +- GVariant *child; ++ gsize i, n_children; + + g_variant_builder_init (&builder, g_variant_get_type (value)); +- g_variant_iter_init (&iter, value); + +- while ((child = g_variant_iter_next_value (&iter))) ++ for (i = 0, n_children = g_variant_n_children (value); i < n_children; i++) + { ++ GVariant *child = g_variant_get_child_value (value, i); + g_variant_builder_add_value (&builder, g_variant_deep_copy (child)); + g_variant_unref (child); + } +-- +2.40.0 + +From 81567d2bd81f303061b93be34da96ef2e2b11693 Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Tue, 25 Oct 2022 13:03:22 +0100 +Subject: [PATCH 14/19] gvariant: Add internal + g_variant_maybe_get_child_value() + +This will be used in a following commit. + +Signed-off-by: Philip Withnall + +Helps: #2540 +--- + glib/gvariant-core.c | 68 ++++++++++++++++++++++++++++++++++++++++++++ + glib/gvariant-core.h | 3 ++ + 2 files changed, 71 insertions(+) + +diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c +index 1efcf200f..dbcf60f69 100644 +--- a/glib/gvariant-core.c ++++ b/glib/gvariant-core.c +@@ -1204,6 +1204,74 @@ g_variant_get_child_value (GVariant *value, + } + } + ++/** ++ * g_variant_maybe_get_child_value: ++ * @value: a container #GVariant ++ * @index_: the index of the child to fetch ++ * ++ * Reads a child item out of a container #GVariant instance, if it is in normal ++ * form. If it is not in normal form, return %NULL. ++ * ++ * This function behaves the same as g_variant_get_child_value(), except that it ++ * returns %NULL if the child is not in normal form. g_variant_get_child_value() ++ * would instead return a new default value of the correct type. ++ * ++ * This is intended to be used internally to avoid unnecessary #GVariant ++ * allocations. ++ * ++ * The returned value is never floating. You should free it with ++ * g_variant_unref() when you're done with it. ++ * ++ * This function is O(1). ++ * ++ * Returns: (transfer full): the child at the specified index ++ * ++ * Since: 2.74 ++ */ ++GVariant * ++g_variant_maybe_get_child_value (GVariant *value, ++ gsize index_) ++{ ++ g_return_val_if_fail (index_ < g_variant_n_children (value), NULL); ++ g_return_val_if_fail (value->depth < G_MAXSIZE, NULL); ++ ++ if (~g_atomic_int_get (&value->state) & STATE_SERIALISED) ++ { ++ g_variant_lock (value); ++ ++ if (~value->state & STATE_SERIALISED) ++ { ++ GVariant *child; ++ ++ child = g_variant_ref (value->contents.tree.children[index_]); ++ g_variant_unlock (value); ++ ++ return child; ++ } ++ ++ g_variant_unlock (value); ++ } ++ ++ { ++ GVariantSerialised serialised = g_variant_to_serialised (value); ++ GVariantSerialised s_child; ++ ++ /* get the serializer to extract the serialized data for the child ++ * from the serialized data for the container ++ */ ++ s_child = g_variant_serialised_get_child (serialised, index_); ++ ++ if (!(value->state & STATE_TRUSTED) && s_child.data == NULL) ++ { ++ g_variant_type_info_unref (s_child.type_info); ++ return NULL; ++ } ++ ++ g_variant_type_info_unref (s_child.type_info); ++ return g_variant_get_child_value (value, index_); ++ } ++} ++ + /** + * g_variant_store: + * @value: the #GVariant to store +diff --git a/glib/gvariant-core.h b/glib/gvariant-core.h +index fc04711ac..947a98ca0 100644 +--- a/glib/gvariant-core.h ++++ b/glib/gvariant-core.h +@@ -36,4 +36,7 @@ GVariantTypeInfo * g_variant_get_type_info (GVarian + + gsize g_variant_get_depth (GVariant *value); + ++GVariant * g_variant_maybe_get_child_value (GVariant *value, ++ gsize index_); ++ + #endif /* __G_VARIANT_CORE_H__ */ +-- +2.40.0 + +From 2db1a61cabfc4919277f004c411c753bcfa71235 Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Tue, 25 Oct 2022 13:03:45 +0100 +Subject: [PATCH 15/19] gvariant: Cut allocs of default values for children of + non-normal arrays +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +This improves a slow case in `g_variant_get_normal_form()` where +allocating many identical default values for the children of a +variable-sized array which has a malformed offset table would take a lot +of time. + +The fix is to make all child values after the first invalid one be +references to the default value emitted for the first invalid one, +rather than identical new `GVariant`s. + +In particular, this fixes a case where an attacker could create an array +of length L of very large tuples of size T each, corrupt the offset table +so they don’t have to specify the array content, and then induce +`g_variant_get_normal_form()` into allocating L×T default values from an +input which is significantly smaller than L×T in length. + +A pre-existing workaround for this issue is for code to call +`g_variant_is_normal_form()` before calling +`g_variant_get_normal_form()`, and to skip the latter call if the former +returns false. This commit improves the behaviour in the case that +`g_variant_get_normal_form()` is called anyway. + +This fix changes the time to run the `fuzz_variant_binary` test on the +testcase from oss-fuzz#19777 from >60s (before being terminated) with +2.3GB of memory usage and 580k page faults; to 32s, 8.3MB of memory +usage and 1500 page faults (as measured by `time -v`). + +Signed-off-by: Philip Withnall + +Fixes: #2540 +oss-fuzz#19777 +--- + glib/gvariant.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++- + 1 file changed, 65 insertions(+), 1 deletion(-) + +diff --git a/glib/gvariant.c b/glib/gvariant.c +index 40d3639bc..63c134839 100644 +--- a/glib/gvariant.c ++++ b/glib/gvariant.c +@@ -5800,7 +5800,6 @@ g_variant_deep_copy (GVariant *value) + switch (g_variant_classify (value)) + { + case G_VARIANT_CLASS_MAYBE: +- case G_VARIANT_CLASS_ARRAY: + case G_VARIANT_CLASS_TUPLE: + case G_VARIANT_CLASS_DICT_ENTRY: + case G_VARIANT_CLASS_VARIANT: +@@ -5820,6 +5819,71 @@ g_variant_deep_copy (GVariant *value) + return g_variant_builder_end (&builder); + } + ++ case G_VARIANT_CLASS_ARRAY: ++ { ++ GVariantBuilder builder; ++ gsize i, n_children; ++ GVariant *first_invalid_child_deep_copy = NULL; ++ ++ /* Arrays are in theory treated the same as maybes, tuples, dict entries ++ * and variants, and could be another case in the above block of code. ++ * ++ * However, they have the property that when dealing with non-normal ++ * data (which is the only time g_variant_deep_copy() is currently ++ * called) in a variable-sized array, the code above can easily end up ++ * creating many default child values in order to return an array which ++ * is of the right length and type, but without containing non-normal ++ * data. This can happen if the offset table for the array is malformed. ++ * ++ * In this case, the code above would end up allocating the same default ++ * value for each one of the child indexes beyond the first malformed ++ * entry in the offset table. This can end up being a lot of identical ++ * allocations of default values, particularly if the non-normal array ++ * is crafted maliciously. ++ * ++ * Avoid that problem by returning a new reference to the same default ++ * value for every child after the first invalid one. This results in ++ * returning an equivalent array, in normal form and trusted — but with ++ * significantly fewer memory allocations. ++ * ++ * See https://gitlab.gnome.org/GNOME/glib/-/issues/2540 */ ++ ++ g_variant_builder_init (&builder, g_variant_get_type (value)); ++ ++ for (i = 0, n_children = g_variant_n_children (value); i < n_children; i++) ++ { ++ /* Try maybe_get_child_value() first; if it returns NULL, this child ++ * is non-normal. get_child_value() would have constructed and ++ * returned a default value in that case. */ ++ GVariant *child = g_variant_maybe_get_child_value (value, i); ++ ++ if (child != NULL) ++ { ++ /* Non-normal children may not always be contiguous, as they may ++ * be non-normal for reasons other than invalid offset table ++ * entries. As they are all the same type, they will all have ++ * the same default value though, so keep that around. */ ++ g_variant_builder_add_value (&builder, g_variant_deep_copy (child)); ++ } ++ else if (child == NULL && first_invalid_child_deep_copy != NULL) ++ { ++ g_variant_builder_add_value (&builder, first_invalid_child_deep_copy); ++ } ++ else if (child == NULL) ++ { ++ child = g_variant_get_child_value (value, i); ++ first_invalid_child_deep_copy = g_variant_ref_sink (g_variant_deep_copy (child)); ++ g_variant_builder_add_value (&builder, first_invalid_child_deep_copy); ++ } ++ ++ g_clear_pointer (&child, g_variant_unref); ++ } ++ ++ g_clear_pointer (&first_invalid_child_deep_copy, g_variant_unref); ++ ++ return g_variant_builder_end (&builder); ++ } ++ + case G_VARIANT_CLASS_BOOLEAN: + return g_variant_new_boolean (g_variant_get_boolean (value)); + +-- +2.40.0 + +From b4cfb50f42950b0f2f382865f23e9ff02ba05aa4 Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Tue, 25 Oct 2022 18:03:56 +0100 +Subject: [PATCH 16/19] gvariant: Fix a leak of a GVariantTypeInfo on an error + handling path + +Signed-off-by: Philip Withnall +--- + glib/gvariant-core.c | 1 + + 1 file changed, 1 insertion(+) + +diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c +index dbcf60f69..2d2508244 100644 +--- a/glib/gvariant-core.c ++++ b/glib/gvariant-core.c +@@ -1183,6 +1183,7 @@ g_variant_get_child_value (GVariant *value, + G_VARIANT_MAX_RECURSION_DEPTH - value->depth) + { + g_assert (g_variant_is_of_type (value, G_VARIANT_TYPE_VARIANT)); ++ g_variant_type_info_unref (s_child.type_info); + return g_variant_new_tuple (NULL, 0); + } + +-- +2.40.0 + +From 6dd5afa8a0daf574210ea21f1536f69960c0d007 Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Thu, 27 Oct 2022 12:00:04 +0100 +Subject: [PATCH 17/19] gvariant-serialiser: Check offset table entry size is + minimal + +The entries in an offset table (which is used for variable sized arrays +and tuples containing variable sized members) are sized so that they can +address every byte in the overall variant. + +The specification requires that for a variant to be in normal form, its +offset table entries must be the minimum width such that they can +address every byte in the variant. + +That minimality requirement was not checked in +`g_variant_is_normal_form()`, leading to two different byte arrays being +interpreted as the normal form of a given variant tree. That kind of +confusion could potentially be exploited, and is certainly a bug. + +Fix it by adding the necessary checks on offset table entry width, and +unit tests. + +Spotted by William Manley. + +Signed-off-by: Philip Withnall + +Fixes: #2794 +--- + glib/gvariant-serialiser.c | 19 +++- + glib/tests/gvariant.c | 176 +++++++++++++++++++++++++++++++++++++ + 2 files changed, 194 insertions(+), 1 deletion(-) + +diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c +index 8e01ab53e..a6141f7d0 100644 +--- a/glib/gvariant-serialiser.c ++++ b/glib/gvariant-serialiser.c +@@ -694,6 +694,10 @@ gvs_variable_sized_array_get_frame_offsets (GVariantSerialised value) + out.data_size = last_end; + out.array = value.data + last_end; + out.length = offsets_array_size / out.offset_size; ++ ++ if (out.length > 0 && gvs_calculate_total_size (last_end, out.length) != value.size) ++ return out; /* offset size not minimal */ ++ + out.is_normal = TRUE; + + return out; +@@ -1201,6 +1205,7 @@ gvs_tuple_is_normal (GVariantSerialised value) + gsize length; + gsize offset; + gsize i; ++ gsize offset_table_size; + + /* as per the comment in gvs_tuple_get_child() */ + if G_UNLIKELY (value.data == NULL && value.size != 0) +@@ -1305,7 +1310,19 @@ gvs_tuple_is_normal (GVariantSerialised value) + } + } + +- return offset_ptr == offset; ++ /* @offset_ptr has been counting backwards from the end of the variant, to ++ * find the beginning of the offset table. @offset has been counting forwards ++ * from the beginning of the variant to find the end of the data. They should ++ * have met in the middle. */ ++ if (offset_ptr != offset) ++ return FALSE; ++ ++ offset_table_size = value.size - offset_ptr; ++ if (value.size > 0 && ++ gvs_calculate_total_size (offset, offset_table_size / offset_size) != value.size) ++ return FALSE; /* offset size not minimal */ ++ ++ return TRUE; + } + + /* Variants {{{2 +diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c +index f3b975652..028543833 100644 +--- a/glib/tests/gvariant.c ++++ b/glib/tests/gvariant.c +@@ -5116,6 +5116,86 @@ test_normal_checking_array_offsets2 (void) + g_variant_unref (variant); + } + ++/* Test that an otherwise-valid serialised GVariant is considered non-normal if ++ * its offset table entries are too wide. ++ * ++ * See §2.3.6 (Framing Offsets) of the GVariant specification. */ ++static void ++test_normal_checking_array_offsets_minimal_sized (void) ++{ ++ GVariantBuilder builder; ++ gsize i; ++ GVariant *aay_constructed = NULL; ++ const guint8 *data = NULL; ++ guint8 *data_owned = NULL; ++ GVariant *aay_deserialised = NULL; ++ GVariant *aay_normalised = NULL; ++ ++ /* Construct an array of type aay, consisting of 128 elements which are each ++ * an empty array, i.e. `[[] * 128]`. This is chosen because the inner ++ * elements are variable sized (making the outer array variable sized, so it ++ * must have an offset table), but they are also zero-sized when serialised. ++ * So the serialised representation of @aay_constructed consists entirely of ++ * its offset table, which is entirely zeroes. ++ * ++ * The array is chosen to be 128 elements long because that means offset ++ * table entries which are 1 byte long. If the elements in the array were ++ * non-zero-sized (to the extent that the overall array is ≥256 bytes long), ++ * the offset table entries would end up being 2 bytes long. */ ++ g_variant_builder_init (&builder, G_VARIANT_TYPE ("aay")); ++ ++ for (i = 0; i < 128; i++) ++ g_variant_builder_add_value (&builder, g_variant_new_array (G_VARIANT_TYPE_BYTE, NULL, 0)); ++ ++ aay_constructed = g_variant_builder_end (&builder); ++ ++ /* Verify that the constructed array is in normal form, and its serialised ++ * form is `b'\0' * 128`. */ ++ g_assert_true (g_variant_is_normal_form (aay_constructed)); ++ g_assert_cmpuint (g_variant_n_children (aay_constructed), ==, 128); ++ g_assert_cmpuint (g_variant_get_size (aay_constructed), ==, 128); ++ ++ data = g_variant_get_data (aay_constructed); ++ for (i = 0; i < g_variant_get_size (aay_constructed); i++) ++ g_assert_cmpuint (data[i], ==, 0); ++ ++ /* Construct a serialised `aay` GVariant which is `b'\0' * 256`. This has to ++ * be a non-normal form of `[[] * 128]`, with 2-byte-long offset table ++ * entries, because each offset table entry has to be able to reference all of ++ * the byte boundaries in the container. All the entries in the offset table ++ * are zero, so all the elements of the array are zero-sized. */ ++ data = data_owned = g_malloc0 (256); ++ aay_deserialised = g_variant_new_from_data (G_VARIANT_TYPE ("aay"), ++ data, ++ 256, ++ FALSE, ++ g_free, ++ g_steal_pointer (&data_owned)); ++ ++ g_assert_false (g_variant_is_normal_form (aay_deserialised)); ++ g_assert_cmpuint (g_variant_n_children (aay_deserialised), ==, 128); ++ g_assert_cmpuint (g_variant_get_size (aay_deserialised), ==, 256); ++ ++ data = g_variant_get_data (aay_deserialised); ++ for (i = 0; i < g_variant_get_size (aay_deserialised); i++) ++ g_assert_cmpuint (data[i], ==, 0); ++ ++ /* Get its normal form. That should change the serialised size. */ ++ aay_normalised = g_variant_get_normal_form (aay_deserialised); ++ ++ g_assert_true (g_variant_is_normal_form (aay_normalised)); ++ g_assert_cmpuint (g_variant_n_children (aay_normalised), ==, 128); ++ g_assert_cmpuint (g_variant_get_size (aay_normalised), ==, 128); ++ ++ data = g_variant_get_data (aay_normalised); ++ for (i = 0; i < g_variant_get_size (aay_normalised); i++) ++ g_assert_cmpuint (data[i], ==, 0); ++ ++ g_variant_unref (aay_normalised); ++ g_variant_unref (aay_deserialised); ++ g_variant_unref (aay_constructed); ++} ++ + /* Test that a tuple with invalidly large values in its offset table is + * normalised successfully without looping infinitely. */ + static void +@@ -5310,6 +5390,98 @@ test_normal_checking_tuple_offsets4 (void) + g_variant_unref (variant); + } + ++/* Test that an otherwise-valid serialised GVariant is considered non-normal if ++ * its offset table entries are too wide. ++ * ++ * See §2.3.6 (Framing Offsets) of the GVariant specification. */ ++static void ++test_normal_checking_tuple_offsets_minimal_sized (void) ++{ ++ GString *type_string = NULL; ++ GVariantBuilder builder; ++ gsize i; ++ GVariant *ray_constructed = NULL; ++ const guint8 *data = NULL; ++ guint8 *data_owned = NULL; ++ GVariant *ray_deserialised = NULL; ++ GVariant *ray_normalised = NULL; ++ ++ /* Construct a tuple of type (ay…ay), consisting of 129 members which are each ++ * an empty array, i.e. `([] * 129)`. This is chosen because the inner ++ * members are variable sized, so the outer tuple must have an offset table, ++ * but they are also zero-sized when serialised. So the serialised ++ * representation of @ray_constructed consists entirely of its offset table, ++ * which is entirely zeroes. ++ * ++ * The tuple is chosen to be 129 members long because that means it has 128 ++ * offset table entries which are 1 byte long each. If the members in the ++ * tuple were non-zero-sized (to the extent that the overall tuple is ≥256 ++ * bytes long), the offset table entries would end up being 2 bytes long. ++ * ++ * 129 members are used unlike 128 array elements in ++ * test_normal_checking_array_offsets_minimal_sized(), because the last member ++ * in a tuple never needs an offset table entry. */ ++ type_string = g_string_new (""); ++ g_string_append_c (type_string, '('); ++ for (i = 0; i < 129; i++) ++ g_string_append (type_string, "ay"); ++ g_string_append_c (type_string, ')'); ++ ++ g_variant_builder_init (&builder, G_VARIANT_TYPE (type_string->str)); ++ ++ for (i = 0; i < 129; i++) ++ g_variant_builder_add_value (&builder, g_variant_new_array (G_VARIANT_TYPE_BYTE, NULL, 0)); ++ ++ ray_constructed = g_variant_builder_end (&builder); ++ ++ /* Verify that the constructed tuple is in normal form, and its serialised ++ * form is `b'\0' * 128`. */ ++ g_assert_true (g_variant_is_normal_form (ray_constructed)); ++ g_assert_cmpuint (g_variant_n_children (ray_constructed), ==, 129); ++ g_assert_cmpuint (g_variant_get_size (ray_constructed), ==, 128); ++ ++ data = g_variant_get_data (ray_constructed); ++ for (i = 0; i < g_variant_get_size (ray_constructed); i++) ++ g_assert_cmpuint (data[i], ==, 0); ++ ++ /* Construct a serialised `(ay…ay)` GVariant which is `b'\0' * 256`. This has ++ * to be a non-normal form of `([] * 129)`, with 2-byte-long offset table ++ * entries, because each offset table entry has to be able to reference all of ++ * the byte boundaries in the container. All the entries in the offset table ++ * are zero, so all the members of the tuple are zero-sized. */ ++ data = data_owned = g_malloc0 (256); ++ ray_deserialised = g_variant_new_from_data (G_VARIANT_TYPE (type_string->str), ++ data, ++ 256, ++ FALSE, ++ g_free, ++ g_steal_pointer (&data_owned)); ++ ++ g_assert_false (g_variant_is_normal_form (ray_deserialised)); ++ g_assert_cmpuint (g_variant_n_children (ray_deserialised), ==, 129); ++ g_assert_cmpuint (g_variant_get_size (ray_deserialised), ==, 256); ++ ++ data = g_variant_get_data (ray_deserialised); ++ for (i = 0; i < g_variant_get_size (ray_deserialised); i++) ++ g_assert_cmpuint (data[i], ==, 0); ++ ++ /* Get its normal form. That should change the serialised size. */ ++ ray_normalised = g_variant_get_normal_form (ray_deserialised); ++ ++ g_assert_true (g_variant_is_normal_form (ray_normalised)); ++ g_assert_cmpuint (g_variant_n_children (ray_normalised), ==, 129); ++ g_assert_cmpuint (g_variant_get_size (ray_normalised), ==, 128); ++ ++ data = g_variant_get_data (ray_normalised); ++ for (i = 0; i < g_variant_get_size (ray_normalised); i++) ++ g_assert_cmpuint (data[i], ==, 0); ++ ++ g_variant_unref (ray_normalised); ++ g_variant_unref (ray_deserialised); ++ g_variant_unref (ray_constructed); ++ g_string_free (type_string, TRUE); ++} ++ + /* Test that an empty object path is normalised successfully to the base object + * path, ‘/’. */ + static void +@@ -5457,6 +5629,8 @@ main (int argc, char **argv) + test_normal_checking_array_offsets); + g_test_add_func ("/gvariant/normal-checking/array-offsets2", + test_normal_checking_array_offsets2); ++ g_test_add_func ("/gvariant/normal-checking/array-offsets/minimal-sized", ++ test_normal_checking_array_offsets_minimal_sized); + g_test_add_func ("/gvariant/normal-checking/tuple-offsets", + test_normal_checking_tuple_offsets); + g_test_add_func ("/gvariant/normal-checking/tuple-offsets2", +@@ -5465,6 +5639,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-offsets/minimal-sized", ++ test_normal_checking_tuple_offsets_minimal_sized); + g_test_add_func ("/gvariant/normal-checking/empty-object-path", + test_normal_checking_empty_object_path); + +-- +2.40.0 + +From 5ebfcae9b5e409cc2ad0c1e73175af2bdab455ac Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Thu, 27 Oct 2022 16:13:54 +0100 +Subject: [PATCH 18/19] gvariant: Fix g_variant_byteswap() returning non-normal + data sometimes +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +If `g_variant_byteswap()` was called on a non-normal variant of a type +which doesn’t need byteswapping, it would return a non-normal output. + +That contradicts the documentation, which says that the return value is +always in normal form. + +Fix the code so it matches the documentation. + +Includes a unit test. + +Signed-off-by: Philip Withnall + +Helps: #2797 +--- + glib/gvariant.c | 8 +++++--- + glib/tests/gvariant.c | 24 ++++++++++++++++++++++++ + 2 files changed, 29 insertions(+), 3 deletions(-) + +diff --git a/glib/gvariant.c b/glib/gvariant.c +index 63c134839..9388c529d 100644 +--- a/glib/gvariant.c ++++ b/glib/gvariant.c +@@ -6026,14 +6026,16 @@ g_variant_byteswap (GVariant *value) + g_variant_serialised_byteswap (serialised); + + bytes = g_bytes_new_take (serialised.data, serialised.size); +- new = g_variant_new_from_bytes (g_variant_get_type (value), bytes, TRUE); ++ new = g_variant_ref_sink (g_variant_new_from_bytes (g_variant_get_type (value), bytes, TRUE)); + g_bytes_unref (bytes); + } + else + /* contains no multi-byte data */ +- new = value; ++ new = g_variant_get_normal_form (value); + +- return g_variant_ref_sink (new); ++ g_assert (g_variant_is_trusted (new)); ++ ++ return g_steal_pointer (&new); + } + + /** +diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c +index 028543833..871a6dab0 100644 +--- a/glib/tests/gvariant.c ++++ b/glib/tests/gvariant.c +@@ -3826,6 +3826,29 @@ test_gv_byteswap (void) + g_free (string); + } + ++static void ++test_gv_byteswap_non_normal_non_aligned (void) ++{ ++ const guint8 data[] = { 0x02 }; ++ GVariant *v = NULL; ++ GVariant *v_byteswapped = NULL; ++ ++ g_test_summary ("Test that calling g_variant_byteswap() on a variant which " ++ "is in non-normal form and doesn’t need byteswapping returns " ++ "the same variant in normal form."); ++ ++ v = g_variant_new_from_data (G_VARIANT_TYPE_BOOLEAN, data, sizeof (data), FALSE, NULL, NULL); ++ g_assert_false (g_variant_is_normal_form (v)); ++ ++ v_byteswapped = g_variant_byteswap (v); ++ g_assert_true (g_variant_is_normal_form (v_byteswapped)); ++ ++ g_assert_cmpvariant (v, v_byteswapped); ++ ++ g_variant_unref (v); ++ g_variant_unref (v_byteswapped); ++} ++ + static void + test_parser (void) + { +@@ -5594,6 +5617,7 @@ main (int argc, char **argv) + g_test_add_func ("/gvariant/builder-memory", test_builder_memory); + g_test_add_func ("/gvariant/hashing", test_hashing); + g_test_add_func ("/gvariant/byteswap", test_gv_byteswap); ++ g_test_add_func ("/gvariant/byteswap/non-normal-non-aligned", test_gv_byteswap_non_normal_non_aligned); + g_test_add_func ("/gvariant/parser", test_parses); + g_test_add_func ("/gvariant/parser/integer-bounds", test_parser_integer_bounds); + g_test_add_func ("/gvariant/parser/recursion", test_parser_recursion); +-- +2.40.0 + +From c6d0ae6c04a4257e9e06edd186d19e8ac4f37024 Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Thu, 27 Oct 2022 22:53:13 +0100 +Subject: [PATCH 19/19] gvariant: Allow g_variant_byteswap() to operate on + tree-form variants +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +This avoids needing to always serialise a variant before byteswapping it. +With variants in non-normal forms, serialisation can result in a large +increase in size of the variant, and a lot of allocations for leaf +`GVariant`s. This can lead to a denial of service attack. + +Avoid that by changing byteswapping so that it happens on the tree form +of the variant if the input is in non-normal form. If the input is in +normal form (either serialised or in tree form), continue using the +existing code as byteswapping an already-serialised normal variant is +about 3× faster than byteswapping on the equivalent tree form. + +The existing unit tests cover byteswapping well, but need some +adaptation so that they operate on tree form variants too. + +I considered dropping the serialised byteswapping code and doing all +byteswapping on tree-form variants, as that would make maintenance +simpler (avoiding having two parallel implementations of byteswapping). +However, most inputs to `g_variant_byteswap()` are likely to be +serialised variants (coming from a byte array of input from some foreign +source) and most of them are going to be in normal form (as corruption +and malicious action are rare). So getting rid of the serialised +byteswapping code would impose quite a performance penalty on the common +case. + +Signed-off-by: Philip Withnall + +Fixes: #2797 +--- + glib/gvariant.c | 87 ++++++++++++++++++++++++++++++++----------- + glib/tests/gvariant.c | 57 ++++++++++++++++++++++++---- + 2 files changed, 115 insertions(+), 29 deletions(-) + +diff --git a/glib/gvariant.c b/glib/gvariant.c +index 9388c529d..1f0eccb73 100644 +--- a/glib/gvariant.c ++++ b/glib/gvariant.c +@@ -5795,7 +5795,8 @@ g_variant_iter_loop (GVariantIter *iter, + + /* Serialised data {{{1 */ + static GVariant * +-g_variant_deep_copy (GVariant *value) ++g_variant_deep_copy (GVariant *value, ++ gboolean byteswap) + { + switch (g_variant_classify (value)) + { +@@ -5812,7 +5813,7 @@ g_variant_deep_copy (GVariant *value) + for (i = 0, n_children = g_variant_n_children (value); i < n_children; i++) + { + GVariant *child = g_variant_get_child_value (value, i); +- g_variant_builder_add_value (&builder, g_variant_deep_copy (child)); ++ g_variant_builder_add_value (&builder, g_variant_deep_copy (child, byteswap)); + g_variant_unref (child); + } + +@@ -5863,7 +5864,7 @@ g_variant_deep_copy (GVariant *value) + * be non-normal for reasons other than invalid offset table + * entries. As they are all the same type, they will all have + * the same default value though, so keep that around. */ +- g_variant_builder_add_value (&builder, g_variant_deep_copy (child)); ++ g_variant_builder_add_value (&builder, g_variant_deep_copy (child, byteswap)); + } + else if (child == NULL && first_invalid_child_deep_copy != NULL) + { +@@ -5872,7 +5873,7 @@ g_variant_deep_copy (GVariant *value) + else if (child == NULL) + { + child = g_variant_get_child_value (value, i); +- first_invalid_child_deep_copy = g_variant_ref_sink (g_variant_deep_copy (child)); ++ first_invalid_child_deep_copy = g_variant_ref_sink (g_variant_deep_copy (child, byteswap)); + g_variant_builder_add_value (&builder, first_invalid_child_deep_copy); + } + +@@ -5891,28 +5892,63 @@ g_variant_deep_copy (GVariant *value) + return g_variant_new_byte (g_variant_get_byte (value)); + + case G_VARIANT_CLASS_INT16: +- return g_variant_new_int16 (g_variant_get_int16 (value)); ++ if (byteswap) ++ return g_variant_new_int16 (GUINT16_SWAP_LE_BE (g_variant_get_int16 (value))); ++ else ++ return g_variant_new_int16 (g_variant_get_int16 (value)); + + case G_VARIANT_CLASS_UINT16: +- return g_variant_new_uint16 (g_variant_get_uint16 (value)); ++ if (byteswap) ++ return g_variant_new_uint16 (GUINT16_SWAP_LE_BE (g_variant_get_uint16 (value))); ++ else ++ return g_variant_new_uint16 (g_variant_get_uint16 (value)); + + case G_VARIANT_CLASS_INT32: +- return g_variant_new_int32 (g_variant_get_int32 (value)); ++ if (byteswap) ++ return g_variant_new_int32 (GUINT32_SWAP_LE_BE (g_variant_get_int32 (value))); ++ else ++ return g_variant_new_int32 (g_variant_get_int32 (value)); + + case G_VARIANT_CLASS_UINT32: +- return g_variant_new_uint32 (g_variant_get_uint32 (value)); ++ if (byteswap) ++ return g_variant_new_uint32 (GUINT32_SWAP_LE_BE (g_variant_get_uint32 (value))); ++ else ++ return g_variant_new_uint32 (g_variant_get_uint32 (value)); + + case G_VARIANT_CLASS_INT64: +- return g_variant_new_int64 (g_variant_get_int64 (value)); ++ if (byteswap) ++ return g_variant_new_int64 (GUINT64_SWAP_LE_BE (g_variant_get_int64 (value))); ++ else ++ return g_variant_new_int64 (g_variant_get_int64 (value)); + + case G_VARIANT_CLASS_UINT64: +- return g_variant_new_uint64 (g_variant_get_uint64 (value)); ++ if (byteswap) ++ return g_variant_new_uint64 (GUINT64_SWAP_LE_BE (g_variant_get_uint64 (value))); ++ else ++ return g_variant_new_uint64 (g_variant_get_uint64 (value)); + + case G_VARIANT_CLASS_HANDLE: +- return g_variant_new_handle (g_variant_get_handle (value)); ++ if (byteswap) ++ return g_variant_new_handle (GUINT32_SWAP_LE_BE (g_variant_get_handle (value))); ++ else ++ return g_variant_new_handle (g_variant_get_handle (value)); + + case G_VARIANT_CLASS_DOUBLE: +- return g_variant_new_double (g_variant_get_double (value)); ++ if (byteswap) ++ { ++ /* We have to convert the double to a uint64 here using a union, ++ * because a cast will round it numerically. */ ++ union ++ { ++ guint64 u64; ++ gdouble dbl; ++ } u1, u2; ++ u1.dbl = g_variant_get_double (value); ++ u2.u64 = GUINT64_SWAP_LE_BE (u1.u64); ++ return g_variant_new_double (u2.dbl); ++ } ++ else ++ return g_variant_new_double (g_variant_get_double (value)); + + case G_VARIANT_CLASS_STRING: + return g_variant_new_string (g_variant_get_string (value, NULL)); +@@ -5969,7 +6005,7 @@ g_variant_get_normal_form (GVariant *value) + if (g_variant_is_normal_form (value)) + return g_variant_ref (value); + +- trusted = g_variant_deep_copy (value); ++ trusted = g_variant_deep_copy (value, FALSE); + g_assert (g_variant_is_trusted (trusted)); + + return g_variant_ref_sink (trusted); +@@ -5989,6 +6025,11 @@ g_variant_get_normal_form (GVariant *value) + * contain multi-byte numeric data. That include strings, booleans, + * bytes and containers containing only these things (recursively). + * ++ * While this function can safely handle untrusted, non-normal data, it is ++ * recommended to check whether the input is in normal form beforehand, using ++ * g_variant_is_normal_form(), and to reject non-normal inputs if your ++ * application can be strict about what inputs it rejects. ++ * + * The returned value is always in normal form and is marked as trusted. + * + * Returns: (transfer full): the byteswapped form of @value +@@ -6006,22 +6047,21 @@ g_variant_byteswap (GVariant *value) + + g_variant_type_info_query (type_info, &alignment, NULL); + +- if (alignment) +- /* (potentially) contains multi-byte numeric data */ ++ if (alignment && g_variant_is_normal_form (value)) + { ++ /* (potentially) contains multi-byte numeric data, but is also already in ++ * normal form so we can use a faster byteswapping codepath on the ++ * serialised data */ + GVariantSerialised serialised = { 0, }; +- GVariant *trusted; + GBytes *bytes; + +- trusted = g_variant_get_normal_form (value); +- serialised.type_info = g_variant_get_type_info (trusted); +- serialised.size = g_variant_get_size (trusted); ++ serialised.type_info = g_variant_get_type_info (value); ++ serialised.size = g_variant_get_size (value); + serialised.data = g_malloc (serialised.size); +- serialised.depth = g_variant_get_depth (trusted); ++ serialised.depth = g_variant_get_depth (value); + serialised.ordered_offsets_up_to = G_MAXSIZE; /* operating on the normal form */ + serialised.checked_offsets_up_to = G_MAXSIZE; +- g_variant_store (trusted, serialised.data); +- g_variant_unref (trusted); ++ g_variant_store (value, serialised.data); + + g_variant_serialised_byteswap (serialised); + +@@ -6029,6 +6069,9 @@ g_variant_byteswap (GVariant *value) + new = g_variant_ref_sink (g_variant_new_from_bytes (g_variant_get_type (value), bytes, TRUE)); + g_bytes_unref (bytes); + } ++ else if (alignment) ++ /* (potentially) contains multi-byte numeric data */ ++ new = g_variant_ref_sink (g_variant_deep_copy (value, TRUE)); + else + /* contains no multi-byte data */ + new = g_variant_get_normal_form (value); +diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c +index 871a6dab0..143057de7 100644 +--- a/glib/tests/gvariant.c ++++ b/glib/tests/gvariant.c +@@ -2288,24 +2288,67 @@ serialise_tree (TreeInstance *tree, + static void + test_byteswap (void) + { +- GVariantSerialised one = { 0, }, two = { 0, }; ++ GVariantSerialised one = { 0, }, two = { 0, }, three = { 0, }; + TreeInstance *tree; +- ++ GVariant *one_variant = NULL; ++ GVariant *two_variant = NULL; ++ GVariant *two_byteswapped = NULL; ++ GVariant *three_variant = NULL; ++ GVariant *three_byteswapped = NULL; ++ guint8 *three_data_copy = NULL; ++ gsize three_size_copy = 0; ++ ++ /* Write a tree out twice, once normally and once byteswapped. */ + tree = tree_instance_new (NULL, 3); + serialise_tree (tree, &one); + ++ one_variant = g_variant_new_from_data (G_VARIANT_TYPE (g_variant_type_info_get_type_string (one.type_info)), ++ one.data, one.size, FALSE, NULL, NULL); ++ + i_am_writing_byteswapped = TRUE; + serialise_tree (tree, &two); ++ serialise_tree (tree, &three); + i_am_writing_byteswapped = FALSE; + +- g_variant_serialised_byteswap (two); +- +- g_assert_cmpmem (one.data, one.size, two.data, two.size); +- g_assert_cmpuint (one.depth, ==, two.depth); +- ++ /* Swap the first byteswapped one back using the function we want to test. */ ++ two_variant = g_variant_new_from_data (G_VARIANT_TYPE (g_variant_type_info_get_type_string (two.type_info)), ++ two.data, two.size, FALSE, NULL, NULL); ++ two_byteswapped = g_variant_byteswap (two_variant); ++ ++ /* Make the second byteswapped one non-normal (hopefully), and then byteswap ++ * it back using the function we want to test in its non-normal mode. ++ * This might not work because it’s not necessarily possible to make an ++ * arbitrary random variant non-normal. Adding a single zero byte to the end ++ * often makes something non-normal but still readable. */ ++ three_size_copy = three.size + 1; ++ three_data_copy = g_malloc (three_size_copy); ++ memcpy (three_data_copy, three.data, three.size); ++ three_data_copy[three.size] = '\0'; ++ ++ three_variant = g_variant_new_from_data (G_VARIANT_TYPE (g_variant_type_info_get_type_string (three.type_info)), ++ three_data_copy, three_size_copy, FALSE, NULL, NULL); ++ three_byteswapped = g_variant_byteswap (three_variant); ++ ++ /* Check they’re the same. We can always compare @one_variant and ++ * @two_byteswapped. We can only compare @two_byteswapped and ++ * @three_byteswapped if @two_variant and @three_variant are equal: in that ++ * case, the corruption to @three_variant was enough to make it non-normal but ++ * not enough to change its value. */ ++ g_assert_cmpvariant (one_variant, two_byteswapped); ++ ++ if (g_variant_equal (two_variant, three_variant)) ++ g_assert_cmpvariant (two_byteswapped, three_byteswapped); ++ ++ g_variant_unref (three_byteswapped); ++ g_variant_unref (three_variant); ++ g_variant_unref (two_byteswapped); ++ g_variant_unref (two_variant); ++ g_variant_unref (one_variant); + tree_instance_free (tree); + g_free (one.data); + g_free (two.data); ++ g_free (three.data); ++ g_free (three_data_copy); + } + + static void +-- +2.40.0 diff --git a/SOURCES/3136.patch b/SOURCES/3136.patch new file mode 100644 index 0000000..57e1b95 --- /dev/null +++ b/SOURCES/3136.patch @@ -0,0 +1,65 @@ +From ba2137b0d9ea3744155be81a5ba770c6535b46f3 Mon Sep 17 00:00:00 2001 +From: Simon McVittie +Date: Thu, 15 Dec 2022 12:51:37 +0000 +Subject: [PATCH] gvariant-serialiser: Convert endianness of offsets +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +The array of offsets is little-endian, even on big-endian architectures +like s390x. + +Fixes: ade71fb5 "gvariant: Don’t allow child elements to overlap with each other" +Resolves: https://gitlab.gnome.org/GNOME/glib/-/issues/2839 +Signed-off-by: Simon McVittie +--- + glib/gvariant-serialiser.c | 19 +++++++++++-------- + 1 file changed, 11 insertions(+), 8 deletions(-) + +diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c +index fadefab659..f443c2eb85 100644 +--- a/glib/gvariant-serialiser.c ++++ b/glib/gvariant-serialiser.c +@@ -714,17 +714,19 @@ gvs_variable_sized_array_n_children (GVariantSerialised value) + /* Find the index of the first out-of-order element in @data, assuming that + * @data is an array of elements of given @type, starting at index @start and + * containing a further @len-@start elements. */ +-#define DEFINE_FIND_UNORDERED(type) \ ++#define DEFINE_FIND_UNORDERED(type, le_to_native) \ + static gsize \ + find_unordered_##type (const guint8 *data, gsize start, gsize len) \ + { \ + gsize off; \ +- type current, previous; \ ++ type current_le, previous_le, current, previous; \ + \ +- memcpy (&previous, data + start * sizeof (current), sizeof (current)); \ ++ memcpy (&previous_le, data + start * sizeof (current), sizeof (current)); \ ++ previous = le_to_native (previous_le); \ + for (off = (start + 1) * sizeof (current); off < len * sizeof (current); off += sizeof (current)) \ + { \ +- memcpy (¤t, data + off, sizeof (current)); \ ++ memcpy (¤t_le, data + off, sizeof (current)); \ ++ current = le_to_native (current_le); \ + if (current < previous) \ + break; \ + previous = current; \ +@@ -732,10 +734,11 @@ gvs_variable_sized_array_n_children (GVariantSerialised value) + return off / sizeof (current) - 1; \ + } + +-DEFINE_FIND_UNORDERED (guint8); +-DEFINE_FIND_UNORDERED (guint16); +-DEFINE_FIND_UNORDERED (guint32); +-DEFINE_FIND_UNORDERED (guint64); ++#define NO_CONVERSION(x) (x) ++DEFINE_FIND_UNORDERED (guint8, NO_CONVERSION); ++DEFINE_FIND_UNORDERED (guint16, GUINT16_FROM_LE); ++DEFINE_FIND_UNORDERED (guint32, GUINT32_FROM_LE); ++DEFINE_FIND_UNORDERED (guint64, GUINT64_FROM_LE); + + static GVariantSerialised + gvs_variable_sized_array_get_child (GVariantSerialised value, +-- +GitLab + diff --git a/SOURCES/3163.patch b/SOURCES/3163.patch new file mode 100644 index 0000000..13fe47c --- /dev/null +++ b/SOURCES/3163.patch @@ -0,0 +1,199 @@ +From 78da5faccb3e065116b75b3ff87ff55381da6c76 Mon Sep 17 00:00:00 2001 +From: Philip Withnall +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 + +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 +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 + +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 + diff --git a/SOURCES/3272.patch b/SOURCES/3272.patch new file mode 100644 index 0000000..c97e428 --- /dev/null +++ b/SOURCES/3272.patch @@ -0,0 +1,141 @@ +From 059f4f3999f1de506417611318c6f27db57fb689 Mon Sep 17 00:00:00 2001 +From: Marius Vollmer +Date: Mon, 13 Feb 2023 14:12:52 +0200 +Subject: [PATCH] gdbus: Never buffer reads during server authentication + +Otherwise, the content of the buffer is thrown away when switching +from reading via a GDataInputStream to unbuffered reads when waiting +for the "BEGIN" line. + +(The code already tried to protect against over-reading like this by +using unbuffered reads for the last few lines of the auth protocol, +but it might already be too late at that point. The buffer of the +GDataInputStream might already contain the "BEGIN" line for example.) + +This matters when connecting a sd-bus client directly to a GDBus +client. A sd-bus client optimistically sends the whole auth +conversation in one go without waiting for intermediate replies. This +is done to improve performance for the many short-lived connections +that are typically made. +--- + gio/gdbusauth.c | 50 ++++++++++++++++++++++++++++++------------------- + 1 file changed, 31 insertions(+), 19 deletions(-) + +diff --git a/gio/gdbusauth.c b/gio/gdbusauth.c +index c430f0cf0..17c7d47b7 100644 +--- a/gio/gdbusauth.c ++++ b/gio/gdbusauth.c +@@ -933,7 +933,6 @@ _g_dbus_auth_run_server (GDBusAuth *auth, + { + gboolean ret; + ServerState state; +- GDataInputStream *dis; + GDataOutputStream *dos; + GError *local_error; + gchar *line; +@@ -949,7 +948,6 @@ _g_dbus_auth_run_server (GDBusAuth *auth, + _g_dbus_auth_add_mechs (auth, observer); + + ret = FALSE; +- dis = NULL; + dos = NULL; + mech = NULL; + negotiated_capabilities = 0; +@@ -965,13 +963,18 @@ _g_dbus_auth_run_server (GDBusAuth *auth, + goto out; + } + +- dis = G_DATA_INPUT_STREAM (g_data_input_stream_new (g_io_stream_get_input_stream (auth->priv->stream))); ++ /* We use an extremely slow (but reliable) line reader for input ++ * instead of something buffered - this basically does a recvfrom() ++ * system call per character ++ * ++ * (the problem with using GDataInputStream's read_line is that ++ * because of buffering it might start reading into the first D-Bus ++ * message that appears after "BEGIN\r\n"....) ++ */ ++ + dos = G_DATA_OUTPUT_STREAM (g_data_output_stream_new (g_io_stream_get_output_stream (auth->priv->stream))); +- g_filter_input_stream_set_close_base_stream (G_FILTER_INPUT_STREAM (dis), FALSE); + g_filter_output_stream_set_close_base_stream (G_FILTER_OUTPUT_STREAM (dos), FALSE); + +- g_data_input_stream_set_newline_type (dis, G_DATA_STREAM_NEWLINE_TYPE_CR_LF); +- + /* read the NUL-byte, possibly with credentials attached */ + #ifdef G_OS_UNIX + #ifndef G_CREDENTIALS_PREFER_MESSAGE_PASSING +@@ -1010,11 +1013,22 @@ _g_dbus_auth_run_server (GDBusAuth *auth, + } + else + { ++ gchar c; ++ gssize num_read; ++ + local_error = NULL; +- (void)g_data_input_stream_read_byte (dis, cancellable, &local_error); +- if (local_error != NULL) ++ num_read = g_input_stream_read (g_io_stream_get_input_stream (auth->priv->stream), ++ &c, 1, ++ cancellable, &local_error); ++ if (num_read != 1 || local_error != NULL) + { +- g_propagate_error (error, local_error); ++ if (local_error == NULL) ++ g_set_error_literal (error, ++ G_IO_ERROR, ++ G_IO_ERROR_FAILED, ++ _ ("Unexpected lack of content trying to read a byte")); ++ else ++ g_propagate_error (error, local_error); + goto out; + } + } +@@ -1050,7 +1064,10 @@ _g_dbus_auth_run_server (GDBusAuth *auth, + { + case SERVER_STATE_WAITING_FOR_AUTH: + debug_print ("SERVER: WaitingForAuth"); +- line = _my_g_data_input_stream_read_line (dis, &line_length, cancellable, error); ++ line = _my_g_input_stream_read_line_safe (g_io_stream_get_input_stream (auth->priv->stream), ++ &line_length, ++ cancellable, ++ error); + debug_print ("SERVER: WaitingForAuth, read '%s'", line); + if (line == NULL) + goto out; +@@ -1260,7 +1277,10 @@ _g_dbus_auth_run_server (GDBusAuth *auth, + + case SERVER_STATE_WAITING_FOR_DATA: + debug_print ("SERVER: WaitingForData"); +- line = _my_g_data_input_stream_read_line (dis, &line_length, cancellable, error); ++ line = _my_g_input_stream_read_line_safe (g_io_stream_get_input_stream (auth->priv->stream), ++ &line_length, ++ cancellable, ++ error); + debug_print ("SERVER: WaitingForData, read '%s'", line); + if (line == NULL) + goto out; +@@ -1299,13 +1319,6 @@ _g_dbus_auth_run_server (GDBusAuth *auth, + + case SERVER_STATE_WAITING_FOR_BEGIN: + debug_print ("SERVER: WaitingForBegin"); +- /* Use extremely slow (but reliable) line reader - this basically +- * does a recvfrom() system call per character +- * +- * (the problem with using GDataInputStream's read_line is that because of +- * buffering it might start reading into the first D-Bus message that +- * appears after "BEGIN\r\n"....) +- */ + line = _my_g_input_stream_read_line_safe (g_io_stream_get_input_stream (auth->priv->stream), + &line_length, + cancellable, +@@ -1364,7 +1377,6 @@ _g_dbus_auth_run_server (GDBusAuth *auth, + + out: + g_clear_object (&mech); +- g_clear_object (&dis); + g_clear_object (&dos); + g_clear_object (&own_credentials); + +-- +2.41.0 +