From 2dc95eef506a72b45e6817a3d49e3e474cbd36f2 Mon Sep 17 00:00:00 2001 From: Nick Harper Date: Tue, 29 Oct 2024 23:42:56 +0000 Subject: [PATCH 1/2] Don't issue early data capable tickets in DTLS 1.3. Our DTLS 1.3 implementation doesn't support early data right now, so we shouldn't issue tickets that indicate we support it, even if early data is enabled via SSL_(CTX_)set_early_data_enabled. Bug: 42290594 Change-Id: I0f7f8ca05b45a6b40bf63ba77ab3c0a73df2ae44 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72667 Commit-Queue: Nick Harper Reviewed-by: David Benjamin Auto-Submit: Nick Harper --- ssl/ssl_test.cc | 29 ++++++++++++++++++++++++++++- ssl/tls13_server.cc | 5 ++++- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 06f90f430..e31c532b6 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -1376,7 +1376,7 @@ static bssl::UniquePtr GetTestKey() { static bssl::UniquePtr CreateContextWithTestCertificate( const SSL_METHOD *method) { - bssl::UniquePtr ctx(SSL_CTX_new(TLS_method())); + bssl::UniquePtr ctx(SSL_CTX_new(method)); bssl::UniquePtr cert = GetTestCertificate(); bssl::UniquePtr key = GetTestKey(); if (!ctx || !cert || !key || @@ -9754,5 +9754,32 @@ TEST(SSLTest, EarlyDataVersionMismatch) { EXPECT_NE(SSL_get0_peer_certificates(client.get()), nullptr); } +TEST(SSLTest, EarlyDataDisabledInDTLS13) { + // Set up some 0-RTT-enabled contexts. + bssl::UniquePtr client_ctx(SSL_CTX_new(DTLS_method())); + bssl::UniquePtr server_ctx = + CreateContextWithTestCertificate(DTLS_method()); + ASSERT_TRUE(client_ctx); + ASSERT_TRUE(server_ctx); + + SSL_CTX_set_early_data_enabled(client_ctx.get(), true); + SSL_CTX_set_early_data_enabled(server_ctx.get(), true); + SSL_CTX_set_session_cache_mode(client_ctx.get(), SSL_SESS_CACHE_BOTH); + SSL_CTX_set_session_cache_mode(server_ctx.get(), SSL_SESS_CACHE_BOTH); + ASSERT_TRUE(SSL_CTX_set_min_proto_version(client_ctx.get(), + DTLS1_3_EXPERIMENTAL_VERSION)); + ASSERT_TRUE(SSL_CTX_set_max_proto_version(client_ctx.get(), + DTLS1_3_EXPERIMENTAL_VERSION)); + ASSERT_TRUE(SSL_CTX_set_min_proto_version(server_ctx.get(), + DTLS1_3_EXPERIMENTAL_VERSION)); + ASSERT_TRUE(SSL_CTX_set_max_proto_version(server_ctx.get(), + DTLS1_3_EXPERIMENTAL_VERSION)); + + bssl::UniquePtr session = + CreateClientSession(client_ctx.get(), server_ctx.get()); + ASSERT_TRUE(session); + EXPECT_FALSE(SSL_SESSION_early_data_capable(session.get())); +} + } // namespace BSSL_NAMESPACE_END diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc index 65232f08c..a8b0c8bd5 100644 --- a/ssl/tls13_server.cc +++ b/ssl/tls13_server.cc @@ -152,9 +152,12 @@ static bool add_new_session_tickets(SSL_HANDSHAKE *hs, bool *out_sent_tickets) { return false; } session->ticket_age_add_valid = true; + // TODO(crbug.com/42290594): Remove the SSL_is_dtls check once we support + // 0-RTT for DTLS 1.3. bool enable_early_data = ssl->enable_early_data && - (!ssl->quic_method || !ssl->config->quic_early_data_context.empty()); + (!ssl->quic_method || !ssl->config->quic_early_data_context.empty()) && + !SSL_is_dtls(ssl); if (enable_early_data) { // QUIC does not use the max_early_data_size parameter and always sets it // to a fixed value. See RFC 9001, section 4.6.1. From fa2b8e9998947c38d55f96954b44a8a3133149aa Mon Sep 17 00:00:00 2001 From: Nick Harper Date: Wed, 30 Oct 2024 01:18:19 +0000 Subject: [PATCH 2/2] Don't attempt to send early data in DTLS 1.3. This implementation doesn't support early data in DTLS 1.3. If configured to support early data, that configuration should be ignored and it should not attempt to negotiate early data. Bug: 42290594 Change-Id: I72799e133cf62a5d81069b610e75921f2f53e437 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72668 Reviewed-by: David Benjamin Reviewed-by: Nick Harper Commit-Queue: Nick Harper --- ssl/handshake_client.cc | 4 +++- ssl/test/bssl_shim.cc | 8 +++++++- ssl/test/runner/handshake_server.go | 4 ++++ ssl/test/runner/runner.go | 28 ++++++++++++++++++++++++---- 4 files changed, 38 insertions(+), 6 deletions(-) diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc index 031ed81ff..9bd84627e 100644 --- a/ssl/handshake_client.cc +++ b/ssl/handshake_client.cc @@ -426,9 +426,11 @@ static ssl_early_data_reason_t should_offer_early_data( return ssl_early_data_disabled; } - if (hs->max_version < TLS1_3_VERSION) { + if (hs->max_version < TLS1_3_VERSION || SSL_is_dtls(ssl)) { // We discard inapplicable sessions, so this is redundant with the session // checks below, but reporting that TLS 1.3 was disabled is more useful. + // + // TODO(crbug.com/42290594): Support early data in DTLS 1.3. return ssl_early_data_protocol_version; } diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index 412417818..94d529b42 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc @@ -660,7 +660,7 @@ static bool CheckHandshakeProperties(SSL *ssl, bool is_resume, } // The early data status is only applicable after the handshake is confirmed. - if (!SSL_in_early_data(ssl)) { + if (!SSL_in_early_data(ssl) && !SSL_is_dtls(ssl)) { if ((config->expect_accept_early_data && !SSL_early_data_accepted(ssl)) || (config->expect_reject_early_data && SSL_early_data_accepted(ssl))) { fprintf(stderr, @@ -679,6 +679,12 @@ static bool CheckHandshakeProperties(SSL *ssl, bool is_resume, } } + if (SSL_is_dtls(ssl) && SSL_in_early_data(ssl)) { + // TODO(crbug.com/42290594): Support early data for DTLS 1.3. + fprintf(stderr, "DTLS unexpectedly in early data\n"); + return false; + } + if (!config->psk.empty()) { if (SSL_get_peer_cert_chain(ssl) != nullptr) { fprintf(stderr, "Received peer certificate on a PSK cipher.\n"); diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go index 7ed4745f7..284831a36 100644 --- a/ssl/test/runner/handshake_server.go +++ b/ssl/test/runner/handshake_server.go @@ -694,6 +694,10 @@ func (hs *serverHandshakeState) doTLS13Handshake() error { hs.finishedHash.addEntropy(hs.finishedHash.zeroSecret()) } + if hs.clientHello.hasEarlyData && c.isDTLS { + return errors.New("tls: early data extension received in DTLS") + } + hs.hello.hasKeyShare = true if hs.sessionState != nil && config.Bugs.NegotiatePSKResumption { hs.hello.hasKeyShare = false diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 4c6590f1d..66d084909 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -1559,10 +1559,15 @@ func runTest(dispatcher *shimDispatcher, statusChan chan statusMsg, test *testCa resumeConfig.MaxEarlyDataSize = 16384 } - // Configure the shim to send some data in early data. - flags = append(flags, "-on-resume-shim-writes-first") - if resumeConfig.Bugs.ExpectEarlyData == nil { - resumeConfig.Bugs.ExpectEarlyData = [][]byte{[]byte(shimInitialWrite)} + // In DTLS 1.3, we're setting flags to configure the client to attempt + // sending early data, but we expect it to realize that it's incapable + // of supporting early data and not send any. + if test.protocol != dtls { + // Configure the shim to send some data in early data. + flags = append(flags, "-on-resume-shim-writes-first") + if resumeConfig.Bugs.ExpectEarlyData == nil { + resumeConfig.Bugs.ExpectEarlyData = [][]byte{[]byte(shimInitialWrite)} + } } } else { // By default, send some early data and expect half-RTT data response. @@ -5236,6 +5241,21 @@ func addStateMachineCoverageTests(config stateMachineTestConfig) { }) } + // Test that early data is disabled for DTLS 1.3. + if config.protocol == dtls { + tests = append(tests, testCase{ + testType: clientTest, + protocol: dtls, + name: "DTLS13-EarlyData", + config: Config{ + MaxVersion: VersionTLS13, + MinVersion: VersionTLS13, + }, + resumeSession: true, + earlyData: true, + }) + } + // TLS client auth. // The following tests have a max version of 1.2, so they are not suitable // for use with QUIC.