BUG/MINOR: ssl: Store client SNI in SSL context in case of ClientHello error

If an error is raised during the ClientHello callback on the server side
(ssl_sock_switchctx_cbk), the servername callback won't be called and
the client's SNI will not be saved in the SSL context. But since we use
the SSL_get_servername function to return this SNI in the ssl_fc_sni
sample fetch, that means that in case of error, such as an SNI mismatch
with a frontend having the strict-sni option enabled, the sample fetch
would not work (making strict-sni related errors hard to debug).

This patch fixes that by storing the SNI as an ex_data in the SSL
context in case the ClientHello callback returns an error. This way the
sample fetch can fallback to getting the SNI this way. It will still
first call the SSL_get_servername function first since it is the proper
way of getting a client's SNI when the handshake succeeded.

In order to avoid memory allocations are runtime into this highly used
runtime function, a new memory pool was created to store those client
SNIs. Its entry size is set to 256 bytes since SNIs can't be longer than
255 characters.

This fixes GitHub #1484.

It can be backported in 2.5.

diff --git a/include/haproxy/ssl_sock.h b/include/haproxy/ssl_sock.h
index cd8d3b6a794091f391c29f20b8a13a6745ceae09..fd45c4beb3ad06358ca1450b0c1f9f2085d4e568 100644 (file)
--- a/include/haproxy/ssl_sock.h
+++ b/include/haproxy/ssl_sock.h
@@ -47,6 +47,7 @@ extern int nb_engines;
 extern struct xprt_ops ssl_sock;
 extern int ssl_capture_ptr_index;
 extern int ssl_keylog_index;
+extern int ssl_client_sni_index;
 extern struct pool_head *pool_head_ssl_keylog;
 extern struct pool_head *pool_head_ssl_keylog_str;
 

