From 26e74f140151d7c1152d3e162dbdb95857a8aac5 Mon Sep 17 00:00:00 2001 From: Matt Suiche Date: Tue, 28 Jul 2026 15:51:09 +0200 Subject: [PATCH] BUG/MAJOR: ssl/ocsp: lock the OCSP response around reads in the stapling callback ssl_sock_ocsp_stapling_cbk() reads ocsp->response.area and ocsp->response.data without any lock, while ssl_sock_load_ocsp_response() -- called from the CLI "set ssl ocsp-response" handler, the ocsp-update task and the reload path -- frees and replaces that buffer via chunk_dup(), also without any synchronization: ssl_buf = OPENSSL_malloc(ocsp->response.data); ... memcpy(ssl_buf, ocsp->response.area, ocsp->response.data); SSL_set_tlsext_status_ocsp_resp(ssl, ssl_buf, ocsp->response.data); A concurrent update frees the old area between the allocation and the copy (heap-use-after-free read, confirmed under ASan as a READ of size 12548 in the callback's memcpy), or yields a torn read pairing the new larger length with the old smaller area (linear over-read). Because the copied bytes are handed to SSL_set_tlsext_status_ocsp_resp() and sent to the TLS client in the status_request extension, freed or reused heap contents can be disclosed to any remote client asking for a stapled response during an update window; updates are periodic by default when ocsp-update is enabled. A crash is the more likely practical outcome, but the disclosure variant makes this heartbleed-class. Let's take ocsp_tree_lock on both sides, which is the file's existing idiom for accessing OCSP response contents (see ssl_get_ocspresponse_detail()). The lock declaration is moved to the top of the !OPENSSL_NO_OCSP section so that the callback can use it. The critical section on the handshake path stays short (a validity check plus a copy of a few KB), and allocating memory under this spinlock is consistent with the existing "show ssl ocsp-response" handler, which base64 encodes the response into a growable chunk while holding the same lock. This must be backported to all supported versions. --- src/ssl_ocsp.c | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/src/ssl_ocsp.c b/src/ssl_ocsp.c index b665bd945..8079bcb23 100644 --- a/src/ssl_ocsp.c +++ b/src/ssl_ocsp.c @@ -89,6 +89,12 @@ static struct sockaddr_storage *ocsp_update_dst; #ifndef OPENSSL_NO_OCSP int ocsp_ex_index = -1; +/* Also protects the content of certificate_ocsp responses against + * concurrent updates (stapling callback vs response update), so it + * must exist whenever the stapling callback is built. + */ +__decl_thread(HA_SPINLOCK_T ocsp_tree_lock); + int ssl_sock_get_ocsp_arg_kt_index(int evp_keytype) { switch (evp_keytype) { @@ -119,6 +125,7 @@ int ssl_sock_ocsp_stapling_cbk(SSL *ssl, void *arg) EVP_PKEY *ssl_pkey; int key_type; int index; + int resp_len; TRACE_ENTER(SSL_EV_CONN_STAPLING, conn); @@ -173,26 +180,36 @@ int ssl_sock_ocsp_stapling_cbk(SSL *ssl, void *arg) } + /* The response can be updated concurrently (see + * ssl_sock_load_ocsp_response()), so its content must be accessed + * under the OCSP lock, like ssl_get_ocspresponse_detail() does. + */ + HA_SPIN_LOCK(OCSP_LOCK, &ocsp_tree_lock); if (!ocsp || !ocsp->response.area || !ocsp->response.data) { + HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock); TRACE_ERROR("Missing OCSP response", SSL_EV_CONN_STAPLING, conn, ssl); goto error; } if (ocsp->expire < date.tv_sec) { + HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock); TRACE_ERROR("Expired OCSP response", SSL_EV_CONN_STAPLING, conn, ssl); goto error; } ssl_buf = OPENSSL_malloc(ocsp->response.data); if (!ssl_buf) { + HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock); TRACE_ERROR("Allocation failure", SSL_EV_CONN_STAPLING, conn); goto error; } - memcpy(ssl_buf, ocsp->response.area, ocsp->response.data); - SSL_set_tlsext_status_ocsp_resp(ssl, (unsigned char*)ssl_buf, ocsp->response.data); + resp_len = ocsp->response.data; + memcpy(ssl_buf, ocsp->response.area, resp_len); + HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock); + SSL_set_tlsext_status_ocsp_resp(ssl, (unsigned char*)ssl_buf, resp_len); if (counters) { HA_ATOMIC_INC(&counters->ocsp_staple); @@ -223,8 +240,6 @@ error: struct eb_root cert_ocsp_tree = EB_ROOT_UNIQUE; -__decl_thread(HA_SPINLOCK_T ocsp_tree_lock); - struct eb_root ocsp_update_tree = EB_ROOT; /* updatable ocsp responses sorted by next_update in absolute time */ /* @@ -379,15 +394,21 @@ int ssl_sock_load_ocsp_response(struct buffer *ocsp_response, HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock); } - /* According to comments on "chunk_dup", the - previous chunk buffer will be freed */ + /* The response can be read concurrently by the stapling callback + * (ssl_sock_ocsp_stapling_cbk()), so it must be swapped under the + * OCSP lock. According to comments on "chunk_dup", the previous + * chunk buffer will be freed. + */ + HA_SPIN_LOCK(OCSP_LOCK, &ocsp_tree_lock); if (!chunk_dup(&ocsp->response, ocsp_response)) { + HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock); memprintf(err, "OCSP response: Memory allocation error"); goto out; } #ifdef HAVE_ASN1_TIME_TO_TM if (ASN1_TIME_to_tm(nextupd, &nextupd_tm) == 0) { + HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock); memprintf(err, "OCSP single response: Invalid \"Next Update\" time"); goto out; } @@ -395,11 +416,13 @@ int ssl_sock_load_ocsp_response(struct buffer *ocsp_response, #else expire = asn1_generalizedtime_to_epoch(nextupd) - OCSP_MAX_RESPONSE_TIME_SKEW; if (expire < 0) { + HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock); memprintf(err, "OCSP single response: Invalid \"Next Update\" time"); goto out; } ocsp->expire = expire; #endif + HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock); if (ocsp->expire < date.tv_sec) { memprintf(err, "OCSP single response: no longer valid. Must be valid during at least %ds.", OCSP_MAX_RESPONSE_TIME_SKEW); -- 2.47.3