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.

diff --git a/include/haproxy/server-t.h b/include/haproxy/server-t.h
index 579e3bbc06f93296102947260d1db7ae093a0f39..b17f0de46fdaf5ce7199da073249cbb906a0c479 100644 (file)
--- a/include/haproxy/server-t.h
+++ b/include/haproxy/server-t.h
@@ -342,6 +342,7 @@ struct server {
                        unsigned char *ptr;
                        int size;
                        int allocated_size;
+                       char *sni; /* SNI used for the session */
                } * reused_sess;
 
                struct ckch_inst *inst; /* Instance of the ckch_store in which the certificate was loaded (might be null if server has no certificate) */

diff --git a/src/ssl_ckch.c b/src/ssl_ckch.c
index eeb031b27e96ca53f0ad351b4561b80a8ae8e81d..82169507f2c9f931f08a10b78d58b7a462ace7db 100644 (file)
--- a/src/ssl_ckch.c
+++ b/src/ssl_ckch.c
@@ -1799,6 +1799,7 @@ static void __ssl_sock_load_new_ckch_instance(struct ckch_inst *ckchi)
 
                /* flush the session cache of the server */
                for (i = 0; i < global.nbthread; i++) {
+                       ha_free(&ckchi->server->ssl_ctx.reused_sess[tid].sni);
                        ha_free(&ckchi->server->ssl_ctx.reused_sess[i].ptr);
                }
                HA_RWLOCK_WRUNLOCK(SSL_SERVER_LOCK, &ckchi->server->ssl_ctx.lock);

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index bc515cc136488119720356a57239c9887e3c18ba..42a4772e9bb45f6997280ad6aeb73ac3b8a34cf7 100644 (file)
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -4128,8 +4128,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);
                HA_RWLOCK_RDLOCK(SSL_SERVER_LOCK, &s->ssl_ctx.lock);
                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;
@@ -4142,6 +4144,18 @@ 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) {
+                               ha_free(&s->ssl_ctx.reused_sess[tid].sni);
+                               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);
+               }
                HA_RWLOCK_RDUNLOCK(SSL_SERVER_LOCK, &s->ssl_ctx.lock);
        } else {
                HA_RWLOCK_RDLOCK(SSL_SERVER_LOCK, &s->ssl_ctx.lock);
@@ -5163,8 +5177,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++) {
                        ha_free(&srv->ssl_ctx.reused_sess[i].ptr);
+                       ha_free(&srv->ssl_ctx.reused_sess[i].sni);
+               }
                ha_free(&srv->ssl_ctx.reused_sess);
        }
 
@@ -6555,22 +6571,26 @@ 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 (!conn_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 */
+
+       HA_RWLOCK_RDLOCK(SSL_SERVER_LOCK, &s->ssl_ctx.lock);
+       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);
+       HA_RWLOCK_RDUNLOCK(SSL_SERVER_LOCK, &s->ssl_ctx.lock);
 
        SSL_set_tlsext_host_name(ctx->ssl, hostname);
 #endif