diff --git a/CHANGES b/CHANGES index 09abb7e88b..8587a64ab9 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,8 @@ +6399. [security] Malicious DNS client that sends many queries over + TCP but never reads responses can cause server to + respond slowly or not respond at all for other + clients. (CVE-2024-0760) [GL #4481] + 6398. [bug] Fix potential data races in our DoH implementation related to HTTP/2 session object management and endpoints set object management after reconfiguration. diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 38e66417b7..58fe4ad959 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -15,6 +15,10 @@ Notes for BIND 9.18.28 Security Fixes ~~~~~~~~~~~~~~ +- Malicious DNS client that sends many queries over TCP but never reads + responses can cause server to respond slowly or not respond at all for other + clients. :cve:`2024-0760` :gl:`#4481` + - Named could trigger an assertion failure when looking up the NS records of parent zones as part of looking up DS records. This has been fixed. :gl:`#4661` diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index f95a51173e..4ceb182e7b 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -62,9 +62,10 @@ #endif /* - * The TCP receive buffer can fit one maximum sized DNS message plus its size, - * the receive buffer here affects TCP, DoT and DoH. + * The TCP send and receive buffers can fit one maximum sized DNS message plus + * its size, the receive buffer here affects TCP, DoT and DoH. */ +#define ISC_NETMGR_TCP_SENDBUF_SIZE (sizeof(uint16_t) + UINT16_MAX) #define ISC_NETMGR_TCP_RECVBUF_SIZE (sizeof(uint16_t) + UINT16_MAX) /* Pick the larger buffer */ @@ -2261,6 +2262,14 @@ isc__nmsocket_readtimeout_cb(uv_timer_t *timer); void isc__nmsocket_writetimeout_cb(void *data, isc_result_t eresult); +/*%< + * + * Maximum number of simultaneous handles in flight supported for a single + * connected TCPDNS socket. This value was chosen arbitrarily, and may be + * changed in the future. + */ +#define STREAM_CLIENTS_PER_CONN 23 + #define UV_RUNTIME_CHECK(func, ret) \ if (ret != 0) { \ FATAL_ERROR("%s failed: %s\n", #func, uv_strerror(ret)); \ diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index 820099553d..336cad4e23 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -2086,6 +2086,7 @@ isc__nmsocket_writetimeout_cb(void *data, isc_result_t eresult) { sock = req->sock; + isc__nm_start_reading(sock); isc__nmsocket_reset(sock); } @@ -2342,8 +2343,10 @@ processbuffer(isc_nmsocket_t *sock) { * timers. If we do have a full message, reset the timer. * * Stop reading if this is a client socket, or if the server socket - * has been set to sequential mode. In this case we'll be called again - * later by isc__nm_resume_processing(). + * has been set to sequential mode, or the number of queries we are + * processing simultaneously has reached the clients-per-connection + * limit. In this case we'll be called again later by + * isc__nm_resume_processing(). */ isc_result_t isc__nm_process_sock_buffer(isc_nmsocket_t *sock) { @@ -2383,7 +2386,8 @@ isc__nm_process_sock_buffer(isc_nmsocket_t *sock) { isc__nmsocket_timer_stop(sock); if (atomic_load(&sock->client) || - atomic_load(&sock->sequential)) + atomic_load(&sock->sequential) || + atomic_load(&sock->ah) >= STREAM_CLIENTS_PER_CONN) { isc__nm_stop_reading(sock); goto done; diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index 16b53cc579..016a9e9059 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -905,6 +905,31 @@ isc__nm_tcp_read_cb(uv_stream_t *stream, ssize_t nread, const uv_buf_t *buf) { /* The readcb could have paused the reading */ if (atomic_load(&sock->reading)) { + if (!sock->client) { + /* + * Stop reading if we have accumulated enough bytes in + * the send queue; this means that the TCP client is not + * reading back the data we sending to it, and there's + * no reason to continue processing more incoming DNS + * messages, if the client is not reading back the + * responses. + */ + size_t write_queue_size = + uv_stream_get_write_queue_size( + &sock->uv_handle.stream); + + if (write_queue_size >= ISC_NETMGR_TCP_SENDBUF_SIZE) { + isc_log_write(isc_lctx, ISC_LOGCATEGORY_GENERAL, + ISC_LOGMODULE_NETMGR, + ISC_LOG_DEBUG(3), + "throttling TCP connection, " + "the other side is " + "not reading the data (%zu)", + write_queue_size); + isc__nm_stop_reading(sock); + } + } + /* The timer will be updated */ isc__nmsocket_timer_restart(sock); } @@ -1095,6 +1120,33 @@ isc__nm_tcp_send(isc_nmhandle_t *handle, const isc_region_t *region, return; } +static void +tcp_maybe_restart_reading(isc_nmsocket_t *sock) { + if (!sock->client && sock->reading && + !uv_is_active(&sock->uv_handle.handle)) + { + /* + * Restart reading if we have less data in the send queue than + * the send buffer size, this means that the TCP client has + * started reading some data again. Starting reading when we go + * under the limit instead of waiting for all data has been + * flushed allows faster recovery (in case there was a + * congestion and now there isn't). + */ + size_t write_queue_size = + uv_stream_get_write_queue_size(&sock->uv_handle.stream); + if (write_queue_size < ISC_NETMGR_TCP_SENDBUF_SIZE) { + isc_log_write( + isc_lctx, ISC_LOGCATEGORY_GENERAL, + ISC_LOGMODULE_NETMGR, ISC_LOG_DEBUG(3), + "resuming TCP connection, the other side " + "is reading the data again (%zu)", + write_queue_size); + isc__nm_start_reading(sock); + } + } +} + static void tcp_send_cb(uv_write_t *req, int status) { isc__nm_uvreq_t *uvreq = (isc__nm_uvreq_t *)req->data; @@ -1112,10 +1164,16 @@ tcp_send_cb(uv_write_t *req, int status) { isc__nm_incstats(sock, STATID_SENDFAIL); isc__nm_failed_send_cb(sock, uvreq, isc__nm_uverr2result(status)); + + if (!sock->client && sock->reading) { + isc__nm_start_reading(sock); + isc__nmsocket_reset(sock); + } return; } isc__nm_sendcb(sock, uvreq, ISC_R_SUCCESS, false); + tcp_maybe_restart_reading(sock); } /* diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c index dd3e9ec3fe..00ecb0f3d2 100644 --- a/lib/isc/netmgr/tcpdns.c +++ b/lib/isc/netmgr/tcpdns.c @@ -913,6 +913,27 @@ isc__nm_tcpdns_read_cb(uv_stream_t *stream, ssize_t nread, result = isc__nm_process_sock_buffer(sock); if (result != ISC_R_SUCCESS) { isc__nm_failed_read_cb(sock, result, true); + } else if (!sock->client) { + /* + * Stop reading if we have accumulated enough bytes in + * the send queue; this means that the TCP client is not + * reading back the data we sending to it, and there's + * no reason to continue processing more incoming DNS + * messages, if the client is not reading back the + * responses. + */ + size_t write_queue_size = + uv_stream_get_write_queue_size(&sock->uv_handle.stream); + + if (write_queue_size >= ISC_NETMGR_TCP_SENDBUF_SIZE) { + isc_log_write(isc_lctx, ISC_LOGCATEGORY_GENERAL, + ISC_LOGMODULE_NETMGR, ISC_LOG_DEBUG(3), + "throttling TCP connection, " + "the other side is " + "not reading the data (%zu)", + write_queue_size); + isc__nm_stop_reading(sock); + } } free: if (nread < 0) { @@ -1135,6 +1156,33 @@ isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region, return; } +static void +tcpdns_maybe_restart_reading(isc_nmsocket_t *sock) { + if (!sock->client && sock->reading && + !uv_is_active(&sock->uv_handle.handle)) + { + /* + * Restart reading if we have less data in the send queue than + * the send buffer size, this means that the TCP client has + * started reading some data again. Starting reading when we go + * under the limit instead of waiting for all data has been + * flushed allows faster recovery (in case there was a + * congestion and now there isn't). + */ + size_t write_queue_size = + uv_stream_get_write_queue_size(&sock->uv_handle.stream); + if (write_queue_size < ISC_NETMGR_TCP_SENDBUF_SIZE) { + isc_log_write( + isc_lctx, ISC_LOGCATEGORY_GENERAL, + ISC_LOGMODULE_NETMGR, ISC_LOG_DEBUG(3), + "resuming TCP connection, the other side " + "is reading the data again (%zu)", + write_queue_size); + isc__nm_start_reading(sock); + } + } +} + static void tcpdns_send_cb(uv_write_t *req, int status) { isc__nm_uvreq_t *uvreq = (isc__nm_uvreq_t *)req->data; @@ -1152,10 +1200,16 @@ tcpdns_send_cb(uv_write_t *req, int status) { isc__nm_incstats(sock, STATID_SENDFAIL); isc__nm_failed_send_cb(sock, uvreq, isc__nm_uverr2result(status)); + + if (!sock->client && sock->reading) { + isc__nm_start_reading(sock); + isc__nmsocket_reset(sock); + } return; } isc__nm_sendcb(sock, uvreq, ISC_R_SUCCESS, false); + tcpdns_maybe_restart_reading(sock); } /* diff --git a/lib/ns/client.c b/lib/ns/client.c index a62343bc7b..590859f020 100644 --- a/lib/ns/client.c +++ b/lib/ns/client.c @@ -101,6 +101,9 @@ #define COOKIE_SIZE 24U /* 8 + 4 + 4 + 8 */ #define ECS_SIZE 20U /* 2 + 1 + 1 + [0..16] */ +#define TCPBUFFERS_FILLCOUNT 1U +#define TCPBUFFERS_FREEMAX 8U + #define WANTNSID(x) (((x)->attributes & NS_CLIENTATTR_WANTNSID) != 0) #define WANTEXPIRE(x) (((x)->attributes & NS_CLIENTATTR_WANTEXPIRE) != 0) #define WANTPAD(x) (((x)->attributes & NS_CLIENTATTR_WANTPAD) != 0) @@ -336,6 +339,29 @@ client_senddone(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { isc_nmhandle_detach(&handle); } +static void +client_setup_tcp_buffer(ns_client_t *client) { + REQUIRE(client->tcpbuf == NULL); + + client->tcpbuf = client->manager->tcp_buffer; + client->tcpbuf_size = NS_CLIENT_TCP_BUFFER_SIZE; +} + +static void +client_put_tcp_buffer(ns_client_t *client) { + if (client->tcpbuf == NULL) { + return; + } + + if (client->tcpbuf != client->manager->tcp_buffer) { + isc_mem_put(client->manager->mctx, client->tcpbuf, + client->tcpbuf_size); + } + + client->tcpbuf = NULL; + client->tcpbuf_size = 0; +} + static void client_allocsendbuf(ns_client_t *client, isc_buffer_t *buffer, unsigned char **datap) { @@ -345,12 +371,9 @@ client_allocsendbuf(ns_client_t *client, isc_buffer_t *buffer, REQUIRE(datap != NULL); if (TCP_CLIENT(client)) { - INSIST(client->tcpbuf == NULL); - client->tcpbuf = isc_mem_get(client->manager->send_mctx, - NS_CLIENT_TCP_BUFFER_SIZE); - client->tcpbuf_size = NS_CLIENT_TCP_BUFFER_SIZE; + client_setup_tcp_buffer(client); data = client->tcpbuf; - isc_buffer_init(buffer, data, NS_CLIENT_TCP_BUFFER_SIZE); + isc_buffer_init(buffer, data, client->tcpbuf_size); } else { data = client->sendbuf; if ((client->attributes & NS_CLIENTATTR_HAVECOOKIE) == 0) { @@ -383,11 +406,49 @@ client_sendpkg(ns_client_t *client, isc_buffer_t *buffer) { if (isc_buffer_base(buffer) == client->tcpbuf) { size_t used = isc_buffer_usedlength(buffer); - client->tcpbuf = isc_mem_reget(client->manager->send_mctx, - client->tcpbuf, - client->tcpbuf_size, used); - client->tcpbuf_size = used; - r.base = client->tcpbuf; + INSIST(client->tcpbuf_size == NS_CLIENT_TCP_BUFFER_SIZE); + + /* + * Copy the data into a smaller buffer before sending, + * and keep the original big TCP send buffer for reuse + * by other clients. + */ + if (used > NS_CLIENT_SEND_BUFFER_SIZE) { + /* + * We can save space by allocating a new buffer with a + * correct size and freeing the big buffer. + */ + unsigned char *new_tcpbuf = + isc_mem_get(client->manager->mctx, used); + memmove(new_tcpbuf, buffer->base, used); + + /* + * Put the big buffer so we can replace the pointer + * and the size with the new ones. + */ + client_put_tcp_buffer(client); + + /* + * Keep the new buffer's information so it can be freed. + */ + client->tcpbuf = new_tcpbuf; + client->tcpbuf_size = used; + + r.base = new_tcpbuf; + } else { + /* + * The data fits in the available space in + * 'sendbuf', there is no need for a new buffer. + */ + memmove(client->sendbuf, buffer->base, used); + + /* + * Put the big buffer, we don't need a dynamic buffer. + */ + client_put_tcp_buffer(client); + + r.base = client->sendbuf; + } r.length = used; } else { isc_buffer_usedregion(buffer, &r); @@ -461,8 +522,7 @@ ns_client_sendraw(ns_client_t *client, dns_message_t *message) { return; done: if (client->tcpbuf != NULL) { - isc_mem_put(client->manager->send_mctx, client->tcpbuf, - client->tcpbuf_size); + client_put_tcp_buffer(client); } ns_client_drop(client, result); @@ -746,8 +806,7 @@ renderend: cleanup: if (client->tcpbuf != NULL) { - isc_mem_put(client->manager->send_mctx, client->tcpbuf, - client->tcpbuf_size); + client_put_tcp_buffer(client); } if (cleanup_cctx) { @@ -1629,8 +1688,7 @@ ns__client_reset_cb(void *client0) { ns_client_endrequest(client); if (client->tcpbuf != NULL) { - isc_mem_put(client->manager->send_mctx, client->tcpbuf, - client->tcpbuf_size); + client_put_tcp_buffer(client); } if (client->keytag != NULL) { @@ -1661,8 +1719,6 @@ ns__client_put_cb(void *client0) { client->magic = 0; client->shuttingdown = true; - isc_mem_put(client->manager->send_mctx, client->sendbuf, - NS_CLIENT_SEND_BUFFER_SIZE); if (client->opt != NULL) { INSIST(dns_rdataset_isassociated(client->opt)); dns_rdataset_disassociate(client->opt); @@ -2339,8 +2395,6 @@ ns__client_setup(ns_client_t *client, ns_clientmgr_t *mgr, bool new) { dns_message_create(client->mctx, DNS_MESSAGE_INTENTPARSE, &client->message); - client->sendbuf = isc_mem_get(client->manager->send_mctx, - NS_CLIENT_SEND_BUFFER_SIZE); /* * Set magic earlier than usual because ns_query_init() * and the functions it calls will require it. @@ -2357,7 +2411,6 @@ ns__client_setup(ns_client_t *client, ns_clientmgr_t *mgr, bool new) { ns_clientmgr_t *oldmgr = client->manager; ns_server_t *sctx = client->sctx; isc_task_t *task = client->task; - unsigned char *sendbuf = client->sendbuf; dns_message_t *message = client->message; isc_mem_t *oldmctx = client->mctx; ns_query_t query = client->query; @@ -2372,7 +2425,6 @@ ns__client_setup(ns_client_t *client, ns_clientmgr_t *mgr, bool new) { .manager = oldmgr, .sctx = sctx, .task = task, - .sendbuf = sendbuf, .message = message, .query = query, .tid = tid }; @@ -2397,8 +2449,6 @@ ns__client_setup(ns_client_t *client, ns_clientmgr_t *mgr, bool new) { return (ISC_R_SUCCESS); cleanup: - isc_mem_put(client->manager->send_mctx, client->sendbuf, - NS_CLIENT_SEND_BUFFER_SIZE); dns_message_detach(&client->message); isc_task_detach(&client->task); ns_clientmgr_detach(&client->manager); @@ -2461,8 +2511,6 @@ clientmgr_destroy(ns_clientmgr_t *manager) { isc_task_detach(&manager->task); ns_server_detach(&manager->sctx); - isc_mem_detach(&manager->send_mctx); - isc_mem_putanddetach(&manager->mctx, manager, sizeof(*manager)); } @@ -2499,61 +2547,6 @@ ns_clientmgr_create(ns_server_t *sctx, isc_taskmgr_t *taskmgr, ISC_LIST_INIT(manager->recursing); - /* - * We create specialised per-worker memory context specifically - * dedicated and tuned for allocating send buffers as it is a very - * common operation. Not doing so may result in excessive memory - * use in certain workloads. - * - * Please see this thread for more details: - * - * https://github.com/jemalloc/jemalloc/issues/2483 - * - * In particular, this information from the jemalloc developers is - * of the most interest: - * - * https://github.com/jemalloc/jemalloc/issues/2483#issuecomment-1639019699 - * https://github.com/jemalloc/jemalloc/issues/2483#issuecomment-1698173849 - * - * In essence, we use the following memory management strategy: - * - * 1. We use a per-worker memory arena for send buffers memory - * allocation to reduce lock contention (In reality, we create a - * per-client manager arena, but we have one client manager per - * worker). - * - * 2. The automatically created arenas settings remain unchanged - * and may be controlled by users (e.g. by setting the - * "MALLOC_CONF" variable). - * - * 3. We attune the arenas to not use dirty pages cache as the - * cache would have a poor reuse rate, and that is known to - * significantly contribute to excessive memory use. - * - * 4. There is no strict need for the dirty cache, as there is a - * per arena bin for each allocation size, so because we initially - * allocate strictly 64K per send buffer (enough for a DNS - * message), allocations would get directed to one bin (an "object - * pool" or a "slab") maintained within an arena. That is, there - * is an object pool already, specifically to optimise for the - * case of frequent allocations of objects of the given size. The - * object pool should suffice our needs, as we will end up - * recycling the objects from there without the need to back it by - * an additional layer of dirty pages cache. The dirty pages cache - * would have worked better in the case when there are more - * allocation bins involved due to a higher reuse rate (the case - * of a more "generic" memory management). - */ - isc_mem_create_arena(&manager->send_mctx); - isc_mem_setname(manager->send_mctx, "sendbufs"); - (void)isc_mem_arena_set_dirty_decay_ms(manager->send_mctx, 0); - /* - * Disable muzzy pages cache too, as versions < 5.2.0 have it - * enabled by default. The muzzy pages cache goes right below the - * dirty pages cache and backs it. - */ - (void)isc_mem_arena_set_muzzy_decay_ms(manager->send_mctx, 0); - manager->magic = MANAGER_MAGIC; MTRACE("create"); diff --git a/lib/ns/include/ns/client.h b/lib/ns/include/ns/client.h index 7a7196f763..ea2d83e079 100644 --- a/lib/ns/include/ns/client.h +++ b/lib/ns/include/ns/client.h @@ -144,7 +144,6 @@ struct ns_clientmgr { unsigned int magic; isc_mem_t *mctx; - isc_mem_t *send_mctx; ns_server_t *sctx; isc_taskmgr_t *taskmgr; isc_timermgr_t *timermgr; @@ -159,6 +158,8 @@ struct ns_clientmgr { /* Lock covers the recursing list */ isc_mutex_t reclock; client_list_t recursing; /*%< Recursing clients */ + + uint8_t tcp_buffer[NS_CLIENT_TCP_BUFFER_SIZE]; }; /*% nameserver client structure */ @@ -187,7 +188,6 @@ struct ns_client { unsigned char *tcpbuf; size_t tcpbuf_size; dns_message_t *message; - unsigned char *sendbuf; dns_rdataset_t *opt; dns_ednsopt_t *ede; uint16_t udpsize; @@ -240,6 +240,8 @@ struct ns_client { * bits will be used as the rcode in the response message. */ int32_t rcode_override; + + uint8_t sendbuf[NS_CLIENT_SEND_BUFFER_SIZE]; }; #define NS_CLIENT_MAGIC ISC_MAGIC('N', 'S', 'C', 'c')