Move ciphersuite selection before session resumption in TLSv1.3

This does things as per the recommendation in the TLSv1.3 spec. It also
means that the server will always choose its preferred ciphersuite.
Previously the server would only select ciphersuites compatible with the
session.

Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/3623)

diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c
index e8bda66d61e7743d3239e571cfacc74aae372cc0..41c44ce62e3dcab009a2d12eb7b5380c04f883b3 100644 (file)
--- a/ssl/s3_lib.c
+++ b/ssl/s3_lib.c
@@ -3680,7 +3680,7 @@ const SSL_CIPHER *ssl3_choose_cipher(SSL *s, STACK_OF(SSL_CIPHER) *clnt,
     const SSL_CIPHER *c, *ret = NULL;
     STACK_OF(SSL_CIPHER) *prio, *allow;
     int i, ii, ok;
-    unsigned long alg_k = 0, alg_a = 0, mask_k, mask_a;
+    unsigned long alg_k = 0, alg_a = 0, mask_k = 0, mask_a = 0;
 
     /* Let's see which ciphers we can support */
 
@@ -3714,8 +3714,10 @@ const SSL_CIPHER *ssl3_choose_cipher(SSL *s, STACK_OF(SSL_CIPHER) *clnt,
         allow = srvr;
     }
 
-    tls1_set_cert_validity(s);
-    ssl_set_masks(s);
+    if (!SSL_IS_TLS13(s)) {
+        tls1_set_cert_validity(s);
+        ssl_set_masks(s);
+    }
 
     for (i = 0; i < sk_SSL_CIPHER_num(prio); i++) {
         c = sk_SSL_CIPHER_value(prio, i);
@@ -3729,23 +3731,11 @@ const SSL_CIPHER *ssl3_choose_cipher(SSL *s, STACK_OF(SSL_CIPHER) *clnt,
              DTLS_VERSION_GT(s->version, c->max_dtls)))
             continue;
 
-        if (SSL_IS_TLS13(s)) {
-            /*
-             * We must choose a ciphersuite that has a digest compatible with
-             * the session, unless we're going to do an HRR in which case we
-             * will just choose our most preferred ciphersuite regardless of
-             * whether it is compatible with the session or not.
-             */
-            if (s->hit
-                    && !s->hello_retry_request
-                    && ssl_md(c->algorithm2)
-                       != ssl_md(s->session->cipher->algorithm2))
-                continue;
-        } else {
-            /*
-             * These tests do not apply to TLS 1.3 ciphersuites because they can
-             * be used with any auth or key exchange scheme.
-             */
+        /*
+         * Since TLS 1.3 ciphersuites can be used with any auth or
+         * key exchange scheme skip tests.
+         */
+        if (!SSL_IS_TLS13(s)) {
             mask_k = s->s3->tmp.mask_k;
             mask_a = s->s3->tmp.mask_a;
 #ifndef OPENSSL_NO_SRP

diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c
index fe181abad0aa3fff7df35b80145ab9663ec0c3c3..39e6c07fe0ba49d65cc0197ebdb48696be70af09 100644 (file)
--- a/ssl/statem/extensions_srvr.c
+++ b/ssl/statem/extensions_srvr.c
@@ -726,11 +726,8 @@ int tls_parse_ctos_psk(SSL *s, PACKET *pkt, unsigned int context, X509 *x,
             continue;
 
         md = ssl_md(sess->cipher->algorithm2);
-        if (md == NULL) {
-            /*
-             * Don't recognise this cipher so we can't use the session.
-             * Ignore it
-             */
+        if (md != ssl_md(s->s3->tmp.new_cipher->algorithm2)) {
+            /* The ciphersuite is not compatible with this session. */
             SSL_SESSION_free(sess);
             sess = NULL;
             continue;

diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index 8137a7da9e95cd61988487bb3a0e01ac0c678a4f..53b8ef943184d186cbb811f2de35b74d5619bcdb 100644 (file)
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -1566,6 +1566,68 @@ static int tls_early_post_process_client_hello(SSL *s, int *pal)
 
     s->hit = 0;
 
+    if (!ssl_cache_cipherlist(s, &clienthello->ciphersuites,
+                              clienthello->isv2, &al) ||
+        !bytes_to_cipher_list(s, &clienthello->ciphersuites, &ciphers, &scsvs,
+                             clienthello->isv2, &al)) {
+        goto err;
+    }
+
+    s->s3->send_connection_binding = 0;
+    /* Check what signalling cipher-suite values were received. */
+    if (scsvs != NULL) {
+        for(i = 0; i < sk_SSL_CIPHER_num(scsvs); i++) {
+            c = sk_SSL_CIPHER_value(scsvs, i);
+            if (SSL_CIPHER_get_id(c) == SSL3_CK_SCSV) {
+                if (s->renegotiate) {
+                    /* SCSV is fatal if renegotiating */
+                    SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO,
+                           SSL_R_SCSV_RECEIVED_WHEN_RENEGOTIATING);
+                    al = SSL_AD_HANDSHAKE_FAILURE;
+                    goto err;
+                }
+                s->s3->send_connection_binding = 1;
+            } else if (SSL_CIPHER_get_id(c) == SSL3_CK_FALLBACK_SCSV &&
+                       !ssl_check_version_downgrade(s)) {
+                /*
+                 * This SCSV indicates that the client previously tried
+                 * a higher version.  We should fail if the current version
+                 * is an unexpected downgrade, as that indicates that the first
+                 * connection may have been tampered with in order to trigger
+                 * an insecure downgrade.
+                 */
+                SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO,
+                       SSL_R_INAPPROPRIATE_FALLBACK);
+                al = SSL_AD_INAPPROPRIATE_FALLBACK;
+                goto err;
+            }
+        }
+    }
+
+    /* For TLSv1.3 we must select the ciphersuite *before* session resumption */
+    if (SSL_IS_TLS13(s)) {
+        const SSL_CIPHER *cipher =
+            ssl3_choose_cipher(s, ciphers, SSL_get_ciphers(s));
+
+        if (cipher == NULL) {
+            SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO,
+                   SSL_R_NO_SHARED_CIPHER);
+            al = SSL_AD_HANDSHAKE_FAILURE;
+            goto err;
+        }
+        if (s->hello_retry_request && s->s3->tmp.new_cipher != NULL
+                && s->s3->tmp.new_cipher->id != cipher->id) {
+            /*
+             * A previous HRR picked a different ciphersuite to the one we
+             * just selected. Something must have changed.
+             */
+            al = SSL_AD_ILLEGAL_PARAMETER;
+            SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, SSL_R_BAD_CIPHER);
+            goto err;
+        }
+        s->s3->tmp.new_cipher = cipher;
+    }
+
     /* We need to do this before getting the session */
     if (!tls_parse_extension(s, TLSEXT_IDX_extended_master_secret,
                              SSL_EXT_CLIENT_HELLO,
@@ -1609,48 +1671,9 @@ static int tls_early_post_process_client_hello(SSL *s, int *pal)
         }
     }
 
-    if (!ssl_cache_cipherlist(s, &clienthello->ciphersuites,
-                              clienthello->isv2, &al) ||
-        !bytes_to_cipher_list(s, &clienthello->ciphersuites, &ciphers, &scsvs,
-                             clienthello->isv2, &al)) {
-        goto err;
-    }
-
-    s->s3->send_connection_binding = 0;
-    /* Check what signalling cipher-suite values were received. */
-    if (scsvs != NULL) {
-        for(i = 0; i < sk_SSL_CIPHER_num(scsvs); i++) {
-            c = sk_SSL_CIPHER_value(scsvs, i);
-            if (SSL_CIPHER_get_id(c) == SSL3_CK_SCSV) {
-                if (s->renegotiate) {
-                    /* SCSV is fatal if renegotiating */
-                    SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO,
-                           SSL_R_SCSV_RECEIVED_WHEN_RENEGOTIATING);
-                    al = SSL_AD_HANDSHAKE_FAILURE;
-                    goto err;
-                }
-                s->s3->send_connection_binding = 1;
-            } else if (SSL_CIPHER_get_id(c) == SSL3_CK_FALLBACK_SCSV &&
-                       !ssl_check_version_downgrade(s)) {
-                /*
-                 * This SCSV indicates that the client previously tried
-                 * a higher version.  We should fail if the current version
-                 * is an unexpected downgrade, as that indicates that the first
-                 * connection may have been tampered with in order to trigger
-                 * an insecure downgrade.
-                 */
-                SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO,
-                       SSL_R_INAPPROPRIATE_FALLBACK);
-                al = SSL_AD_INAPPROPRIATE_FALLBACK;
-                goto err;
-            }
-        }
-    }
-
     /*
-     * If it is a hit, check that the cipher is in the list. In TLSv1.3 we can
-     * resume with a differnt cipher as long as the hash is the same so this
-     * check does not apply.
+     * If it is a hit, check that the cipher is in the list. In TLSv1.3 we check
+     * ciphersuite compatibility with the session as part of resumption.
      */
     if (!SSL_IS_TLS13(s) && s->hit) {
         j = 0;
@@ -1720,7 +1743,11 @@ static int tls_early_post_process_client_hello(SSL *s, int *pal)
         }
     }
 
