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) {
