Use spans for the various TLS 1.3 secrets. This undoes a lot of the MakeConstSpans and MakeSpans that were just added, though it does require a bit of helper machinery. This should make us much more consistent about which buffer is sized with which size (even though they are secretly all the same size). Change-Id: I772ffd2e69141ff20511bcd3add865afa82cf3a0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/37127 Commit-Queue: Steven Valdez <svaldez@google.com> Reviewed-by: Steven Valdez <svaldez@google.com>
diff --git a/ssl/handshake.cc b/ssl/handshake.cc index 3df004c..d62f986 100644 --- a/ssl/handshake.cc +++ b/ssl/handshake.cc
@@ -157,6 +157,13 @@ ssl->ctx->x509_method->hs_flush_cached_ca_names(this); } +void SSL_HANDSHAKE::ResizeSecrets(size_t hash_len) { + if (hash_len > SSL_MAX_MD_SIZE) { + abort(); + } + hash_len_ = hash_len; +} + UniquePtr<SSL_HANDSHAKE> ssl_handshake_new(SSL *ssl) { UniquePtr<SSL_HANDSHAKE> hs = MakeUnique<SSL_HANDSHAKE>(ssl); if (!hs || !hs->transcript.Init()) {
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc index e1b6afe..4e8ca6a 100644 --- a/ssl/handshake_client.cc +++ b/ssl/handshake_client.cc
@@ -460,9 +460,8 @@ hs, MakeConstSpan(ssl->session->master_key, ssl->session->master_key_length)) || !tls13_derive_early_secrets(hs) || - !tls13_set_traffic_key( - ssl, ssl_encryption_early_data, evp_aead_seal, - MakeConstSpan(hs->early_traffic_secret, hs->hash_len))) { + !tls13_set_traffic_key(ssl, ssl_encryption_early_data, evp_aead_seal, + hs->early_traffic_secret())) { return ssl_hs_error; }
diff --git a/ssl/internal.h b/ssl/internal.h index e9b8201..ed37993 100644 --- a/ssl/internal.h +++ b/ssl/internal.h
@@ -1452,14 +1452,38 @@ // |SSL_OP_NO_*| and |SSL_CTX_set_max_proto_version| APIs. uint16_t max_version = 0; - size_t hash_len = 0; - uint8_t secret[SSL_MAX_MD_SIZE] = {0}; - uint8_t early_traffic_secret[SSL_MAX_MD_SIZE] = {0}; - uint8_t client_handshake_secret[SSL_MAX_MD_SIZE] = {0}; - uint8_t server_handshake_secret[SSL_MAX_MD_SIZE] = {0}; - uint8_t client_traffic_secret_0[SSL_MAX_MD_SIZE] = {0}; - uint8_t server_traffic_secret_0[SSL_MAX_MD_SIZE] = {0}; - uint8_t expected_client_finished[SSL_MAX_MD_SIZE] = {0}; + private: + size_t hash_len_ = 0; + uint8_t secret_[SSL_MAX_MD_SIZE] = {0}; + uint8_t early_traffic_secret_[SSL_MAX_MD_SIZE] = {0}; + uint8_t client_handshake_secret_[SSL_MAX_MD_SIZE] = {0}; + uint8_t server_handshake_secret_[SSL_MAX_MD_SIZE] = {0}; + uint8_t client_traffic_secret_0_[SSL_MAX_MD_SIZE] = {0}; + uint8_t server_traffic_secret_0_[SSL_MAX_MD_SIZE] = {0}; + uint8_t expected_client_finished_[SSL_MAX_MD_SIZE] = {0}; + + public: + void ResizeSecrets(size_t hash_len); + + Span<uint8_t> secret() { return MakeSpan(secret_, hash_len_); } + Span<uint8_t> early_traffic_secret() { + return MakeSpan(early_traffic_secret_, hash_len_); + } + Span<uint8_t> client_handshake_secret() { + return MakeSpan(client_handshake_secret_, hash_len_); + } + Span<uint8_t> server_handshake_secret() { + return MakeSpan(server_handshake_secret_, hash_len_); + } + Span<uint8_t> client_traffic_secret_0() { + return MakeSpan(client_traffic_secret_0_, hash_len_); + } + Span<uint8_t> server_traffic_secret_0() { + return MakeSpan(server_traffic_secret_0_, hash_len_); + } + Span<uint8_t> expected_client_finished() { + return MakeSpan(expected_client_finished_, hash_len_); + } union { // sent is a bitset where the bits correspond to elements of kExtensions
diff --git a/ssl/tls13_both.cc b/ssl/tls13_both.cc index 1a49e4c..7457155 100644 --- a/ssl/tls13_both.cc +++ b/ssl/tls13_both.cc
@@ -384,21 +384,20 @@ bool use_saved_value) { SSL *const ssl = hs->ssl; uint8_t verify_data_buf[EVP_MAX_MD_SIZE]; - const uint8_t *verify_data; - size_t verify_data_len; + Span<const uint8_t> verify_data; if (use_saved_value) { assert(ssl->server); - verify_data = hs->expected_client_finished; - verify_data_len = hs->hash_len; + verify_data = hs->expected_client_finished(); } else { - if (!tls13_finished_mac(hs, verify_data_buf, &verify_data_len, - !ssl->server)) { + size_t len; + if (!tls13_finished_mac(hs, verify_data_buf, &len, !ssl->server)) { return false; } - verify_data = verify_data_buf; + verify_data = MakeConstSpan(verify_data_buf, len); } - bool finished_ok = CBS_mem_equal(&msg.body, verify_data, verify_data_len); + bool finished_ok = + CBS_mem_equal(&msg.body, verify_data.data(), verify_data.size()); #if defined(BORINGSSL_UNSAFE_FUZZER_MODE) finished_ok = true; #endif
diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc index ba7dc55..a7d0d89 100644 --- a/ssl/tls13_client.cc +++ b/ssl/tls13_client.cc
@@ -393,18 +393,16 @@ if (!tls13_advance_key_schedule(hs, dhe_secret) || !ssl_hash_message(hs, msg) || !tls13_derive_handshake_secrets(hs) || - !tls13_set_traffic_key( - ssl, ssl_encryption_handshake, evp_aead_open, - MakeConstSpan(hs->server_handshake_secret, hs->hash_len))) { + !tls13_set_traffic_key(ssl, ssl_encryption_handshake, evp_aead_open, + hs->server_handshake_secret())) { return ssl_hs_error; } if (!hs->early_data_offered) { // If not sending early data, set client traffic keys now so that alerts are // encrypted. - if (!tls13_set_traffic_key( - ssl, ssl_encryption_handshake, evp_aead_seal, - MakeConstSpan(hs->client_handshake_secret, hs->hash_len))) { + if (!tls13_set_traffic_key(ssl, ssl_encryption_handshake, evp_aead_seal, + hs->client_handshake_secret())) { return ssl_hs_error; } } @@ -619,7 +617,8 @@ !tls13_process_finished(hs, msg, false /* don't use saved value */) || !ssl_hash_message(hs, msg) || // Update the secret to the master secret and derive traffic keys. - !tls13_advance_key_schedule(hs, MakeConstSpan(kZeroes, hs->hash_len)) || + !tls13_advance_key_schedule( + hs, MakeConstSpan(kZeroes, hs->transcript.DigestLen())) || !tls13_derive_application_secrets(hs)) { return ssl_hs_error; } @@ -644,9 +643,8 @@ } if (hs->early_data_offered) { - if (!tls13_set_traffic_key( - ssl, ssl_encryption_handshake, evp_aead_seal, - MakeConstSpan(hs->client_handshake_secret, hs->hash_len))) { + if (!tls13_set_traffic_key(ssl, ssl_encryption_handshake, evp_aead_seal, + hs->client_handshake_secret())) { return ssl_hs_error; } } @@ -740,12 +738,10 @@ } // Derive the final keys and enable them. - if (!tls13_set_traffic_key( - ssl, ssl_encryption_application, evp_aead_open, - MakeConstSpan(hs->server_traffic_secret_0, hs->hash_len)) || - !tls13_set_traffic_key( - ssl, ssl_encryption_application, evp_aead_seal, - MakeConstSpan(hs->client_traffic_secret_0, hs->hash_len)) || + if (!tls13_set_traffic_key(ssl, ssl_encryption_application, evp_aead_open, + hs->server_traffic_secret_0()) || + !tls13_set_traffic_key(ssl, ssl_encryption_application, evp_aead_seal, + hs->client_traffic_secret_0()) || !tls13_derive_resumption_secret(hs)) { return ssl_hs_error; }
diff --git a/ssl/tls13_enc.cc b/ssl/tls13_enc.cc index c07afd1..b8bebe1 100644 --- a/ssl/tls13_enc.cc +++ b/ssl/tls13_enc.cc
@@ -17,6 +17,7 @@ #include <assert.h> #include <string.h> +#include <algorithm> #include <utility> #include <openssl/aead.h> @@ -38,31 +39,38 @@ return false; } - assert(hs->transcript.DigestLen() <= SSL_MAX_MD_SIZE); - hs->hash_len = hs->transcript.DigestLen(); - // Initialize the secret to the zero key. - OPENSSL_memset(hs->secret, 0, hs->hash_len); + hs->ResizeSecrets(hs->transcript.DigestLen()); + OPENSSL_memset(hs->secret().data(), 0, hs->secret().size()); return true; } +static bool hkdf_extract_to_secret(SSL_HANDSHAKE *hs, Span<const uint8_t> in) { + size_t len; + if (!HKDF_extract(hs->secret().data(), &len, hs->transcript.Digest(), + in.data(), in.size(), hs->secret().data(), + hs->secret().size())) { + return false; + } + assert(len == hs->secret().size()); + return true; +} + bool tls13_init_key_schedule(SSL_HANDSHAKE *hs, Span<const uint8_t> psk) { if (!init_key_schedule(hs, ssl_protocol_version(hs->ssl), hs->new_cipher)) { return false; } hs->transcript.FreeBuffer(); - return HKDF_extract(hs->secret, &hs->hash_len, hs->transcript.Digest(), - psk.data(), psk.size(), hs->secret, hs->hash_len); + return hkdf_extract_to_secret(hs, psk); } bool tls13_init_early_key_schedule(SSL_HANDSHAKE *hs, Span<const uint8_t> psk) { SSL *const ssl = hs->ssl; return init_key_schedule(hs, ssl_session_protocol_version(ssl->session.get()), ssl->session->cipher) && - HKDF_extract(hs->secret, &hs->hash_len, hs->transcript.Digest(), - psk.data(), psk.size(), hs->secret, hs->hash_len); + hkdf_extract_to_secret(hs, psk); } static Span<const char> label_to_span(const char *label) { @@ -101,21 +109,12 @@ bool tls13_advance_key_schedule(SSL_HANDSHAKE *hs, Span<const uint8_t> in) { uint8_t derive_context[EVP_MAX_MD_SIZE]; unsigned derive_context_len; - if (!EVP_Digest(nullptr, 0, derive_context, &derive_context_len, - hs->transcript.Digest(), nullptr)) { - return false; - } - - if (!hkdf_expand_label(MakeSpan(hs->secret, hs->hash_len), - hs->transcript.Digest(), - MakeConstSpan(hs->secret, hs->hash_len), - label_to_span(kTLS13LabelDerived), - MakeConstSpan(derive_context, derive_context_len))) { - return false; - } - - return HKDF_extract(hs->secret, &hs->hash_len, hs->transcript.Digest(), - in.data(), in.size(), hs->secret, hs->hash_len); + return EVP_Digest(nullptr, 0, derive_context, &derive_context_len, + hs->transcript.Digest(), nullptr) && + hkdf_expand_label(hs->secret(), hs->transcript.Digest(), hs->secret(), + label_to_span(kTLS13LabelDerived), + MakeConstSpan(derive_context, derive_context_len)) && + hkdf_extract_to_secret(hs, in); } // derive_secret derives a secret of length |out.size()| and writes the result @@ -130,8 +129,7 @@ return false; } - return hkdf_expand_label(out, hs->transcript.Digest(), - MakeConstSpan(hs->secret, hs->hash_len), label, + return hkdf_expand_label(out, hs->transcript.Digest(), hs->secret(), label, MakeConstSpan(context_hash, context_hash_len)); } @@ -230,25 +228,26 @@ bool tls13_derive_early_secrets(SSL_HANDSHAKE *hs) { SSL *const ssl = hs->ssl; - if (!derive_secret(hs, MakeSpan(hs->early_traffic_secret, hs->hash_len), + if (!derive_secret(hs, hs->early_traffic_secret(), label_to_span(kTLS13LabelClientEarlyTraffic)) || !ssl_log_secret(ssl, "CLIENT_EARLY_TRAFFIC_SECRET", - MakeConstSpan(hs->early_traffic_secret, hs->hash_len))) { + hs->early_traffic_secret())) { return false; } if (ssl->quic_method != nullptr) { if (ssl->server) { if (!ssl->quic_method->set_encryption_secrets( - ssl, ssl_encryption_early_data, nullptr, hs->early_traffic_secret, - hs->hash_len)) { + ssl, ssl_encryption_early_data, nullptr, + hs->early_traffic_secret().data(), + hs->early_traffic_secret().size())) { OPENSSL_PUT_ERROR(SSL, SSL_R_QUIC_INTERNAL_ERROR); return false; } } else { if (!ssl->quic_method->set_encryption_secrets( - ssl, ssl_encryption_early_data, hs->early_traffic_secret, nullptr, - hs->hash_len)) { + ssl, ssl_encryption_early_data, hs->early_traffic_secret().data(), + nullptr, hs->early_traffic_secret().size())) { OPENSSL_PUT_ERROR(SSL, SSL_R_QUIC_INTERNAL_ERROR); return false; } @@ -258,82 +257,65 @@ return true; } +static bool set_quic_secrets(SSL_HANDSHAKE *hs, ssl_encryption_level_t level, + Span<const uint8_t> client_write_secret, + Span<const uint8_t> server_write_secret) { + SSL *const ssl = hs->ssl; + assert(client_write_secret.size() == server_write_secret.size()); + if (ssl->quic_method == nullptr) { + return true; + } + if (!ssl->server) { + std::swap(client_write_secret, server_write_secret); + } + return ssl->quic_method->set_encryption_secrets( + ssl, level, + /*read_secret=*/client_write_secret.data(), + /*write_secret=*/server_write_secret.data(), client_write_secret.size()); +} + bool tls13_derive_handshake_secrets(SSL_HANDSHAKE *hs) { SSL *const ssl = hs->ssl; - if (!derive_secret(hs, MakeSpan(hs->client_handshake_secret, hs->hash_len), + if (!derive_secret(hs, hs->client_handshake_secret(), label_to_span(kTLS13LabelClientHandshakeTraffic)) || - !ssl_log_secret( - ssl, "CLIENT_HANDSHAKE_TRAFFIC_SECRET", - MakeConstSpan(hs->client_handshake_secret, hs->hash_len)) || - !derive_secret(hs, MakeSpan(hs->server_handshake_secret, hs->hash_len), + !ssl_log_secret(ssl, "CLIENT_HANDSHAKE_TRAFFIC_SECRET", + hs->client_handshake_secret()) || + !derive_secret(hs, hs->server_handshake_secret(), label_to_span(kTLS13LabelServerHandshakeTraffic)) || - !ssl_log_secret( - ssl, "SERVER_HANDSHAKE_TRAFFIC_SECRET", - MakeConstSpan(hs->server_handshake_secret, hs->hash_len))) { + !ssl_log_secret(ssl, "SERVER_HANDSHAKE_TRAFFIC_SECRET", + hs->server_handshake_secret()) || + !set_quic_secrets(hs, ssl_encryption_handshake, + hs->client_handshake_secret(), + hs->server_handshake_secret())) { return false; } - if (ssl->quic_method != nullptr) { - if (ssl->server) { - if (!ssl->quic_method->set_encryption_secrets( - ssl, ssl_encryption_handshake, hs->client_handshake_secret, - hs->server_handshake_secret, hs->hash_len)) { - OPENSSL_PUT_ERROR(SSL, SSL_R_QUIC_INTERNAL_ERROR); - return false; - } - } else { - if (!ssl->quic_method->set_encryption_secrets( - ssl, ssl_encryption_handshake, hs->server_handshake_secret, - hs->client_handshake_secret, hs->hash_len)) { - OPENSSL_PUT_ERROR(SSL, SSL_R_QUIC_INTERNAL_ERROR); - return false; - } - } - } - return true; } bool tls13_derive_application_secrets(SSL_HANDSHAKE *hs) { SSL *const ssl = hs->ssl; - ssl->s3->exporter_secret_len = hs->hash_len; - if (!derive_secret(hs, MakeSpan(hs->client_traffic_secret_0, hs->hash_len), + ssl->s3->exporter_secret_len = hs->transcript.DigestLen(); + if (!derive_secret(hs, hs->client_traffic_secret_0(), label_to_span(kTLS13LabelClientApplicationTraffic)) || - !ssl_log_secret( - ssl, "CLIENT_TRAFFIC_SECRET_0", - MakeConstSpan(hs->client_traffic_secret_0, hs->hash_len)) || - !derive_secret(hs, MakeSpan(hs->server_traffic_secret_0, hs->hash_len), + !ssl_log_secret(ssl, "CLIENT_TRAFFIC_SECRET_0", + hs->client_traffic_secret_0()) || + !derive_secret(hs, hs->server_traffic_secret_0(), label_to_span(kTLS13LabelServerApplicationTraffic)) || - !ssl_log_secret( - ssl, "SERVER_TRAFFIC_SECRET_0", - MakeConstSpan(hs->server_traffic_secret_0, hs->hash_len)) || + !ssl_log_secret(ssl, "SERVER_TRAFFIC_SECRET_0", + hs->server_traffic_secret_0()) || !derive_secret( hs, MakeSpan(ssl->s3->exporter_secret, ssl->s3->exporter_secret_len), label_to_span(kTLS13LabelExporter)) || !ssl_log_secret(ssl, "EXPORTER_SECRET", MakeConstSpan(ssl->s3->exporter_secret, - ssl->s3->exporter_secret_len))) { + ssl->s3->exporter_secret_len)) || + !set_quic_secrets(hs, ssl_encryption_application, + hs->client_traffic_secret_0(), + hs->server_traffic_secret_0())) { return false; } - if (ssl->quic_method != nullptr) { - if (ssl->server) { - if (!ssl->quic_method->set_encryption_secrets( - ssl, ssl_encryption_application, hs->client_traffic_secret_0, - hs->server_traffic_secret_0, hs->hash_len)) { - OPENSSL_PUT_ERROR(SSL, SSL_R_QUIC_INTERNAL_ERROR); - return false; - } - } else { - if (!ssl->quic_method->set_encryption_secrets( - ssl, ssl_encryption_application, hs->server_traffic_secret_0, - hs->client_traffic_secret_0, hs->hash_len)) { - OPENSSL_PUT_ERROR(SSL, SSL_R_QUIC_INTERNAL_ERROR); - return false; - } - } - } - return true; } @@ -359,11 +341,11 @@ static const char kTLS13LabelResumption[] = "res master"; bool tls13_derive_resumption_secret(SSL_HANDSHAKE *hs) { - if (hs->hash_len > SSL_MAX_MASTER_KEY_LENGTH) { + if (hs->transcript.DigestLen() > SSL_MAX_MASTER_KEY_LENGTH) { OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR); return false; } - hs->new_session->master_key_length = hs->hash_len; + hs->new_session->master_key_length = hs->transcript.DigestLen(); return derive_secret( hs, MakeSpan(hs->new_session->master_key, hs->new_session->master_key_length), @@ -394,12 +376,8 @@ bool tls13_finished_mac(SSL_HANDSHAKE *hs, uint8_t *out, size_t *out_len, bool is_server) { - Span<const uint8_t> traffic_secret; - if (is_server) { - traffic_secret = MakeConstSpan(hs->server_handshake_secret, hs->hash_len); - } else { - traffic_secret = MakeConstSpan(hs->client_handshake_secret, hs->hash_len); - } + Span<const uint8_t> traffic_secret = + is_server ? hs->server_handshake_secret() : hs->client_handshake_secret(); uint8_t context_hash[EVP_MAX_MD_SIZE]; size_t context_hash_len;
diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc index 7b6f5df..d99b5b4 100644 --- a/ssl/tls13_server.cc +++ b/ssl/tls13_server.cc
@@ -607,9 +607,8 @@ // Derive and enable the handshake traffic secrets. if (!tls13_derive_handshake_secrets(hs) || - !tls13_set_traffic_key( - ssl, ssl_encryption_handshake, evp_aead_seal, - MakeConstSpan(hs->server_handshake_secret, hs->hash_len))) { + !tls13_set_traffic_key(ssl, ssl_encryption_handshake, evp_aead_seal, + hs->server_handshake_secret())) { return ssl_hs_error; } @@ -717,11 +716,11 @@ SSL *const ssl = hs->ssl; if (!tls13_add_finished(hs) || // Update the secret to the master secret and derive traffic keys. - !tls13_advance_key_schedule(hs, MakeConstSpan(kZeroes, hs->hash_len)) || + !tls13_advance_key_schedule( + hs, MakeConstSpan(kZeroes, hs->transcript.DigestLen())) || !tls13_derive_application_secrets(hs) || - !tls13_set_traffic_key( - ssl, ssl_encryption_application, evp_aead_seal, - MakeConstSpan(hs->server_traffic_secret_0, hs->hash_len))) { + !tls13_set_traffic_key(ssl, ssl_encryption_application, evp_aead_seal, + hs->server_traffic_secret_0())) { return ssl_hs_error; } @@ -738,12 +737,12 @@ } size_t finished_len; - if (!tls13_finished_mac(hs, hs->expected_client_finished, &finished_len, - false /* client */)) { + if (!tls13_finished_mac(hs, hs->expected_client_finished().data(), + &finished_len, false /* client */)) { return ssl_hs_error; } - if (finished_len != hs->hash_len) { + if (finished_len != hs->expected_client_finished().size()) { OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR); return ssl_hs_error; } @@ -753,13 +752,13 @@ // // TODO(davidben): This will need to be updated for DTLS 1.3. assert(!SSL_is_dtls(hs->ssl)); - assert(hs->hash_len <= 0xff); - uint8_t header[4] = {SSL3_MT_FINISHED, 0, 0, - static_cast<uint8_t>(hs->hash_len)}; + assert(hs->expected_client_finished().size() <= 0xff); + uint8_t header[4] = { + SSL3_MT_FINISHED, 0, 0, + static_cast<uint8_t>(hs->expected_client_finished().size())}; bool unused_sent_tickets; if (!hs->transcript.Update(header) || - !hs->transcript.Update( - MakeConstSpan(hs->expected_client_finished, hs->hash_len)) || + !hs->transcript.Update(hs->expected_client_finished()) || !tls13_derive_resumption_secret(hs) || !add_new_session_tickets(hs, &unused_sent_tickets)) { return ssl_hs_error; @@ -773,9 +772,8 @@ static enum ssl_hs_wait_t do_read_second_client_flight(SSL_HANDSHAKE *hs) { SSL *const ssl = hs->ssl; if (ssl->s3->early_data_accepted) { - if (!tls13_set_traffic_key( - ssl, ssl_encryption_early_data, evp_aead_open, - MakeConstSpan(hs->early_traffic_secret, hs->hash_len))) { + if (!tls13_set_traffic_key(ssl, ssl_encryption_early_data, evp_aead_open, + hs->early_traffic_secret())) { return ssl_hs_error; } hs->can_early_write = true; @@ -809,9 +807,8 @@ ssl->method->next_message(ssl); } } - if (!tls13_set_traffic_key( - ssl, ssl_encryption_handshake, evp_aead_open, - MakeConstSpan(hs->client_handshake_secret, hs->hash_len))) { + if (!tls13_set_traffic_key(ssl, ssl_encryption_handshake, evp_aead_open, + hs->client_handshake_secret())) { return ssl_hs_error; } hs->tls13_state = ssl->s3->early_data_accepted @@ -917,9 +914,8 @@ // and derived the resumption secret. !tls13_process_finished(hs, msg, ssl->s3->early_data_accepted) || // evp_aead_seal keys have already been switched. - !tls13_set_traffic_key( - ssl, ssl_encryption_application, evp_aead_open, - MakeConstSpan(hs->client_traffic_secret_0, hs->hash_len))) { + !tls13_set_traffic_key(ssl, ssl_encryption_application, evp_aead_open, + hs->client_traffic_secret_0())) { return ssl_hs_error; }