BUG/MEDIUM: ssl: Fix unprotected 'ssl_sock_choose_sni_ctx' calls

The 'ssl_sock_choose_sni_ctx' function accesses the sni tree and it
mostly covered by a read/write lock which was not taken around two of
its uses. One of them could be reached by using the SSL-related CLI
commands and the other one when the generate-cert mechanism is used. So
even if it's in a not that common use case, a race could still happen
and end up crashing.

Note that this requires that the read lock is properly released
before calling ssl_sock_generate_certificate() so as to avoid a
possible read-lock recursion (it was verified that this is safe).

This patch should be backported up to 3.0.

Reported-by: Claude (ANT-2026-VFR5JH3R)

diff --git a/src/ssl_clienthello.c b/src/ssl_clienthello.c
index 3148de24d700637eeee4d10802d9ce70da2abcc1..5cb94e6855d18290b111b7efd2286a1844bfb5b6 100644 (file)
--- a/src/ssl_clienthello.c
+++ b/src/ssl_clienthello.c
@@ -629,15 +629,18 @@ sni_lookup:
                }
        }
        if (!node) {
+               /* Nothing more to look up in the tree, and
+                * ssl_sock_generate_certificate() takes this same lock in read
+                * mode, which must not be done recursively.
+                */
+               HA_RWLOCK_RDUNLOCK(SNI_LOCK, &s->sni_lock);
 #if (!defined SSL_NO_GENERATE_CERTIFICATES)
                if (s->options & BC_O_GENERATE_CERTS && ssl_sock_generate_certificate(servername, s, ssl)) {
                        /* switch ctx done in ssl_sock_generate_certificate */
-                       HA_RWLOCK_RDUNLOCK(SNI_LOCK, &s->sni_lock);
                        TRACE_STATE("No context found, generate certificate", SSL_EV_CONN_SWITCHCTX_CB, NULL, ssl, servername);
                        return SSL_TLSEXT_ERR_OK;
                }
 #endif
-               HA_RWLOCK_RDUNLOCK(SNI_LOCK, &s->sni_lock);
 
                if (!(s->ssl_options & BC_SSL_O_STRICT_SNI) && !default_lookup) {
                        /* we didn't find a SNI, and we didn't look for a default

diff --git a/src/ssl_gencert.c b/src/ssl_gencert.c
index b608f552c30bc1868b2e426d26c801c1d301a19b..4bff2d2d84348ce01c43a35b91b77f20736c50c0 100644 (file)
--- a/src/ssl_gencert.c
+++ b/src/ssl_gencert.c
@@ -132,9 +132,12 @@ static SSL_CTX *ssl_sock_do_create_cert(const char *servername, struct bind_conf
        if (!ssl_sock_sni_is_valid(servername))
                goto mkcert_error;
 
+       HA_RWLOCK_RDLOCK(SNI_LOCK, &bind_conf->sni_lock);
        sni_ctx = ssl_sock_choose_sni_ctx(bind_conf, NULL, "", 1, 1);
-       if (!sni_ctx)
+       if (!sni_ctx) {
+               HA_RWLOCK_RDUNLOCK(SNI_LOCK, &bind_conf->sni_lock);
                goto mkcert_error;
+       }
 
        /* Get the private key of the default certificate and use it */
 #ifdef HAVE_SSL_CTX_get0_privatekey
@@ -144,8 +147,12 @@ static SSL_CTX *ssl_sock_do_create_cert(const char *servername, struct bind_conf
        if (tmp_ssl)
                pkey = SSL_get_privatekey(tmp_ssl);
 #endif
-       if (!pkey)
+       if (!pkey) {
+               HA_RWLOCK_RDUNLOCK(SNI_LOCK, &bind_conf->sni_lock);
                goto mkcert_error;
+       }
+       EVP_PKEY_up_ref(pkey);
+       HA_RWLOCK_RDUNLOCK(SNI_LOCK, &bind_conf->sni_lock);
 
        /* Create the certificate */
        if (!(newcrt = X509_new()))
@@ -302,6 +309,7 @@ static SSL_CTX *ssl_sock_do_create_cert(const char *servername, struct bind_conf
  end:
        if (ctmp) NCONF_free(ctmp);
        if (tmp_ssl) SSL_free(tmp_ssl);
+       EVP_PKEY_free(pkey);
        return ssl_ctx;
 
  mkcert_error:
@@ -309,6 +317,7 @@ static SSL_CTX *ssl_sock_do_create_cert(const char *servername, struct bind_conf
        if (tmp_ssl) SSL_free(tmp_ssl);
        if (ssl_ctx) SSL_CTX_free(ssl_ctx);
        if (newcrt)  X509_free(newcrt);
+       EVP_PKEY_free(pkey);
        return NULL;
 }
 

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 4fcf0ab67121f689beb1f06ce97b710200486b2e..628f936ad665c6e995065592e48f6d788c8cbd1b 100644 (file)
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -5657,12 +5657,15 @@ int ssl_sock_prepare_bind_conf(struct bind_conf *bind_conf)
                struct sni_ctx *sni_ctx;
 
                /* if we use the generate-certificates option, look for the first default cert available */
+               HA_RWLOCK_RDLOCK(SNI_LOCK, &bind_conf->sni_lock);
                sni_ctx = ssl_sock_choose_sni_ctx(bind_conf, NULL, "", 1, 1);
                if (!sni_ctx) {
+                       HA_RWLOCK_RDUNLOCK(SNI_LOCK, &bind_conf->sni_lock);
                        ha_alert("Proxy '%s': no SSL certificate specified for bind '%s' and 'generate-certificates' option at [%s:%d] (use 'crt').\n",
                                 px->id, bind_conf->arg, bind_conf->file, bind_conf->line);
                        return -1;
                }
+               HA_RWLOCK_RDUNLOCK(SNI_LOCK, &bind_conf->sni_lock);
        }
 
        if (!ssl_shctx && global.tune.sslcachesize) {