From fc7129dc37f38022382338cf37cee795d975450f Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Mon, 13 Nov 2017 11:24:51 +0000 Subject: [PATCH] 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 (Merged from https://github.com/openssl/openssl/pull/4701) --- ssl/record/ssl3_record.c | 4 ++-- ssl/ssl_locl.h | 3 ++- ssl/statem/extensions.c | 12 ++++++------ ssl/statem/extensions_clnt.c | 10 +++++----- ssl/statem/extensions_srvr.c | 4 ++-- ssl/statem/statem_clnt.c | 12 +++++++----- ssl/statem/statem_lib.c | 7 ++++--- ssl/statem/statem_srvr.c | 35 +++++++++++++++++++++++------------ 8 files changed, 51 insertions(+), 36 deletions(-) diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c index c1776e98f1..28ee2cc005 100644 --- a/ssl/record/ssl3_record.c +++ b/ssl/record/ssl3_record.c @@ -276,7 +276,7 @@ int ssl3_get_record(SSL *s) * 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 @@ int ssl3_get_record(SSL *s) 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 23cb31ab69..6b899691f9 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -1117,7 +1117,8 @@ struct ssl_st { 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 988e919044..026126d4d8 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -1235,7 +1235,7 @@ static int final_key_share(SSL *s, unsigned int context, int sent) */ 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 @@ static int final_key_share(SSL *s, unsigned int context, int sent) 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 @@ static int final_key_share(SSL *s, unsigned int context, int sent) } /* 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 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart, * 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 @@ static int final_early_data(SSL *s, unsigned int context, int sent) || 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 2640756134..1fbf9f6e0e 100644 --- a/ssl/statem/extensions_clnt.c +++ b/ssl/statem/extensions_clnt.c @@ -599,7 +599,7 @@ static int add_key_share(SSL *s, WPACKET *pkt, unsigned int curve_id) 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 @@ EXT_RETURN tls_construct_ctos_early_data(SSL *s, WPACKET *pkt, 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 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context, || (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 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context, 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 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context, 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 93ac98f116..d34a7c5ee5 100644 --- a/ssl/statem/extensions_srvr.c +++ b/ssl/statem/extensions_srvr.c @@ -704,7 +704,7 @@ int tls_parse_ctos_early_data(SSL *s, PACKET *pkt, unsigned int context, 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 @@ EXT_RETURN tls_construct_stoc_key_share(SSL *s, WPACKET *pkt, 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 af9e1dcd7d..80148fa531 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -387,7 +387,7 @@ static WRITE_TRAN ossl_statem_client13_write_transition(SSL *s) || 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 @@ int tls_construct_client_hello(SSL *s, WPACKET *pkt) 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 @@ int tls_construct_client_hello(SSL *s, WPACKET *pkt) } } } 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 @@ int tls_construct_client_hello(SSL *s, WPACKET *pkt) 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 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt) && 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 d64ddffffd..b65dfa1587 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -1656,7 +1656,7 @@ int ssl_choose_server_version(SSL *s, CLIENTHELLO_MSG *hello, DOWNGRADE *dgrd) 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 @@ int ssl_choose_server_version(SSL *s, CLIENTHELLO_MSG *hello, DOWNGRADE *dgrd) } 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 @@ int ssl_choose_client_version(SSL *s, int version, RAW_EXTENSION *extensions) 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 3ebe76530a..7d1d15dcc1 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -48,7 +48,7 @@ static int ossl_statem_server13_read_transition(SSL *s, int mt) 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 @@ static WRITE_TRAN ossl_statem_server13_write_transition(SSL *s) 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 @@ WORK_STATE ossl_statem_server_pre_work(SSL *s, WORK_STATE wst) 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 @@ WORK_STATE ossl_statem_server_post_work(SSL *s, WORK_STATE wst) 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 @@ WORK_STATE ossl_statem_server_post_work(SSL *s, WORK_STATE wst) } #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 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt) 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 @@ static int tls_early_post_process_client_hello(SSL *s) 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 @@ int tls_construct_server_hello(SSL *s, WPACKET *pkt) 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 @@ int tls_construct_server_hello(SSL *s, WPACKET *pkt) * 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 @@ int tls_construct_server_hello(SSL *s, WPACKET *pkt) || !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 @@ int tls_construct_server_hello(SSL *s, WPACKET *pkt) 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; -- 2.25.1