diff --git a/CHANGES b/CHANGES index 5a036e684b..fae091bf04 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +5663. [bug] Properly handle non-zero OPCODEs when receiving the + queries over DoT and DoH channels. [GL #2787] + 5662. [bug] Views with recursion disabled are now configured with a default cache size of 2 MB, unless "max-cache-size" is explicitly set. This prevents cache RBT hash tables from diff --git a/bin/tests/system/doth/tests.sh b/bin/tests/system/doth/tests.sh index 3ef4016ac3..e41dd47c35 100644 --- a/bin/tests/system/doth/tests.sh +++ b/bin/tests/system/doth/tests.sh @@ -9,6 +9,7 @@ # See the COPYRIGHT file distributed with this work for additional # information regarding copyright ownership. +# shellcheck disable=SC1091 . ../conf.sh dig_with_tls_opts() { @@ -218,5 +219,40 @@ grep "ANSWER: 2500" dig.out.test$n > /dev/null || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status + ret)) +test_opcodes() { + EXPECT_STATUS="$1" + shift + for op in "$@"; + do + n=$((n + 1)) + echo_i "checking unexpected opcode query over DoH for opcode $op ($n)" + ret=0 + dig_with_https_opts +https @10.53.0.1 +opcode="$op" > dig.out.test$n + grep "status: $EXPECT_STATUS" dig.out.test$n > /dev/null || ret=1 + if [ $ret != 0 ]; then echo_i "failed"; fi + status=$((status + ret)) + + n=$((n + 1)) + echo_i "checking unexpected opcode query over DoH without encryption for opcode $op ($n)" + ret=0 + dig_with_http_opts +http-plain @10.53.0.1 +opcode="$op" > dig.out.test$n + grep "status: $EXPECT_STATUS" dig.out.test$n > /dev/null || ret=1 + if [ $ret != 0 ]; then echo_i "failed"; fi + status=$((status + ret)) + + n=$((n + 1)) + echo_i "checking unexpected opcode query over DoT for opcode $op ($n)" + ret=0 + dig_with_tls_opts +tls @10.53.0.1 +opcode="$op" > dig.out.test$n + grep "status: $EXPECT_STATUS" dig.out.test$n > /dev/null || ret=1 + if [ $ret != 0 ]; then echo_i "failed"; fi + status=$((status + ret)) + done +} + +test_opcodes NOERROR 0 +test_opcodes NOTIMP 1 2 3 6 7 8 9 10 11 12 13 14 15 +test_opcodes FORMERR 4 5 + echo_i "exit status: $status" [ $status -eq 0 ] || exit 1 diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 6e46da2b39..ced974c1f0 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -14,7 +14,11 @@ Notes for BIND 9.17.15 Security Fixes ~~~~~~~~~~~~~~ -- None. +- Sending non-zero opcode via DoT or DoH channels would trigger an assertion + failure in ``named``. This has been fixed. + + ISC would like to thank Ville Heikkila of Synopsys Cybersecurity Research + Center for responsibly disclosing the vulnerability to us. :gl:`#2787` Known Issues ~~~~~~~~~~~~ @@ -58,4 +62,4 @@ Bug Fixes - A deadlock at startup was introduced when fixing :gl:`#1875` because when locking key files for reading and writing, "in-view" logic was not taken into - account. This has been fixed. [GL #2783] + account. This has been fixed. :gl:`#2783` diff --git a/lib/isc/include/isc/netmgr.h b/lib/isc/include/isc/netmgr.h index e2ce614df4..6f654424b0 100644 --- a/lib/isc/include/isc/netmgr.h +++ b/lib/isc/include/isc/netmgr.h @@ -364,7 +364,7 @@ isc_nm_listentlsdns(isc_nm_t *mgr, isc_sockaddr_t *iface, */ void -isc_nm_tcpdns_sequential(isc_nmhandle_t *handle); +isc_nm_sequential(isc_nmhandle_t *handle); /*%< * Disable pipelining on this connection. Each DNS packet will be only * processed after the previous completes. @@ -390,24 +390,6 @@ isc_nm_tcpdns_keepalive(isc_nmhandle_t *handle, bool value); * to determine when to close a connection, rather than the idle timeout. */ -void -isc_nm_tlsdns_sequential(isc_nmhandle_t *handle); -/*%< - * Disable pipelining on this connection. Each DNS packet will be only - * processed after the previous completes. - * - * The socket must be unpaused after the query is processed. This is done - * the response is sent, or if we're dropping the query, it will be done - * when a handle is fully dereferenced by calling the socket's - * closehandle_cb callback. - * - * Note: This can only be run while a message is being processed; if it is - * run before any messages are read, no messages will be read. - * - * Also note: once this has been set, it cannot be reversed for a given - * connection. - */ - void isc_nm_tlsdns_keepalive(isc_nmhandle_t *handle, bool value); /*%< diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index 50ff5a2722..69c520e7bc 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -3255,6 +3255,40 @@ isc_nm_work_offload(isc_nm_t *netmgr, isc_nm_workcb_t work_cb, RUNTIME_CHECK(r == 0); } +void +isc_nm_sequential(isc_nmhandle_t *handle) { + isc_nmsocket_t *sock = NULL; + + REQUIRE(VALID_NMHANDLE(handle)); + REQUIRE(VALID_NMSOCK(handle->sock)); + + sock = handle->sock; + + switch (sock->type) { + case isc_nm_tcpdnssocket: + case isc_nm_tlsdnssocket: + break; + case isc_nm_httpsocket: + return; + default: + INSIST(0); + ISC_UNREACHABLE(); + } + + /* + * We don't want pipelining on this connection. That means + * that we need to pause after reading each request, and + * resume only after the request has been processed. This + * is done in resume_processing(), which is the socket's + * closehandle_cb callback, called whenever a handle + * is released. + */ + + isc__nmsocket_timer_stop(sock); + isc__nm_stop_reading(sock); + atomic_store(&sock->sequential, true); +} + #ifdef NETMGR_TRACE /* * Dump all active sockets in netmgr. We output to stderr diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c index 223c8d6303..da225cae16 100644 --- a/lib/isc/netmgr/tcpdns.c +++ b/lib/isc/netmgr/tcpdns.c @@ -1436,30 +1436,6 @@ isc__nm_async_tcpdnscancel(isc__networker_t *worker, isc__netievent_t *ev0) { isc__nm_failed_read_cb(sock, ISC_R_EOF, false); } -void -isc_nm_tcpdns_sequential(isc_nmhandle_t *handle) { - isc_nmsocket_t *sock = NULL; - - REQUIRE(VALID_NMHANDLE(handle)); - REQUIRE(VALID_NMSOCK(handle->sock)); - REQUIRE(handle->sock->type == isc_nm_tcpdnssocket); - - sock = handle->sock; - - /* - * We don't want pipelining on this connection. That means - * that we need to pause after reading each request, and - * resume only after the request has been processed. This - * is done in resume_processing(), which is the socket's - * closehandle_cb callback, called whenever a handle - * is released. - */ - - isc__nmsocket_timer_stop(sock); - isc__nm_stop_reading(sock); - atomic_store(&sock->sequential, true); -} - void isc_nm_tcpdns_keepalive(isc_nmhandle_t *handle, bool value) { isc_nmsocket_t *sock = NULL; diff --git a/lib/isc/netmgr/tlsdns.c b/lib/isc/netmgr/tlsdns.c index 41bf6caef8..db62ac8343 100644 --- a/lib/isc/netmgr/tlsdns.c +++ b/lib/isc/netmgr/tlsdns.c @@ -2006,30 +2006,6 @@ isc__nm_async_tlsdnscancel(isc__networker_t *worker, isc__netievent_t *ev0) { isc__nm_failed_read_cb(sock, ISC_R_EOF, false); } -void -isc_nm_tlsdns_sequential(isc_nmhandle_t *handle) { - isc_nmsocket_t *sock = NULL; - - REQUIRE(VALID_NMHANDLE(handle)); - REQUIRE(VALID_NMSOCK(handle->sock)); - REQUIRE(handle->sock->type == isc_nm_tlsdnssocket); - - sock = handle->sock; - - /* - * We don't want pipelining on this connection. That means - * that we need to pause after reading each request, and - * resume only after the request has been processed. This - * is done in resume_processing(), which is the socket's - * closehandle_cb callback, called whenever a handle - * is released. - */ - - isc__nmsocket_timer_stop(sock); - isc__nm_stop_reading(sock); - atomic_store(&sock->sequential, true); -} - void isc_nm_tlsdns_keepalive(isc_nmhandle_t *handle, bool value) { isc_nmsocket_t *sock = NULL; diff --git a/lib/ns/client.c b/lib/ns/client.c index ff098cc646..814581b8fa 100644 --- a/lib/ns/client.c +++ b/lib/ns/client.c @@ -1818,7 +1818,7 @@ ns__client_request(isc_nmhandle_t *handle, isc_result_t eresult, dns_acl_allowed(&netaddr, NULL, client->sctx->keepresporder, env)))) { - isc_nm_tcpdns_sequential(handle); + isc_nm_sequential(handle); } dns_opcodestats_increment(client->sctx->opcodestats,