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.

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index d997775cc58e3fb12fde9a7f2372f9dff85ca7ad..16ca0b755f6b203fd1932bdbaf2ccd22a758a98c 100644 (file)
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -1938,7 +1938,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. */