Toshaan Bharvani
12 months ago
5 changed files with 3704 additions and 0 deletions
@ -0,0 +1,278 @@
@@ -0,0 +1,278 @@
|
||||
From 764f071909df70622e79ee71323973c18c055c8c Mon Sep 17 00:00:00 2001 |
||||
From: Giuseppe Scrivano <giuseppe@scrivano.org> |
||||
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 <giuseppe@scrivano.org> |
||||
Co-authored-by: Simon McVittie <smcv@collabora.com> |
||||
Signed-off-by: Simon McVittie <smcv@collabora.com> |
||||
--- |
||||
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 <giuseppe@scrivano.org> |
||||
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 <smcv@collabora.com> |
||||
--- |
||||
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 <giuseppe@scrivano.org> |
||||
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 <smcv@collabora.com> |
||||
--- |
||||
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 <smcv@collabora.com> |
||||
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 <smcv@collabora.com> |
||||
--- |
||||
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 |
@ -0,0 +1,65 @@
@@ -0,0 +1,65 @@
|
||||
From ba2137b0d9ea3744155be81a5ba770c6535b46f3 Mon Sep 17 00:00:00 2001 |
||||
From: Simon McVittie <smcv@collabora.com> |
||||
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 <smcv@collabora.com> |
||||
--- |
||||
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 |
||||
|
@ -0,0 +1,199 @@
@@ -0,0 +1,199 @@
|
||||
From 78da5faccb3e065116b75b3ff87ff55381da6c76 Mon Sep 17 00:00:00 2001 |
||||
From: Philip Withnall <pwithnall@endlessos.org> |
||||
Date: Thu, 15 Dec 2022 13:00:39 +0000 |
||||
Subject: [PATCH 1/2] =?UTF-8?q?gvariant:=20Check=20offset=20table=20doesn?= |
||||
=?UTF-8?q?=E2=80=99t=20fall=20outside=20variant=20bounds?= |
||||
MIME-Version: 1.0 |
||||
Content-Type: text/plain; charset=UTF-8 |
||||
Content-Transfer-Encoding: 8bit |
||||
|
||||
When dereferencing the first entry in the offset table for a tuple, |
||||
check that it doesn’t fall outside the bounds of the variant first. |
||||
|
||||
This prevents an out-of-bounds read from some non-normal tuples. |
||||
|
||||
This bug was introduced in commit 73d0aa81c2575a5c9ae77d. |
||||
|
||||
Includes a unit test, although the test will likely only catch the |
||||
original bug if run with asan enabled. |
||||
|
||||
Signed-off-by: Philip Withnall <pwithnall@endlessos.org> |
||||
|
||||
Fixes: #2840 |
||||
oss-fuzz#54302 |
||||
--- |
||||
glib/gvariant-serialiser.c | 12 ++++++-- |
||||
glib/tests/gvariant.c | 63 ++++++++++++++++++++++++++++++++++++++ |
||||
2 files changed, 72 insertions(+), 3 deletions(-) |
||||
|
||||
diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c |
||||
index f443c2eb85..4e4a73ad17 100644 |
||||
--- a/glib/gvariant-serialiser.c |
||||
+++ b/glib/gvariant-serialiser.c |
||||
@@ -984,7 +984,8 @@ gvs_tuple_get_member_bounds (GVariantSerialised value, |
||||
|
||||
member_info = g_variant_type_info_member_info (value.type_info, index_); |
||||
|
||||
- if (member_info->i + 1) |
||||
+ if (member_info->i + 1 && |
||||
+ offset_size * (member_info->i + 1) <= value.size) |
||||
member_start = gvs_read_unaligned_le (value.data + value.size - |
||||
offset_size * (member_info->i + 1), |
||||
offset_size); |
||||
@@ -995,7 +996,8 @@ gvs_tuple_get_member_bounds (GVariantSerialised value, |
||||
member_start &= member_info->b; |
||||
member_start |= member_info->c; |
||||
|
||||
- if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_LAST) |
||||
+ if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_LAST && |
||||
+ offset_size * (member_info->i + 1) <= value.size) |
||||
member_end = value.size - offset_size * (member_info->i + 1); |
||||
|
||||
else if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_FIXED) |
||||
@@ -1006,11 +1008,15 @@ gvs_tuple_get_member_bounds (GVariantSerialised value, |
||||
member_end = member_start + fixed_size; |
||||
} |
||||
|
||||
- else /* G_VARIANT_MEMBER_ENDING_OFFSET */ |
||||
+ else if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_OFFSET && |
||||
+ offset_size * (member_info->i + 2) <= value.size) |
||||
member_end = gvs_read_unaligned_le (value.data + value.size - |
||||
offset_size * (member_info->i + 2), |
||||
offset_size); |
||||
|
||||
+ else /* invalid */ |
||||
+ member_end = G_MAXSIZE; |
||||
+ |
||||
if (out_member_start != NULL) |
||||
*out_member_start = member_start; |
||||
if (out_member_end != NULL) |
||||
diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c |
||||
index b360888e4d..98c51a1d75 100644 |
||||
--- a/glib/tests/gvariant.c |
||||
+++ b/glib/tests/gvariant.c |
||||
@@ -5576,6 +5576,67 @@ test_normal_checking_tuple_offsets4 (void) |
||||
g_variant_unref (variant); |
||||
} |
||||
|
||||
+/* This is a regression test that dereferencing the first element in the offset |
||||
+ * table doesn’t dereference memory before the start of the GVariant. The first |
||||
+ * element in the offset table gives the offset of the final member in the |
||||
+ * tuple (the offset table is stored in reverse), and the position of this final |
||||
+ * member is needed to check that none of the tuple members overlap with the |
||||
+ * offset table |
||||
+ * |
||||
+ * See https://gitlab.gnome.org/GNOME/glib/-/issues/2840 */ |
||||
+static void |
||||
+test_normal_checking_tuple_offsets5 (void) |
||||
+{ |
||||
+ /* A tuple of type (sss) in normal form would have an offset table with two |
||||
+ * entries: |
||||
+ * - The first entry (lowest index in the table) gives the offset of the |
||||
+ * third `s` in the tuple, as the offset table is reversed compared to the |
||||
+ * tuple members. |
||||
+ * - The second entry (highest index in the table) gives the offset of the |
||||
+ * second `s` in the tuple. |
||||
+ * - The offset of the first `s` in the tuple is always 0. |
||||
+ * |
||||
+ * See §2.5.4 (Structures) of the GVariant specification for details, noting |
||||
+ * that the table is only layed out this way because all three members of the |
||||
+ * tuple have non-fixed sizes. |
||||
+ * |
||||
+ * It’s not clear whether the 0xaa data of this variant is part of the strings |
||||
+ * in the tuple, or part of the offset table. It doesn’t really matter. This |
||||
+ * is a regression test to check that the code to validate the offset table |
||||
+ * doesn’t unconditionally try to access the first entry in the offset table |
||||
+ * by subtracting the table size from the end of the GVariant data. |
||||
+ * |
||||
+ * In this non-normal case, that would result in an address off the start of |
||||
+ * the GVariant data, and an out-of-bounds read, because the GVariant is one |
||||
+ * byte long, but the offset table is calculated as two bytes long (with 1B |
||||
+ * sized entries) from the tuple’s type. |
||||
+ */ |
||||
+ const GVariantType *data_type = G_VARIANT_TYPE ("(sss)"); |
||||
+ const guint8 data[] = { 0xaa }; |
||||
+ gsize size = sizeof (data); |
||||
+ GVariant *variant = NULL; |
||||
+ GVariant *normal_variant = NULL; |
||||
+ GVariant *expected = NULL; |
||||
+ |
||||
+ g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/2840"); |
||||
+ |
||||
+ variant = g_variant_new_from_data (data_type, data, size, FALSE, NULL, NULL); |
||||
+ g_assert_nonnull (variant); |
||||
+ |
||||
+ g_assert_false (g_variant_is_normal_form (variant)); |
||||
+ |
||||
+ normal_variant = g_variant_get_normal_form (variant); |
||||
+ g_assert_nonnull (normal_variant); |
||||
+ |
||||
+ expected = g_variant_new_parsed ("('', '', '')"); |
||||
+ g_assert_cmpvariant (expected, variant); |
||||
+ g_assert_cmpvariant (expected, normal_variant); |
||||
+ |
||||
+ g_variant_unref (expected); |
||||
+ g_variant_unref (normal_variant); |
||||
+ g_variant_unref (variant); |
||||
+} |
||||
+ |
||||
/* Test that an otherwise-valid serialised GVariant is considered non-normal if |
||||
* its offset table entries are too wide. |
||||
* |
||||
@@ -5827,6 +5888,8 @@ main (int argc, char **argv) |
||||
test_normal_checking_tuple_offsets3); |
||||
g_test_add_func ("/gvariant/normal-checking/tuple-offsets4", |
||||
test_normal_checking_tuple_offsets4); |
||||
+ g_test_add_func ("/gvariant/normal-checking/tuple-offsets5", |
||||
+ test_normal_checking_tuple_offsets5); |
||||
g_test_add_func ("/gvariant/normal-checking/tuple-offsets/minimal-sized", |
||||
test_normal_checking_tuple_offsets_minimal_sized); |
||||
g_test_add_func ("/gvariant/normal-checking/empty-object-path", |
||||
-- |
||||
GitLab |
||||
|
||||
|
||||
From 21a204147b16539b3eda3143b32844c49e29f4d4 Mon Sep 17 00:00:00 2001 |
||||
From: Philip Withnall <pwithnall@endlessos.org> |
||||
Date: Thu, 15 Dec 2022 16:49:28 +0000 |
||||
Subject: [PATCH 2/2] gvariant: Propagate trust when getting a child of a |
||||
serialised variant |
||||
|
||||
If a variant is trusted, that means all its children are trusted, so |
||||
ensure that their checked offsets are set as such. |
||||
|
||||
This allows a lot of the offset table checks to be avoided when getting |
||||
children from trusted serialised tuples, which speeds things up. |
||||
|
||||
No unit test is included because this is just a performance fix. If |
||||
there are other slownesses, or regressions, in serialised `GVariant` |
||||
performance, the fuzzing setup will catch them like it did this one. |
||||
|
||||
This change does reduce the time to run the oss-fuzz reproducer from 80s |
||||
to about 0.7s on my machine. |
||||
|
||||
Signed-off-by: Philip Withnall <pwithnall@endlessos.org> |
||||
|
||||
Fixes: #2841 |
||||
oss-fuzz#54314 |
||||
--- |
||||
glib/gvariant-core.c | 4 ++-- |
||||
1 file changed, 2 insertions(+), 2 deletions(-) |
||||
|
||||
diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c |
||||
index f441c4757e..4778022829 100644 |
||||
--- a/glib/gvariant-core.c |
||||
+++ b/glib/gvariant-core.c |
||||
@@ -1198,8 +1198,8 @@ g_variant_get_child_value (GVariant *value, |
||||
child->contents.serialised.bytes = |
||||
g_bytes_ref (value->contents.serialised.bytes); |
||||
child->contents.serialised.data = s_child.data; |
||||
- child->contents.serialised.ordered_offsets_up_to = s_child.ordered_offsets_up_to; |
||||
- child->contents.serialised.checked_offsets_up_to = s_child.checked_offsets_up_to; |
||||
+ child->contents.serialised.ordered_offsets_up_to = (value->state & STATE_TRUSTED) ? G_MAXSIZE : s_child.ordered_offsets_up_to; |
||||
+ child->contents.serialised.checked_offsets_up_to = (value->state & STATE_TRUSTED) ? G_MAXSIZE : s_child.checked_offsets_up_to; |
||||
|
||||
return child; |
||||
} |
||||
-- |
||||
GitLab |
||||
|
@ -0,0 +1,141 @@
@@ -0,0 +1,141 @@
|
||||
From 059f4f3999f1de506417611318c6f27db57fb689 Mon Sep 17 00:00:00 2001 |
||||
From: Marius Vollmer <mvollmer@redhat.com> |
||||
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 |
||||
|
Loading…
Reference in new issue