On 7/1/26 08:27, Ismail Ramzi wrote:
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) {
Note that it's not possible to reach this code with a DoH dns parameter value containing any character greater than 0x7F at present, as the parser used to extract the value before calling this code is extremely strict and will reject the query. There is no other use of this code as of this writing.
While the signed issue here should be fixed in my personal opinion, this issue is purely theoretical at this point.
-Doug -- Visit https://lists.isc.org/mailman/listinfo/bind-users to unsubscribe from this list.
