diff --git a/src/include/openssl/span.h b/src/include/openssl/span.h index a6cd5b7a1..e0de1acc9 100644 --- a/src/include/openssl/span.h +++ b/src/include/openssl/span.h @@ -234,6 +234,11 @@ constexpr auto MakeSpan(C &c) -> decltype(MakeSpan(c.data(), c.size())) { return MakeSpan(c.data(), c.size()); } +template +constexpr Span MakeSpan(T (&array)[N]) { + return Span(array, N); +} + template constexpr Span MakeConstSpan(T *ptr, size_t size) { return Span(ptr, size); diff --git a/src/include/openssl/ssl.h b/src/include/openssl/ssl.h index f76e0a1b5..a5c5ab0b5 100644 --- a/src/include/openssl/ssl.h +++ b/src/include/openssl/ssl.h @@ -5894,9 +5894,12 @@ OPENSSL_EXPORT bool SSL_serialize_handback(const SSL *ssl, CBB *out); OPENSSL_EXPORT bool SSL_apply_handback(SSL *ssl, Span handback); // SSL_get_traffic_secrets sets |*out_read_traffic_secret| and -// |*out_write_traffic_secret| to reference the TLS 1.3 traffic secrets for -// |ssl|. This function is only valid on TLS 1.3 connections that have -// completed the handshake. It returns true on success and false on error. +// |*out_write_traffic_secret| to reference the current TLS 1.3 traffic secrets +// for |ssl|. It returns true on success and false on error. +// +// This function is only valid on TLS 1.3 connections that have completed the +// handshake. It is not valid for QUIC or DTLS, where multiple traffic secrets +// may be active at a time. OPENSSL_EXPORT bool SSL_get_traffic_secrets( const SSL *ssl, Span *out_read_traffic_secret, Span *out_write_traffic_secret); diff --git a/src/ssl/internal.h b/src/ssl/internal.h index 36883e5af..30e610407 100644 --- a/src/ssl/internal.h +++ b/src/ssl/internal.h @@ -816,6 +816,7 @@ class RecordNumberEncrypter { public: virtual ~RecordNumberEncrypter() = default; static constexpr bool kAllowUniquePtr = true; + static constexpr size_t kMaxKeySize = 32; virtual size_t KeySize() = 0; virtual bool SetKey(Span key) = 0; diff --git a/src/ssl/ssl_key_share.cc b/src/ssl/ssl_key_share.cc index 923b8bb48..88144b060 100644 --- a/src/ssl/ssl_key_share.cc +++ b/src/ssl/ssl_key_share.cc @@ -94,7 +94,7 @@ class ECKeyShare : public SSLKeyShare { !EC_POINT_oct2point(group_, peer_point.get(), ciphertext.data(), ciphertext.size(), /*ctx=*/nullptr)) { OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_ECPOINT); - *out_alert = SSL_AD_DECODE_ERROR; + *out_alert = SSL_AD_ILLEGAL_PARAMETER; return false; } @@ -168,7 +168,7 @@ class X25519KeyShare : public SSLKeyShare { if (ciphertext.size() != 32 || // !X25519(secret.data(), private_key_, ciphertext.data())) { - *out_alert = SSL_AD_DECODE_ERROR; + *out_alert = SSL_AD_ILLEGAL_PARAMETER; OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_ECPOINT); return false; } @@ -236,7 +236,7 @@ class X25519Kyber768KeyShare : public SSLKeyShare { !X25519(secret.data(), x25519_private_key_, CBS_data(&peer_x25519_cbs)) || !KYBER_parse_public_key(&peer_kyber_pub, &peer_kyber_cbs)) { - *out_alert = SSL_AD_DECODE_ERROR; + *out_alert = SSL_AD_ILLEGAL_PARAMETER; OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_ECPOINT); return false; } @@ -266,7 +266,7 @@ class X25519Kyber768KeyShare : public SSLKeyShare { if (ciphertext.size() != 32 + KYBER_CIPHERTEXT_BYTES || !X25519(secret.data(), x25519_private_key_, ciphertext.data())) { - *out_alert = SSL_AD_DECODE_ERROR; + *out_alert = SSL_AD_ILLEGAL_PARAMETER; OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_ECPOINT); return false; } @@ -325,7 +325,7 @@ class X25519MLKEM768KeyShare : public SSLKeyShare { CBS_len(&peer_key_cbs) != 0 || !X25519(secret.data() + MLKEM_SHARED_SECRET_BYTES, x25519_private_key_, CBS_data(&peer_x25519_cbs))) { - *out_alert = SSL_AD_DECODE_ERROR; + *out_alert = SSL_AD_ILLEGAL_PARAMETER; OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_ECPOINT); return false; } @@ -359,7 +359,7 @@ class X25519MLKEM768KeyShare : public SSLKeyShare { MLKEM768_CIPHERTEXT_BYTES, &mlkem_private_key_) || !X25519(secret.data() + MLKEM_SHARED_SECRET_BYTES, x25519_private_key_, ciphertext.data() + MLKEM768_CIPHERTEXT_BYTES)) { - *out_alert = SSL_AD_DECODE_ERROR; + *out_alert = SSL_AD_ILLEGAL_PARAMETER; OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_ECPOINT); return false; } diff --git a/src/ssl/ssl_lib.cc b/src/ssl/ssl_lib.cc index 2a80a0579..071709f50 100644 --- a/src/ssl/ssl_lib.cc +++ b/src/ssl/ssl_lib.cc @@ -472,8 +472,10 @@ void SSL_set_handoff_mode(SSL *ssl, bool on) { bool SSL_get_traffic_secrets(const SSL *ssl, Span *out_read_traffic_secret, Span *out_write_traffic_secret) { - if (SSL_version(ssl) < TLS1_3_VERSION) { - OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_SSL_VERSION); + // This API is not well-defined for DTLS 1.3 (see https://crbug.com/42290608) + // or QUIC, where multiple epochs may be alive at once. + if (SSL_is_dtls(ssl) || ssl->quic_method != nullptr) { + OPENSSL_PUT_ERROR(SSL, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); return false; } @@ -482,6 +484,11 @@ bool SSL_get_traffic_secrets(const SSL *ssl, return false; } + if (SSL_version(ssl) < TLS1_3_VERSION) { + OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_SSL_VERSION); + return false; + } + *out_read_traffic_secret = Span( ssl->s3->read_traffic_secret, ssl->s3->read_traffic_secret_len); *out_write_traffic_secret = Span( diff --git a/src/ssl/ssl_test.cc b/src/ssl/ssl_test.cc index 46d5af943..34dd5edeb 100644 --- a/src/ssl/ssl_test.cc +++ b/src/ssl/ssl_test.cc @@ -7098,6 +7098,13 @@ TEST_F(QUICMethodTest, Basic) { EXPECT_FALSE(SSL_session_reused(client_.get())); EXPECT_FALSE(SSL_session_reused(server_.get())); + // SSL_get_traffic_secrets is not defined for QUIC. + Span read_secret, write_secret; + EXPECT_FALSE( + SSL_get_traffic_secrets(client_.get(), &read_secret, &write_secret)); + EXPECT_FALSE( + SSL_get_traffic_secrets(server_.get(), &read_secret, &write_secret)); + // The server sent NewSessionTicket messages in the handshake. EXPECT_FALSE(g_last_session); ASSERT_TRUE(ProvideHandshakeData(client_.get())); @@ -9682,14 +9689,20 @@ TEST_P(SSLVersionTest, KeyLog) { Key("SERVER_HANDSHAKE_TRAFFIC_SECRET"), Key("SERVER_TRAFFIC_SECRET_0"))); - // Ideally we'd check the other values, but those are harder to check - // without actually decrypting the records. - Span read_secret, write_secret; - ASSERT_TRUE(bssl::SSL_get_traffic_secrets(client_.get(), &read_secret, - &write_secret)); - EXPECT_EQ(Bytes(read_secret), Bytes(client_log["SERVER_TRAFFIC_SECRET_0"])); - EXPECT_EQ(Bytes(write_secret), - Bytes(client_log["CLIENT_TRAFFIC_SECRET_0"])); + if (!is_dtls()) { + // Ideally we'd check the other values, but those are harder to check + // without actually decrypting the records. + // + // TODO(crbug.com/42290608): Check the secrets in DTLS, once we have an + // API for them. + Span read_secret, write_secret; + ASSERT_TRUE( + SSL_get_traffic_secrets(client_.get(), &read_secret, &write_secret)); + EXPECT_EQ(Bytes(read_secret), + Bytes(client_log["SERVER_TRAFFIC_SECRET_0"])); + EXPECT_EQ(Bytes(write_secret), + Bytes(client_log["CLIENT_TRAFFIC_SECRET_0"])); + } } else { EXPECT_THAT(client_log, ElementsAre(Key("CLIENT_RANDOM"))); @@ -9706,6 +9719,25 @@ TEST_P(SSLVersionTest, KeyLog) { EXPECT_EQ(client_log, server_log); } +TEST_P(SSLVersionTest, GetTrafficSecrets) { + ASSERT_TRUE(Connect()); + + Span client_read, client_write, server_read, server_write; + bool client_ok = + SSL_get_traffic_secrets(client_.get(), &client_read, &client_write); + bool server_ok = + SSL_get_traffic_secrets(server_.get(), &server_read, &server_write); + if (!is_dtls() && version() >= TLS1_3_VERSION) { + ASSERT_TRUE(client_ok); + ASSERT_TRUE(server_ok); + EXPECT_EQ(Bytes(client_read), Bytes(server_write)); + EXPECT_EQ(Bytes(server_read), Bytes(client_write)); + } else { + EXPECT_FALSE(client_ok); + EXPECT_FALSE(server_ok); + } +} + TEST_P(SSLVersionTest, GetIVs) { std::vector ciphers; if (version() == TLS1_2_VERSION || version() == DTLS1_2_VERSION) { diff --git a/src/ssl/test/runner/runner.go b/src/ssl/test/runner/runner.go index f3a946287..650a0b62f 100644 --- a/src/ssl/test/runner/runner.go +++ b/src/ssl/test/runner/runner.go @@ -11868,7 +11868,7 @@ func addCurveTests() { }, }) - badKeyShareLocalError := "remote error: error decoding message" + badKeyShareLocalError := "remote error: illegal parameter" if testType == clientTest && ver.version >= VersionTLS13 { // If the shim is a TLS 1.3 client and the runner sends a bad // key share, the runner never reads the client's cleartext diff --git a/src/ssl/tls13_enc.cc b/src/ssl/tls13_enc.cc index d4193fd75..4a642da7c 100644 --- a/src/ssl/tls13_enc.cc +++ b/src/ssl/tls13_enc.cc @@ -203,27 +203,19 @@ bool tls13_set_traffic_key(SSL *ssl, enum ssl_encryption_level_t level, return false; } - // Derive the key. - size_t key_len = EVP_AEAD_key_length(aead); - uint8_t key_buf[EVP_AEAD_MAX_KEY_LENGTH]; - auto key = MakeSpan(key_buf, key_len); + // Derive the key and IV. + uint8_t key_buf[EVP_AEAD_MAX_KEY_LENGTH], iv_buf[EVP_AEAD_MAX_NONCE_LENGTH]; + auto key = MakeSpan(key_buf).first(EVP_AEAD_key_length(aead)); + auto iv = MakeSpan(iv_buf).first(EVP_AEAD_nonce_length(aead)); if (!hkdf_expand_label(key, digest, traffic_secret, label_to_span("key"), - {}, is_dtls)) { - return false; - } - - // Derive the IV. - size_t iv_len = EVP_AEAD_nonce_length(aead); - uint8_t iv_buf[EVP_AEAD_MAX_NONCE_LENGTH]; - auto iv = MakeSpan(iv_buf, iv_len); - if (!hkdf_expand_label(iv, digest, traffic_secret, label_to_span("iv"), {}, + {}, is_dtls) || + !hkdf_expand_label(iv, digest, traffic_secret, label_to_span("iv"), {}, is_dtls)) { return false; } - traffic_aead = - SSLAEADContext::Create(direction, session->ssl_version, is_dtls, - session->cipher, key, Span(), iv); + traffic_aead = SSLAEADContext::Create( + direction, session->ssl_version, is_dtls, session->cipher, key, {}, iv); } if (!traffic_aead) { @@ -236,11 +228,11 @@ bool tls13_set_traffic_key(SSL *ssl, enum ssl_encryption_level_t level, if (!rn_encrypter) { return false; } - Array rne_key; - if (!rne_key.Init(rn_encrypter->KeySize()) || - !hkdf_expand_label(MakeSpan(rne_key), digest, traffic_secret, - label_to_span("sn"), {}, is_dtls) || - !rn_encrypter->SetKey(MakeSpan(rne_key))) { + uint8_t rne_key_buf[RecordNumberEncrypter::kMaxKeySize]; + auto rne_key = MakeSpan(rne_key_buf).first(rn_encrypter->KeySize()); + if (!hkdf_expand_label(rne_key, digest, traffic_secret, label_to_span("sn"), + {}, is_dtls) || + !rn_encrypter->SetKey(rne_key)) { return false; } }