BUG/MEDIUM: ssl: crt-list curves ignored with AWS-LC

SSL_new() copies the curves of the SSL_CTX into the SSL object, and
SSL_set_SSL_CTX() does not update this copy while the handshake uses it.
The "curves" and "ecdhe" options of a crt-list line were thus silently
ignored: a client could negotiate a curve excluded by the certificate as
long as the bind line allowed it.

The ClientHello callback used by AWS-LC now sets them again on the SSL
object after the context switch. The value is picked in the same order
as when the SSL_CTX is prepared, so the curves of the bind line still
win over the <ecdhe> of the crt-list line.

The curves setter is inefficient here: it parses the configuration
string for each connection.

This adds the ssl_crt-list_curves reg-tests for TCP and QUIC.

Tested with AWS-LC 3.6.0 (LTS) and 4.2.0. The curves regression tests
pass over TCP and QUIC with both versions.
The TLSv1.2, TLSv1.3 and QUIC cipher regression tests also pass.

Thank you to @ygkat for having reported this issue in GH #3500

This should be backported to all stable versions. Note the code is in
ssl_sock.c before 3.1.

Co-authored-by: William Lallemand <wlallemand@haproxy.com>

diff --git a/reg-tests/quic/ssl_crt-list_curves.vtc b/reg-tests/quic/ssl_crt-list_curves.vtc
new file mode 100644 (file)
index 0000000..120e8b5
--- /dev/null
+++ b/reg-tests/quic/ssl_crt-list_curves.vtc
@@ -0,0 +1,9 @@
+#REGTEST_TYPE=bug
+varnishtest "Test for per-certificate curves in a crt-list over QUIC"
+feature cmd "$HAPROXY_PROGRAM -cc 'version_atleast(2.8)'"
+feature cmd "$HAPROXY_PROGRAM -cc 'feature(QUIC) && !feature(QUIC_OPENSSL_COMPAT) && !feature(OPENSSL_WOLFSSL) && !ssllib_name_startswith(LibreSSL)'"
+# This test checks that the curves set on a crt-list line are enforced over
+# QUIC, even when the bind line allows more of them.
+
+setenv VTC_SOCK_TYPE quic
+include ${testdir}/../ssl/ssl_crt-list_curves.vtci

diff --git a/reg-tests/ssl/certs/curves.crt-list b/reg-tests/ssl/certs/curves.crt-list
new file mode 100644 (file)
index 0000000..b9d80c1
--- /dev/null
+++ b/reg-tests/ssl/certs/curves.crt-list
@@ -0,0 +1,2 @@
+common.pem [curves P-384] restricted.curves.domain.tld
+common.pem open.curves.domain.tld

diff --git a/reg-tests/ssl/ssl_crt-list_curves.vtc b/reg-tests/ssl/ssl_crt-list_curves.vtc
new file mode 100644 (file)
index 0000000..41003be
--- /dev/null
+++ b/reg-tests/ssl/ssl_crt-list_curves.vtc
@@ -0,0 +1,9 @@
+#REGTEST_TYPE=bug
+varnishtest "Test for per-certificate curves in a crt-list"
+feature cmd "$HAPROXY_PROGRAM -cc 'version_atleast(2.8)'"
+feature cmd "$HAPROXY_PROGRAM -cc 'feature(OPENSSL) && ssllib_name_startswith(OpenSSL) && openssl_version_atleast(1.1.1) || feature(OPENSSL_AWSLC)'"
+# This test checks that the curves set on a crt-list line are enforced, even
+# when the bind line allows more of them.
+
+setenv VTC_SOCK_TYPE stream
+include ${testdir}/ssl_crt-list_curves.vtci

