From 58e533017762cf744e2144fbb0d199020cac23c2 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 24 Sep 2024 18:28:50 -0400 Subject: [PATCH 1/3] Do not implement SSL_get_traffic_secrets for QUIC and DTLS This is implemented by looking at the saved current read and write secrets. That state is used by KeyUpdate and this logic. As part of tidying up the epoch state for DTLS 1.3, I ran into that state because DTLS does not have a single current read/write secret. But it also isn't ideal for QUIC. For QUIC, the problem is that QUIC drives KeyUpdates outside of TLS, but that means we'll just hold on to the initial traffic secrets in memory, which can derive all the rotated ones. So let's for now, just limit this API to TLS. We can decide later whether to also allow it for DTLS (after very carefully defining what the "current" epoch means). I don't think we'd ever allow it for QUIC given how QUIC is intended to work. (This change doesn't actually fix any of the internal storage, just breaks the API that would leak it. Changing the internal storage will be in later CLs.) Bug: 42290608 Change-Id: I5d4b170a5a80a7cc0657a957ae20135d742891d2 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/71647 Reviewed-by: Bob Beck Commit-Queue: David Benjamin Auto-Submit: David Benjamin --- include/openssl/ssl.h | 9 +++++--- ssl/ssl_lib.cc | 11 ++++++++-- ssl/ssl_test.cc | 48 +++++++++++++++++++++++++++++++++++-------- 3 files changed, 55 insertions(+), 13 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index f76e0a1b5..a5c5ab0b5 100644 --- a/include/openssl/ssl.h +++ b/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/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 2a80a0579..071709f50 100644 --- a/ssl/ssl_lib.cc +++ b/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/ssl/ssl_test.cc b/ssl/ssl_test.cc index 46d5af943..34dd5edeb 100644 --- a/ssl/ssl_test.cc +++ b/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) { From 37571ee525bf1484fa76b4731053e7e7ddb93d31 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 27 Sep 2024 15:23:01 -0400 Subject: [PATCH 2/3] Avoid an allocation when deriving the DTLS record number key We really should make like a bounded array helper type. Maybe call it bssl::InplaceVector to match C++26's upcoming name. Bug: 42290594 Change-Id: I5d03a867fd1bae5e3ecca04d91a3d0900fdeac1b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/71648 Commit-Queue: David Benjamin Reviewed-by: Bob Beck --- include/openssl/span.h | 5 +++++ ssl/internal.h | 1 + ssl/tls13_enc.cc | 34 +++++++++++++--------------------- 3 files changed, 19 insertions(+), 21 deletions(-) diff --git a/include/openssl/span.h b/include/openssl/span.h index a6cd5b7a1..e0de1acc9 100644 --- a/include/openssl/span.h +++ b/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/ssl/internal.h b/ssl/internal.h index 36883e5af..30e610407 100644 --- a/ssl/internal.h +++ b/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/ssl/tls13_enc.cc b/ssl/tls13_enc.cc index d4193fd75..4a642da7c 100644 --- a/ssl/tls13_enc.cc +++ b/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; } } From 72a60506ded3407454d6ddc1d848c266020c0c82 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 27 Sep 2024 12:07:47 -0400 Subject: [PATCH 3/3] Use illegal_parameter instead of decode_error for invalid key shares This is extremely silly and a huge waste of everyone's time (I deeply regret disambiguating illegal_parameter and decode_error for RFC 8446), but so it goes. Technically these errors pass the TLS syntax and are an invalid value, so they should be illegal_parameter. Note: TLS defines two generic alerts (see Section 6) to use upon failure to parse a message. Peers which receive a message which cannot be parsed according to the syntax (e.g., have a length extending beyond the message boundary or contain an out-of-range length) MUST terminate the connection with a "decode_error" alert. Peers which receive a message which is syntactically correct but semantically invalid (e.g., a DHE share of p - 1, or an invalid enum) MUST terminate the connection with an "illegal_parameter" alert. Update-Note: The error sent on invalid key share is now more correct. This does not change which connections do or do not fail, only which of two practically identical alert codes is sent to the other side. Change-Id: If0ddf511d6cf23383c6134ad30e3ae080c4f2769 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/71627 Auto-Submit: David Benjamin Reviewed-by: Bob Beck Commit-Queue: Bob Beck --- ssl/ssl_key_share.cc | 12 ++++++------ ssl/test/runner/runner.go | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/ssl/ssl_key_share.cc b/ssl/ssl_key_share.cc index 923b8bb48..88144b060 100644 --- a/ssl/ssl_key_share.cc +++ b/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/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index f3a946287..650a0b62f 100644 --- a/ssl/test/runner/runner.go +++ b/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