From 7b390a7fb6ca10b60147b72acec0e951da251a7d Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 17 Jan 2024 15:40:56 +0200 Subject: [PATCH 1/3] Fix reading extra messages in TLS DNS in client mode When connecting to a remote party the TLS DNS code could process more than one message at a time despite the fact that it is expected that we should stop after every DNS message. Every DNS message is handled and consumed from the input buffer by isc__nm_process_sock_buffer(). However, as opposed to TCP DNS code, it can be called more than once when processing incoming data from a server (see tls_cycle_input()). That, in turn means that we can process more than one message at a time. Some higher level code might not expect that, as it breaks the contract. In particular, in the original report that happened during isc__nm_async_tlsdnsshutdown() call: when shutting down multiple calls to tls_cycle() are possible (each possibly leading to a isc__nm_process_sock_buffer()). If there are any non processed messages left, for any of the messages left the read callback will be called even when it is not expected as there were no preceding isc_nm_read(). To keep TCP DNS and TLS DNS code in sync, we make a similar change to it as well, although it should not matter. --- lib/isc/netmgr/tcpdns.c | 7 +++++++ lib/isc/netmgr/tlsdns.c | 7 +++++++ 2 files changed, 14 insertions(+) diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c index eda6aa62ce..cabc90533d 100644 --- a/lib/isc/netmgr/tcpdns.c +++ b/lib/isc/netmgr/tcpdns.c @@ -808,6 +808,13 @@ isc__nm_tcpdns_processbuffer(isc_nmsocket_t *sock) { return (ISC_R_CANCELED); } + if (sock->client && !sock->recv_read) { + /* + * We are not reading data - stop here. + */ + return (ISC_R_CANCELED); + } + req = isc__nm_get_read_req(sock, NULL); REQUIRE(VALID_UVREQ(req)); diff --git a/lib/isc/netmgr/tlsdns.c b/lib/isc/netmgr/tlsdns.c index d30e33fbfd..cfc62eb5e9 100644 --- a/lib/isc/netmgr/tlsdns.c +++ b/lib/isc/netmgr/tlsdns.c @@ -1016,6 +1016,13 @@ isc__nm_tlsdns_processbuffer(isc_nmsocket_t *sock) { return (ISC_R_CANCELED); } + if (sock->client && !sock->recv_read) { + /* + * We are not reading data - stop here. + */ + return (ISC_R_CANCELED); + } + req = isc__nm_get_read_req(sock, NULL); REQUIRE(VALID_UVREQ(req)); From a15c5b168766f5b6de956fed50d9c153eb94a1b9 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Thu, 18 Jan 2024 22:51:42 +0200 Subject: [PATCH 2/3] Add a unit test which would fail on excessive reads This commit adds a unit tests which would fail/crash/abort if excessive reads were possible. See [GL #4487] --- tests/isc/netmgr_test.c | 172 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 172 insertions(+) diff --git a/tests/isc/netmgr_test.c b/tests/isc/netmgr_test.c index f75207a223..49e5e01722 100644 --- a/tests/isc/netmgr_test.c +++ b/tests/isc/netmgr_test.c @@ -2401,6 +2401,176 @@ ISC_RUN_TEST_IMPL(tlsdns_recv_one) { atomic_assert_int_eq(ssends, 0); } +static void +tlsdns_many_listen_read_cb(isc_nmhandle_t *handle, isc_result_t eresult, + isc_region_t *region, void *cbarg) { + uint64_t magic = 0; + isc_nmhandle_t *sendhandle = NULL; + isc_buffer_t *send_data = (isc_buffer_t *)cbarg; + isc_region_t send_messages = { 0 }; + + assert_non_null(handle); + assert_non_null(send_data); + + F(); + + if (eresult != ISC_R_SUCCESS) { + goto unref; + } + + atomic_fetch_add(&sreads, 1); + + assert_true(region->length >= sizeof(magic)); + + memmove(&magic, region->base + sizeof(uint16_t), sizeof(magic)); + assert_true(magic == stop_magic || magic == send_magic); + + isc_nmhandle_attach(handle, &sendhandle); + isc_refcount_increment0(&active_ssends); + isc_nmhandle_setwritetimeout(sendhandle, T_IDLE); + /* send multiple DNS messages at once */ + isc_buffer_usedregion(send_data, &send_messages); + isc_nm_send(sendhandle, &send_messages, listen_send_cb, cbarg); +unref: + isc_refcount_decrement(&active_sreads); + isc_nmhandle_detach(&handle); +} + +static isc_result_t +tlsdns_many_listen_accept_cb(isc_nmhandle_t *handle, isc_result_t eresult, + void *cbarg) { + isc_nmhandle_t *readhandle = NULL; + + UNUSED(cbarg); + + F(); + + if (eresult != ISC_R_SUCCESS) { + return (eresult); + } + + atomic_fetch_add(&saccepts, 1); + + isc_refcount_increment0(&active_sreads); + isc_nmhandle_attach(handle, &readhandle); + isc_nm_read(handle, tlsdns_many_listen_read_cb, cbarg); + + return (ISC_R_SUCCESS); +} + +static void +tlsdns_many_connect_read_cb(isc_nmhandle_t *handle, isc_result_t eresult, + isc_region_t *region, void *cbarg) { + isc_nmhandle_t *sendhandle = NULL; + uint64_t magic = 0; + + UNUSED(cbarg); + + assert_non_null(handle); + + F(); + + if (eresult != ISC_R_SUCCESS) { + goto unref; + } + + assert_true(region->length >= sizeof(magic)); + + atomic_fetch_add(&creads, 1); + + memmove(&magic, region->base, sizeof(magic)); + + assert_true(magic == stop_magic || magic == send_magic); + + isc_refcount_increment0(&active_csends); + isc_nmhandle_attach(handle, &sendhandle); + isc_nmhandle_setwritetimeout(handle, T_IDLE); + /* + * At this point the read is completed, so we should stop that - + * but the sending code will make a cycling through input + * attempt. When not properly handled, this situation will cause + * excessive reads. + */ + isc_nm_send(sendhandle, &send_msg, connect_send_cb, NULL); + +unref: + isc_refcount_decrement(&active_creads); + isc_nmhandle_detach(&handle); +} + +/* + * A unit test *VERY* specific to #4487 - it would crash the unit test + * suite without the related fix due to excessive/unexpected reads. + * + * The intention behind the test is to (needlessly ;-)) prove that the + * author of the fix is not fantasising and excessive reads are + * possible in principle. Also, it proves that there is more than one + * way to do that. + * + * It is *not* reproducing the situation from the bug report 1:1, as + * it is impossible to understand what exactly was going on with this + * custom/proprietary server without having access to it (and even in + * that case the bug was hard to reproduce to the point, where the + * reporters considered it to be fixed for a while). There are far too + * many things a play. + */ +ISC_RUN_TEST_IMPL(tlsdns_server_send_many_recv_one) { + isc_result_t result = ISC_R_SUCCESS; + isc_nmsocket_t *listen_sock = NULL; + uint8_t buf[512]; + isc_buffer_t server_send_buf = { 0 }; + + isc_buffer_init(&server_send_buf, buf, sizeof(buf)); + + /* + * Prepare a buffer with three "DNS" messages which we will send + * at once (our code does not normally do that do that). + */ + isc_buffer_putuint16(&server_send_buf, (uint16_t)send_msg.length); + isc_buffer_putmem(&server_send_buf, send_msg.base, send_msg.length); + isc_buffer_putuint16(&server_send_buf, (uint16_t)send_msg.length); + isc_buffer_putmem(&server_send_buf, send_msg.base, send_msg.length); + isc_buffer_putuint16(&server_send_buf, (uint16_t)send_msg.length); + isc_buffer_putmem(&server_send_buf, send_msg.base, send_msg.length); + + atomic_store(&nsends, 1); + + result = isc_nm_listentls( + listen_nm, &tcp_listen_addr, tlsdns_many_listen_accept_cb, + &server_send_buf, 0, 0, NULL, tcp_listen_tlsctx, &listen_sock); + assert_int_equal(result, ISC_R_SUCCESS); + + connect_readcb = tlsdns_many_connect_read_cb; + isc_refcount_increment0(&active_cconnects); + isc_nm_tlsdnsconnect(connect_nm, &tcp_connect_addr, &tcp_listen_addr, + connect_connect_cb, NULL, T_CONNECT, 0, + tcp_connect_tlsctx, tcp_tlsctx_client_sess_cache); + + WAIT_FOR_EQ(cconnects, 1); + WAIT_FOR_LE(nsends, 0); + WAIT_FOR_EQ(csends, 2); + WAIT_FOR_EQ(sreads, 1); + WAIT_FOR_EQ(ssends, 1); + WAIT_FOR_EQ(creads, 1); + + isc_nm_stoplistening(listen_sock); + isc_nmsocket_close(&listen_sock); + assert_null(listen_sock); + isc__netmgr_shutdown(connect_nm); + + X(cconnects); + X(csends); + X(creads); + X(sreads); + X(ssends); + + atomic_assert_int_eq(cconnects, 1); + atomic_assert_int_eq(csends, 2); + atomic_assert_int_eq(creads, 1); + atomic_assert_int_eq(sreads, 1); + atomic_assert_int_eq(ssends, 1); +} + ISC_RUN_TEST_IMPL(tlsdns_recv_two) { isc_result_t result = ISC_R_SUCCESS; isc_nmsocket_t *listen_sock = NULL; @@ -2879,6 +3049,8 @@ ISC_TEST_ENTRY_CUSTOM(tls_half_recv_half_send_quota_sendback, setup_test, /* TLSDNS */ ISC_TEST_ENTRY_CUSTOM(tlsdns_recv_one, setup_test, teardown_test) +ISC_TEST_ENTRY_CUSTOM(tlsdns_server_send_many_recv_one, setup_test, + teardown_test) ISC_TEST_ENTRY_CUSTOM(tlsdns_recv_two, setup_test, teardown_test) ISC_TEST_ENTRY_CUSTOM(tlsdns_noop, setup_test, teardown_test) ISC_TEST_ENTRY_CUSTOM(tlsdns_noresponse, setup_test, teardown_test) From 6b9ccae5371be92f3e7e734a06e051853a1ec119 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 17 Jan 2024 16:01:08 +0200 Subject: [PATCH 3/3] Modify CHANGES [GL #4487] Mention that TLS DNS will not process more than one message at a time when that was not expected. --- CHANGES | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES b/CHANGES index b37eb34e0c..6bccc42646 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +6320. [bug] Under some circumstances, the DoT code in client + mode could process more than one message at a time when + that was not expected. That has been fixed. [GL #4487] + --- 9.18.22 released --- 6319. [func] Limit isc_task_send() overhead for RBTDB tree pruning.