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 <bbe@google.com> Commit-Queue: David Benjamin <davidben@google.com> Auto-Submit: David Benjamin <davidben@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index f76e0a1..a5c5ab0 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h
@@ -5894,9 +5894,12 @@ OPENSSL_EXPORT bool SSL_apply_handback(SSL *ssl, Span<const uint8_t> 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<const uint8_t> *out_read_traffic_secret, Span<const uint8_t> *out_write_traffic_secret);
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 2a80a05..071709f 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc
@@ -472,8 +472,10 @@ bool SSL_get_traffic_secrets(const SSL *ssl, Span<const uint8_t> *out_read_traffic_secret, Span<const uint8_t> *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 @@ 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<const uint8_t>( ssl->s3->read_traffic_secret, ssl->s3->read_traffic_secret_len); *out_write_traffic_secret = Span<const uint8_t>(
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 46d5af9..34dd5ed 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc
@@ -7098,6 +7098,13 @@ EXPECT_FALSE(SSL_session_reused(client_.get())); EXPECT_FALSE(SSL_session_reused(server_.get())); + // SSL_get_traffic_secrets is not defined for QUIC. + Span<const uint8_t> 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 @@ 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<const uint8_t> 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<const uint8_t> 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 @@ EXPECT_EQ(client_log, server_log); } +TEST_P(SSLVersionTest, GetTrafficSecrets) { + ASSERT_TRUE(Connect()); + + Span<const uint8_t> 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<const char *> ciphers; if (version() == TLS1_2_VERSION || version() == DTLS1_2_VERSION) {