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>

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) {

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<SSLAEADContext> aead_ctx) {

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;
 

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

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)
 	}

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() {