Update state machine to send CCS based on whether we did an HRR
The CCS may be sent at different times based on whether or not we
sent an HRR earlier. In order to make that decision this commit
also updates things to make sure we remember whether an HRR was
used or not.
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/4701)
diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c
index c1776e9..28ee2cc 100644
--- a/ssl/record/ssl3_record.c
+++ b/ssl/record/ssl3_record.c
@@ -276,7 +276,7 @@
* that explicitly
*/
if (!s->first_packet && !SSL_IS_TLS13(s)
- && !s->hello_retry_request
+ && s->hello_retry_request != SSL_HRR_PENDING
&& version != (unsigned int)s->version) {
if ((s->version & 0xFF00) == (version & 0xFF00)
&& !s->enc_write_ctx && !s->write_hash) {
@@ -449,7 +449,7 @@
if (num_recs == 1
&& thisrr->type == SSL3_RT_CHANGE_CIPHER_SPEC
- && SSL_IS_TLS13(s)
+ && (SSL_IS_TLS13(s) || s->hello_retry_request != SSL_HRR_NONE)
&& SSL_IS_FIRST_HANDSHAKE(s)) {
/*
* CCS messages must be exactly 1 byte long, containing the value 0x01
diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index 23cb31a..6b89969 100644
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -1117,7 +1117,8 @@
size_t cert_verify_hash_len;
/* Flag to indicate whether we should send a HelloRetryRequest or not */
- int hello_retry_request;
+ enum {SSL_HRR_NONE = 0, SSL_HRR_PENDING, SSL_HRR_COMPLETE}
+ hello_retry_request;
/*
* the session_id_context is used to ensure sessions are only reused in
diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c
index 988e919..026126d 100644
--- a/ssl/statem/extensions.c
+++ b/ssl/statem/extensions.c
@@ -1235,7 +1235,7 @@
*/
if (s->server && s->s3->peer_tmp == NULL) {
/* No suitable share */
- if (s->hello_retry_request == 0 && sent
+ if (s->hello_retry_request == SSL_HRR_NONE && sent
&& (!s->hit
|| (s->ext.psk_kex_mode & TLSEXT_KEX_MODE_FLAG_KE_DHE)
!= 0)) {
@@ -1260,7 +1260,7 @@
if (i < num_groups) {
/* A shared group exists so send a HelloRetryRequest */
s->s3->group_id = group_id;
- s->hello_retry_request = 1;
+ s->hello_retry_request = SSL_HRR_PENDING;
return 1;
}
}
@@ -1275,8 +1275,8 @@
}
/* We have a key_share so don't send any more HelloRetryRequest messages */
- if (s->server)
- s->hello_retry_request = 0;
+ if (s->server && s->hello_retry_request == SSL_HRR_PENDING)
+ s->hello_retry_request = SSL_HRR_COMPLETE;
/*
* For a client side resumption with no key_share we need to generate
@@ -1405,7 +1405,7 @@
* following a HelloRetryRequest then this includes the hash of the first
* ClientHello and the HelloRetryRequest itself.
*/
- if (s->hello_retry_request) {
+ if (s->hello_retry_request == SSL_HRR_PENDING) {
size_t hdatalen;
void *hdata;
@@ -1516,7 +1516,7 @@
|| s->session->ext.tick_identity != 0
|| s->early_data_state != SSL_EARLY_DATA_ACCEPTING
|| !s->ext.early_data_ok
- || s->hello_retry_request) {
+ || s->hello_retry_request != SSL_HRR_NONE) {
s->ext.early_data = SSL_EARLY_DATA_REJECTED;
} else {
s->ext.early_data = SSL_EARLY_DATA_ACCEPTED;
diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c
index 2640756..1fbf9f6 100644
--- a/ssl/statem/extensions_clnt.c
+++ b/ssl/statem/extensions_clnt.c
@@ -599,7 +599,7 @@
size_t encodedlen;
if (s->s3->tmp.pkey != NULL) {
- if (!ossl_assert(s->hello_retry_request)) {
+ if (!ossl_assert(s->hello_retry_request == SSL_HRR_PENDING)) {
SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_ADD_KEY_SHARE,
ERR_R_INTERNAL_ERROR);
return 0;
@@ -749,7 +749,7 @@
SSL_SESSION *edsess = NULL;
const EVP_MD *handmd = NULL;
- if (s->hello_retry_request)
+ if (s->hello_retry_request == SSL_HRR_PENDING)
handmd = ssl_handshake_md(s);
if (s->psk_use_session_cb != NULL
@@ -961,7 +961,7 @@
|| (s->session->ext.ticklen == 0 && s->psksession == NULL))
return EXT_RETURN_NOT_SENT;
- if (s->hello_retry_request)
+ if (s->hello_retry_request == SSL_HRR_PENDING)
handmd = ssl_handshake_md(s);
if (s->session->ext.ticklen != 0) {
@@ -980,7 +980,7 @@
goto dopsksess;
}
- if (s->hello_retry_request && mdres != handmd) {
+ if (s->hello_retry_request == SSL_HRR_PENDING && mdres != handmd) {
/*
* Selected ciphersuite hash does not match the hash for the session
* so we can't use it.
@@ -1044,7 +1044,7 @@
return EXT_RETURN_FAIL;
}
- if (s->hello_retry_request && mdpsk != handmd) {
+ if (s->hello_retry_request == SSL_HRR_PENDING && mdpsk != handmd) {
/*
* Selected ciphersuite hash does not match the hash for the PSK
* session. This is an application bug.
diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c
index 93ac98f..d34a7c5 100644
--- a/ssl/statem/extensions_srvr.c
+++ b/ssl/statem/extensions_srvr.c
@@ -704,7 +704,7 @@
return 0;
}
- if (s->hello_retry_request) {
+ if (s->hello_retry_request != SSL_HRR_NONE) {
SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER,
SSL_F_TLS_PARSE_CTOS_EARLY_DATA, SSL_R_BAD_EXTENSION);
return 0;
@@ -1245,7 +1245,7 @@
if (ckey == NULL) {
/* No key_share received from client */
- if (s->hello_retry_request) {
+ if (s->hello_retry_request == SSL_HRR_PENDING) {
if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_key_share)
|| !WPACKET_start_sub_packet_u16(pkt)
|| !WPACKET_put_bytes_u16(pkt, s->s3->group_id)
diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index af9e1dc..80148fa 100644
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -387,7 +387,7 @@
|| s->early_data_state == SSL_EARLY_DATA_FINISHED_WRITING)
st->hand_state = TLS_ST_PENDING_EARLY_DATA_END;
else if ((s->options & SSL_OP_ENABLE_MIDDLEBOX_COMPAT) != 0
- && !s->hello_retry_request)
+ && s->hello_retry_request == SSL_HRR_NONE)
st->hand_state = TLS_ST_CW_CHANGE;
else
st->hand_state = (s->s3->tmp.cert_req != 0) ? TLS_ST_CW_CERT
@@ -1055,7 +1055,8 @@
if (sess == NULL
|| !ssl_version_supported(s, sess->ssl_version)
|| !SSL_SESSION_is_resumable(sess)) {
- if (!s->hello_retry_request && !ssl_get_new_session(s, 0)) {
+ if (s->hello_retry_request == SSL_HRR_NONE
+ && !ssl_get_new_session(s, 0)) {
/* SSLfatal() already called */
return 0;
}
@@ -1078,7 +1079,7 @@
}
}
} else {
- i = s->hello_retry_request == 0;
+ i = (s->hello_retry_request == SSL_HRR_NONE);
}
if (i && ssl_fill_hello_random(s, 0, p, sizeof(s->s3->client_random),
@@ -1136,7 +1137,7 @@
sess_id_len = sizeof(s->tmp_session_id);
s->tmp_session_id_len = sess_id_len;
session_id = s->tmp_session_id;
- if (!s->hello_retry_request
+ if (s->hello_retry_request == SSL_HRR_NONE
&& ssl_randbytes(s, s->tmp_session_id,
sess_id_len) <= 0) {
SSLfatal(s, SSL_AD_INTERNAL_ERROR,
@@ -1360,7 +1361,8 @@
&& sversion == TLS1_2_VERSION
&& PACKET_remaining(pkt) >= SSL3_RANDOM_SIZE
&& memcmp(hrrrandom, PACKET_data(pkt), SSL3_RANDOM_SIZE) == 0) {
- s->hello_retry_request = hrr = 1;
+ s->hello_retry_request = SSL_HRR_PENDING;
+ hrr = 1;
if (!PACKET_forward(pkt, SSL3_RANDOM_SIZE)) {
SSLfatal(s, SSL_AD_DECODE_ERROR, SSL_F_TLS_PROCESS_SERVER_HELLO,
SSL_R_LENGTH_MISMATCH);
diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index d64ddff..b65dfa1 100644
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -1656,7 +1656,7 @@
suppversions = &hello->pre_proc_exts[TLSEXT_IDX_supported_versions];
/* If we did an HRR then supported versions is mandatory */
- if (!suppversions->present && s->hello_retry_request)
+ if (!suppversions->present && s->hello_retry_request != SSL_HRR_NONE)
return SSL_R_UNSUPPORTED_PROTOCOL;
if (suppversions->present && !SSL_IS_DTLS(s)) {
@@ -1703,7 +1703,7 @@
}
if (best_vers > 0) {
- if (s->hello_retry_request) {
+ if (s->hello_retry_request != SSL_HRR_NONE) {
/*
* This is after a HelloRetryRequest so we better check that we
* negotiated TLSv1.3
@@ -1779,7 +1779,8 @@
return 0;
}
- if (s->hello_retry_request && s->version != TLS1_3_VERSION) {
+ if (s->hello_retry_request != SSL_HRR_NONE
+ && s->version != TLS1_3_VERSION) {
s->version = origv;
SSLfatal(s, SSL_AD_PROTOCOL_VERSION, SSL_F_SSL_CHOOSE_CLIENT_VERSION,
SSL_R_WRONG_SSL_VERSION);
diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index 3ebe765..7d1d15d 100644
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -48,7 +48,7 @@
break;
case TLS_ST_EARLY_DATA:
- if (s->hello_retry_request) {
+ if (s->hello_retry_request == SSL_HRR_PENDING) {
if (mt == SSL3_MT_CLIENT_HELLO) {
st->hand_state = TLS_ST_SR_CLNT_HELLO;
return 1;
@@ -395,16 +395,20 @@
return WRITE_TRAN_CONTINUE;
case TLS_ST_SW_SRVR_HELLO:
- if (s->hello_retry_request)
- st->hand_state = TLS_ST_EARLY_DATA;
- else if ((s->options & SSL_OP_ENABLE_MIDDLEBOX_COMPAT) != 0)
+ if ((s->options & SSL_OP_ENABLE_MIDDLEBOX_COMPAT) != 0
+ && s->hello_retry_request != SSL_HRR_COMPLETE)
st->hand_state = TLS_ST_SW_CHANGE;
+ else if (s->hello_retry_request == SSL_HRR_PENDING)
+ st->hand_state = TLS_ST_EARLY_DATA;
else
st->hand_state = TLS_ST_SW_ENCRYPTED_EXTENSIONS;
return WRITE_TRAN_CONTINUE;
case TLS_ST_SW_CHANGE:
- st->hand_state = TLS_ST_SW_ENCRYPTED_EXTENSIONS;
+ if (s->hello_retry_request == SSL_HRR_PENDING)
+ st->hand_state = TLS_ST_EARLY_DATA;
+ else
+ st->hand_state = TLS_ST_SW_ENCRYPTED_EXTENSIONS;
return WRITE_TRAN_CONTINUE;
case TLS_ST_SW_ENCRYPTED_EXTENSIONS:
@@ -664,6 +668,8 @@
break;
case TLS_ST_SW_CHANGE:
+ if (SSL_IS_TLS13(s))
+ break;
s->session->cipher = s->s3->tmp.new_cipher;
if (!s->method->ssl3_enc->setup_key_block(s)) {
/* SSLfatal() already called */
@@ -733,7 +739,7 @@
break;
case TLS_ST_SW_SRVR_HELLO:
- if (SSL_IS_TLS13(s) && s->hello_retry_request) {
+ if (SSL_IS_TLS13(s) && s->hello_retry_request == SSL_HRR_PENDING) {
if (statem_flush(s) != 1)
return WORK_MORE_A;
break;
@@ -765,11 +771,14 @@
}
#endif
if (!SSL_IS_TLS13(s)
- || (s->options & SSL_OP_ENABLE_MIDDLEBOX_COMPAT) != 0)
+ || ((s->options & SSL_OP_ENABLE_MIDDLEBOX_COMPAT) != 0
+ && s->hello_retry_request != SSL_HRR_COMPLETE))
break;
/* Fall through */
case TLS_ST_SW_CHANGE:
+ if (s->hello_retry_request == SSL_HRR_PENDING)
+ break;
/*
* TODO(TLS1.3): This actually causes a problem. We don't yet know
* whether the next record we are going to receive is an unencrypted
@@ -1267,7 +1276,8 @@
if (clienthello->isv2) {
unsigned int mt;
- if (!SSL_IS_FIRST_HANDSHAKE(s) || s->hello_retry_request) {
+ if (!SSL_IS_FIRST_HANDSHAKE(s)
+ || s->hello_retry_request != SSL_HRR_NONE) {
SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE,
SSL_F_TLS_PROCESS_CLIENT_HELLO, SSL_R_UNEXPECTED_MESSAGE);
goto err;
@@ -1624,7 +1634,7 @@
SSL_R_NO_SHARED_CIPHER);
goto err;
}
- if (s->hello_retry_request
+ if (s->hello_retry_request == SSL_HRR_PENDING
&& (s->s3->tmp.new_cipher == NULL
|| s->s3->tmp.new_cipher->id != cipher->id)) {
/*
@@ -2200,7 +2210,7 @@
size_t sl, len;
int version;
unsigned char *session_id;
- int usetls13 = SSL_IS_TLS13(s) || s->hello_retry_request;
+ int usetls13 = SSL_IS_TLS13(s) || s->hello_retry_request == SSL_HRR_PENDING;
version = usetls13 ? TLS1_2_VERSION : s->version;
if (!WPACKET_put_bytes_u16(pkt, version)
@@ -2209,7 +2219,7 @@
* tls_process_client_hello()
*/
|| !WPACKET_memcpy(pkt,
- s->hello_retry_request
+ s->hello_retry_request == SSL_HRR_PENDING
? hrrrandom : s->s3->server_random,
SSL3_RANDOM_SIZE)) {
SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_SERVER_HELLO,
@@ -2269,6 +2279,7 @@
|| !WPACKET_put_bytes_u8(pkt, compm)
|| !tls_construct_extensions(s, pkt,
s->hello_retry_request
+ == SSL_HRR_PENDING
? SSL_EXT_TLS1_3_HELLO_RETRY_REQUEST
: (SSL_IS_TLS13(s)
? SSL_EXT_TLS1_3_SERVER_HELLO
@@ -2278,7 +2289,7 @@
return 0;
}
- if (s->hello_retry_request) {
+ if (s->hello_retry_request == SSL_HRR_PENDING) {
/* Ditch the session. We'll create a new one next time around */
SSL_SESSION_free(s->session);
s->session = NULL;