-    if (!s->hit && s->version >= TLS1_VERSION && s->ext.session_secret_cb) {
+    if (!s->hit
+            && s->version >= TLS1_VERSION
+            && !SSL_IS_TLS13(s)
+            && !SSL_IS_DTLS(s)
+            && s->ext.session_secret_cb) {
         const SSL_CIPHER *pref_cipher = NULL;
         /*
          * s->session->master_key_length is a size_t, but this is an int for
@@ -1962,7 +1989,7 @@ WORK_STATE tls_post_process_client_hello(SSL *s, WORK_STATE wst)
     if (wst == WORK_MORE_B) {
         if (!s->hit || SSL_IS_TLS13(s)) {
             /* Let cert callback update server certificates if required */
-            if (s->cert->cert_cb) {
+            if (!s->hit && s->cert->cert_cb != NULL) {
                 int rv = s->cert->cert_cb(s, s->cert->cert_cb_arg);
                 if (rv == 0) {
                     al = SSL_AD_INTERNAL_ERROR;
@@ -1976,25 +2003,19 @@ WORK_STATE tls_post_process_client_hello(SSL *s, WORK_STATE wst)
                 }
                 s->rwstate = SSL_NOTHING;
             }
-            cipher =
-                ssl3_choose_cipher(s, s->session->ciphers, SSL_get_ciphers(s));
 
-            if (cipher == NULL) {
-                SSLerr(SSL_F_TLS_POST_PROCESS_CLIENT_HELLO,
-                       SSL_R_NO_SHARED_CIPHER);
-                goto f_err;
-            }
-            if (s->hello_retry_request && s->s3->tmp.new_cipher != NULL
-                    && s->s3->tmp.new_cipher->id != cipher->id) {
-                /*
-                 * A previous HRR picked a different ciphersuite to the one we
-                 * just selected. Something must have changed.
-                 */
-                al = SSL_AD_ILLEGAL_PARAMETER;
-                SSLerr(SSL_F_TLS_POST_PROCESS_CLIENT_HELLO, SSL_R_BAD_CIPHER);
-                goto f_err;
+            /* In TLSv1.3 we selected the ciphersuite before resumption */
+            if (!SSL_IS_TLS13(s)) {
+                cipher =
+                    ssl3_choose_cipher(s, s->session->ciphers, SSL_get_ciphers(s));
+
+                if (cipher == NULL) {
+                    SSLerr(SSL_F_TLS_POST_PROCESS_CLIENT_HELLO,
+                           SSL_R_NO_SHARED_CIPHER);
+                    goto f_err;
+                }
+                s->s3->tmp.new_cipher = cipher;
             }
-            s->s3->tmp.new_cipher = cipher;
             if (!s->hit) {
                 if (!tls_choose_sigalg(s, &al))
                     goto f_err;

diff --git a/test/sslapitest.c b/test/sslapitest.c
index 6162625d062b9ad8474cbc435ab99ecd64e3afa0..13ba727c5daa74441a178c02372bb5404f966e0a 100644 (file)
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -1850,16 +1850,15 @@ static int test_ciphersuite_change(void)
 
     /*
      * Check attempting to resume a SHA-256 session with no SHA-256 ciphersuites
-     * fails.
+     * succeeds but does not resume.
      */
     if (!TEST_true(SSL_CTX_set_cipher_list(cctx, "TLS13-AES-256-GCM-SHA384"))
             || !TEST_true(create_ssl_objects(sctx, cctx, &serverssl, &clientssl,
                                              NULL, NULL))
             || !TEST_true(SSL_set_session(clientssl, clntsess))
-            || !TEST_false(create_ssl_connection(serverssl, clientssl,
+            || !TEST_true(create_ssl_connection(serverssl, clientssl,
                                                 SSL_ERROR_SSL))
-            || !TEST_int_eq(ERR_GET_REASON(ERR_get_error()),
-                            SSL_R_NO_SHARED_CIPHER))
+            || !TEST_false(SSL_session_reused(clientssl)))
         goto end;
 
     SSL_SESSION_free(clntsess);
@@ -1887,6 +1886,8 @@ static int test_ciphersuite_change(void)
 
     if (!TEST_true(SSL_CTX_set_cipher_list(cctx,
                    "TLS13-AES-128-GCM-SHA256:TLS13-AES-256-GCM-SHA384"))
+            || !TEST_true(SSL_CTX_set_cipher_list(sctx,
+                                                  "TLS13-AES-256-GCM-SHA384"))
             || !TEST_true(create_ssl_objects(sctx, cctx, &serverssl, &clientssl,
                                              NULL, NULL))
             || !TEST_true(SSL_set_session(clientssl, clntsess))