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

[Claudio Saavedra <csaavedra src gnome org>]

commit ab859a8adaf37e2cbdb11b4e4b825b872b93ed74
Author: Philip Withnall <withnall endlessm com>
Date:   Wed Jul 1 13:01:32 2020 +0100

    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 <withnall endlessm com>

 libsoup/soup-uri.c       | 12 ++++++++++--
 tests/uri-parsing-test.c | 12 +++++++++++-
 2 files changed, 21 insertions(+), 3 deletions(-)
---
diff --git a/libsoup/soup-uri.c b/libsoup/soup-uri.c
index c01b3e3b..4bb55b81 100644
--- a/libsoup/soup-uri.c
+++ b/libsoup/soup-uri.c
@@ -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;
diff --git a/tests/uri-parsing-test.c b/tests/uri-parsing-test.c
index 85f09b9e..d463f1f4 100644
--- a/tests/uri-parsing-test.c
+++ b/tests/uri-parsing-test.c
@@ -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 ();