diff --git a/reg-tests/ssl/ssl_errors.vtc b/reg-tests/ssl/ssl_errors.vtc
index 1ac6b087cacf244f2b62829a33781cbcf33d69d8..6148a9dee6d22b1f76d3b249def54eabb291e1c3 100644 (file)
--- a/reg-tests/ssl/ssl_errors.vtc
+++ b/reg-tests/ssl/ssl_errors.vtc
@@ -63,24 +63,24 @@ syslog Slg_cust_fmt -level info {
 
 syslog Slg_https_fmt -level info {
     recv
-    expect ~ ".*https_logfmt_ssl_lst~ https_logfmt_ssl_lst/s1.*0/0000000000000000/0/0/.? -/TLSv1.2/AES256-GCM-SHA384"
+    expect ~ ".*https_logfmt_ssl_lst~ https_logfmt_ssl_lst/s1.*0/0000000000000000/0/0/.? foo.com/TLSv1.2/AES256-GCM-SHA384"
 
     barrier b1 sync
 } -start
 
 syslog Slg_https_fmt_err -level info {
     recv
-    expect ~ "ERROR.*https_logfmt_ssl_lst~ https_logfmt_ssl_lst/<NOSRV>.*30/0000000000000086/0/2/.? -/TLSv1.2/\\(NONE\\)"
+    expect ~ "ERROR.*https_logfmt_ssl_lst~ https_logfmt_ssl_lst/<NOSRV>.*30/0000000000000086/0/2/.? foo.com/TLSv1.2/\\(NONE\\)"
 
     barrier b1 sync
 
     recv
-    expect ~ "ERROR.*https_logfmt_ssl_lst~ https_logfmt_ssl_lst/<NOSRV>.*31/0000000000000086/20/0/.? -/TLSv1.2/\\(NONE\\)"
+    expect ~ "ERROR.*https_logfmt_ssl_lst~ https_logfmt_ssl_lst/<NOSRV>.*31/0000000000000086/20/0/.? foo.com/TLSv1.2/\\(NONE\\)"
 
     barrier b1 sync
 
     recv
-    expect ~ "ERROR.*https_logfmt_ssl_lst~ https_logfmt_ssl_lst/<NOSRV>.*34/00000000000000C1/0/0/.? -/TLSv1.2/\\(NONE\\)"
+    expect ~ "ERROR.*https_logfmt_ssl_lst~ https_logfmt_ssl_lst/<NOSRV>.*34/00000000000000C1/0/0/.? foo.com/TLSv1.2/\\(NONE\\)"
 } -start
 
 syslog Slg_logconnerror -level info {
@@ -115,7 +115,7 @@ syslog Slg_bcknd -level info {
     barrier b2 sync
 
     recv
-    expect ~ ".*bc_err:32:\"Server presented an SSL certificate different from the configured one\" ssl_bc_err:134:.*:certificate verify failed"
+    expect ~ ".*bc_err:33:\"Server presented an SSL certificate different from the expected one\" ssl_bc_err:134:.*:certificate verify failed"
 
     barrier b2 sync
 
@@ -134,6 +134,32 @@ syslog Slg_bcknd -level info {
     expect ~ ".*bc_err:34:\"SSL handshake failure\" ssl_bc_err:1040:.*:sslv3 alert handshake failure"
 } -start
 
+syslog Slg_bcknd_fe -level info {
+    # Client c13 - No error
+    recv
+    expect ~ ".* Server/TLSv1.3/TLS_AES_256_GCM_SHA384"
+
+    # Client c14 - Server certificate rejected
+    recv
+    expect ~ ".* foo.com/TLSv1.3/TLS_AES_256_GCM_SHA384"
+
+    # Client c15 - Server certificate mismatch (verifyhost option on backend)
+    recv
+    expect ~ ".* foo.com/TLSv1.3/TLS_AES_256_GCM_SHA384"
+
+    # Client c16 - Client certificate rejected
+    recv
+    expect ~ ".* foo.com/TLSv1.2/\\(NONE\\)"
+
+    # Client c17 - Wrong ciphers TLSv1.2
+    recv
+    expect ~ ".* foo.com/TLSv1.2/\\(NONE\\)"
+
+    # Client c18 - Wrong ciphers TLSv1.3 - the client does not get to send its certificate because the error happens before
+    recv
+    expect ~ ".* -/TLSv1.3/\\(NONE\\)"
+} -start
+
 
 haproxy h1 -conf {
     global
@@ -154,7 +180,7 @@ haproxy h1 -conf {
 
     listen clear_lst
         bind "fd@${clearlst}"
-        default-server ssl crt ${testdir}/set_cafile_client.pem ca-file ${testdir}/set_cafile_interCA2.crt verify none no-ssl-reuse force-tlsv12
+        default-server ssl crt ${testdir}/set_cafile_client.pem ca-file ${testdir}/set_cafile_interCA2.crt verify none no-ssl-reuse force-tlsv12 sni str(foo.com)
 
         balance roundrobin
         server cust_fmt "${tmpdir}/cust_logfmt_ssl.sock"
@@ -164,7 +190,7 @@ haproxy h1 -conf {
 
     listen clear_wrong_ciphers_lst
         bind "fd@${wrongcipherslst}"
-        default-server ssl crt ${testdir}/set_cafile_client.pem ca-file ${testdir}/set_cafile_interCA2.crt verify none no-ssl-reuse force-tlsv12 ciphers "aECDSA"
+        default-server ssl crt ${testdir}/set_cafile_client.pem ca-file ${testdir}/set_cafile_interCA2.crt verify none no-ssl-reuse force-tlsv12 ciphers "aECDSA" sni str(foo.com)
 
         balance roundrobin
         server cust_fmt "${tmpdir}/cust_logfmt_ssl.sock"
@@ -180,21 +206,20 @@ haproxy h1 -conf {
         error-log-format "ERROR bc_err:%[bc_err]:%{+Q}[bc_err_str]\ ssl_bc_err:%[ssl_bc_err,and(proc.ssl_error_mask)]:%[ssl_bc_err_str]"
 
         balance roundrobin
-        server no_err "${tmpdir}/no_err_ssl.sock" ssl crt ${testdir}/set_cafile_client.pem ca-file ${testdir}/set_cafile_interCA2.crt verify required
-        server srv_cert_rejected "${tmpdir}/srv_rejected_ssl.sock" ssl crt ${testdir}/set_cafile_client.pem ca-file ${testdir}/set_cafile_interCA1.crt verify required
-        server mismatch_frontend "${tmpdir}/mismatch_fe_ssl.sock" ssl crt ${testdir}/set_cafile_client.pem ca-file ${testdir}/set_cafile_interCA2.crt verify required verifyhost str(toto)
-        # We force TLSv1.2 for this specific case because server-side
+        server no_err "${tmpdir}/no_err_ssl.sock" ssl crt ${testdir}/set_cafile_client.pem ca-file ${testdir}/set_cafile_interCA2.crt verify required sni str(Server)
+        server srv_cert_rejected "${tmpdir}/srv_rejected_ssl.sock" ssl crt ${testdir}/set_cafile_client.pem ca-file ${testdir}/set_cafile_interCA1.crt verify required sni str(foo.com)
+        server mismatch_frontend "${tmpdir}/mismatch_fe_ssl.sock" ssl crt ${testdir}/set_cafile_client.pem ca-file ${testdir}/set_cafile_interCA2.crt verify required sni str(foo.com) verifyhost str(toto)         # We force TLSv1.2 for this specific case because server-side
         # verification errors cannot be caught by the backend fetches when
         # using TLSv1.3
-        server clt_cert_rejected "${tmpdir}/rejected_ssl.sock" ssl crt ${testdir}/set_cafile_client.pem ca-file ${testdir}/set_cafile_interCA2.crt verify none force-tlsv12
-        server wrong_ciphers "${tmpdir}/wrong_ciphers_ssl.sock" ssl verify none crt ${testdir}/client1.pem ca-file ${testdir}/ca-auth.crt force-tlsv12 ciphers "aECDSA"
+        server clt_cert_rejected "${tmpdir}/rejected_ssl.sock" ssl crt ${testdir}/set_cafile_client.pem ca-file ${testdir}/set_cafile_interCA2.crt verify none force-tlsv12 sni str(foo.com)
+        server wrong_ciphers "${tmpdir}/wrong_ciphers_ssl.sock" ssl verify none crt ${testdir}/client1.pem ca-file ${testdir}/ca-auth.crt force-tlsv12 ciphers "aECDSA" sni str(foo.com)
 
         # No TLSv1.3 support with OpenSSL 1.0.2 so we duplicate the previous
         # wrong cipher test in this case so that the error log remains the same
 .if openssl_version_before(1.1.1)
-        server wrong_ciphers2 "${tmpdir}/wrong_ciphers_ssl.sock" ssl verify none crt ${testdir}/client1.pem ca-file ${testdir}/ca-auth.crt force-tlsv12 ciphers "aECDSA"
+        server wrong_ciphers2 "${tmpdir}/wrong_ciphers_ssl.sock" ssl verify none crt ${testdir}/client1.pem ca-file ${testdir}/ca-auth.crt force-tlsv12 ciphers "aECDSA" sni str(foo.com)
 .else
-        server wrong_ciphers_tls13 "${tmpdir}/wrong_ciphers_tls13_ssl.sock" ssl verify none crt ${testdir}/client1.pem ca-file ${testdir}/ca-auth.crt ciphersuites "TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256" force-tlsv13
+        server wrong_ciphers_tls13 "${tmpdir}/wrong_ciphers_tls13_ssl.sock" ssl verify none crt ${testdir}/client1.pem ca-file ${testdir}/ca-auth.crt ciphersuites "TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256" force-tlsv13 sni str(foo.com)
 .endif
 
 
@@ -226,29 +251,39 @@ haproxy h1 -conf {
         server s1 ${s1_addr}:${s1_port}
 
 
+
+    defaults bknd_err_dflt
+        timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout client  "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout server  "${HAPROXY_TEST_TIMEOUT-5s}"
+        retries 0
+        log ${Slg_bcknd_fe_addr}:${Slg_bcknd_fe_port} local0
+        log-format "%ci:%cp %[ssl_fc_sni]/%sslv/%sslc"
+        error-log-format "ERROR %ci:%cp %[ssl_fc_sni]/%sslv/%sslc"
+
     # The following listeners allow to test backend error fetches
-    listen no_backend_err_ssl_lst
+    listen no_backend_err_ssl_lst from bknd_err_dflt
         bind "${tmpdir}/no_err_ssl.sock" ssl crt ${testdir}/set_cafile_server.pem ca-file ${testdir}/set_cafile_interCA2.crt verify none
         server s1 ${s1_addr}:${s1_port}
 
-    listen srv_rejected_ssl_lst
+    listen srv_rejected_ssl_lst from bknd_err_dflt
         bind "${tmpdir}/srv_rejected_ssl.sock" ssl crt ${testdir}/set_cafile_server.pem ca-file ${testdir}/set_cafile_interCA2.crt verify none
         server s1 ${s1_addr}:${s1_port}
 
-    listen mismatch_fe_ssl_lst
+    listen mismatch_fe_ssl_lst from bknd_err_dflt
         bind "${tmpdir}/mismatch_fe_ssl.sock" ssl crt ${testdir}/set_cafile_server.pem ca-file ${testdir}/set_cafile_interCA2.crt verify none
         server s1 ${s1_addr}:${s1_port}
 
-    listen rejected_clt_ssl_lst
+    listen rejected_clt_ssl_lst from bknd_err_dflt
         bind "${tmpdir}/rejected_ssl.sock" ssl crt ${testdir}/set_cafile_server.pem ca-file ${testdir}/set_cafile_interCA2.crt verify required
         server s1 ${s1_addr}:${s1_port}
 
-    listen wrong_ciphers_ssl_lst
+    listen wrong_ciphers_ssl_lst from bknd_err_dflt
         bind "${tmpdir}/wrong_ciphers_ssl.sock" ssl crt ${testdir}/common.pem ca-file ${testdir}/ca-auth.crt verify none force-tlsv12 ciphers "kRSA"
         server s1 ${s1_addr}:${s1_port}
 
 .if openssl_version_atleast(1.1.1)
-    listen wrong_ciphers_tls13_ssl_lst
+    listen wrong_ciphers_tls13_ssl_lst from bknd_err_dflt
         bind "${tmpdir}/wrong_ciphers_tls13_ssl.sock" ssl crt ${testdir}/common.pem ca-file ${testdir}/ca-auth.crt verify none force-tlsv13 ciphersuites "TLS_AES_128_GCM_SHA256"
         server s1 ${s1_addr}:${s1_port}
 .endif
@@ -382,3 +417,4 @@ syslog Slg_https_fmt -wait
 syslog Slg_https_fmt_err -wait
 syslog Slg_logconnerror -wait
 syslog Slg_bcknd -wait
+syslog Slg_bcknd_fe -wait

diff --git a/src/ssl_sample.c b/src/ssl_sample.c
index 8aaead2402bd8f4b2016244ccf10c78ce9146022..ca09829b1d429fc63970a900eec9f934f5643758 100644 (file)
--- a/src/ssl_sample.c
+++ b/src/ssl_sample.c
@@ -1565,10 +1565,18 @@ smp_fetch_ssl_fc_sni(const struct arg *args, struct sample *smp, const char *kw,
                return 0;
 
        smp->data.u.str.area = (char *)SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name);
-       if (!smp->data.u.str.area)
-               return 0;
+       if (!smp->data.u.str.area) {
+               /* We might have stored the SNI ourselves, look for it in the
+                * context's ex_data.
+                */
+               smp->data.u.str.area = SSL_get_ex_data(ssl, ssl_client_sni_index);
+
+               if (!smp->data.u.str.area)
+                       return 0;
+       }
 
        smp->data.u.str.data = strlen(smp->data.u.str.area);
+
        return 1;
 #else
        /* SNI not supported */

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 47bbf1da1c192c80027f3d007ac91c2c19ffe588..576b21d38287827699a2c15c544104ebe14a2ecd 100644 (file)
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -134,6 +134,8 @@ static BIO_METHOD *ha_meth;
 
 DECLARE_STATIC_POOL(ssl_sock_ctx_pool, "ssl_sock_ctx_pool", sizeof(struct ssl_sock_ctx));
 
+DECLARE_STATIC_POOL(ssl_sock_client_sni_pool, "ssl_sock_client_sni_pool", TLSEXT_MAXLEN_host_name + 1);
+
 /* ssl stats module */
 enum {
        SSL_ST_SESS,
@@ -443,6 +445,9 @@ struct pool_head *pool_head_ssl_keylog_str __read_mostly = NULL;
 
 int ssl_client_crt_ref_index = -1;
 
+/* Used to store the client's SNI in case of ClientHello callback error */
+int ssl_client_sni_index = -1;
+
 #if (defined SSL_CTRL_SET_TLSEXT_TICKET_KEY_CB && TLS_TICKETS_NO > 0)
 struct list tlskeys_reference = LIST_HEAD_INIT(tlskeys_reference);
 #endif
@@ -2705,6 +2710,21 @@ int ssl_sock_switchctx_cbk(SSL *ssl, int *al, void *arg)
                goto allow_early;
        }
 
+       /* We are about to raise an handshake error so the servername extension
+        * callback will never be called and the SNI will never be stored in the
+        * SSL context. In order for the ssl_fc_sni sample fetch to still work
+        * in such a case, we store the SNI ourselves as an ex_data information
+        * in the SSL context.
+        */
+       {
+               char *client_sni = pool_alloc(ssl_sock_client_sni_pool);
+               if (client_sni) {
+                       strncpy(client_sni, trash.area, TLSEXT_MAXLEN_host_name);
+                       client_sni[TLSEXT_MAXLEN_host_name] = '\0';
+                       SSL_set_ex_data(ssl, ssl_client_sni_index, client_sni);
+               }
+       }
+
        /* other cases fallback on abort, if strict-sni is set but no node was found */
 
  abort:
@@ -7584,6 +7604,11 @@ static void ssl_sock_clt_crt_free_func(void *parent, void *ptr, CRYPTO_EX_DATA *
        X509_free((X509*)ptr);
 }
 
+static void ssl_sock_clt_sni_free_func(void *parent, void *ptr, CRYPTO_EX_DATA *ad, int idx, long argl, void *argp)
+{
+       pool_free(ssl_sock_client_sni_pool, ptr);
+}
+
 __attribute__((constructor))
 static void __ssl_sock_init(void)
 {
@@ -7632,6 +7657,7 @@ static void __ssl_sock_init(void)
        ssl_keylog_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, ssl_sock_keylog_free_func);
 #endif
        ssl_client_crt_ref_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, ssl_sock_clt_crt_free_func);
+       ssl_client_sni_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, ssl_sock_clt_sni_free_func);
 #ifndef OPENSSL_NO_ENGINE
        ENGINE_load_builtin_engines();
        hap_register_post_check(ssl_check_async_engine_count);