Merge branch '3149-drop-TCP-connection-when-garbage-is-received' into 'main'

Reset the TCP connection when garbage is received

Closes #3149

See merge request isc-projects/bind9!5849

CHANGES

@@ -1,3 +1,6 @@
+5809.	[bug]		Reset client TCP connection when data received cannot
+			be parsed as a valid DNS request. [GL #3149]
+
 5808.	[bug]		Certain TCP failures were not caught and handled
 			correctly by the dispatch manager, causing
 			connections to time out rather than returning

bin/tests/system/tcp/clean.sh

@@ -11,10 +11,10 @@
 # See the COPYRIGHT file distributed with this work for additional
 # information regarding copyright ownership.
 
-rm -f */named.memstats
-rm -f */named.run
-rm -f */named.conf
-rm -f */named.stats*
+rm -f ./*/named.memstats
+rm -f ./*/named.run
+rm -f ./*/named.conf
+rm -f ./*/named.stats*
 rm -f ans6/ans.run*
 rm -f dig.out*
 rm -f rndc.out*

bin/tests/system/tcp/conftest.py

@@ -0,0 +1,59 @@
+# Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+#
+# SPDX-License-Identifier: MPL-2.0
+#
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0.  If a copy of the MPL was not distributed with this
+# file, you can obtain one at https://mozilla.org/MPL/2.0/.
+#
+# See the COPYRIGHT file distributed with this work for additional
+# information regarding copyright ownership.
+
+import os
+import pytest
+
+
+def pytest_configure(config):
+    config.addinivalue_line(
+        "markers", "dnspython: mark tests that need dnspython to function"
+    )
+    config.addinivalue_line(
+        "markers", "dnspython2: mark tests that need dnspython >= 2.0.0"
+    )
+
+
+def pytest_collection_modifyitems(config, items):
+    # pylint: disable=unused-argument,unused-import,too-many-branches
+    # pylint: disable=import-outside-toplevel
+
+    # Test for dnspython module
+    skip_dnspython = pytest.mark.skip(
+        reason="need dnspython module to run")
+    try:
+        import dns.query  # noqa: F401
+    except ModuleNotFoundError:
+        for item in items:
+            if "dnspython" in item.keywords:
+                item.add_marker(skip_dnspython)
+
+    # Test for dnspython >= 2.0.0 module
+    skip_dnspython2 = pytest.mark.skip(
+        reason="need dnspython >= 2.0.0 module to run")
+    try:
+        from dns.query import send_tcp  # noqa: F401
+    except ImportError:
+        for item in items:
+            if "dnspython2" in item.keywords:
+                item.add_marker(skip_dnspython2)
+
+
+@pytest.fixture
+def port(request):
+    # pylint: disable=unused-argument
+    env_port = os.getenv("PORT")
+    if port is None:
+        env_port = 5300
+    else:
+        env_port = int(env_port)
+
+    return env_port

bin/tests/system/tcp/ns7/named.conf.in

@@ -0,0 +1,41 @@
+/*
+ * Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+ *
+ * SPDX-License-Identifier: MPL-2.0
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0.  If a copy of the MPL was not distributed with this
+ * file, you can obtain one at https://mozilla.org/MPL/2.0/.
+ *
+ * See the COPYRIGHT file distributed with this work for additional
+ * information regarding copyright ownership.
+ */
+
+options {
+	query-source address 10.53.0.7;
+	notify-source 10.53.0.7;
+	transfer-source 10.53.0.7;
+	port @PORT@;
+	pid-file "named.pid";
+	listen-on { 10.53.0.7; };
+	listen-on-v6 { none; };
+	recursion no;
+	notify yes;
+	statistics-file "named.stats";
+	tcp-clients 1;
+	keep-response-order { any; };
+};
+
+key rndc_key {
+	secret "1234abcd8765";
+	algorithm hmac-sha256;
+};
+
+controls {
+	inet 10.53.0.7 port @CONTROLPORT@ allow { any; } keys { rndc_key; };
+};
+
+zone "." {
+	type primary;
+	file "root.db";
+};

bin/tests/system/tcp/ns7/root.db

@@ -0,0 +1,24 @@
+; Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+;
+; SPDX-License-Identifier: MPL-2.0
+;
+; This Source Code Form is subject to the terms of the Mozilla Public
+; License, v. 2.0.  If a copy of the MPL was not distributed with this
+; file, you can obtain one at https://mozilla.org/MPL/2.0/.
+;
+; See the COPYRIGHT file distributed with this work for additional
+; information regarding copyright ownership.
+
+$TTL 300
+. 			IN SOA	gson.nominum.com. a.root.servers.nil. (
+				2000042100   	; serial
+				600         	; refresh
+				600         	; retry
+				1200    	; expire
+				600       	; minimum
+				)
+.			NS	a.root-servers.nil.
+a.root-servers.nil.	A	10.53.0.7
+
+example.		NS	ns2.example.
+ns2.example.		A	10.53.0.2

bin/tests/system/tcp/setup.sh

@@ -20,3 +20,4 @@ copy_setports ns2/named.conf.in ns2/named.conf
 copy_setports ns3/named.conf.in ns3/named.conf
 copy_setports ns4/named.conf.in ns4/named.conf
 copy_setports ns5/named.conf.in ns5/named.conf
+copy_setports ns7/named.conf.in ns7/named.conf

bin/tests/system/tcp/tests-tcp.py

@@ -0,0 +1,94 @@
+#!/usr/bin/python3
+
+# Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+#
+# SPDX-License-Identifier: MPL-2.0
+#
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0.  If a copy of the MPL was not distributed with this
+# file, you can obtain one at https://mozilla.org/MPL/2.0/.
+#
+# See the COPYRIGHT file distributed with this work for additional
+# information regarding copyright ownership.
+
+# pylint: disable=unused-variable
+
+import socket
+import struct
+import time
+
+import pytest
+
+TIMEOUT = 10
+
+
+def create_msg(qname, qtype):
+    import dns.message
+    msg = dns.message.make_query(qname, qtype, want_dnssec=True,
+                                 use_edns=0, payload=4096)
+    return msg
+
+
+def timeout():
+    return time.time() + TIMEOUT
+
+
+def create_socket(host, port):
+    sock = socket.create_connection((host, port), timeout=1)
+    sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, True)
+    return sock
+
+
+@pytest.mark.dnspython
+@pytest.mark.dnspython2
+def test_tcp_garbage(port):
+    import dns.query
+
+    with create_socket("10.53.0.7", port) as sock:
+
+        msg = create_msg("a.example.", "A")
+        (sbytes, stime) = dns.query.send_tcp(sock, msg, timeout())
+        (response, rtime) = dns.query.receive_tcp(sock, timeout())
+
+        wire = msg.to_wire()
+        assert len(wire) > 0
+
+        # Send DNS message shorter than DNS message header (12),
+        # this should cause the connection to be terminated
+        sock.send(struct.pack('!H', 11))
+        sock.send(struct.pack('!s', b'0123456789a'))
+
+        with pytest.raises(EOFError):
+            try:
+                (sbytes, stime) = dns.query.send_tcp(sock, msg, timeout())
+                (response, rtime) = dns.query.receive_tcp(sock, timeout())
+            except ConnectionError as e:
+                raise EOFError from e
+
+
+@pytest.mark.dnspython
+@pytest.mark.dnspython2
+def test_tcp_garbage_response(port):
+    import dns.query
+    import dns.message
+
+    with create_socket("10.53.0.7", port) as sock:
+
+        msg = create_msg("a.example.", "A")
+        (sbytes, stime) = dns.query.send_tcp(sock, msg, timeout())
+        (response, rtime) = dns.query.receive_tcp(sock, timeout())
+
+        wire = msg.to_wire()
+        assert len(wire) > 0
+
+        # Send DNS response instead of DNS query, this should cause
+        # the connection to be terminated
+
+        rmsg = dns.message.make_response(msg)
+        (sbytes, stime) = dns.query.send_tcp(sock, rmsg, timeout())
+
+        with pytest.raises(EOFError):
+            try:
+                (response, rtime) = dns.query.receive_tcp(sock, timeout())
+            except ConnectionError as e:
+                raise EOFError from e