diff --git a/reg-tests/ssl/ssl_crt-list_curves.vtci b/reg-tests/ssl/ssl_crt-list_curves.vtci
new file mode 100644 (file)
index 0000000..10e48f0
--- /dev/null
+++ b/reg-tests/ssl/ssl_crt-list_curves.vtci
@@ -0,0 +1,68 @@
+feature ignore_unknown_macro
+
+server s1 -repeat 2 {
+    rxreq
+    txresp
+} -start
+
+haproxy h1 -conf {
+    global
+    .if streq("$VTC_SOCK_TYPE",quic)
+        # required for backend connections
+        expose-experimental-directives
+    .endif
+    .if feature(THREAD)
+        thread-groups 1
+    .endif
+        crt-base ${testdir}/certs
+
+    defaults
+        mode http
+        option httplog
+        retries 0
+        log stderr local0 debug err
+        option logasap
+        timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout client  "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout server  "${HAPROXY_TEST_TIMEOUT-5s}"
+
+    listen clear-lst
+        bind "fd@${clearlst}"
+        use_backend allowed  if { path /allowed }
+        use_backend excluded if { path /excluded }
+        use_backend open     if { path /open }
+
+    # curve allowed by the crt-list line
+    backend allowed
+        server s1 ${VTC_SOCK_TYPE}+${h1_ssllst_sock} ssl verify none sni str(restricted.curves.domain.tld) curves P-384
+
+    # curve allowed by the bind line but excluded by the crt-list line
+    backend excluded
+        server s1 ${VTC_SOCK_TYPE}+${h1_ssllst_sock} ssl verify none sni str(restricted.curves.domain.tld) curves P-256
+
+    # same excluded curve, but for a certificate without restriction
+    backend open
+        server s1 ${VTC_SOCK_TYPE}+${h1_ssllst_sock} ssl verify none sni str(open.curves.domain.tld) curves P-256
+
+    listen ssl-lst
+        bind ${VTC_SOCK_TYPE}+fd@${ssllst} ssl strict-sni crt-list ${testdir}/certs/curves.crt-list curves P-256:P-384
+        server s1 ${s1_addr}:${s1_port}
+} -start
+
+client c1 -connect ${h1_clearlst_sock} {
+    txreq -url "/allowed"
+    rxresp
+    expect resp.status == 200
+} -run
+
+client c1 -connect ${h1_clearlst_sock} {
+    txreq -url "/excluded"
+    rxresp
+    expect resp.status == 503
+} -run
+
+client c1 -connect ${h1_clearlst_sock} {
+    txreq -url "/open"
+    rxresp
+    expect resp.status == 200
+} -run

diff --git a/src/ssl_clienthello.c b/src/ssl_clienthello.c
index c08e1ca328bd847201edde7fc45dc0e68a3e150a..c2a749a129dcf3c2ab1d45540df8185e67875c16 100644 (file)
--- a/src/ssl_clienthello.c
+++ b/src/ssl_clienthello.c
@@ -457,6 +457,10 @@ sni_lookup:
                struct ssl_bind_conf *conf = sni_ctx->conf;
                ssl_sock_switchctx_set(ssl, sni_ctx->ctx);
                if (conf) {
+#if defined(SSL_CTX_set1_curves_list)
+                       const char *curves;
+#endif
+
                        methodVersions[conf->ssl_methods.min].ssl_set_version(ssl, SET_MIN);
                        methodVersions[conf->ssl_methods.max].ssl_set_version(ssl, SET_MAX);
                        if (conf->early_data)
@@ -476,6 +480,24 @@ sni_lookup:
                                            SSL_EV_CONN_SWITCHCTX_CB|SSL_EV_CONN_ERR, conn);
                                goto abort;
                        }
+#endif
+#if defined(SSL_CTX_set1_curves_list)
+                       /* Reapply the curves copied at SSL_new() time, using the
+                        * same precedence as when preparing the SSL_CTX: crt-list
+                        * curves, bind curves, crt-list ecdhe, then bind ecdhe.
+                        * This call is inefficient as it parses the curves
+                        * configuration string for each connection.
+                        */
+                       curves = conf->curves ? conf->curves : s->ssl_conf.curves;
+                       if (!curves)
+                               curves = conf->ecdhe ? conf->ecdhe : s->ssl_conf.ecdhe;
+
+                       if (curves && !SSL_set1_curves_list(ssl, curves)) {
+                               HA_RWLOCK_RDUNLOCK(SNI_LOCK, &s->sni_lock);
+                               TRACE_ERROR("Cannot set the crt-list curves on the SSL object",
+                                           SSL_EV_CONN_SWITCHCTX_CB|SSL_EV_CONN_ERR, conn);
+                               goto abort;
+                       }
 #endif
                }
                HA_RWLOCK_RDUNLOCK(SNI_LOCK, &s->sni_lock);