Preserve connection custom extensions in SSL_set_SSL_CTX()

The SSL_set_SSL_CTX() function is used to switch SSL contexts for
the given SSL object.  If contexts differ, this includes updating
a cert structure with custom extensions from the new context.  This
however overwrites connection custom extensions previously set on
top of inherited from the old context.

The fix is to preserve connection custom extensions using a newly
introduced flag SSL_EXT_FLAG_CONN in custom_ext_copy_conn().
Similar to custom_ext_copy(), it is a no-op if there are no custom
extensions to copy.

The only such consumer is ossl_quic_tls_configure() used to set the
"quic_transport_parameters" extension.  Before this change, context
switch resulted in transport parameters not being sent due to the
missing extension.

Initially reported at https://github.com/nginx/nginx/issues/711

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/27706)

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 18ed42e5040a0b05c03f89b0e5095f6c365d004a..34cde49496474ff18052d6f85bb51ed5ec0c32fb 100644 (file)
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -5476,6 +5476,8 @@ SSL_CTX *SSL_set_SSL_CTX(SSL *ssl, SSL_CTX *ctx)
     new_cert = ssl_cert_dup(ctx->cert);
     if (new_cert == NULL)
         goto err;
+    if (!custom_exts_copy_conn(&new_cert->custext, &sc->cert->custext))
+        goto err;
     if (!custom_exts_copy_flags(&new_cert->custext, &sc->cert->custext))
         goto err;
 

