From 80defe9243d57cf50636aa9a21f2ac18a9326b3f Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 5 Dec 2024 17:57:03 -0500 Subject: [PATCH 1/4] Correctly re-ACK client Finished in DTLS 1.3 If client Finished gets through, but the server's responding ACK is lost, the client will retransmit Finished (epoch 2). The server must then process that retransmission and send another ACK. However, the client may have since sent application data (epoch 3), which the server may have since processed. Despite receiving data at epoch 3, the server must keep epoch 2 open for some period of time to accomodate this. Unfortunately, there is no explicit signal in the protocol to do this. The best we have is some guidance to be willing to do this for at least 2x MSL. See this discussion in the TLS mailing list: https://mailarchive.ietf.org/arch/msg/tls/rof4SqkDrwU8o7WqSa9AO3YEUJI/ Implement this by keeping old epochs around for 2x MSL. This has the side effect of also keeping old epochs around for a spell on KeyUpdate, making us more resilient to packet reordering around KeyUpdate. For simplicity, I've just get the same timeout for both, 2x MSL, or 4 minutes. This opens a few cans of worms, because now record processing logic must accomodate old epochs: - The check against fragments from old epochs gets more complex. - The check for maximum message size must move later; after the handshake, the server has a tight maximum message size, but it must be willing to receive (and skip over) retransmits from before the handshake when the limit was higher. - Application data is fine. We already, at a lower layer, forbid any application data from coming in epochs 0 and 2. - The ACK logic already did not make any particular assumptions here. Now we'll process ACKs from older epochs for slightly longer, but we already check that old epochs cannot ACK new epochs' messsages. - ChangeCipherSpec is silly and would probably become moot once we start rejecting CCS in DTLS 1.3, but I just made it drop old epochs CCS on the floor to match the old behavior for now. (May as well avoid relying too much on DTLS 1.2 keeping one epoch around at once.) This also requires tweaking the test slightly. flushHandshake needs to run after the application data keys are installed, or the callback cannot send data. That, in turn, means OutEpoch() is no longer the right epoch for sending an ACK so some of the tests need to be updated to still pick up epoch 2. (What's going on is that sending ACKs happens before you construct the flight, but we want to test reordering, so we simulate it afterwards.) Bug: 42290594 Change-Id: I199b304fa9c3d8d252d258ab8ff231e3b5688e2e Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74027 Auto-Submit: David Benjamin Reviewed-by: Nick Harper Commit-Queue: David Benjamin --- ssl/d1_both.cc | 54 +++++----- ssl/dtls_record.cc | 42 ++++++-- ssl/internal.h | 28 ++++- ssl/test/runner/common.go | 5 + ssl/test/runner/handshake_client.go | 15 +-- ssl/test/runner/runner.go | 154 ++++++++++++++++++++++++---- 6 files changed, 236 insertions(+), 62 deletions(-) diff --git a/ssl/d1_both.cc b/ssl/d1_both.cc index db51c9a36..648bd66db 100644 --- a/ssl/d1_both.cc +++ b/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/ssl/dtls_record.cc b/ssl/dtls_record.cc index 5b124d9a0..1bfc43f6a 100644 --- a/ssl/dtls_record.cc +++ b/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/ssl/internal.h b/ssl/internal.h index b2eb5647e..eab3af728 100644 --- a/ssl/internal.h +++ b/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/ssl/test/runner/common.go b/ssl/test/runner/common.go index 76195f6f3..7dfba1416 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go @@ -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 diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go index 454333983..b6b95156e 100644 --- a/ssl/test/runner/handshake_client.go +++ b/ssl/test/runner/handshake_client.go @@ -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/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index fe9c1bdaf..ddd665a65 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -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", @@ -11548,6 +11587,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 +11942,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 +11955,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 +11993,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 +12005,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 +12033,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 +12045,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 +12073,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 +12092,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 +12118,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 +12238,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 +12349,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 +22366,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}) From bbf03ea966c726dee05769cb3c499d78c4eb2311 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 27 Nov 2024 19:33:26 -0500 Subject: [PATCH 2/4] Switch to the actual DTLS 1.3 codepoint I believe our implementation is interoperable at this point. Bug: 42290594 Change-Id: Id802b626a3028a3f2d4e89dfd3fcb69b51572b7d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73650 Commit-Queue: David Benjamin Reviewed-by: Nick Harper --- include/openssl/ssl.h | 12 +----------- ssl/ssl_test.cc | 25 ++++++++++--------------- ssl/ssl_versions.cc | 8 +++++--- ssl/test/runner/common.go | 10 +++++----- ssl/test/runner/handshake_client.go | 2 +- ssl/test/runner/runner.go | 2 +- ssl/test/test_config.cc | 3 ++- 7 files changed, 25 insertions(+), 37 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index e7c57c708..9e9acee23 100644 --- a/include/openssl/ssl.h +++ b/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/ssl/ssl_test.cc b/ssl/ssl_test.cc index 055015fd6..5fb78da46 100644 --- a/ssl/ssl_test.cc +++ b/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/ssl/ssl_versions.cc b/ssl/ssl_versions.cc index a34278dc1..20e17e13c 100644 --- a/ssl/ssl_versions.cc +++ b/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/ssl/test/runner/common.go b/ssl/test/runner/common.go index 7dfba1416..8a766729c 100644 --- a/ssl/test/runner/common.go +++ b/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, } @@ -2126,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/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go index b6b95156e..f9dfb9835 100644 --- a/ssl/test/runner/handshake_client.go +++ b/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) diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index ddd665a65..19e3740ed 100644 --- a/ssl/test/runner/runner.go +++ b/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, }, } diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index 9471379d0..bf16b518c 100644 --- a/ssl/test/test_config.cc +++ b/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 && From 4c647a5623c3da9bfa8f9cd7d4f60c992b0685c3 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 27 Nov 2024 18:16:51 -0500 Subject: [PATCH 3/4] Implement the downgrade protection signal in DTLS 1.3 This was originally looking for the client to specifically support the final TLS 1.3 version 0x0304. This has a side effect of not picking up DTLS 1.3, which has a different codepoint. We did it this way because, early in TLS 1.3's development, we had draft versions of TLS 1.3 flying around, and only the final TLS 1.3 can safely ship enforcing this check. Those draft versions are now gone, and this check is now getting in the way of DTLS 1.3. Switch it to checking hs->max_version. Bug: 42290594 Change-Id: Ic2d143af965b4b8bafef524f3f0e85cc3efa42fe Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73728 Commit-Queue: David Benjamin Reviewed-by: Nick Harper --- ssl/handshake_client.cc | 2 +- ssl/handshake_server.cc | 2 +- ssl/test/runner/runner.go | 93 ++++++++++++++++++++------------------- 3 files changed, 50 insertions(+), 47 deletions(-) diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc index b904e4f3c..afc99be95 100644 --- a/ssl/handshake_client.cc +++ b/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/ssl/handshake_server.cc b/ssl/handshake_server.cc index e41b23354..843e2badf 100644 --- a/ssl/handshake_server.cc +++ b/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/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 19e3740ed..41fa4541a 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -6891,8 +6891,9 @@ func addVersionNegotiationTests() { name: "MinorVersionTolerance-DTLS", config: Config{ Bugs: ProtocolBugs{ - SendClientVersion: 0xfe00, - OmitSupportedVersions: true, + SendClientVersion: 0xfe00, + OmitSupportedVersions: true, + IgnoreTLS13DowngradeRandom: true, }, }, expectations: connectionExpectations{ @@ -6905,8 +6906,9 @@ func addVersionNegotiationTests() { name: "MajorVersionTolerance-DTLS", config: Config{ Bugs: ProtocolBugs{ - SendClientVersion: 0xfdff, - OmitSupportedVersions: true, + SendClientVersion: 0xfdff, + OmitSupportedVersions: true, + IgnoreTLS13DowngradeRandom: true, }, }, expectations: connectionExpectations{ @@ -6953,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 From 9559c4566a6d12194c42db5f3dbbcb5de35cfec2 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 3 Dec 2024 15:31:14 -0500 Subject: [PATCH 4/4] Cover DTLS 1.3 in the TLS fuzzers Also restore 1.0 and 1.1 coverage. We need to explicitly configure the minimum and maximum supported, since the default may be narrower. Bug: 42290594 Change-Id: I690bd7b98401bfd48ea819053152d21120c5922f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73888 Commit-Queue: David Benjamin Reviewed-by: Nick Harper --- ssl/test/fuzzer.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/ssl/test/fuzzer.h b/ssl/test/fuzzer.h index ca7b55a77..01b689ae6 100644 --- a/ssl/test/fuzzer.h +++ b/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; }