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 <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/ssl/d1_both.cc b/ssl/d1_both.cc
index 2538d28..71a7161 100644
--- a/ssl/d1_both.cc
+++ b/ssl/d1_both.cc
@@ -444,6 +444,11 @@
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 @@
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 9189427..1f95970 100644
--- a/ssl/dtls_method.cc
+++ b/ssl/dtls_method.cc
@@ -73,10 +73,13 @@
}
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<SSLAEADContext> aead_ctx) {
diff --git a/ssl/internal.h b/ssl/internal.h
index 40e4e4e..b9c3998 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1831,6 +1831,11 @@
* |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 b402f38..bf56ca2 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -886,6 +886,10 @@
// 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 42b3a65..c81955e 100644
--- a/ssl/test/runner/dtls.go
+++ b/ssl/test/runner/dtls.go
@@ -248,7 +248,11 @@
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 2ae697c..a0f5a9c 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -8782,6 +8782,38 @@
"-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() {