From c41fb499b979aa82ed63ff59b4ccff8f462d1594 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Fri, 8 Dec 2023 14:26:46 +0200 Subject: [PATCH 1/4] DoH: avoid potential use after free for HTTP/2 session objects It was reported that HTTP/2 session might get closed or even deleted before all async. processing has been completed. This commit addresses that: now we are avoiding using the object when we do not need it or specifically check if the pointers used are not 'NULL' and by ensuring that there is at least one reference to the session object while we are doing incoming data processing. This commit makes the code more resilient to such issues in the future. --- lib/isc/netmgr/http.c | 59 +++++++++++++++++++++++++------------------ 1 file changed, 35 insertions(+), 24 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index a807124c1d..42e2a9fda1 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -221,8 +221,8 @@ call_pending_callbacks(isc__nm_http_pending_callbacks_t pending_callbacks, isc_result_t result); static void -server_call_cb(isc_nmsocket_t *socket, isc_nm_http_session_t *session, - const isc_result_t result, isc_region_t *data); +server_call_cb(isc_nmsocket_t *socket, const isc_result_t result, + isc_region_t *data); static isc_nm_httphandler_t * http_endpoints_find(const char *request_path, @@ -979,23 +979,30 @@ static void http_readcb(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result, isc_region_t *region, void *data) { isc_nm_http_session_t *session = (isc_nm_http_session_t *)data; + isc_nm_http_session_t *tmpsess = NULL; ssize_t readlen; REQUIRE(VALID_HTTP2_SESSION(session)); + /* + * Let's ensure that HTTP/2 session and its associated data will + * not go "out of scope" too early. + */ + isc__nm_httpsession_attach(session, &tmpsess); + if (result != ISC_R_SUCCESS) { if (result != ISC_R_TIMEDOUT) { session->reading = false; } failed_read_cb(result, session); - return; + goto done; } readlen = nghttp2_session_mem_recv(session->ngsession, region->base, region->length); if (readlen < 0) { failed_read_cb(ISC_R_UNEXPECTED, session); - return; + goto done; } if ((size_t)readlen < region->length) { @@ -1011,6 +1018,9 @@ http_readcb(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result, /* We might have something to receive or send, do IO */ http_do_bio(session, NULL, NULL, NULL); + +done: + isc__nm_httpsession_detach(&tmpsess); } static void @@ -2023,8 +2033,6 @@ static void log_server_error_response(const isc_nmsocket_t *socket, const struct http_error_responses *response) { const int log_level = ISC_LOG_DEBUG(1); - isc_sockaddr_t client_addr; - isc_sockaddr_t local_addr; char client_sabuf[ISC_SOCKADDR_FORMATSIZE]; char local_sabuf[ISC_SOCKADDR_FORMATSIZE]; @@ -2032,10 +2040,8 @@ log_server_error_response(const isc_nmsocket_t *socket, return; } - client_addr = isc_nmhandle_peeraddr(socket->h2->session->handle); - local_addr = isc_nmhandle_localaddr(socket->h2->session->handle); - isc_sockaddr_format(&client_addr, client_sabuf, sizeof(client_sabuf)); - isc_sockaddr_format(&local_addr, local_sabuf, sizeof(local_sabuf)); + isc_sockaddr_format(&socket->peer, client_sabuf, sizeof(client_sabuf)); + isc_sockaddr_format(&socket->iface, local_sabuf, sizeof(local_sabuf)); isc__nmsocket_log(socket, log_level, "HTTP/2 request from %s (on %s) failed: %s %s", client_sabuf, local_sabuf, response->header.value, @@ -2074,17 +2080,14 @@ server_send_error_response(const isc_http_error_responses_t error, } static void -server_call_cb(isc_nmsocket_t *socket, isc_nm_http_session_t *session, - const isc_result_t result, isc_region_t *data) { - isc_sockaddr_t addr; +server_call_cb(isc_nmsocket_t *socket, const isc_result_t result, + isc_region_t *data) { isc_nmhandle_t *handle = NULL; REQUIRE(VALID_NMSOCK(socket)); - REQUIRE(VALID_HTTP2_SESSION(session)); REQUIRE(socket->h2->cb != NULL); - addr = isc_nmhandle_peeraddr(session->handle); - handle = isc__nmhandle_get(socket, &addr, NULL); + handle = isc__nmhandle_get(socket, NULL, NULL); socket->h2->cb(handle, result, data, socket->h2->cbarg); isc_nmhandle_detach(&handle); } @@ -2105,8 +2108,7 @@ isc__nm_http_bad_request(isc_nmhandle_t *handle) { } static int -server_on_request_recv(nghttp2_session *ngsession, - isc_nm_http_session_t *session, isc_nmsocket_t *socket) { +server_on_request_recv(nghttp2_session *ngsession, isc_nmsocket_t *socket) { isc_result_t result; isc_http_error_responses_t code = ISC_HTTP_ERROR_SUCCESS; isc_region_t data; @@ -2177,7 +2179,7 @@ server_on_request_recv(nghttp2_session *ngsession, UNREACHABLE(); } - server_call_cb(socket, session, ISC_R_SUCCESS, &data); + server_call_cb(socket, ISC_R_SUCCESS, &data); return (0); @@ -2364,9 +2366,10 @@ isc__nm_http_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg) { static int server_on_frame_recv_callback(nghttp2_session *ngsession, const nghttp2_frame *frame, void *user_data) { - isc_nm_http_session_t *session = (isc_nm_http_session_t *)user_data; isc_nmsocket_t *socket = NULL; + UNUSED(user_data); + switch (frame->hd.type) { case NGHTTP2_DATA: case NGHTTP2_HEADERS: @@ -2387,8 +2390,7 @@ server_on_frame_recv_callback(nghttp2_session *ngsession, return (0); } - return (server_on_request_recv(ngsession, session, - socket)); + return (server_on_request_recv(ngsession, socket)); } break; default: @@ -2806,7 +2808,7 @@ failed_httpstream_read_cb(isc_nmsocket_t *sock, isc_result_t result, session->ngsession, NGHTTP2_FLAG_END_STREAM, sock->h2->stream_id, NGHTTP2_REFUSED_STREAM); isc_buffer_usedregion(&sock->h2->rbuf, &data); - server_call_cb(sock, session, result, &data); + server_call_cb(sock, result, &data); } static void @@ -2835,7 +2837,8 @@ client_call_failed_read_cb(isc_result_t result, } if (result != ISC_R_TIMEDOUT || cstream->read_cb == NULL || - !isc__nmsocket_timer_running(session->handle->sock)) + !(session->handle != NULL && + isc__nmsocket_timer_running(session->handle->sock))) { ISC_LIST_DEQUEUE(session->cstreams, cstream, link); put_http_cstream(session->mctx, cstream); @@ -2923,6 +2926,10 @@ isc__nm_http_has_encryption(const isc_nmhandle_t *handle) { INSIST(VALID_HTTP2_SESSION(session)); + if (session->handle == NULL) { + return (false); + } + return (isc_nm_has_encryption(session->handle)); } @@ -2953,6 +2960,10 @@ isc__nm_http_verify_tls_peer_result_string(const isc_nmhandle_t *handle) { INSIST(VALID_HTTP2_SESSION(session)); + if (session->handle == NULL) { + return (NULL); + } + return (isc_nm_verify_tls_peer_result_string(session->handle)); } From d80dfbf7457617c863703e071aac005f57371554 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 13 Mar 2024 18:04:46 +0200 Subject: [PATCH 2/4] Keep the endpoints set reference within an HTTP/2 socket This commit ensures that an HTTP endpoints set reference is stored in a socket object associated with an HTTP/2 stream instead of referencing the global set stored inside a listener. This helps to prevent an issue like follows: 1. BIND is configured to serve DoH clients; 2. A client is connected and one or more HTTP/2 stream is created. Internal pointers are now pointing to the data on the associated HTTP endpoints set; 3. BIND is reconfigured - the new endpoints set object is created and promoted to all listeners; 4. The old pointers to the HTTP endpoints set data are now invalid. Instead referencing a global object that is updated on re-configurations we now store a local reference which prevents the endpoints set objects to go out of scope prematurely. --- lib/isc/netmgr/http.c | 107 +++++++++++++----------------------- lib/isc/netmgr/netmgr-int.h | 11 +--- 2 files changed, 41 insertions(+), 77 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index 42e2a9fda1..4dbf6080df 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -182,6 +182,9 @@ typedef struct isc_http_send_req { #define HTTP_ENDPOINTS_MAGIC ISC_MAGIC('H', 'T', 'E', 'P') #define VALID_HTTP_ENDPOINTS(t) ISC_MAGIC_VALID(t, HTTP_ENDPOINTS_MAGIC) +#define HTTP_HANDLER_MAGIC ISC_MAGIC('H', 'T', 'H', 'L') +#define VALID_HTTP_HANDLER(t) ISC_MAGIC_VALID(t, HTTP_HANDLER_MAGIC) + static bool http_send_outgoing(isc_nm_http_session_t *session, isc_nmhandle_t *httphandle, isc_nm_cb_t cb, void *cbarg); @@ -1714,6 +1717,9 @@ server_on_begin_headers_callback(nghttp2_session *ngsession, }; isc_buffer_initnull(&socket->h2->rbuf); isc_buffer_initnull(&socket->h2->wbuf); + isc_nm_http_endpoints_attach( + http_get_listener_endpoints(session->serversocket, socket->tid), + &socket->h2->peer_endpoints); session->nsstreams++; isc__nm_httpsession_attach(session, &socket->h2->session); ISC_LIST_APPEND(session->sstreams, socket->h2, link); @@ -1723,19 +1729,6 @@ server_on_begin_headers_callback(nghttp2_session *ngsession, return (0); } -static isc_nm_httphandler_t * -find_server_request_handler(const char *request_path, - isc_nmsocket_t *serversocket, const int tid) { - isc_nm_httphandler_t *handler = NULL; - - REQUIRE(VALID_NMSOCK(serversocket)); - - handler = http_endpoints_find( - request_path, http_get_listener_endpoints(serversocket, tid)); - - return (handler); -} - static isc_http_error_responses_t server_handle_path_header(isc_nmsocket_t *socket, const uint8_t *value, const size_t valuelen) { @@ -1760,9 +1753,8 @@ server_handle_path_header(isc_nmsocket_t *socket, const uint8_t *value, return (ISC_HTTP_ERROR_BAD_REQUEST); } - handler = find_server_request_handler(socket->h2->request_path, - socket->h2->session->serversocket, - socket->tid); + handler = http_endpoints_find(socket->h2->request_path, + socket->h2->peer_endpoints); if (handler != NULL) { socket->h2->cb = handler->cb; socket->h2->cbarg = handler->cbarg; @@ -2085,9 +2077,20 @@ server_call_cb(isc_nmsocket_t *socket, const isc_result_t result, isc_nmhandle_t *handle = NULL; REQUIRE(VALID_NMSOCK(socket)); - REQUIRE(socket->h2->cb != NULL); + + /* + * In some cases the callback could not have been set (e.g. when + * the stream was closed prematurely (before processing its HTTP + * path). + */ + if (socket->h2->cb == NULL) { + return; + } handle = isc__nmhandle_get(socket, NULL, NULL); + if (result != ISC_R_SUCCESS) { + data = NULL; + } socket->h2->cb(handle, result, data, socket->h2->cbarg); isc_nmhandle_detach(&handle); } @@ -2505,7 +2508,6 @@ isc_nm_listenhttp(isc_nm_t *mgr, uint32_t workers, isc_sockaddr_t *iface, REQUIRE(VALID_NM(mgr)); REQUIRE(!ISC_LIST_EMPTY(eps->handlers)); - REQUIRE(!ISC_LIST_EMPTY(eps->handler_cbargs)); REQUIRE(atomic_load(&eps->in_use) == false); REQUIRE(isc_tid() == 0); @@ -2576,7 +2578,6 @@ isc_nm_http_endpoints_new(isc_mem_t *mctx) { *eps = (isc_nm_http_endpoints_t){ .mctx = NULL }; isc_mem_attach(mctx, &eps->mctx); - ISC_LIST_INIT(eps->handler_cbargs); ISC_LIST_INIT(eps->handlers); isc_refcount_init(&eps->references, 1); atomic_init(&eps->in_use, false); @@ -2590,7 +2591,6 @@ isc_nm_http_endpoints_detach(isc_nm_http_endpoints_t **restrict epsp) { isc_nm_http_endpoints_t *restrict eps; isc_mem_t *mctx; isc_nm_httphandler_t *handler = NULL; - isc_nm_httpcbarg_t *httpcbarg = NULL; REQUIRE(epsp != NULL); eps = *epsp; @@ -2611,20 +2611,11 @@ isc_nm_http_endpoints_detach(isc_nm_http_endpoints_t **restrict epsp) { next = ISC_LIST_NEXT(handler, link); ISC_LIST_DEQUEUE(eps->handlers, handler, link); isc_mem_free(mctx, handler->path); + handler->magic = 0; isc_mem_put(mctx, handler, sizeof(*handler)); handler = next; } - httpcbarg = ISC_LIST_HEAD(eps->handler_cbargs); - while (httpcbarg != NULL) { - isc_nm_httpcbarg_t *next = NULL; - - next = ISC_LIST_NEXT(httpcbarg, link); - ISC_LIST_DEQUEUE(eps->handler_cbargs, httpcbarg, link); - isc_mem_put(mctx, httpcbarg, sizeof(isc_nm_httpcbarg_t)); - httpcbarg = next; - } - eps->magic = 0; isc_mem_putanddetach(&mctx, eps, sizeof(*eps)); @@ -2657,6 +2648,8 @@ http_endpoints_find(const char *request_path, handler = ISC_LIST_NEXT(handler, link)) { if (!strcmp(request_path, handler->path)) { + INSIST(VALID_HTTP_HANDLER(handler)); + INSIST(handler->cb != NULL); break; } } @@ -2664,61 +2657,33 @@ http_endpoints_find(const char *request_path, return (handler); } -/* - * In DoH we just need to intercept the request - the response can be sent - * to the client code via the nmhandle directly as it's always just the - * http content. - */ -static void -http_callback(isc_nmhandle_t *handle, isc_result_t result, isc_region_t *data, - void *arg) { - isc_nm_httpcbarg_t *httpcbarg = arg; - - REQUIRE(VALID_NMHANDLE(handle)); - - if (result != ISC_R_SUCCESS) { - /* Shut down the client, then ourselves */ - httpcbarg->cb(handle, result, NULL, httpcbarg->cbarg); - /* XXXWPK FREE */ - return; - } - httpcbarg->cb(handle, result, data, httpcbarg->cbarg); -} - isc_result_t isc_nm_http_endpoints_add(isc_nm_http_endpoints_t *restrict eps, const char *uri, const isc_nm_recv_cb_t cb, void *cbarg) { isc_mem_t *mctx; isc_nm_httphandler_t *restrict handler = NULL; - isc_nm_httpcbarg_t *restrict httpcbarg = NULL; - bool newhandler = false; REQUIRE(VALID_HTTP_ENDPOINTS(eps)); REQUIRE(isc_nm_http_path_isvalid(uri)); + REQUIRE(cb != NULL); REQUIRE(atomic_load(&eps->in_use) == false); mctx = eps->mctx; - httpcbarg = isc_mem_get(mctx, sizeof(isc_nm_httpcbarg_t)); - *httpcbarg = (isc_nm_httpcbarg_t){ .cb = cb, .cbarg = cbarg }; - ISC_LINK_INIT(httpcbarg, link); - if (http_endpoints_find(uri, eps) == NULL) { handler = isc_mem_get(mctx, sizeof(*handler)); - *handler = (isc_nm_httphandler_t){ .cb = http_callback, - .cbarg = httpcbarg, - .path = isc_mem_strdup( - mctx, uri) }; - ISC_LINK_INIT(handler, link); + *handler = (isc_nm_httphandler_t){ + .cb = cb, + .cbarg = cbarg, + .path = isc_mem_strdup(mctx, uri), + .link = ISC_LINK_INITIALIZER, + .magic = HTTP_HANDLER_MAGIC + }; - newhandler = true; - } - - if (newhandler) { ISC_LIST_APPEND(eps->handlers, handler, link); } - ISC_LIST_APPEND(eps->handler_cbargs, httpcbarg, link); + return (ISC_R_SUCCESS); } @@ -2802,8 +2767,6 @@ failed_httpstream_read_cb(isc_nmsocket_t *sock, isc_result_t result, return; } - INSIST(sock->h2->cbarg != NULL); - (void)nghttp2_submit_rst_stream( session->ngsession, NGHTTP2_FLAG_END_STREAM, sock->h2->stream_id, NGHTTP2_REFUSED_STREAM); @@ -3252,6 +3215,12 @@ isc__nm_http_cleanup_data(isc_nmsocket_t *sock) { http_cleanup_listener_endpoints(sock); } + if (sock->type == isc_nm_httpsocket && + sock->h2->peer_endpoints != NULL) + { + isc_nm_http_endpoints_detach(&sock->h2->peer_endpoints); + } + if (sock->h2->request_path != NULL) { isc_mem_free(sock->worker->mctx, sock->h2->request_path); diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index 13485dfb58..56e5bef4a9 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -410,13 +410,8 @@ typedef enum isc_http_scheme_type { ISC_HTTP_SCHEME_UNSUPPORTED } isc_http_scheme_type_t; -typedef struct isc_nm_httpcbarg { - isc_nm_recv_cb_t cb; - void *cbarg; - LINK(struct isc_nm_httpcbarg) link; -} isc_nm_httpcbarg_t; - typedef struct isc_nm_httphandler { + int magic; char *path; isc_nm_recv_cb_t cb; void *cbarg; @@ -428,7 +423,6 @@ struct isc_nm_http_endpoints { isc_mem_t *mctx; ISC_LIST(isc_nm_httphandler_t) handlers; - ISC_LIST(isc_nm_httpcbarg_t) handler_cbargs; isc_refcount_t references; atomic_bool in_use; @@ -440,7 +434,6 @@ typedef struct isc_nmsocket_h2 { char *query_data; size_t query_data_len; bool query_too_large; - isc_nm_httphandler_t *handler; isc_buffer_t rbuf; isc_buffer_t wbuf; @@ -471,6 +464,8 @@ typedef struct isc_nmsocket_h2 { isc_nm_http_endpoints_t **listener_endpoints; size_t n_listener_endpoints; + isc_nm_http_endpoints_t *peer_endpoints; + bool response_submitted; struct { char *uri; From a51ffa58d79b5b08e8d63d7f079d59ed8fcbeeeb Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 20 Dec 2023 19:54:31 +0200 Subject: [PATCH 3/4] Modify CHANGES [GL #4473] Mention that an intermittent BIND process termination in DoH code has been fixed. --- CHANGES | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGES b/CHANGES index a846ad7f23..7f5b381442 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,9 @@ +6398. [bug] Fix potential data races in our DoH implementation + related to HTTP/2 session object management and + endpoints set object management after reconfiguration. + We would like to thank Dzintars and Ivo from nic.lv + for bringing this to our attention. [GL #4473] + 6397. [placeholder] 6396. [func] Outgoing zone transfers are no longer enabled by From cdb5ae35e80c5c05dc25b98c4ada413004c6a324 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 20 Dec 2023 19:58:49 +0200 Subject: [PATCH 4/4] Modify release notes [GL #4473] Mention that an intermittent BIND process termination in DoH code has been fixed. --- doc/notes/notes-current.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index ee6f2ef748..4a3db14ed4 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -42,6 +42,14 @@ Bug Fixes - An RPZ response's SOA record TTL was set to 1 instead of the SOA TTL, if ``add-soa`` was used. This has been fixed. :gl:`#3323` +- Potential data races were found in our DoH implementation related + to HTTP/2 session object management and endpoints set object + management after reconfiguration. These issues have been + fixed. :gl:`#4473` + + ISC would like to thank Dzintars and Ivo from nic.lv for bringing + this to our attention. + Known Issues ~~~~~~~~~~~~