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)
--
Visit https://lists.isc.org/mailman/listinfo/bind-users to unsubscribe from
this list.
