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 e8bda66..41c44ce 100644
--- a/ssl/s3_lib.c
+++ b/ssl/s3_lib.c
@@ -3680,7 +3680,7 @@
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 @@
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 @@
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 fe181ab..39e6c07 100644
--- a/ssl/statem/extensions_srvr.c
+++ b/ssl/statem/extensions_srvr.c
@@ -726,11 +726,8 @@
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 8137a7d..53b8ef9 100644
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -1566,6 +1566,68 @@
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 @@
}
}
- 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 @@
}
}
- 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 @@
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 @@
}
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;
+ /* 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;
}
- 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;
diff --git a/test/sslapitest.c b/test/sslapitest.c
index 6162625..13ba727 100644
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -1850,16 +1850,15 @@
/*
* 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 @@
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))
