From 295b1043dda42d3e5461ea2d58d1e4ea50ae7398 Mon Sep 17 00:00:00 2001 From: William Lallemand Date: Wed, 17 Nov 2021 02:59:21 +0100 Subject: [PATCH] BUG/MEDIUM: ssl: backend TLS resumption with sni and TLSv1.3 When establishing an outboud connection, haproxy checks if the cached TLS session has the same SNI as the connection we are trying to resume. This test was done by calling SSL_get_servername() which in TLSv1.2 returned the SNI. With TLSv1.3 this is not the case anymore and this function returns NULL, which invalidates any outboud connection we are trying to resume if it uses the sni keyword on its server line. This patch fixes the problem by storing the SNI in the "reused_sess" structure beside the session itself. The ssl_sock_set_servername() now has a RWLOCK because this session cache entry could be accessed by the CLI when trying to update a certificate on the backend. This fix must be backported in every maintained version, however the RWLOCK only exists since version 2.4. (cherry picked from commit e18d4e82862eb5c9b54b89f85d4c3b7c82d7395e) Signed-off-by: William Lallemand (cherry picked from commit c78ac5ade6bdaf46634edf08fecd735ab7d0a5f1) [wla: ha_free was replaced by free+NULL, some context changed, there is no server side certificate change on the CLI in 2.3, so the backend cache is not flushed] Signed-off-by: William Lallemand --- include/haproxy/server-t.h | 1 + src/ssl_sock.c | 33 ++++++++++++++++++++++++++------- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/include/haproxy/server-t.h b/include/haproxy/server-t.h index 3a95a74..8660021 100644 --- a/include/haproxy/server-t.h +++ b/include/haproxy/server-t.h @@ -306,6 +306,7 @@ struct server { unsigned char *ptr; int size; int allocated_size; + char *sni; /* SNI used for the session */ } * reused_sess; char *ciphers; /* cipher suite to use if non-null */ #if (HA_OPENSSL_VERSION_NUMBER >= 0x10101000L && !defined OPENSSL_IS_BORINGSSL && !defined LIBRESSL_VERSION_NUMBER) diff --git a/src/ssl_sock.c b/src/ssl_sock.c index e5f6029..0894a89 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -3758,8 +3758,10 @@ static int ssl_sess_new_srv_cb(SSL *ssl, SSL_SESSION *sess) if (!(s->ssl_ctx.options & SRV_SSL_O_NO_REUSE)) { int len; unsigned char *ptr; + const char *sni; len = i2d_SSL_SESSION(sess, NULL); + sni = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name); if (s->ssl_ctx.reused_sess[tid].ptr && s->ssl_ctx.reused_sess[tid].allocated_size >= len) { ptr = s->ssl_ctx.reused_sess[tid].ptr; } else { @@ -3771,6 +3773,19 @@ static int ssl_sess_new_srv_cb(SSL *ssl, SSL_SESSION *sess) s->ssl_ctx.reused_sess[tid].size = i2d_SSL_SESSION(sess, &ptr); } + + if (s->ssl_ctx.reused_sess[tid].sni) { + /* if the new sni is empty or isn' t the same as the old one */ + if ((!sni) || strcmp(s->ssl_ctx.reused_sess[tid].sni, sni) != 0) { + free(s->ssl_ctx.reused_sess[tid].sni); + s->ssl_ctx.reused_sess[tid].sni = NULL; + if (sni) + s->ssl_ctx.reused_sess[tid].sni = strdup(sni); + } + } else if (sni) { + /* if there wasn't an old sni but there is a new one */ + s->ssl_ctx.reused_sess[tid].sni = strdup(sni); + } } else { free(s->ssl_ctx.reused_sess[tid].ptr); s->ssl_ctx.reused_sess[tid].ptr = NULL; @@ -4754,8 +4769,10 @@ void ssl_sock_free_srv_ctx(struct server *srv) if (srv->ssl_ctx.reused_sess) { int i; - for (i = 0; i < global.nbthread; i++) + for (i = 0; i < global.nbthread; i++) { free(srv->ssl_ctx.reused_sess[i].ptr); + free(srv->ssl_ctx.reused_sess[i].sni); + } free(srv->ssl_ctx.reused_sess); } @@ -6079,19 +6096,21 @@ void ssl_sock_set_servername(struct connection *conn, const char *hostname) { #ifdef SSL_CTRL_SET_TLSEXT_HOSTNAME struct ssl_sock_ctx *ctx; - + struct server *s; char *prev_name; if (!ssl_sock_is_ssl(conn)) return; ctx = conn->xprt_ctx; + s = __objt_server(conn->target); /* if the SNI changes, we must destroy the reusable context so that a - * new connection will present a new SNI. As an optimization we could - * later imagine having a small cache of ssl_ctx to hold a few SNI per - * server. - */ - prev_name = (char *)SSL_get_servername(ctx->ssl, TLSEXT_NAMETYPE_host_name); + * new connection will present a new SNI. compare with the SNI + * previously stored in the reused_sess */ + /* the RWLOCK is used to ensure that we are not trying to flush the + * cache from the CLI */ + + prev_name = s->ssl_ctx.reused_sess[tid].sni; if ((!prev_name && hostname) || (prev_name && (!hostname || strcmp(hostname, prev_name) != 0))) SSL_set_session(ctx->ssl, NULL); -- 1.7.10.4