From 7f4300c5dad0adea9907c35b4999a725011ca48d Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Thu, 19 Sep 2024 09:59:13 -0500 Subject: [PATCH 1/6] gvariant-parser: add assert to ensure we don't write too far This function is complicated and it's hard to assess whether it is correct or not, so let's add an assert just to be sure. Notably, when writing this I initially had an off-by-one error in my assert, causing the assertion to actually fire when running the unit tests. So we know it's definitely possible for the function to use the entirety of the buffer. The upper bound is not loose, and writing one additional byte would be a security bug. Note my assertion possibly doesn't protect against security issues because it will be hit after the bad write rather than before. Fixes #3469 --- glib/gvariant-parser.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/glib/gvariant-parser.c b/glib/gvariant-parser.c index efad35619..dc8cb3e5a 100644 --- a/glib/gvariant-parser.c +++ b/glib/gvariant-parser.c @@ -431,6 +431,7 @@ pattern_coalesce (const gchar *left, { gchar *result; gchar *out; + size_t buflen; /* the length of the output is loosely bound by the sum of the input * lengths, not simply the greater of the two lengths. @@ -439,7 +440,8 @@ pattern_coalesce (const gchar *left, * * 8 + 8 = 12 */ - out = result = g_malloc (strlen (left) + strlen (right)); + buflen = strlen (left) + strlen (right); + out = result = g_malloc (buflen); while (*left && *right) { @@ -493,6 +495,9 @@ pattern_coalesce (const gchar *left, } } + /* Need at least one byte remaining for trailing nul. */ + g_assert (out < result + buflen); + if (*left || *right) { g_free (result); From 3d5ada268988542172953df67fdde58b49eb4f99 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 19 Sep 2024 16:35:27 +0100 Subject: [PATCH 2/6] =?UTF-8?q?gvariant-parser:=20Add=20a=20proof=20for=20?= =?UTF-8?q?pattern=5Fcoalesce()=E2=80=99s=20safety?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This proof backs up the already-present assertion that “the length of the output is loosely bounded by the sum of the input lengths”. Signed-off-by: Philip Withnall Helps: #3469 --- glib/gvariant-parser.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/glib/gvariant-parser.c b/glib/gvariant-parser.c index dc8cb3e5a..81d96ee29 100644 --- a/glib/gvariant-parser.c +++ b/glib/gvariant-parser.c @@ -401,6 +401,8 @@ token_stream_end_ref (TokenStream *stream, ref->end = stream->stream - stream->start; } +/* This is guaranteed to write exactly as many bytes to `out` as it consumes + * from `in`. i.e. The `out` buffer doesn’t need to be any longer than `in`. */ static void pattern_copy (gchar **out, const gchar **in) @@ -436,9 +438,12 @@ pattern_coalesce (const gchar *left, /* the length of the output is loosely bound by the sum of the input * lengths, not simply the greater of the two lengths. * - * (*(iii)) + ((iii)*) ((iii)(iii)) + * (*(iii)) + ((iii)*) = ((iii)(iii)) * - * 8 + 8 = 12 + * 8 + 8 = 12 + * + * This can be proven by the fact that `out` is never incremented by more + * bytes than are consumed from `left` or `right` in each iteration. */ buflen = strlen (left) + strlen (right); out = result = g_malloc (buflen); From 1cb682b3201c26b04fb98734b33e814d3df41743 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 19 Sep 2024 17:40:24 +0100 Subject: [PATCH 3/6] tests: Fix a minor typo in a comment in gvariant tests Signed-off-by: Philip Withnall --- glib/tests/gvariant.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c index 27586be71..c00a559e6 100644 --- a/glib/tests/gvariant.c +++ b/glib/tests/gvariant.c @@ -4014,7 +4014,7 @@ test_parses (void) g_free (printed); } - /* pattern coalese of `MN` and `*` is `MN` */ + /* pattern coalesce of `MN` and `*` is `MN` */ { GVariant *value = NULL; GError *error = NULL; From c7e351290252acc14054baf569a4674615e22af0 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 19 Sep 2024 17:40:45 +0100 Subject: [PATCH 4/6] tests: Add some additional pattern_coalesce() tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first of these tests is to cover the extreme case where the number of bytes written out by `pattern_coalesce()` is as close to `strlen (left) + strlen (right)` as we can manage, due to both inputs being really small and hence `max (strlen (left), strlen (right))` being only 1 less than `strlen (left) + strlen (right)`, which is as close as we can get to triggering an off-by-one error. The second of these tests is an attempt to encode the case used in the proof for correctness of `pattern_coalesce()` not overflowing its buffer bounds: `(*(iii)) + ((iii)*) = ((iii)(iii))`. I don’t think it’s actually possible to hit that case, as it’s not possible to generate a plain `*` in either of the input patterns. `Ma*` is the best we can manage in practice. See https://gitlab.gnome.org/GNOME/glib/-/issues/3469#note_2226901 for reasoning. Signed-off-by: Philip Withnall Helps: #3469 --- glib/tests/gvariant.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c index c00a559e6..2aa80d6da 100644 --- a/glib/tests/gvariant.c +++ b/glib/tests/gvariant.c @@ -4025,6 +4025,29 @@ test_parses (void) g_variant_unref (value); } + /* pattern coalesce of `u` and `u` is `u`; this operates close to the string + * length bounds in pattern_coalesce() */ + { + GVariant *value = NULL; + GError *error = NULL; + + value = g_variant_parse (NULL, "[@u 5, @u 15]", NULL, NULL, &error); + g_assert_no_error (error); + g_assert_cmpstr (g_variant_get_type_string (value), ==, "au"); + g_variant_unref (value); + } + + /* pattern coalesce of `(Ma*Ma(iii))` and `(Ma(iii)Ma*)` is `(Ma(iii)Ma(iii))` */ + { + GVariant *value = NULL; + GError *error = NULL; + + value = g_variant_parse (NULL, "[([], [(1,2,3)]), ([(1,2,3)], [])]", NULL, NULL, &error); + g_assert_no_error (error); + g_assert_cmpstr (g_variant_get_type_string (value), ==, "a(a(iii)a(iii))"); + g_variant_unref (value); + } + #ifndef _MSC_VER /* inf/nan strings are C99 features which Visual C++ does not support */ /* inf/nan mini test */ From 785b61cfcb77cbe37a5c80b6e59c2ab69c57167c Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 19 Sep 2024 17:44:28 +0100 Subject: [PATCH 5/6] gvariant-parser: Add additional buffer byte for nul terminator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In `pattern_coalesce()`. I am fairly sure this byte is never needed, as the most extreme case of `max (strlen (left), strlen (right)` vs `strlen (left) + strlen (right)` I can think of still gives 1 byte left in the buffer (without the need for this commit). However, the proof we have of how much space is needed in the buffer only goes far enough to show that the number of bytes needed before the nul terminator is at most `strlen (left) + strlen (right)`. That means we still technically need to add an additional byte for the nul terminator. The need for this could be eliminated by coming up with a stronger proof to show that the number of bytes needed is strictly less than `strlen (left) + strlen (right)`, but I can’t do that, and adding one byte is easy to do. Type strings are short and we’re nowhere near making a large allocation here. Signed-off-by: Philip Withnall Helps: #3469 --- glib/gvariant-parser.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/glib/gvariant-parser.c b/glib/gvariant-parser.c index 81d96ee29..822b46651 100644 --- a/glib/gvariant-parser.c +++ b/glib/gvariant-parser.c @@ -445,7 +445,7 @@ pattern_coalesce (const gchar *left, * This can be proven by the fact that `out` is never incremented by more * bytes than are consumed from `left` or `right` in each iteration. */ - buflen = strlen (left) + strlen (right); + buflen = strlen (left) + strlen (right) + 1; out = result = g_malloc (buflen); while (*left && *right) From 2842e4a86f50816a8ae849309869f7bf3d0c580d Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 19 Sep 2024 17:49:10 +0100 Subject: [PATCH 6/6] =?UTF-8?q?gvariant-parser:=20Assert=20that=20pattern?= =?UTF-8?q?=20lengths=20don=E2=80=99t=20overflow?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I can’t see it being possible for this to be hit in practice, as it would require two very long GVariant text format inputs, which would probably hit input limits earlier on somewhere else. But in order to avoid a silent integer overflow, let’s check that the addition won’t overflow before going ahead with it. Signed-off-by: Philip Withnall Helps: #3469 --- glib/gvariant-parser.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/glib/gvariant-parser.c b/glib/gvariant-parser.c index 822b46651..1a6697797 100644 --- a/glib/gvariant-parser.c +++ b/glib/gvariant-parser.c @@ -434,6 +434,7 @@ pattern_coalesce (const gchar *left, gchar *result; gchar *out; size_t buflen; + size_t left_len = strlen (left), right_len = strlen (right); /* the length of the output is loosely bound by the sum of the input * lengths, not simply the greater of the two lengths. @@ -445,7 +446,8 @@ pattern_coalesce (const gchar *left, * This can be proven by the fact that `out` is never incremented by more * bytes than are consumed from `left` or `right` in each iteration. */ - buflen = strlen (left) + strlen (right) + 1; + g_assert (left_len < G_MAXSIZE - right_len); + buflen = left_len + right_len + 1; out = result = g_malloc (buflen); while (*left && *right)