[PATCH] netmgr: avoid out-of-bounds read from sign-extended char in DoH base64url decoder

[Ismail Ramzi]

isc__nm_base64url_to_base64() and isc__nm_base64_to_base64url() in
lib/isc/netmgr/http.c index the 256-entry base64url_validation_table[]
with a plain char cast straight to size_t:

	base64url_validation_table[(size_t)base64url[i]]

char is signed on the platforms BIND commonly runs on (x86-64, arm64),
so any input byte with the high bit set (0x80-0xFF) is sign-extended
before the conversion to size_t.  For example (size_t)(char)0x80 is
0xffffffffffffff80, which is used as an index into a 256-element array,
i.e. a wild out-of-bounds read.  Depending on what the read returns the
byte is either accepted (and copied into the freshly allocated output,
producing corrupt data and a leak) or the process reads unmapped memory
and crashes.

These are the decoders used to turn the base64url "?dns=" parameter of a
DoH GET request into base64, so they are meant to operate on
attacker-supplied bytes.  The fix belongs in the decoders themselves and
not in the callers: cast each byte to unsigned char before using it as
an index, which is the convention already used elsewhere in the tree
(e.g. lib/isc/httpd.c).  An unsigned char is always a valid index into
the 256-entry table, so the extra size_t cast is no longer needed.

A regression case is added to the doh_base64url_to_base64 unit test that
feeds a byte with the high bit set; before the fix it reads out of
bounds (and wrongly returns a non-NULL result), after the fix the byte
is rejected and NULL is returned.
---
diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c
index 8f6cf21..063aa10 100644
--- a/lib/isc/netmgr/http.c
+++ b/lib/isc/netmgr/http.c
@@ -3529,7 +3529,9 @@ isc__nm_base64url_to_base64(isc_mem_t *mem, const char *base64url,
 			res[i] = '/';
 			break;
 		default:
-			if (base64url_validation_table[(size_t)base64url[i]]) {
+			if (base64url_validation_table[(unsigned char)
+							       base64url[i]])
+			{
 				res[i] = base64url[i];
 			} else {
 				isc_mem_free(mem, res);
@@ -3587,7 +3589,7 @@ isc__nm_base64_to_base64url(isc_mem_t *mem, const char *base64,
 			 * the rest of the characters.
 			 */
 			if (base64[i] != '-' && base64[i] != '_' &&
-			    base64url_validation_table[(size_t)base64[i]])
+			    base64url_validation_table[(unsigned char)base64[i]])
 			{
 				res[i] = base64[i];
 			} else {
diff --git a/tests/isc/doh_test.c b/tests/isc/doh_test.c
index fa5c9cb..c0637d9 100644
--- a/tests/isc/doh_test.c
+++ b/tests/isc/doh_test.c
@@ -1484,6 +1484,19 @@ ISC_RUN_TEST_IMPL(doh_base64url_to_base64) {
 		assert_null(res);
 		assert_true(res_len == 0);
 	}
+	/*
+	 * invalid: a byte with the high bit set must be rejected and must
+	 * not be used as a (sign-extended) index into the validation table.
+	 */
+	{
+		char test[] = { 'P', 'D', 'w', (char)0x80, '\0' };
+		res_len = 0;
+
+		res = isc__nm_base64url_to_base64(isc_g_mctx, test,
+						  strlen(test), &res_len);
+		assert_null(res);
+		assert_true(res_len == 0);
+	}
 }
 
 ISC_RUN_TEST_IMPL(doh_base64_to_base64url) {
-- 
2.50.1 (Apple Git-155)