From 302b818d4b88f5708661070143454d40c7598572 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 22 Aug 2017 14:47:22 -0700 Subject: [PATCH] Only enable DTLS post-handshake rexmits if we sent the final Finished. I messed up https://boringssl-review.googlesource.com/8883 and caused both sides to believe they had sent the final Finished. Use next_message to detect whether our last flight had a reply. Change-Id: Ia4d8c8eefa818c9a69acc94d63c9c863293c3cf5 Reviewed-on: https://boringssl-review.googlesource.com/19604 Reviewed-by: Steven Valdez Commit-Queue: David Benjamin CQ-Verified: CQ bot account: commit-bot@chromium.org --- ssl/d1_both.cc | 6 ++++++ ssl/dtls_method.cc | 9 ++++++--- ssl/internal.h | 5 +++++ ssl/test/runner/common.go | 4 ++++ ssl/test/runner/dtls.go | 6 +++++- ssl/test/runner/runner.go | 32 ++++++++++++++++++++++++++++++++ 6 files changed, 58 insertions(+), 4 deletions(-) diff --git a/ssl/d1_both.cc b/ssl/d1_both.cc index 2538d28d..71a7161a 100644 --- a/ssl/d1_both.cc +++ b/ssl/d1_both.cc @@ -444,6 +444,11 @@ void dtls1_next_message(SSL *ssl) { ssl->d1->incoming_messages[index] = NULL; ssl->d1->handshake_read_seq++; ssl->s3->has_message = 0; + /* If we previously sent a flight, mark it as having a reply, so + * |on_handshake_complete| can manage post-handshake retransmission. */ + if (ssl->d1->outgoing_messages_complete) { + ssl->d1->flight_has_reply = true; + } } void dtls_clear_incoming_messages(SSL *ssl) { @@ -509,6 +514,7 @@ void dtls_clear_outgoing_messages(SSL *ssl) { ssl->d1->outgoing_written = 0; ssl->d1->outgoing_offset = 0; ssl->d1->outgoing_messages_complete = false; + ssl->d1->flight_has_reply = false; } int dtls1_init_message(SSL *ssl, CBB *cbb, CBB *body, uint8_t type) { diff --git a/ssl/dtls_method.cc b/ssl/dtls_method.cc index 91894276..1f95970e 100644 --- a/ssl/dtls_method.cc +++ b/ssl/dtls_method.cc @@ -73,10 +73,13 @@ static int dtls1_supports_cipher(const SSL_CIPHER *cipher) { } static void dtls1_on_handshake_complete(SSL *ssl) { - /* If we wrote the last flight, we'll have a timer left over without waiting - * for a read. Stop the timer but leave the flight around for post-handshake - * transmission logic. */ + /* Stop the reply timer left by the last flight we sent. */ dtls1_stop_timer(ssl); + /* If the final flight had a reply, we know the peer has received it. If not, + * we must leave the flight around for post-handshake retransmission. */ + if (ssl->d1->flight_has_reply) { + dtls_clear_outgoing_messages(ssl); + } } static int dtls1_set_read_state(SSL *ssl, UniquePtr aead_ctx) { diff --git a/ssl/internal.h b/ssl/internal.h index 40e4e4e5..b9c3998f 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -1831,6 +1831,11 @@ struct DTLS1_STATE { * |add_change_cipher_spec| will start a new flight. */ bool outgoing_messages_complete:1; + /* flight_has_reply is true if the current outgoing flight is complete and has + * processed at least one message. This is used to detect whether we or the + * peer sent the final flight. */ + bool flight_has_reply:1; + uint8_t cookie[DTLS1_COOKIE_LENGTH]; size_t cookie_len; diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index b402f382..bf56ca21 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go @@ -886,6 +886,10 @@ type ProtocolBugs struct { // and include a copy of the full one. MixCompleteMessageWithFragments bool + // RetransmitFinished, if true, causes the DTLS Finished message to be + // sent twice. + RetransmitFinished bool + // SendInvalidRecordType, if true, causes a record with an invalid // content type to be sent immediately following the handshake. SendInvalidRecordType bool diff --git a/ssl/test/runner/dtls.go b/ssl/test/runner/dtls.go index 42b3a655..c81955e3 100644 --- a/ssl/test/runner/dtls.go +++ b/ssl/test/runner/dtls.go @@ -248,7 +248,11 @@ func (c *Conn) dtlsWriteRecord(typ recordType, data []byte) (n int, err error) { fragOffset += fragLen n += fragLen } - if !isFinished && c.config.Bugs.MixCompleteMessageWithFragments { + shouldSendTwice := c.config.Bugs.MixCompleteMessageWithFragments + if isFinished { + shouldSendTwice = c.config.Bugs.RetransmitFinished + } + if shouldSendTwice { fragment := c.makeFragment(header, data, 0, len(data)) c.pendingFragments = append(c.pendingFragments, fragment) } diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 2ae697cb..a0f5a9c7 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -8782,6 +8782,38 @@ func addDTLSRetransmitTests() { "-initial-timeout-duration-ms", "250", }, }) + + // If the shim sends the last Finished (server full or client resume + // handshakes), it must retransmit that Finished when it sees a + // post-handshake penultimate Finished from the runner. The above tests + // cover this. Conversely, if the shim sends the penultimate Finished + // (client full or server resume), test that it does not retransmit. + testCases = append(testCases, testCase{ + protocol: dtls, + testType: clientTest, + name: "DTLS-StrayRetransmitFinished-ClientFull", + config: Config{ + MaxVersion: VersionTLS12, + Bugs: ProtocolBugs{ + RetransmitFinished: true, + }, + }, + }) + testCases = append(testCases, testCase{ + protocol: dtls, + testType: serverTest, + name: "DTLS-StrayRetransmitFinished-ServerResume", + config: Config{ + MaxVersion: VersionTLS12, + }, + resumeConfig: &Config{ + MaxVersion: VersionTLS12, + Bugs: ProtocolBugs{ + RetransmitFinished: true, + }, + }, + resumeSession: true, + }) } func addExportKeyingMaterialTests() {