From 10ee7301e8edb13e59143ee5653cd2b46e26c044 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 9 Aug 2018 01:00:20 +0100 Subject: [PATCH] gvariant: Fix bounds checking in GVariant text format parser The token_stream_peek() functions were not doing any bounds checking, so could potentially read 1 byte off the end of the input blob. This was never noticed, since the input stream is almost always a nul-terminated string. However, g_variant_parse() does allow non-nul-terminated strings to be used with a @limit parameter, and the bugs become apparent under valgrind if that parameter is used. This includes modifications to the test cases to cover the non-nul-terminated case. Spotted by ossfuzz. Signed-off-by: Philip Withnall --- glib/gvariant-parser.c | 21 ++++++++++++++------- glib/tests/gvariant.c | 39 ++++++++++++++++++++++++++++++--------- 2 files changed, 44 insertions(+), 16 deletions(-) diff --git a/glib/gvariant-parser.c b/glib/gvariant-parser.c index 3261bc1af..233a19f7c 100644 --- a/glib/gvariant-parser.c +++ b/glib/gvariant-parser.c @@ -260,6 +260,9 @@ token_stream_prepare (TokenStream *stream) stream->this = stream->stream; stream->stream = end; + /* We must have at least one byte in a token. */ + g_assert (stream->stream - stream->this >= 1); + return TRUE; } @@ -276,7 +279,8 @@ token_stream_peek (TokenStream *stream, if (!token_stream_prepare (stream)) return FALSE; - return stream->this[0] == first_char; + return stream->stream - stream->this >= 1 && + stream->this[0] == first_char; } static gboolean @@ -287,7 +291,8 @@ token_stream_peek2 (TokenStream *stream, if (!token_stream_prepare (stream)) return FALSE; - return stream->this[0] == first_char && + return stream->stream - stream->this >= 2 && + stream->this[0] == first_char && stream->this[1] == second_char; } @@ -297,7 +302,8 @@ token_stream_is_keyword (TokenStream *stream) if (!token_stream_prepare (stream)) return FALSE; - return g_ascii_isalpha (stream->this[0]) && + return stream->stream - stream->this >= 2 && + g_ascii_isalpha (stream->this[0]) && g_ascii_isalpha (stream->this[1]); } @@ -307,10 +313,11 @@ token_stream_is_numeric (TokenStream *stream) if (!token_stream_prepare (stream)) return FALSE; - return (g_ascii_isdigit (stream->this[0]) || - stream->this[0] == '-' || - stream->this[0] == '+' || - stream->this[0] == '.'); + return (stream->stream - stream->this >= 1 && + (g_ascii_isdigit (stream->this[0]) || + stream->this[0] == '-' || + stream->this[0] == '+' || + stream->this[0] == '.')); } static gboolean diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c index fdaed1acc..5aac3de53 100644 --- a/glib/tests/gvariant.c +++ b/glib/tests/gvariant.c @@ -3889,27 +3889,48 @@ test_parse_failures (void) "boolean 4", "8-9:", "can not parse as", "int32 true", "6-10:", "can not parse as", "[double 5, int32 5]", "1-9,11-18:", "common type", - "string 4", "7-8:", "can not parse as" + "string 4", "7-8:", "can not parse as", + "\x0a", "1:", "expected value", + "((", "2:", "expected value", }; gint i; for (i = 0; i < G_N_ELEMENTS (test); i += 3) { - GError *error = NULL; + GError *error1 = NULL, *error2 = NULL; GVariant *value; - value = g_variant_parse (NULL, test[i], NULL, NULL, &error); - g_assert (value == NULL); + /* Copy the test string and drop its nul terminator, then use the @limit + * parameter of g_variant_parse() to set the length. This allows valgrind + * to catch 1-byte heap buffer overflows. */ + gsize test_len = MAX (strlen (test[i]), 1); + gchar *test_blob = g_malloc0 (test_len); /* no nul terminator */ - if (!strstr (error->message, test[i+2])) + memcpy (test_blob, test[i], test_len); + value = g_variant_parse (NULL, test_blob, test_blob + test_len, NULL, &error1); + g_assert_null (value); + + g_free (test_blob); + + if (!strstr (error1->message, test[i+2])) g_error ("test %d: Can't find '%s' in '%s'", i / 3, - test[i+2], error->message); + test[i+2], error1->message); - if (!g_str_has_prefix (error->message, test[i+1])) + if (!g_str_has_prefix (error1->message, test[i+1])) g_error ("test %d: Expected location '%s' in '%s'", i / 3, - test[i+1], error->message); + test[i+1], error1->message); + + /* Test again with the nul terminator this time. The behaviour should be + * the same. */ + value = g_variant_parse (NULL, test[i], NULL, NULL, &error2); + g_assert_null (value); + + g_assert_cmpint (error1->domain, ==, error2->domain); + g_assert_cmpint (error1->code, ==, error2->code); + g_assert_cmpstr (error1->message, ==, error2->message); - g_error_free (error); + g_clear_error (&error1); + g_clear_error (&error2); } } -- 2.11.4.GIT