Merge branch '2787-assertion-failure-handling-non-zero-opcodes-in-dot-and-doh' into 'main'

Replace netmgr per-protocol sequential function with a common one

Closes #2787

See merge request isc-projects/bind9!5208

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

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

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`

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);
 /*%<

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

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;

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;

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,