From cec6b5c14777d85ece2117eb1b3a5227f9ca7fe3 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Thu, 9 Oct 2025 16:13:18 +0200 Subject: [PATCH] BUG/MEDIUM: ssl: take care of second client hello For a long time we've been observing some sporadic leaks of ssl-capture pool entries on haproxy.org without figuring exactly the root cause. All that was seen was that less calls to the free callback were made than calls to the hello parsing callback, and these were never reproduced locally. It recently turned out to be triggered by the presence of "curves" or "ecdhe" on the "bind" line. Captures have shown the presence of a second client hello, called "Change Cipher Client Hello" in wireshark traces, that calls the client hello callback again. That one wasn't prepared for being called twice per connection, so it allocates an ssl-capture entry and assigns it to the ex_data entry, possibly overwriting the previous one. In this case, the fix is super simple, just reuse the current ex_data if it exists, otherwise allocate a new one. This completely solves the problem. Other callbacks have been audited for the same issue and are not affected: ssl_ini_keylog() already performs this check and ignores subsequent calls, and other ones do not allocate data. This must be backported to all supported versions. (cherry picked from commit 336170007c280ac7e8c8edc413f10cbb78af0ec5) Signed-off-by: Willy Tarreau (cherry picked from commit c3974c9da408fe1d387ad333a32fdf062ceb41eb) Signed-off-by: Willy Tarreau (cherry picked from commit 1a9cddac05fe90d7299d2a84c00bae290dc14d42) Signed-off-by: Willy Tarreau --- src/ssl_sock.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/ssl_sock.c b/src/ssl_sock.c index b6a3d30..c54d9f8 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -1709,7 +1709,17 @@ static void ssl_sock_parse_clienthello(struct connection *conn, int write_p, int if (msg + rec_len > end || msg + rec_len < msg) return; - capture = pool_zalloc(pool_head_ssl_capture); + /* BEWARE below! one could believe that there's a single client hello + * per connection, but captures show that a second one may happen, + * logged as "Change Cipher Client Hello" in wireshark, that can be + * triggered for example by the presence of "curves" or "ecdhe" on the + * "bind" line. The core below MUST NOT assume that it's called for the + * first time and must first verify if the capture field had already + * been allocated before trying to allocate a new one. + */ + capture = SSL_get_ex_data(ssl, ssl_capture_ptr_index); + if (!capture) + capture = pool_zalloc(pool_head_ssl_capture); if (!capture) return; /* Compute the xxh64 of the ciphersuite. */ -- 1.7.10.4