Commit ab859a8a authored by Philip Withnall's avatar Philip Withnall Committed by Claudio Saavedra

soup-uri: Check string lengths before reading bytes of %-encoded chars

There are two instances in `SoupURI` where `g_ascii_isxdigit()` is
called two bytes ahead of the read pointer to check if a %-encoding is
valid. This is fine when the string being parsed is nul-terminated (as
the first `g_ascii_isxdigit()` call will safely return `FALSE`), but
will result in a read off the end of the buffer if it’s
length-terminated (and doesn’t happen to also be nul-terminated).

Thankfully, that’s not the case in any of the code paths in `SoupURI`
leading to these two instances, so this is not a security issue.

However, the functions should probably be fixed to do an appropriate
length check, just in case they get called from somewhere else in
future.

Spotted by oss-fuzz in oss-fuzz#23815 and oss-fuzz#23818, when it was
fuzzing the new `GUri` implementation in GLib, which is heavily based
off this code.

Includes two unit tests which don’t actually trigger the original
failure (as all strings passed into `SoupURI` are forced to be
nul-terminated), but would trigger it if the nul termination was not
present.
Signed-off-by: Philip Withnall's avatarPhilip Withnall <withnall@endlessm.com>
parent 7199743b
Pipeline #193211 passed with stage
in 1 minute and 5 seconds
......@@ -754,6 +754,8 @@ soup_uri_encode (const char *part, const char *escape_extra)
#define XDIGIT(c) ((c) <= '9' ? (c) - '0' : ((c) & 0x4F) - 'A' + 10)
#define HEXCHAR(s) ((XDIGIT (s[1]) << 4) + XDIGIT (s[2]))
/* length must be set (e.g. from strchr()) such that [part, part + length]
* contains no nul bytes */
char *
soup_uri_decoded_copy (const char *part, int length, int *decoded_length)
{
......@@ -766,7 +768,9 @@ soup_uri_decoded_copy (const char *part, int length, int *decoded_length)
s = d = (unsigned char *)decoded;
do {
if (*s == '%') {
if (!g_ascii_isxdigit (s[1]) ||
if (s[1] == '\0' ||
s[2] == '\0' ||
!g_ascii_isxdigit (s[1]) ||
!g_ascii_isxdigit (s[2])) {
*d++ = *s;
continue;
......@@ -803,6 +807,8 @@ soup_uri_decode (const char *part)
return soup_uri_decoded_copy (part, strlen (part), NULL);
}
/* length must be set (e.g. from strchr()) such that [part, part + length]
* contains no nul bytes */
static char *
uri_normalized_copy (const char *part, int length,
const char *unescape_extra)
......@@ -817,7 +823,9 @@ uri_normalized_copy (const char *part, int length,
s = d = (unsigned char *)normalized;
while (*s) {
if (*s == '%') {
if (!g_ascii_isxdigit (s[1]) ||
if (s[1] == '\0' ||
s[2] == '\0' ||
!g_ascii_isxdigit (s[1]) ||
!g_ascii_isxdigit (s[2])) {
*d++ = *s++;
continue;
......
......@@ -466,7 +466,8 @@ static struct {
{ "foo bar", NULL, "foo%20bar" },
{ "foo bar", " ", "foo bar" },
{ "fo\xc3\xb6" "bar", NULL, "fo%C3%B6bar" },
{ "fo\xc3\xb6 bar", " ", "fo%C3%B6 bar" }
{ "fo\xc3\xb6 bar", " ", "fo%C3%B6 bar" },
{ "%", NULL, "%" },
};
static int num_normalization_tests = G_N_ELEMENTS (normalization_tests);
......@@ -563,6 +564,14 @@ do_data_tests (void)
soup_test_session_abort_unref (session);
}
static void
test_uri_decode (void)
{
gchar *decoded = soup_uri_decode ("%");
g_assert_cmpstr (decoded, ==, "%");
g_free (decoded);
}
int
main (int argc, char **argv)
{
......@@ -576,6 +585,7 @@ main (int argc, char **argv)
g_test_add_func ("/uri/null", do_soup_uri_null_tests);
g_test_add_func ("/uri/normalization", do_normalization_tests);
g_test_add_func ("/uri/data", do_data_tests);
g_test_add_func ("/uri/decode", test_uri_decode);
ret = g_test_run ();
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment