diff --git a/CHANGES b/CHANGES index 5bc1050368..7c6bfed958 100644 --- a/CHANGES +++ b/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 diff --git a/bin/tests/system/tcp/clean.sh b/bin/tests/system/tcp/clean.sh index 850f5f9b1c..1ea5b60d0f 100644 --- a/bin/tests/system/tcp/clean.sh +++ b/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* diff --git a/bin/tests/system/tcp/conftest.py b/bin/tests/system/tcp/conftest.py new file mode 100644 index 0000000000..0ce749b3fd --- /dev/null +++ b/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 diff --git a/bin/tests/system/tcp/ns7/named.conf.in b/bin/tests/system/tcp/ns7/named.conf.in new file mode 100644 index 0000000000..bf434d9913 --- /dev/null +++ b/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"; +}; diff --git a/bin/tests/system/tcp/ns7/root.db b/bin/tests/system/tcp/ns7/root.db new file mode 100644 index 0000000000..bb31741496 --- /dev/null +++ b/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 diff --git a/bin/tests/system/tcp/setup.sh b/bin/tests/system/tcp/setup.sh index 124326f882..475f399048 100644 --- a/bin/tests/system/tcp/setup.sh +++ b/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 diff --git a/bin/tests/system/tcp/tests-tcp.py b/bin/tests/system/tcp/tests-tcp.py new file mode 100644 index 0000000000..7ff84f264b --- /dev/null +++ b/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 diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 18e67bd992..b712aca52e 100644 --- a/doc/notes/notes-current.rst +++ b/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` diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index 188d90340e..4f8db118ea 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/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); /*%< diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index ddcfe23276..409838befd 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/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: diff --git a/lib/ns/client.c b/lib/ns/client.c index e48eb56322..214c83fedf 100644 --- a/lib/ns/client.c +++ b/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; }