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>

1 files changed, 10 insertions, 2 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;