From 4716c56ebb4e537845fb4753cba004f5b01923c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 15 Feb 2022 14:41:15 +0100 Subject: [PATCH 1/3] Reset the TCP connection when garbage is received When invalid DNS message is received, there was a handling mechanism for DoH that would be called to return proper HTTP response. Reuse this mechanism and reset the TCP connection when the client is blackholed, DNS message is completely bogus or the ns_client receives response instead of query. --- lib/isc/netmgr/netmgr-int.h | 6 +++++ lib/isc/netmgr/netmgr.c | 54 +++++++++++++++++++++++++------------ lib/ns/client.c | 7 ++++- 3 files changed, 49 insertions(+), 18 deletions(-) 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; } From ebfdb50ac78c40323b2adbd0a9cbbf256e199b8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 15 Feb 2022 21:01:25 +0100 Subject: [PATCH 2/3] Add TCP garbage system test Test if the TCP connection gets reset when garbage instead of DNS message is sent. I'm only happy when it rains Pour some misery down on me - Garbage --- bin/tests/system/tcp/clean.sh | 8 +-- bin/tests/system/tcp/conftest.py | 59 ++++++++++++++++ bin/tests/system/tcp/ns7/named.conf.in | 41 +++++++++++ bin/tests/system/tcp/ns7/root.db | 24 +++++++ bin/tests/system/tcp/setup.sh | 1 + bin/tests/system/tcp/tests-tcp.py | 94 ++++++++++++++++++++++++++ 6 files changed, 223 insertions(+), 4 deletions(-) create mode 100644 bin/tests/system/tcp/conftest.py create mode 100644 bin/tests/system/tcp/ns7/named.conf.in create mode 100644 bin/tests/system/tcp/ns7/root.db create mode 100644 bin/tests/system/tcp/tests-tcp.py 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 From 9f1c439335bf8054e2dfc940a305b3fe34661fb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 15 Feb 2022 21:06:18 +0100 Subject: [PATCH 3/3] Add CHANGES and release note for [GL #3149] --- CHANGES | 3 +++ doc/notes/notes-current.rst | 3 +++ 2 files changed, 6 insertions(+) diff --git a/CHANGES b/CHANGES index 91d3c9acf1..65a9f828cb 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/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`