diff --git a/ssl/ssl_local.h b/ssl/ssl_local.h
index 597b4d4b6395ee646d14b213d8e5ffdd41138fa5..c53ebc36ae9dfd4ebc5c7ac6456ff57cfaa9723b 100644 (file)
--- a/ssl/ssl_local.h
+++ b/ssl/ssl_local.h
@@ -2067,6 +2067,11 @@ typedef struct {
  * corresponding ServerHello extension.
  */
 # define SSL_EXT_FLAG_SENT       0x2
+/*
+ * Indicates an extension that was set on SSL object and needs to be
+ * preserved when switching SSL contexts.
+ */
+# define SSL_EXT_FLAG_CONN       0x4
 
 typedef struct {
     custom_ext_method *meths;
@@ -2998,6 +3003,8 @@ __owur int custom_ext_add(SSL_CONNECTION *s, int context, WPACKET *pkt, X509 *x,
 
 __owur int custom_exts_copy(custom_ext_methods *dst,
                             const custom_ext_methods *src);
+__owur int custom_exts_copy_conn(custom_ext_methods *dst,
+                                 const custom_ext_methods *src);
 __owur int custom_exts_copy_flags(custom_ext_methods *dst,
                                   const custom_ext_methods *src);
 void custom_exts_free(custom_ext_methods *exts);

diff --git a/ssl/statem/extensions_cust.c b/ssl/statem/extensions_cust.c
index 2a52e2b67edc2480164db9807a6b040af2f29551..aa352529c4cc763159d5972e3a592ae1792f346d 100644 (file)
--- a/ssl/statem/extensions_cust.c
+++ b/ssl/statem/extensions_cust.c
@@ -107,7 +107,7 @@ void custom_ext_init(custom_ext_methods *exts)
     custom_ext_method *meth = exts->meths;
 
     for (i = 0; i < exts->meths_count; i++, meth++)
-        meth->ext_flags = 0;
+        meth->ext_flags &= ~(SSL_EXT_FLAG_SENT | SSL_EXT_FLAG_RECEIVED);
 }
 
 /* Pass received custom extension data to the application for parsing. */
@@ -330,6 +330,58 @@ int custom_exts_copy(custom_ext_methods *dst, const custom_ext_methods *src)
     return 1;
 }
 
+/* Copy custom extensions that were set on connection */
+int custom_exts_copy_conn(custom_ext_methods *dst,
+                          const custom_ext_methods *src)
+{
+    size_t i;
+    int err = 0;
+
+    if (src->meths_count > 0) {
+        size_t meths_count = 0;
+
+        for (i = 0; i < src->meths_count; i++)
+            if ((src->meths[i].ext_flags & SSL_EXT_FLAG_CONN) != 0)
+                meths_count++;
+
+        if (meths_count > 0) {
+            custom_ext_method *methdst =
+                OPENSSL_realloc(dst->meths,
+                                (dst->meths_count + meths_count) *
+                                sizeof(custom_ext_method));
+
+            if (methdst == NULL)
+                return 0;
+
+            for (i = 0; i < dst->meths_count; i++)
+                custom_ext_copy_old_cb(&methdst[i], &dst->meths[i], &err);
+
+            dst->meths = methdst;
+            methdst += dst->meths_count;
+
+            for (i = 0; i < src->meths_count; i++) {
+                custom_ext_method *methsrc = &src->meths[i];
+
+                if ((methsrc->ext_flags & SSL_EXT_FLAG_CONN) == 0)
+                    continue;
+
+                memcpy(methdst, methsrc, sizeof(custom_ext_method));
+                custom_ext_copy_old_cb(methdst, methsrc, &err);
+                methdst++;
+            }
+
+            dst->meths_count += meths_count;
+        }
+    }
+
+    if (err) {
+        custom_exts_free(dst);
+        return 0;
+    }
+
+    return 1;
+}
+
 void custom_exts_free(custom_ext_methods *exts)
 {
     size_t i;
@@ -417,6 +469,7 @@ int ossl_tls_add_custom_ext_intern(SSL_CTX *ctx, custom_ext_methods *exts,
     meth->add_cb = add_cb;
     meth->free_cb = free_cb;
     meth->ext_type = ext_type;
+    meth->ext_flags = (ctx == NULL) ? SSL_EXT_FLAG_CONN : 0;
     meth->add_arg = add_arg;
     meth->parse_arg = parse_arg;
     exts->meths_count++;

diff --git a/test/sslapitest.c b/test/sslapitest.c
index fb27ed788f8b6ed4e12cc7d74af3d81c39a7efe9..e4e0b9c085c83af8a38f119a1be383a695422452 100644 (file)
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -12915,10 +12915,11 @@ static int alert_cb(SSL *s, unsigned char alert_code, void *arg)
  * Test 1: Force a failure
  * Test 3: Use a CCM based ciphersuite
  * Test 4: fail yield_secret_cb to see double free
+ * Test 5: Normal run with SNI
  */
 static int test_quic_tls(int idx)
 {
-    SSL_CTX *sctx = NULL, *cctx = NULL;
+    SSL_CTX *sctx = NULL, *sctx2 = NULL, *cctx = NULL;
     SSL *serverssl = NULL, *clientssl = NULL;
     int testresult = 0;
     OSSL_DISPATCH qtdis[] = {
@@ -12946,6 +12947,7 @@ static int test_quic_tls(int idx)
     if (idx == 4)
         qtdis[3].function = (void (*)(void))yield_secret_cb_fail;
 
+    snicb = 0;
     memset(secret_history, 0, sizeof(secret_history));
     secret_history_idx = 0;
     memset(&sdata, 0, sizeof(sdata));
@@ -12960,6 +12962,18 @@ static int test_quic_tls(int idx)
                                        &sctx, &cctx, cert, privkey)))
         goto end;
 
+    if (idx == 5) {
+        if (!TEST_true(create_ssl_ctx_pair(libctx, TLS_server_method(), NULL,
+                                           TLS1_3_VERSION, 0,
+                                           &sctx2, NULL, cert, privkey)))
+            goto end;
+
+        /* Set up SNI */
+        if (!TEST_true(SSL_CTX_set_tlsext_servername_callback(sctx, sni_cb))
+                || !TEST_true(SSL_CTX_set_tlsext_servername_arg(sctx, sctx2)))
+            goto end;
+    }
+
     if (!TEST_true(create_ssl_objects(sctx, cctx, &serverssl, &clientssl, NULL,
                                       NULL)))
         goto end;
@@ -13000,6 +13014,12 @@ static int test_quic_tls(int idx)
         goto end;
     }
 
+    /* We should have had the SNI callback called exactly once */
+    if (idx == 5) {
+        if (!TEST_int_eq(snicb, 1))
+            goto end;
+    }
+
     /* Check no problems during the handshake */
     if (!TEST_false(sdata.alert)
             || !TEST_false(cdata.alert)
@@ -13041,6 +13061,7 @@ static int test_quic_tls(int idx)
  end:
     SSL_free(serverssl);
     SSL_free(clientssl);
+    SSL_CTX_free(sctx2);
     SSL_CTX_free(sctx);
     SSL_CTX_free(cctx);
 
@@ -13625,7 +13646,7 @@ int setup_tests(void)
 #endif
     ADD_ALL_TESTS(test_alpn, 4);
 #if !defined(OSSL_NO_USABLE_TLS1_3)
-    ADD_ALL_TESTS(test_quic_tls, 5);
+    ADD_ALL_TESTS(test_quic_tls, 6);
     ADD_TEST(test_quic_tls_early_data);
 #endif
     ADD_ALL_TESTS(test_no_renegotiation, 2);