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: NBen Kaduk <kaduk@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/3623)

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;
     }
 
+    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

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;

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,6 +2003,9 @@ WORK_STATE tls_post_process_client_hello(SSL *s, WORK_STATE wst)
                 }
                 s->rwstate = SSL_NOTHING;
             }
+
+            /* 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));
 
@@ -1984,17 +2014,8 @@ WORK_STATE tls_post_process_client_hello(SSL *s, WORK_STATE wst)
                            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;
-            }
                 s->s3->tmp.new_cipher = cipher;
+            }
             if (!s->hit) {
                 if (!tls_choose_sigalg(s, &al))
                     goto f_err;

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))