BUG/MEDIUM: ssl: crt-list curves ignored

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.

ssl_sock_switchctx_set_curves() now sets them again on the SSL object
after the context switch, in the ClientHello callback used by OpenSSL and
AWS-LC. 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 libraries without SSL_CLIENT_HELLO_CB are not covered. LibreSSL calls
the servername callback once the group is already chosen, so setting the
curves there has no effect on a TLS 1.3 handshake. wolfSSL has its own
callback, left untouched. The reg-tests skip both libraries.

This adds the ssl_crt-list_curves reg-tests for TCP and QUIC, and
ssl_crt-list_ecdhe for the "ecdhe" option. The latter is skipped in FIPS
mode, where the bind lines get a default curves list which wins over the
"ecdhe" of a crt-list line.

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.

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/certs/ecdhe.crt-list b/reg-tests/ssl/certs/ecdhe.crt-list
new file mode 100644 (file)
index 0000000..13ebb67
--- /dev/null
+++ b/reg-tests/ssl/certs/ecdhe.crt-list
@@ -0,0 +1,2 @@
+common.pem [ecdhe secp384r1] restricted.ecdhe.domain.tld
+common.pem open.ecdhe.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/reg-tests/ssl/ssl_crt-list_ecdhe.vtc b/reg-tests/ssl/ssl_crt-list_ecdhe.vtc
new file mode 100644 (file)
index 0000000..b12cb45
--- /dev/null
+++ b/reg-tests/ssl/ssl_crt-list_ecdhe.vtc
@@ -0,0 +1,91 @@
+#REGTEST_TYPE=bug
+varnishtest "Test for the per-certificate ecdhe option 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)'"
+# In FIPS mode the bind lines get a default curves list, which wins over the
+# "ecdhe" option of a crt-list line, so this test does not apply there.
+feature cmd "$HAPROXY_PROGRAM -cc '!fips_mode()'"
+# This test checks that the curve set by the "ecdhe" option of a crt-list line
+# is enforced when the bind line sets no curves, and that the curves of the
+# bind line win over it when it sets some.
+feature ignore_unknown_macro
+
+server s1 -repeat 3 {
+    rxreq
+    txresp
+} -start
+
+haproxy h1 -conf {
+    global
+    .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 }
+        use_backend bind-wins if { path /bind-wins }
+
+    # curve allowed by the "ecdhe" option of the crt-list line
+    backend allowed
+        server s1 ${h1_ssllst_sock} ssl verify none sni str(restricted.ecdhe.domain.tld) curves P-384
+
+    # curve excluded by the "ecdhe" option of the crt-list line
+    backend excluded
+        server s1 ${h1_ssllst_sock} ssl verify none sni str(restricted.ecdhe.domain.tld) curves P-256
+
+    # same excluded curve, but for a certificate without restriction
+    backend open
+        server s1 ${h1_ssllst_sock} ssl verify none sni str(open.ecdhe.domain.tld) curves P-256
+
+    # the curves of the bind line win over the "ecdhe" of the crt-list line
+    backend bind-wins
+        server s1 ${h1_ssllstc_sock} ssl verify none sni str(restricted.ecdhe.domain.tld) curves P-256
+
+    # no curves on this bind line, so the crt-list "ecdhe" applies
+    listen ssl-lst
+        bind "fd@${ssllst}" ssl strict-sni crt-list ${testdir}/certs/ecdhe.crt-list
+        server s1 ${s1_addr}:${s1_port}
+
+    # same crt-list, but this bind line sets its own curves
+    listen ssl-lstc
+        bind "fd@${ssllstc}" ssl strict-sni crt-list ${testdir}/certs/ecdhe.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
+
+client c1 -connect ${h1_clearlst_sock} {
+    txreq -url "/bind-wins"
+    rxresp
+    expect resp.status == 200
+} -run

diff --git a/src/ssl_clienthello.c b/src/ssl_clienthello.c
index 5cb94e6855d18290b111b7efd2286a1844bfb5b6..b0a940c47cc70f072c3c469a8593b915df59f53d 100644 (file)
--- a/src/ssl_clienthello.c
+++ b/src/ssl_clienthello.c
@@ -145,6 +145,30 @@ int ssl_sock_switchctx_err_cbk(SSL *ssl, int *al, void *priv)
        return SSL_TLSEXT_ERR_NOACK;
 }
 
+#if defined(SSL_CTX_set1_curves_list)
+/* Sets the curves of the crt-list line <conf> on <ssl>.
+ * The SSL libraries copy the curves of the SSL_CTX into the SSL object at
+ * SSL_new() time. SSL_set_SSL_CTX() does not update this copy. So they must be
+ * set again on the connection after a context switch.
+ * The value is picked in the same order as when the SSL_CTX is prepared:
+ * <curves> of the crt-list line, else <curves> of the bind line, else
+ * <ecdhe> of the crt-list line, else <ecdhe> of the bind line.
+ * Returns 0 on success, -1 on failure.
+ */
+static int ssl_sock_switchctx_set_curves(SSL *ssl, struct bind_conf *s, struct ssl_bind_conf *conf)
+{
+       const char *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))
+               return -1;
+
+       return 0;
+}
+#endif /* SSL_CTX_set1_curves_list */
+
 #if defined(OPENSSL_IS_BORINGSSL) || defined(OPENSSL_IS_AWSLC)
 int ssl_sock_switchctx_cbk(const struct ssl_early_callback_ctx *ctx)
 {
@@ -461,6 +485,14 @@ sni_lookup:
                        methodVersions[conf->ssl_methods.max].ssl_set_version(ssl, SET_MAX);
                        if (conf->early_data)
                                allow_early = 1;
+#if defined(SSL_CTX_set1_curves_list)
+                       if (ssl_sock_switchctx_set_curves(ssl, s, conf) < 0) {
+                               HA_RWLOCK_RDUNLOCK(SNI_LOCK, &s->sni_lock);
+                               TRACE_ERROR("Cannot set the crt-list curves on the connection",
+                                           SSL_EV_CONN_SWITCHCTX_CB|SSL_EV_CONN_ERR, conn);
+                               goto abort;
+                       }
+#endif
                }
                HA_RWLOCK_RDUNLOCK(SNI_LOCK, &s->sni_lock);
                goto allow_early;