Clean up TLS 1.3 handback logic. There's no need to treat the 1-RTT and 0-RTT handback flows differently. This aligns the 1-RTT handback with the 0-RTT point. This consistently installs the decryption keys in the state machine after handback rather than while applying the handback. Update-Note: This changes the serialization format for TLS 1.3 split handshakes, which were only just added. Change-Id: I0e109cb8d9ecd3c8658dfa26059cbe0139f82eed Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/39988 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Matt Braithwaite <mab@google.com>
diff --git a/ssl/handoff.cc b/ssl/handoff.cc index 3926939..a9da05e 100644 --- a/ssl/handoff.cc +++ b/ssl/handoff.cc
@@ -258,16 +258,10 @@ type = handback_after_handshake; break; case state12_tls13: - switch (hs->tls13_state) { - case state13_send_half_rtt_ticket: - type = handback_tls13_early_data; - break; - case state13_process_end_of_early_data: - type = handback_tls13; - break; - default: - return false; + if (hs->tls13_state != state13_send_half_rtt_ticket) { + return false; } + type = handback_tls13; break; default: return false; @@ -303,7 +297,7 @@ // TODO(mab): make sure everything is serialized. CBB seq, key_share; const SSL_SESSION *session; - if (type == handback_tls13 || type == handback_tls13_early_data) { + if (type == handback_tls13) { session = hs->new_session.get(); } else { session = s3->session_reused ? ssl->session.get() : hs->new_session.get(); @@ -348,7 +342,7 @@ !s3->hs->key_shares[0]->Serialize(&key_share)) { return false; } - if (type == handback_tls13 || type == handback_tls13_early_data) { + if (type == handback_tls13) { early_data_t early_data; // Check early data invariants. if (ssl->enable_early_data == @@ -389,7 +383,7 @@ !CBB_add_asn1_uint64(&seq, early_data)) { return false; } - if (type == handback_tls13_early_data && + if (early_data == early_data_accepted && !CBB_add_asn1_octet_string(&seq, hs->early_traffic_secret().data(), hs->early_traffic_secret().size())) { return false; @@ -413,7 +407,7 @@ } SSL3_STATE *const s3 = ssl->s3; - uint64_t handback_version, negotiated_token_binding_param, cipher, type; + uint64_t handback_version, negotiated_token_binding_param, cipher, type_u64; CBS seq, read_seq, write_seq, server_rand, client_rand, read_iv, write_iv, next_proto, alpn, hostname, channel_id, transcript, key_share; @@ -425,10 +419,12 @@ if (!CBS_get_asn1(&handback_cbs, &seq, CBS_ASN1_SEQUENCE) || !CBS_get_asn1_uint64(&seq, &handback_version) || handback_version != kHandbackVersion || - !CBS_get_asn1_uint64(&seq, &type)) { + !CBS_get_asn1_uint64(&seq, &type_u64) || + type_u64 > handback_max_value) { return false; } + handback_t type = static_cast<handback_t>(type_u64); if (!CBS_get_asn1(&seq, &read_seq, CBS_ASN1_OCTETSTRING) || CBS_len(&read_seq) != sizeof(s3->read_sequence) || !CBS_get_asn1(&seq, &write_seq, CBS_ASN1_OCTETSTRING) || @@ -450,8 +446,7 @@ s3->hs = ssl_handshake_new(ssl); SSL_HANDSHAKE *const hs = s3->hs.get(); - if (!session_reused || type == handback_tls13 || - type == handback_tls13_early_data) { + if (!session_reused || type == handback_tls13) { hs->new_session = SSL_SESSION_parse(&seq, ssl->ctx->x509_method, ssl->ctx->pool); session = hs->new_session.get(); @@ -487,7 +482,7 @@ } CBS client_handshake_secret, server_handshake_secret, client_traffic_secret_0, server_traffic_secret_0, secret, exporter_secret, early_traffic_secret; - if (type == handback_tls13 || type == handback_tls13_early_data) { + if (type == handback_tls13) { int used_hello_retry_request, accept_psk_mode; uint64_t early_data, early_data_reason; int64_t ticket_age_skew; @@ -506,7 +501,8 @@ early_data > early_data_max_value) { return false; } - if (type == handback_tls13_early_data && + early_data_t early_data_type = static_cast<early_data_t>(early_data); + if (early_data_type == early_data_accepted && !CBS_get_asn1(&seq, &early_traffic_secret, CBS_ASN1_OCTETSTRING)) { return false; } @@ -524,11 +520,6 @@ s3->skip_early_data = false; s3->early_data_accepted = false; hs->early_data_offered = false; - early_data_t early_data_type = static_cast<early_data_t>(early_data); - if ((type == handback_tls13_early_data) != - (early_data_type == early_data_accepted)) { - return false; - } switch (early_data_type) { case early_data_not_offered: break; @@ -581,10 +572,6 @@ break; case handback_tls13: hs->state = state12_tls13; - hs->tls13_state = state13_read_client_certificate; - break; - case handback_tls13_early_data: - hs->state = state12_tls13; hs->tls13_state = state13_send_half_rtt_ticket; break; default: @@ -622,7 +609,7 @@ !hs->transcript.Update(transcript))) { return false; } - if (type == handback_tls13 || type == handback_tls13_early_data) { + if (type == handback_tls13) { hs->ResizeSecrets(hs->transcript.DigestLen()); if (!CopyExact(hs->client_traffic_secret_0(), &client_traffic_secret_0) || !CopyExact(hs->server_traffic_secret_0(), &server_traffic_secret_0) || @@ -635,32 +622,42 @@ } s3->exporter_secret_len = CBS_len(&exporter_secret); - if (type == handback_tls13_early_data && + if (s3->early_data_accepted && !CopyExact(hs->early_traffic_secret(), &early_traffic_secret)) { return false; } } Array<uint8_t> key_block; - if ((type == handback_after_session_resumption || - type == handback_after_handshake) && - !tls1_configure_aead(ssl, evp_aead_seal, &key_block, session->cipher, - write_iv)) { - return false; - } - if (type == handback_after_handshake && - !tls1_configure_aead(ssl, evp_aead_open, &key_block, session->cipher, - read_iv)) { - return false; - } - if (type == handback_tls13 || type == handback_tls13_early_data) { - auto open_key = type == handback_tls13 ? hs->client_handshake_secret() - : hs->early_traffic_secret(); - if (!tls13_set_traffic_key(ssl, ssl_encryption_handshake, evp_aead_open, - open_key) || - !tls13_set_traffic_key(ssl, ssl_encryption_application, evp_aead_seal, - hs->server_traffic_secret_0())) { - return false; - } + switch (type) { + case handback_after_session_resumption: + // The write keys are installed after server Finished, but the client + // keys must wait for ChangeCipherSpec. + if (!tls1_configure_aead(ssl, evp_aead_seal, &key_block, session->cipher, + write_iv)) { + return false; + } + break; + case handback_after_ecdhe: + // The premaster secret is not yet computed, so install no keys. + break; + case handback_after_handshake: + // The handshake is complete, so both keys are installed. + if (!tls1_configure_aead(ssl, evp_aead_seal, &key_block, session->cipher, + write_iv) || + !tls1_configure_aead(ssl, evp_aead_open, &key_block, session->cipher, + read_iv)) { + return false; + } + break; + case handback_tls13: + // After server Finished, the application write keys are installed, but + // none of the read keys. The read keys are installed in the state machine + // immediately after processing handback. + if (!tls13_set_traffic_key(ssl, ssl_encryption_application, evp_aead_seal, + hs->server_traffic_secret_0())) { + return false; + } + break; } if (!CopyExact({s3->read_sequence, sizeof(s3->read_sequence)}, &read_seq) || !CopyExact({s3->write_sequence, sizeof(s3->write_sequence)},
diff --git a/ssl/internal.h b/ssl/internal.h index 32303cb..4a93e15 100644 --- a/ssl/internal.h +++ b/ssl/internal.h
@@ -1499,11 +1499,11 @@ // handback_t lists the points in the state machine where a handback can occur. // These are the different points at which key material is no longer needed. enum handback_t { - handback_after_session_resumption, - handback_after_ecdhe, - handback_after_handshake, - handback_tls13, - handback_tls13_early_data, + handback_after_session_resumption = 0, + handback_after_ecdhe = 1, + handback_after_handshake = 2, + handback_tls13 = 3, + handback_max_value = handback_tls13, };
diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc index baf2a04..fda0bca 100644 --- a/ssl/tls13_server.cc +++ b/ssl/tls13_server.cc
@@ -705,17 +705,13 @@ } hs->tls13_state = state13_send_half_rtt_ticket; - return ssl_hs_ok; + return hs->handback ? ssl_hs_handback : ssl_hs_ok; } static enum ssl_hs_wait_t do_send_half_rtt_ticket(SSL_HANDSHAKE *hs) { SSL *const ssl = hs->ssl; if (ssl->s3->early_data_accepted) { - if (hs->handback) { - return ssl_hs_handback; - } - // We defer releasing the early traffic secret to QUIC to this point. First, // the early traffic secret is derived before ECDHE, but ECDHE may later // reject 0-RTT. We only release the secret after 0-RTT is fully resolved. @@ -828,9 +824,6 @@ hs->client_handshake_secret())) { return ssl_hs_error; } - if (hs->handback) { - return ssl_hs_handback; - } hs->tls13_state = state13_read_client_certificate; return ssl_hs_ok; }