diff --git a/src/include/openssl/ssl.h b/src/include/openssl/ssl.h index e7c57c708..9e9acee23 100644 --- a/src/include/openssl/ssl.h +++ b/src/include/openssl/ssl.h @@ -672,17 +672,7 @@ OPENSSL_EXPORT int DTLSv1_handle_timeout(SSL *ssl); #define DTLS1_VERSION 0xfeff #define DTLS1_2_VERSION 0xfefd -// DTLS1_3_EXPERIMENTAL_VERSION gates experimental, in-progress code for DTLS -// 1.3. -// -// WARNING: Do not use this value. BoringSSL's DTLS 1.3 implementation is still -// under development. The code enabled by this value is neither stable nor -// secure. It does not correspond to any real protocol. It is also incompatible -// with other DTLS implementations, and it is not compatible with future or past -// versions of BoringSSL. -// -// When the DTLS 1.3 implementation is complete, this symbol will be replaced. -#define DTLS1_3_EXPERIMENTAL_VERSION 0xfc25 +#define DTLS1_3_VERSION 0xfefc // SSL_CTX_set_min_proto_version sets the minimum protocol version for |ctx| to // |version|. If |version| is zero, the default minimum version is used. It diff --git a/src/ssl/d1_both.cc b/src/ssl/d1_both.cc index db51c9a36..648bd66db 100644 --- a/src/ssl/d1_both.cc +++ b/src/ssl/d1_both.cc @@ -368,9 +368,8 @@ bool dtls1_process_handshake_fragments(SSL *ssl, uint8_t *out_alert, const size_t frag_off = msg_hdr.frag_off; const size_t frag_len = msg_hdr.frag_len; const size_t msg_len = msg_hdr.msg_len; - if (frag_off > msg_len || frag_len > msg_len - frag_off || - msg_len > ssl_max_handshake_message_len(ssl)) { - OPENSSL_PUT_ERROR(SSL, SSL_R_EXCESSIVE_MESSAGE_SIZE); + if (frag_off > msg_len || frag_len > msg_len - frag_off) { + OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_HANDSHAKE_RECORD); *out_alert = SSL_AD_ILLEGAL_PARAMETER; return false; } @@ -384,19 +383,24 @@ bool dtls1_process_handshake_fragments(SSL *ssl, uint8_t *out_alert, continue; } - assert(record_number.epoch() == ssl->d1->read_epoch.epoch); - if (ssl->d1->next_read_epoch != nullptr) { - // Any any time, we only expect new messages in one epoch. If - // |next_read_epoch| is set, we've started a new epoch but haven't - // received records in it yet. (Once a record is received in the new - // epoch, |next_read_epoch| becomes the current read epoch.) This new - // fragment is in the old epoch, but we expect handshake messages to be in - // the next epoch, so this is an error. + if (record_number.epoch() != ssl->d1->read_epoch.epoch || + ssl->d1->next_read_epoch != nullptr) { + // New messages can only arrive in the latest epoch. This can fail if the + // record came from |prev_read_epoch|, or if it came from |read_epoch| but + // |next_read_epoch| exists. (It cannot come from |next_read_epoch| + // because |next_read_epoch| becomes |read_epoch| once it receives a + // record.) OPENSSL_PUT_ERROR(SSL, SSL_R_EXCESS_HANDSHAKE_DATA); *out_alert = SSL_AD_UNEXPECTED_MESSAGE; return false; } + if (msg_len > ssl_max_handshake_message_len(ssl)) { + OPENSSL_PUT_ERROR(SSL, SSL_R_EXCESSIVE_MESSAGE_SIZE); + *out_alert = SSL_AD_ILLEGAL_PARAMETER; + return false; + } + if (SSL_in_init(ssl) && ssl_has_final_version(ssl) && ssl_protocol_version(ssl) >= TLS1_3_VERSION) { // During the handshake, if we receive any portion of the next flight, the @@ -482,18 +486,23 @@ ssl_open_record_t dtls1_open_handshake(SSL *ssl, size_t *out_consumed, return ssl_open_record_discard; case SSL3_RT_CHANGE_CIPHER_SPEC: + if (record.size() != 1u || record[0] != SSL3_MT_CCS) { + OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_CHANGE_CIPHER_SPEC); + *out_alert = SSL_AD_ILLEGAL_PARAMETER; + return ssl_open_record_error; + } + // We do not support renegotiation, so encrypted ChangeCipherSpec records // are illegal. - if (ssl->d1->read_epoch.epoch != 0) { + if (record_number.epoch() != 0) { OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD); *out_alert = SSL_AD_UNEXPECTED_MESSAGE; return ssl_open_record_error; } - if (record.size() != 1u || record[0] != SSL3_MT_CCS) { - OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_CHANGE_CIPHER_SPEC); - *out_alert = SSL_AD_ILLEGAL_PARAMETER; - return ssl_open_record_error; + // Ignore ChangeCipherSpec from a previous epoch. + if (record_number.epoch() != ssl->d1->read_epoch.epoch) { + return ssl_open_record_discard; } // Flag the ChangeCipherSpec for later. @@ -507,20 +516,17 @@ ssl_open_record_t dtls1_open_handshake(SSL *ssl, size_t *out_consumed, return dtls1_process_ack(ssl, out_alert, record_number, record); case SSL3_RT_HANDSHAKE: - // Break out to main processing. - break; + if (!dtls1_process_handshake_fragments(ssl, out_alert, record_number, + record)) { + return ssl_open_record_error; + } + return ssl_open_record_success; default: OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD); *out_alert = SSL_AD_UNEXPECTED_MESSAGE; return ssl_open_record_error; } - - if (!dtls1_process_handshake_fragments(ssl, out_alert, record_number, - record)) { - return ssl_open_record_error; - } - return ssl_open_record_success; } bool dtls1_get_message(const SSL *ssl, SSLMessage *out) { diff --git a/src/ssl/dtls_record.cc b/src/ssl/dtls_record.cc index 5b124d9a0..1bfc43f6a 100644 --- a/src/ssl/dtls_record.cc +++ b/src/ssl/dtls_record.cc @@ -265,6 +265,13 @@ static bool parse_dtls13_record(SSL *ssl, CBS *in, ParsedDTLSRecord *out) { BSSL_CHECK(CBS_get_bytes(in, &out->body, CBS_len(in))); } + // Drop the previous read epoch if expired. + if (ssl->d1->prev_read_epoch != nullptr && + ssl_ctx_get_current_time(ssl->ctx.get()).tv_sec > + ssl->d1->prev_read_epoch->expire) { + ssl->d1->prev_read_epoch = nullptr; + } + // Look up the corresponding epoch. This header form only matches encrypted // DTLS 1.3 epochs. DTLSReadEpoch *read_epoch = nullptr; @@ -273,6 +280,9 @@ static bool parse_dtls13_record(SSL *ssl, CBS *in, ParsedDTLSRecord *out) { } else if (ssl->d1->next_read_epoch != nullptr && epoch == ssl->d1->next_read_epoch->epoch) { read_epoch = ssl->d1->next_read_epoch.get(); + } else if (ssl->d1->prev_read_epoch != nullptr && + epoch == ssl->d1->prev_read_epoch->epoch.epoch) { + read_epoch = &ssl->d1->prev_read_epoch->epoch; } if (read_epoch != nullptr && use_dtls13_record_header(ssl, epoch)) { out->read_epoch = read_epoch; @@ -434,22 +444,32 @@ enum ssl_open_record_t dtls_open_record(SSL *ssl, uint8_t *out_type, record.read_epoch->bitmap.Record(record.number.sequence()); - // Once we receive a record from the next epoch, it becomes the current epoch. + // Once we receive a record from the next epoch in DTLS 1.3, it becomes the + // current epoch. Also save the previous epoch. This allows us to handle + // packet reordering on KeyUpdate, as well as ACK retransmissions of the + // Finished flight. if (record.read_epoch == ssl->d1->next_read_epoch.get()) { + assert(ssl_protocol_version(ssl) >= TLS1_3_VERSION); + auto prev = MakeUnique(); + if (prev == nullptr) { + *out_alert = SSL_AD_INTERNAL_ERROR; + return ssl_open_record_error; + } + + // Release the epoch after a timeout. + prev->expire = ssl_ctx_get_current_time(ssl->ctx.get()).tv_sec; + if (prev->expire >= UINT64_MAX - DTLS_PREV_READ_EPOCH_EXPIRE_SECONDS) { + prev->expire = UINT64_MAX; // Saturate on overflow. + } else { + prev->expire += DTLS_PREV_READ_EPOCH_EXPIRE_SECONDS; + } + + prev->epoch = std::move(ssl->d1->read_epoch); + ssl->d1->prev_read_epoch = std::move(prev); ssl->d1->read_epoch = std::move(*ssl->d1->next_read_epoch); ssl->d1->next_read_epoch = nullptr; } - // We do not retain previous epochs, so it is guaranteed records come in at - // the "current" epoch. (But the current epoch may be one behind the - // handshake.) - // - // TODO(crbug.com/374890768): In DTLS 1.3, where rekeys may occur - // mid-connection, retaining previous epochs would make us more robust to - // packet reordering. If we do this, we'll need to take care to not - // accidentally accept data at the wrong epoch. - assert(record.number.epoch() == ssl->d1->read_epoch.epoch); - // TODO(davidben): Limit the number of empty records as in TLS? This is only // useful if we also limit discarded packets. diff --git a/src/ssl/handshake_client.cc b/src/ssl/handshake_client.cc index b904e4f3c..afc99be95 100644 --- a/src/ssl/handshake_client.cc +++ b/src/ssl/handshake_client.cc @@ -796,7 +796,7 @@ static enum ssl_hs_wait_t do_read_server_hello(SSL_HANDSHAKE *hs) { // Enforce the TLS 1.3 anti-downgrade feature. if (!ssl->s3->initial_handshake_complete && - ssl_supports_version(hs, TLS1_3_VERSION)) { + hs->max_version >= TLS1_3_VERSION) { static_assert( sizeof(kTLS12DowngradeRandom) == sizeof(kTLS13DowngradeRandom), "downgrade signals have different size"); diff --git a/src/ssl/handshake_server.cc b/src/ssl/handshake_server.cc index e41b23354..843e2badf 100644 --- a/src/ssl/handshake_server.cc +++ b/src/ssl/handshake_server.cc @@ -1057,7 +1057,7 @@ static enum ssl_hs_wait_t do_send_server_hello(SSL_HANDSHAKE *hs) { } // Implement the TLS 1.3 anti-downgrade feature. - if (ssl_supports_version(hs, TLS1_3_VERSION)) { + if (hs->max_version >= TLS1_3_VERSION) { if (ssl_protocol_version(ssl) == TLS1_2_VERSION) { if (hs->apply_jdk11_workaround) { // JDK 11 implements the TLS 1.3 downgrade signal, so we cannot send it diff --git a/src/ssl/internal.h b/src/ssl/internal.h index b2eb5647e..eab3af728 100644 --- a/src/ssl/internal.h +++ b/src/ssl/internal.h @@ -3540,6 +3540,27 @@ enum class QueuedKeyUpdate { kUpdateRequested, }; +// DTLS_PREV_READ_EPOCH_EXPIRE_SECONDS is how long to retain the previous read +// epoch in DTLS 1.3. This value is set based on the following: +// +// - Section 4.2.1 of RFC 9147 recommends retaining past read epochs for the +// default TCP MSL. This accommodates packet reordering with KeyUpdate. +// +// - Section 5.8.1 of RFC 9147 requires being capable of ACKing the client's +// final flight for at least twice the default MSL. That requires retaining +// epoch 2 after the handshake. +// +// - Section 4 of RFC 9293 defines the MSL to be two minutes. +#define DTLS_PREV_READ_EPOCH_EXPIRE_SECONDS (4 * 60) + +struct DTLSPrevReadEpoch { + static constexpr bool kAllowUniquePtr = true; + DTLSReadEpoch epoch; + // expire is the expiration time of the read epoch, expressed as a POSIX + // timestamp in seconds. + uint64_t expire; +}; + struct DTLS1_STATE { static constexpr bool kAllowUniquePtr = true; @@ -3579,13 +3600,16 @@ struct DTLS1_STATE { uint16_t handshake_write_seq = 0; uint16_t handshake_read_seq = 0; - // read_epoch is the current DTLS read epoch. + // read_epoch is the current read epoch. DTLSReadEpoch read_epoch; - // next_read_epoch is the next DTLS read epoch in DTLS 1.3. It will become + // next_read_epoch is the next read epoch in DTLS 1.3. It will become // current once a record is received from it. UniquePtr next_read_epoch; + // prev_read_epoch is the previous read epoch in DTLS 1.3. + UniquePtr prev_read_epoch; + // write_epoch is the current DTLS write epoch. Non-retransmit records will // generally use this epoch. // TODO(crbug.com/381113363): 0-RTT will be the exception, when implemented. diff --git a/src/ssl/ssl_test.cc b/src/ssl/ssl_test.cc index 055015fd6..5fb78da46 100644 --- a/src/ssl/ssl_test.cc +++ b/src/ssl/ssl_test.cc @@ -92,7 +92,7 @@ static const VersionParam kAllVersions[] = { {TLS1_3_VERSION, VersionParam::is_tls, "TLS1_3"}, {DTLS1_VERSION, VersionParam::is_dtls, "DTLS1"}, {DTLS1_2_VERSION, VersionParam::is_dtls, "DTLS1_2"}, - {DTLS1_3_EXPERIMENTAL_VERSION, VersionParam::is_dtls, "DTLS1_3"}, + {DTLS1_3_VERSION, VersionParam::is_dtls, "DTLS1_3"}, }; struct ExpectedCipher { @@ -2765,8 +2765,7 @@ class SSLVersionTest : public ::testing::TestWithParam { uint16_t version() const { return GetParam().version; } bool is_tls13() const { - return version() == TLS1_3_VERSION || - version() == DTLS1_3_EXPERIMENTAL_VERSION; + return version() == TLS1_3_VERSION || version() == DTLS1_3_VERSION; } bool is_dtls() const { @@ -2799,7 +2798,7 @@ TEST_P(SSLVersionTest, SequenceNumber) { uint64_t server_write_seq = SSL_get_write_sequence(server_.get()); if (is_dtls()) { - if (version() == DTLS1_3_EXPERIMENTAL_VERSION) { + if (version() == DTLS1_3_VERSION) { // Both client and server must be at epoch 3 (application data). EXPECT_EQ(EpochFromSequence(client_write_seq), 3); EXPECT_EQ(EpochFromSequence(server_write_seq), 3); @@ -2830,7 +2829,7 @@ TEST_P(SSLVersionTest, SequenceNumber) { EXPECT_EQ(SSL_write(client_.get(), &byte, 1), 1); EXPECT_EQ(SSL_read(server_.get(), &byte, 1), 1); - if (version() == DTLS1_3_EXPERIMENTAL_VERSION) { + if (version() == DTLS1_3_VERSION) { // TODO(crbug.com/42290608): Write an appropriate test for incrementing both // sequence number and epoch in the following test. The server read seq was // in epoch 2, but after the write it's in epoch 3, so adding 1 doesn't work @@ -3985,7 +3984,7 @@ static const char *GetVersionName(uint16_t version) { return "DTLSv1"; case DTLS1_2_VERSION: return "DTLSv1.2"; - case DTLS1_3_EXPERIMENTAL_VERSION: + case DTLS1_3_VERSION: return "DTLSv1.3"; default: return "???"; @@ -4376,7 +4375,7 @@ TEST_P(SSLVersionTest, SSLWriteRetry) { } TEST_P(SSLVersionTest, RecordCallback) { - if (version() == DTLS1_3_EXPERIMENTAL_VERSION) { + if (version() == DTLS1_3_VERSION) { // The DTLS 1.3 record header is vastly different than the TLS or DTLS < 1.3 // header format. Instead of checking that the record header is formatted as // expected here, the runner implementation in dtls.go is strict about what @@ -9749,14 +9748,10 @@ TEST(SSLTest, EarlyDataDisabledInDTLS13) { 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)); + ASSERT_TRUE(SSL_CTX_set_min_proto_version(client_ctx.get(), DTLS1_3_VERSION)); + ASSERT_TRUE(SSL_CTX_set_max_proto_version(client_ctx.get(), DTLS1_3_VERSION)); + ASSERT_TRUE(SSL_CTX_set_min_proto_version(server_ctx.get(), DTLS1_3_VERSION)); + ASSERT_TRUE(SSL_CTX_set_max_proto_version(server_ctx.get(), DTLS1_3_VERSION)); bssl::UniquePtr session = CreateClientSession(client_ctx.get(), server_ctx.get()); diff --git a/src/ssl/ssl_versions.cc b/src/ssl/ssl_versions.cc index a34278dc1..20e17e13c 100644 --- a/src/ssl/ssl_versions.cc +++ b/src/ssl/ssl_versions.cc @@ -46,7 +46,7 @@ bool ssl_protocol_version_from_wire(uint16_t *out, uint16_t version) { *out = TLS1_2_VERSION; return true; - case DTLS1_3_EXPERIMENTAL_VERSION: + case DTLS1_3_VERSION: *out = TLS1_3_VERSION; return true; @@ -66,7 +66,7 @@ static const uint16_t kTLSVersions[] = { }; static const uint16_t kDTLSVersions[] = { - DTLS1_3_EXPERIMENTAL_VERSION, + DTLS1_3_VERSION, DTLS1_2_VERSION, DTLS1_VERSION, }; @@ -104,7 +104,7 @@ static const VersionInfo kVersionNames[] = { {TLS1_VERSION, "TLSv1"}, {DTLS1_VERSION, "DTLSv1"}, {DTLS1_2_VERSION, "DTLSv1.2"}, - {DTLS1_3_EXPERIMENTAL_VERSION, "DTLSv1.3"}, + {DTLS1_3_VERSION, "DTLSv1.3"}, }; static const char *ssl_version_to_string(uint16_t version) { @@ -156,6 +156,8 @@ static bool set_min_version(const SSL_PROTOCOL_METHOD *method, uint16_t *out, static bool set_max_version(const SSL_PROTOCOL_METHOD *method, uint16_t *out, uint16_t version) { // Zero is interpreted as the default maximum version. + // TODO(crbug.com/42290594): Enable DTLS 1.3 by default, after it's + // successfully shipped in WebRTC. if (version == 0) { *out = method->is_dtls ? DTLS1_2_VERSION : TLS1_3_VERSION; return true; diff --git a/src/ssl/test/fuzzer.h b/src/ssl/test/fuzzer.h index ca7b55a77..01b689ae6 100644 --- a/src/ssl/test/fuzzer.h +++ b/src/ssl/test/fuzzer.h @@ -414,7 +414,12 @@ class TLSFuzzer { SSL_CTX_enable_ocsp_stapling(ctx_.get()); // Enable versions and ciphers that are off by default. - if (!SSL_CTX_set_strict_cipher_list(ctx_.get(), "ALL:3DES")) { + uint16_t min_version = protocol_ == kDTLS ? DTLS1_VERSION : TLS1_VERSION; + uint16_t max_version = + protocol_ == kDTLS ? DTLS1_3_VERSION : TLS1_3_VERSION; + if (!SSL_CTX_set_min_proto_version(ctx_.get(), min_version) || + !SSL_CTX_set_max_proto_version(ctx_.get(), max_version) || + !SSL_CTX_set_strict_cipher_list(ctx_.get(), "ALL:3DES")) { return false; } diff --git a/src/ssl/test/runner/common.go b/src/ssl/test/runner/common.go index 76195f6f3..8a766729c 100644 --- a/src/ssl/test/runner/common.go +++ b/src/ssl/test/runner/common.go @@ -31,9 +31,9 @@ const ( ) const ( - VersionDTLS10 = 0xfeff - VersionDTLS12 = 0xfefd - VersionDTLS125Experimental = 0xfc25 + VersionDTLS10 = 0xfeff + VersionDTLS12 = 0xfefd + VersionDTLS13 = 0xfefc ) var allTLSWireVersions = []uint16{ @@ -45,7 +45,7 @@ var allTLSWireVersions = []uint16{ } var allDTLSWireVersions = []uint16{ - VersionDTLS125Experimental, + VersionDTLS13, VersionDTLS12, VersionDTLS10, } @@ -1291,6 +1291,11 @@ type ProtocolBugs struct { // post-handshake transaction) in DTLS. See DTLSController for details. ACKFlightDTLS ACKFlightFunc + // SkipImplicitACKRead, if true, causes the DTLS 1.3 client to skip + // implicitly reading the ACK at the end of the handshake. This may be used + // when WriteFlightDTLS consumes the ACK itself. + SkipImplicitACKRead bool + // MockQUICTransport is the mockQUICTransport used when testing // QUIC interfaces. MockQUICTransport *mockQUICTransport @@ -2121,7 +2126,7 @@ func (c *Config) echCipherSuitePreferences() []HPKECipherSuite { func wireToVersion(vers uint16, isDTLS bool) (uint16, bool) { if isDTLS { switch vers { - case VersionDTLS125Experimental: + case VersionDTLS13: return VersionTLS13, true case VersionDTLS12: return VersionTLS12, true diff --git a/src/ssl/test/runner/handshake_client.go b/src/ssl/test/runner/handshake_client.go index 454333983..f9dfb9835 100644 --- a/src/ssl/test/runner/handshake_client.go +++ b/src/ssl/test/runner/handshake_client.go @@ -848,7 +848,7 @@ func (hs *clientHandshakeState) createClientHello(innerHello *clientHelloMsg, ec if session.vers < VersionTLS13 { version = VersionTLS13 if c.isDTLS { - version = VersionDTLS125Experimental + version = VersionDTLS13 } } generatePSKBinders(version, c.isDTLS, hello, session, nil, nil, c.config) @@ -1477,9 +1477,6 @@ func (hs *clientHandshakeState) doTLS13Handshake(msg any) error { if c.config.Bugs.SendExtraFinished { c.writeRecord(recordTypeHandshake, finished.marshal()) } - if err := c.flushHandshake(); err != nil { - return err - } if data := c.config.Bugs.AppDataBeforeTLS13KeyChange; data != nil { c.writeRecord(recordTypeApplicationData, data) @@ -1488,14 +1485,18 @@ func (hs *clientHandshakeState) doTLS13Handshake(msg any) error { // Switch to application data keys. c.useOutTrafficSecret(uint16(encryptionApplication), c.wireVersion, hs.suite, clientTrafficSecret) c.resumptionSecret = hs.finishedHash.deriveSecret(resumptionLabel) - for _, ticket := range deferredTickets { - if err := c.processTLS13NewSessionTicket(ticket, hs.suite); err != nil { + + if err := c.flushHandshake(); err != nil { + return err + } + if c.isDTLS && len(c.expectedACK) != 0 && !c.config.Bugs.SkipImplicitACKRead { + if err := c.readRecord(recordTypeACK); err != nil { return err } } - if c.isDTLS && len(c.expectedACK) != 0 { - if err := c.readRecord(recordTypeACK); err != nil { + for _, ticket := range deferredTickets { + if err := c.processTLS13NewSessionTicket(ticket, hs.suite); err != nil { return err } } diff --git a/src/ssl/test/runner/runner.go b/src/ssl/test/runner/runner.go index fe9c1bdaf..41fa4541a 100644 --- a/src/ssl/test/runner/runner.go +++ b/src/ssl/test/runner/runner.go @@ -1903,7 +1903,7 @@ var tlsVersions = []tlsVersion{ excludeFlag: "-no-tls13", hasQUIC: true, hasDTLS: true, - versionDTLS: VersionDTLS125Experimental, + versionDTLS: VersionDTLS13, versionWire: VersionTLS13, }, } @@ -3466,6 +3466,45 @@ read alert 1 0 expectedError: ":EXCESS_HANDSHAKE_DATA:", expectedLocalError: "remote error: unexpected message", }, + { + protocol: dtls, + testType: serverTest, + name: "DTLS13-SendExtraFinished-AfterAppData", + config: Config{ + MaxVersion: VersionTLS13, + Bugs: ProtocolBugs{ + SkipImplicitACKRead: true, + WriteFlightDTLS: func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo) { + if next[len(next)-1].Type != typeFinished { + c.WriteFlight(next) + return + } + + // Complete the handshake. + c.WriteFlight(next) + c.ReadACK(c.InEpoch()) + + // Send some application data. The shim is now on epoch 3. + msg := []byte("hello") + c.WriteAppData(c.OutEpoch(), msg) + c.ReadAppData(c.InEpoch(), expectedReply(msg)) + + // The shim is still accepting data from epoch 2, so it can + // ACK a retransmit if needed, but it should not accept new + // messages at epoch three. + extraFinished := next[len(next)-1] + extraFinished.Sequence++ + c.WriteFlight([]DTLSMessage{extraFinished}) + }, + }, + }, + shouldFail: true, + expectedError: ":EXCESS_HANDSHAKE_DATA:", + expectedLocalError: "remote error: unexpected message", + // Disable tickets on the shim to avoid NewSessionTicket + // interfering with the test callback. + flags: []string{"-no-ticket"}, + }, { testType: serverTest, name: "V2ClientHello-EmptyRecordPrefix", @@ -6852,8 +6891,9 @@ func addVersionNegotiationTests() { name: "MinorVersionTolerance-DTLS", config: Config{ Bugs: ProtocolBugs{ - SendClientVersion: 0xfe00, - OmitSupportedVersions: true, + SendClientVersion: 0xfe00, + OmitSupportedVersions: true, + IgnoreTLS13DowngradeRandom: true, }, }, expectations: connectionExpectations{ @@ -6866,8 +6906,9 @@ func addVersionNegotiationTests() { name: "MajorVersionTolerance-DTLS", config: Config{ Bugs: ProtocolBugs{ - SendClientVersion: 0xfdff, - OmitSupportedVersions: true, + SendClientVersion: 0xfdff, + OmitSupportedVersions: true, + IgnoreTLS13DowngradeRandom: true, }, }, expectations: connectionExpectations{ @@ -6914,49 +6955,50 @@ func addVersionNegotiationTests() { }) // Test TLS 1.3's downgrade signal. - var downgradeTests = []struct { - name string - version uint16 - clientShimError string - }{ - {"TLS12", VersionTLS12, "tls: downgrade from TLS 1.3 detected"}, - {"TLS11", VersionTLS11, "tls: downgrade from TLS 1.2 detected"}, - // TLS 1.0 does not have a dedicated value. - {"TLS10", VersionTLS10, "tls: downgrade from TLS 1.2 detected"}, - } - - for _, test := range downgradeTests { - // The client should enforce the downgrade sentinel. - testCases = append(testCases, testCase{ - name: "Downgrade-" + test.name + "-Client", - config: Config{ - Bugs: ProtocolBugs{ - NegotiateVersion: test.version, + for _, protocol := range []protocol{tls, dtls} { + for _, vers := range allVersions(protocol) { + if vers.version >= VersionTLS13 { + continue + } + clientShimError := "tls: downgrade from TLS 1.3 detected" + if vers.version < VersionTLS12 { + clientShimError = "tls: downgrade from TLS 1.2 detected" + } + // for _, test := range downgradeTests { + // The client should enforce the downgrade sentinel. + testCases = append(testCases, testCase{ + protocol: protocol, + name: "Downgrade-" + vers.name + "-Client-" + protocol.String(), + config: Config{ + Bugs: ProtocolBugs{ + NegotiateVersion: vers.wire(protocol), + }, }, - }, - expectations: connectionExpectations{ - version: test.version, - }, - shouldFail: true, - expectedError: ":TLS13_DOWNGRADE:", - expectedLocalError: "remote error: illegal parameter", - }) - - // The server should emit the downgrade signal. - testCases = append(testCases, testCase{ - testType: serverTest, - name: "Downgrade-" + test.name + "-Server", - config: Config{ - Bugs: ProtocolBugs{ - SendSupportedVersions: []uint16{test.version}, + expectations: connectionExpectations{ + version: vers.version, }, - }, - expectations: connectionExpectations{ - version: test.version, - }, - shouldFail: true, - expectedLocalError: test.clientShimError, - }) + shouldFail: true, + expectedError: ":TLS13_DOWNGRADE:", + expectedLocalError: "remote error: illegal parameter", + }) + + // The server should emit the downgrade signal. + testCases = append(testCases, testCase{ + protocol: protocol, + testType: serverTest, + name: "Downgrade-" + vers.name + "-Server-" + protocol.String(), + config: Config{ + Bugs: ProtocolBugs{ + SendSupportedVersions: []uint16{vers.wire(protocol)}, + }, + }, + expectations: connectionExpectations{ + version: vers.version, + }, + shouldFail: true, + expectedLocalError: clientShimError, + }) + } } // SSL 3.0 support has been removed. Test that the shim does not @@ -11548,6 +11590,10 @@ var shortTimeouts = []time.Duration{ 60 * time.Second, } +// dtlsPrevEpochExpiration is how long before the shim releases old epochs. Add +// an extra second to allow the shim to be less precise. +const dtlsPrevEpochExpiration = 4*time.Minute + 1*time.Second + func addDTLSRetransmitTests() { for _, shortTimeout := range []bool{false, true} { for _, vers := range allVersions(dtls) { @@ -11899,8 +11945,9 @@ func addDTLSRetransmitTests() { MaxHandshakeRecordLength: 512, WriteFlightDTLS: func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo) { if len(received) > 0 { + ackEpoch := received[len(received)-1].Epoch c.ExpectNextTimeout(useTimeouts[0]) - c.WriteACK(c.OutEpoch(), records) + c.WriteACK(ackEpoch, records) // After everything is ACKed, the shim should stop the timer // and wait for the next flight. c.ExpectNoNextTimeout() @@ -11911,8 +11958,9 @@ func addDTLSRetransmitTests() { c.WriteFlight(next) }, ACKFlightDTLS: handleNewSessionTicket(func(c *DTLSController, prev, received []DTLSMessage, records []DTLSRecordNumberInfo) { + ackEpoch := received[len(received)-1].Epoch c.ExpectNextTimeout(useTimeouts[0]) - c.WriteACK(c.OutEpoch(), records) + c.WriteACK(ackEpoch, records) // After everything is ACKed, the shim should stop the timer. c.ExpectNoNextTimeout() for _, t := range useTimeouts { @@ -11948,9 +11996,10 @@ func addDTLSRetransmitTests() { MaxPacketLength: 512, WriteFlightDTLS: func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo) { if len(received) > 0 { + ackEpoch := received[len(received)-1].Epoch for _, t := range useTimeouts[:len(useTimeouts)-1] { if len(records) > 0 { - c.WriteACK(c.OutEpoch(), []DTLSRecordNumberInfo{records[len(records)-1]}) + c.WriteACK(ackEpoch, []DTLSRecordNumberInfo{records[len(records)-1]}) } c.AdvanceClock(t) records = c.ReadRetransmit() @@ -11959,9 +12008,10 @@ func addDTLSRetransmitTests() { c.WriteFlight(next) }, ACKFlightDTLS: handleNewSessionTicket(func(c *DTLSController, prev, received []DTLSMessage, records []DTLSRecordNumberInfo) { + ackEpoch := received[len(received)-1].Epoch for _, t := range useTimeouts[:len(useTimeouts)-1] { if len(records) > 0 { - c.WriteACK(c.OutEpoch(), []DTLSRecordNumberInfo{records[len(records)-1]}) + c.WriteACK(ackEpoch, []DTLSRecordNumberInfo{records[len(records)-1]}) } c.AdvanceClock(t) records = c.ReadRetransmit() @@ -11986,9 +12036,10 @@ func addDTLSRetransmitTests() { MaxPacketLength: 512, WriteFlightDTLS: func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo) { if len(received) > 0 { + ackEpoch := received[len(received)-1].Epoch for _, t := range useTimeouts[:len(useTimeouts)-1] { if len(records) > 0 { - c.WriteACK(c.OutEpoch(), []DTLSRecordNumberInfo{records[0]}) + c.WriteACK(ackEpoch, []DTLSRecordNumberInfo{records[0]}) } c.AdvanceClock(t) records = c.ReadRetransmit() @@ -11997,9 +12048,10 @@ func addDTLSRetransmitTests() { c.WriteFlight(next) }, ACKFlightDTLS: handleNewSessionTicket(func(c *DTLSController, prev, received []DTLSMessage, records []DTLSRecordNumberInfo) { + ackEpoch := received[len(received)-1].Epoch for _, t := range useTimeouts[:len(useTimeouts)-1] { if len(records) > 0 { - c.WriteACK(c.OutEpoch(), []DTLSRecordNumberInfo{records[0]}) + c.WriteACK(ackEpoch, []DTLSRecordNumberInfo{records[0]}) } c.AdvanceClock(t) records = c.ReadRetransmit() @@ -12024,13 +12076,14 @@ func addDTLSRetransmitTests() { MaxPacketLength: 512, WriteFlightDTLS: func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo) { if len(received) > 0 { + ackEpoch := received[len(received)-1].Epoch for i, t := range useTimeouts[:len(useTimeouts)-1] { if len(records) > 0 { ack := make([]DTLSRecordNumberInfo, 0, (len(records)+2)/3) for i := 0; i < len(records); i += 3 { ack = append(ack, records[i]) } - c.WriteACK(c.OutEpoch(), ack) + c.WriteACK(ackEpoch, ack) } // Change the MTU every iteration, to make the fragment // patterns more complex. @@ -12042,9 +12095,10 @@ func addDTLSRetransmitTests() { c.WriteFlight(next) }, ACKFlightDTLS: handleNewSessionTicket(func(c *DTLSController, prev, received []DTLSMessage, records []DTLSRecordNumberInfo) { + ackEpoch := received[len(received)-1].Epoch for _, t := range useTimeouts[:len(useTimeouts)-1] { if len(records) > 0 { - c.WriteACK(c.OutEpoch(), []DTLSRecordNumberInfo{records[0]}) + c.WriteACK(ackEpoch, []DTLSRecordNumberInfo{records[0]}) } c.AdvanceClock(t) records = c.ReadRetransmit() @@ -12067,26 +12121,28 @@ func addDTLSRetransmitTests() { SendHelloRetryRequestCookie: []byte("cookie"), WriteFlightDTLS: func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo) { if len(received) > 0 { + ackEpoch := received[len(received)-1].Epoch // Keep ACKing the same record over and over. - c.WriteACK(c.OutEpoch(), records[:1]) + c.WriteACK(ackEpoch, records[:1]) c.AdvanceClock(useTimeouts[0]) c.ReadRetransmit() - c.WriteACK(c.OutEpoch(), records[:1]) + c.WriteACK(ackEpoch, records[:1]) c.AdvanceClock(useTimeouts[1]) c.ReadRetransmit() } c.WriteFlight(next) }, ACKFlightDTLS: handleNewSessionTicket(func(c *DTLSController, prev, received []DTLSMessage, records []DTLSRecordNumberInfo) { + ackEpoch := received[len(received)-1].Epoch // Keep ACKing the same record over and over. - c.WriteACK(c.OutEpoch(), records[:1]) + c.WriteACK(ackEpoch, records[:1]) c.AdvanceClock(useTimeouts[0]) c.ReadRetransmit() - c.WriteACK(c.OutEpoch(), records[:1]) + c.WriteACK(ackEpoch, records[:1]) c.AdvanceClock(useTimeouts[1]) c.ReadRetransmit() // ACK everything to clear the timer. - c.WriteACK(c.OutEpoch(), records) + c.WriteACK(ackEpoch, records) }), }, }, @@ -12185,10 +12241,11 @@ func addDTLSRetransmitTests() { MaxPacketLength: 512, WriteFlightDTLS: func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo) { if len(received) > 0 { - c.WriteACK(c.OutEpoch(), records[len(records)/2:]) + ackEpoch := received[len(received)-1].Epoch + c.WriteACK(ackEpoch, records[len(records)/2:]) c.AdvanceClock(useTimeouts[0]) c.ReadRetransmit() - c.WriteACK(c.OutEpoch(), records[:len(records)/2]) + c.WriteACK(ackEpoch, records[:len(records)/2]) // Everything should be ACKed now. The shim should not // retransmit anything. c.AdvanceClock(useTimeouts[1]) @@ -12295,6 +12352,59 @@ func addDTLSRetransmitTests() { flags: flags, }) + // The server must continue to ACK the Finished flight even after + // receiving application data from the client. + testCases = append(testCases, testCase{ + protocol: dtls, + testType: serverTest, + name: "DTLS-Retransmit-Server-ACKFinishedAfterAppData" + suffix, + config: Config{ + MaxVersion: vers.version, + Bugs: ProtocolBugs{ + // WriteFlightDTLS will handle consuming ACKs. + SkipImplicitACKRead: true, + WriteFlightDTLS: func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo) { + if next[len(next)-1].Type != typeFinished { + c.WriteFlight(next) + return + } + + // Write Finished. The shim should ACK it immediately. + c.WriteFlight(next) + c.ReadACK(c.InEpoch()) + + // Exchange some application data. + msg := []byte("hello") + c.WriteAppData(c.OutEpoch(), msg) + c.ReadAppData(c.InEpoch(), expectedReply(msg)) + + // Act as if the ACK was dropped and retransmit Finished. + // The shim should process the retransmit from epoch 2 and + // ACK, although it has already received data at epoch 3. + c.WriteFlight(next) + ackTimeout := useTimeouts[0] / 4 + c.AdvanceClock(ackTimeout) + c.ReadACK(c.InEpoch()) + + // Partially retransmit Finished. The shim should continue + // to ACK. + c.WriteFragments([]DTLSFragment{next[0].Fragment(0, 1)}) + c.WriteFragments([]DTLSFragment{next[0].Fragment(1, 1)}) + c.AdvanceClock(ackTimeout) + c.ReadACK(c.InEpoch()) + + // Eventually, the shim assumes we have received the ACK + // and drops epoch 2. Retransmits now go unanswered. + c.AdvanceClock(dtlsPrevEpochExpiration) + c.WriteFlight(next) + }, + }, + }, + // Disable tickets on the shim to avoid NewSessionTicket + // interfering with the test callback. + flags: slices.Concat(flags, []string{"-no-ticket"}), + }) + // As a client, the shim must tolerate ACKs in response to its // initial ClientHello, but it will not process them because the // version is not yet known. The second ClientHello, in response @@ -22259,9 +22369,20 @@ func addKeyUpdateTests() { c.WriteAppData(c.OutEpoch(), msg) c.ReadAppData(c.InEpoch(), expectedReply(msg)) - // Having received something at the new epoch, the shim - // should discard the old epoch. The following writes should - // be ignored. + // The shim continues to accept application data at the old + // epoch, for a period of time. + c.WriteAppData(c.OutEpoch()-1, msg) + c.ReadAppData(c.InEpoch(), expectedReply(msg)) + + // It will even ACK the retransmission, though it knows the + // shim has seen the ACK. + c.WriteFlight(next) + c.AdvanceClock(ackTimeout) + c.ReadACK(c.InEpoch()) + + // After some time has passed, the shim will discard the old + // epoch. The following writes should be ignored. + c.AdvanceClock(dtlsPrevEpochExpiration) f := next[0].Fragment(0, len(next[0].Data)) f.ShouldDiscard = true c.WriteFragments([]DTLSFragment{f}) diff --git a/src/ssl/test/test_config.cc b/src/ssl/test/test_config.cc index 9471379d0..bf16b518c 100644 --- a/src/ssl/test/test_config.cc +++ b/src/ssl/test/test_config.cc @@ -2316,8 +2316,9 @@ bssl::UniquePtr TestConfig::NewSSL( !SSL_set_min_proto_version(ssl.get(), min_version)) { return nullptr; } + // TODO(crbug.com/42290594): Remove this once DTLS 1.3 is enabled by default. if (is_dtls && max_version == 0 && - !SSL_set_max_proto_version(ssl.get(), DTLS1_3_EXPERIMENTAL_VERSION)) { + !SSL_set_max_proto_version(ssl.get(), DTLS1_3_VERSION)) { return nullptr; } if (max_version != 0 &&