doc/notes/notes-current.rst

@@ -76,3 +76,6 @@ Bug Fixes
   been fixed by adding a "write" timer. Connections that are hung
   while writing will now time out after the ``tcp-idle-timeout`` period
   has elapsed. :gl:`#3132`
+
+- Client TCP connections are now closed immediately when data received
+  cannot be parsed as a valid DNS request. :gl:`#3149`

lib/isc/netmgr/netmgr-int.h

@@ -1247,6 +1247,12 @@ isc__nmsocket_shutdown(isc_nmsocket_t *sock);
  * callbacks.
  */
 
+void
+isc__nmsocket_reset(isc_nmsocket_t *sock);
+/*%<
+ * Reset and close the socket.
+ */
+
 bool
 isc__nmsocket_active(isc_nmsocket_t *sock);
 /*%<

lib/isc/netmgr/netmgr.c

@@ -1835,9 +1835,6 @@ isc__nmhandle_detach(isc_nmhandle_t **handlep FLARG) {
 	}
 }
 
-void
-isc__nmsocket_shutdown(isc_nmsocket_t *sock);
-
 static void
 nmhandle_detach_cb(isc_nmhandle_t **handlep FLARG) {
 	isc_nmsocket_t *sock = NULL;
@@ -2085,11 +2082,7 @@ isc__nmsocket_writetimeout_cb(uv_timer_t *timer) {
 	int r = uv_timer_stop(&sock->write_timer);
 	UV_RUNTIME_CHECK(uv_timer_stop, r);
 
-	/* The shutdown will be handled in the respective close functions */
-	r = uv_tcp_close_reset(&sock->uv_handle.tcp, NULL);
-	UV_RUNTIME_CHECK(uv_tcp_close_reset, r);
-
-	isc__nmsocket_shutdown(sock);
+	isc__nmsocket_reset(sock);
 }
 
 void
