From b601a5b7818f6ccc44ed3c89b645686fc91c08d1 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. (cherry picked from commit 0cca550dff403c6100b7c0da8f252e7967765ba7) --- lib/isc/netmgr/http.c | 58 +++++++++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 24 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index eaa90c7094..1e3c00f804 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, @@ -971,25 +971,31 @@ static void http_readcb(isc_nmhandle_t *handle, 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)); UNUSED(handle); + /* + * 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) { @@ -1006,6 +1012,9 @@ http_readcb(isc_nmhandle_t *handle, isc_result_t result, isc_region_t *region, /* 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 @@ -1974,8 +1983,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]; @@ -1983,10 +1990,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_log_write(isc_lctx, ISC_LOGCATEGORY_GENERAL, ISC_LOGMODULE_NETMGR, log_level, "HTTP/2 request from %s (on %s) failed: %s %s", client_sabuf, local_sabuf, response->header.value, @@ -2025,17 +2030,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); } @@ -2056,8 +2058,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; @@ -2128,7 +2129,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); @@ -2318,9 +2319,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: @@ -2338,8 +2340,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: @@ -2784,7 +2785,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 @@ -2813,7 +2814,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); @@ -2901,6 +2903,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_socket_type(session->handle) == isc_nm_tlssocket); } @@ -2931,6 +2937,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 998522e68eaad29284a6be1fe8e051031db312a8 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. (cherry picked from commit b9b5d0c01a3a546c4a6a8b3bff8ae9dd31fee224) --- lib/isc/netmgr/http.c | 106 ++++++++++++------------------------ lib/isc/netmgr/netmgr-int.h | 11 +--- 2 files changed, 39 insertions(+), 78 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index 1e3c00f804..1d4b82fd0e 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); @@ -1659,6 +1662,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); socket->tid = session->handle->sock->tid; @@ -1670,21 +1676,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)); - - if (atomic_load(&serversocket->listening)) { - 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) { @@ -1709,9 +1700,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; @@ -2035,9 +2025,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); } @@ -2489,7 +2490,6 @@ isc_nm_listenhttp(isc_nm_t *mgr, isc_sockaddr_t *iface, int backlog, REQUIRE(VALID_NM(mgr)); REQUIRE(!ISC_LIST_EMPTY(eps->handlers)); - REQUIRE(!ISC_LIST_EMPTY(eps->handler_cbargs)); REQUIRE(atomic_load(&eps->in_use) == false); sock = isc_mem_get(mgr->mctx, sizeof(*sock)); @@ -2542,7 +2542,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); @@ -2556,7 +2555,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; @@ -2577,20 +2575,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)); @@ -2623,6 +2612,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; } } @@ -2630,63 +2621,34 @@ 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, const size_t extrahandlesize) { 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, + .cb = cb, + .cbarg = cbarg, + .path = isc_mem_strdup(mctx, uri), .extrahandlesize = extrahandlesize, - .path = isc_mem_strdup(mctx, uri) + .link = ISC_LINK_INITIALIZER, + .magic = HTTP_HANDLER_MAGIC }; - ISC_LINK_INIT(handler, link); - newhandler = true; - } - - if (newhandler) { ISC_LIST_APPEND(eps->handlers, handler, link); } - ISC_LIST_APPEND(eps->handler_cbargs, httpcbarg, link); + return (ISC_R_SUCCESS); } @@ -2779,8 +2741,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); @@ -3214,6 +3174,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->mgr->mctx, sock->h2.request_path); sock->h2.request_path = NULL; diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index da9e8c3a59..f95a51173e 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -883,13 +883,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; @@ -902,7 +897,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; @@ -914,7 +908,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; @@ -947,6 +940,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 8132f4c0206cf0af6a5525e79386ac3c42b7a25c 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. (cherry picked from commit 773a8108f307b4f6cc7776050d85432295b13a4d) --- CHANGES | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGES b/CHANGES index bb4ea1c5d8..09abb7e88b 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. [bug] Clear DNS_FETCHOPT_TRYSTALE_ONTIMEOUT when looking for parent NS records needed to get the DS result. [GL #4661] From a98607d2cefecb1993db12a4d9198bfdd4d6bc78 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 667bd2ab11..38e66417b7 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -37,6 +37,14 @@ Feature Changes Bug Fixes ~~~~~~~~~ +- 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. + - 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`