From ec8675940179e5bc8a741d2e84607845b5134307 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 22 Jun 2021 12:24:44 +0200 Subject: [PATCH 1/3] Replace netmgr per-protocol sequential function with a common one Previously, each protocol (TCPDNS, TLSDNS) has specified own function to disable pipelining on the connection. An oversight would lead to assertion failure when opcode is not query over non-TCPDNS protocol because the isc_nm_tcpdns_sequential() function would be called over non-TCPDNS socket. This commit removes the per-protocol functions and refactors the code to have and use common isc_nm_sequential() function that would either disable the pipelining on the socket or would handle the request in per specific manner. Currently it ignores the call for HTTP sockets and causes assertion failure for protocols where it doesn't make sense to call the function at all. --- lib/isc/include/isc/netmgr.h | 20 +------------------- lib/isc/netmgr/netmgr.c | 34 ++++++++++++++++++++++++++++++++++ lib/isc/netmgr/tcpdns.c | 24 ------------------------ lib/isc/netmgr/tlsdns.c | 24 ------------------------ lib/ns/client.c | 2 +- 5 files changed, 36 insertions(+), 68 deletions(-) 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, From ef9f09252c3acaebfe0d696145f31372f4d46182 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Tue, 22 Jun 2021 13:32:24 +0300 Subject: [PATCH 2/3] System tests to check named behaviour for unexpected opcodes This commit adds a set of tests to verify that BIND will not crash when some opcodes are sent over DoT or DoH, leading to marking network handle in question as sequential. --- bin/tests/system/doth/tests.sh | 36 ++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) 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 From dd0e3b021322bae4aba621f05b1c0ffbb17df2c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 22 Jun 2021 12:33:50 +0200 Subject: [PATCH 3/3] Add CHANGES and release notes for [GL #2787] --- CHANGES | 3 +++ doc/notes/notes-current.rst | 8 ++++++-- 2 files changed, 9 insertions(+), 2 deletions(-) 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/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`