@@ -2902,6 +2895,32 @@ isc__nm_async_detach(isc__networker_t *worker, isc__netievent_t *ev0) {
 	nmhandle_detach_cb(&ievent->handle FLARG_PASS);
 }
 
+void
+isc__nmsocket_reset(isc_nmsocket_t *sock) {
+	REQUIRE(VALID_NMSOCK(sock));
+
+	switch (sock->type) {
+	case isc_nm_tcpdnssocket:
+	case isc_nm_tlsdnssocket:
+		REQUIRE(sock->parent == NULL);
+		break;
+	default:
+		INSIST(0);
+		ISC_UNREACHABLE();
+		break;
+	}
+
+	if (!uv_is_closing(&sock->uv_handle.handle)) {
+		/*
+		 * The real shutdown will be handled in the respective
+		 * close functions.
+		 */
+		int r = uv_tcp_close_reset(&sock->uv_handle.tcp, NULL);
+		UV_RUNTIME_CHECK(uv_tcp_close_reset, r);
+	}
+	isc__nmsocket_shutdown(sock);
+}
+
 void
 isc__nmsocket_shutdown(isc_nmsocket_t *sock) {
 	REQUIRE(VALID_NMSOCK(sock));
@@ -3467,25 +3486,26 @@ isc_nm_sequential(isc_nmhandle_t *handle) {
 
 void
 isc_nm_bad_request(isc_nmhandle_t *handle) {
-	isc_nmsocket_t *sock;
+	isc_nmsocket_t *sock = NULL;
 
 	REQUIRE(VALID_NMHANDLE(handle));
 	REQUIRE(VALID_NMSOCK(handle->sock));
 
 	sock = handle->sock;
+
 	switch (sock->type) {
+	case isc_nm_udpsocket:
+		return;
+	case isc_nm_tcpdnssocket:
+	case isc_nm_tlsdnssocket:
+		REQUIRE(sock->parent == NULL);
+		isc__nmsocket_reset(sock);
+		return;
 #if HAVE_LIBNGHTTP2
 	case isc_nm_httpsocket:
 		isc__nm_http_bad_request(handle);
-		break;
-#endif /* HAVE_LIBNGHTTP2 */
-
-	case isc_nm_udpsocket:
-	case isc_nm_tcpdnssocket:
-	case isc_nm_tlsdnssocket:
 		return;
-		break;
-
+#endif /* HAVE_LIBNGHTTP2 */
 	case isc_nm_tcpsocket:
 #if HAVE_LIBNGHTTP2
 	case isc_nm_tlssocket:

lib/ns/client.c

@@ -1822,6 +1822,9 @@ ns__client_request(isc_nmhandle_t *handle, isc_result_t eresult,
 		 * There isn't enough header to determine whether
 		 * this was a request or a response.  Drop it.
 		 */
+		ns_client_log(client, DNS_LOGCATEGORY_SECURITY,
+			      NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(10),
+			      "dropped request: invalid message header");
 		isc_nm_bad_request(handle);
 		return;
 	}
@@ -1838,7 +1841,9 @@ ns__client_request(isc_nmhandle_t *handle, isc_result_t eresult,
 	 * If it's a TCP response, discard it here.
 	 */
 	if ((flags & DNS_MESSAGEFLAG_QR) != 0) {
-		CTRACE("unexpected response");
+		ns_client_log(client, DNS_LOGCATEGORY_SECURITY,
+			      NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(10),
+			      "dropped request: unexpected response");
 		isc_nm_bad_request(handle);
 		return;
 	}