Remove now redundant check for extra DTLS messages in epoch 1

This dates to
https://boringssl-review.googlesource.com/c/boringssl/+/8988, which was
several iterations back of the message read API. We now catch extra
Finished by way of:

In DTLS 1.2, there is no key change at the end of the handshake:

- If the fragment was received before the handshake was over, we queue
  it up and then the handshake makes sure incoming messages are clear
  before completing.

- If the fragment was received after the handshake was over, we reject
  almost all handshake fragments post-handshake. (This will need to be
  rejiggered.

In DTLS 1.3, there is a key change at the end of the handshake

- If the fragment was received before the handshake was over, key
  changes require incoming messages are clear and we reject it.

- If the fragment was received after the handshake was over, we
  currently do not notice. We do not retain the old epoch and just drop
  the record. (This is fine, since we won't actually process it.) But
  when we implement DTLS 1.3 key changes properly, we might end up
  picking it up.

Bug: 42290594
Change-Id: I52621f209321a0eb1ce59aa36db68922206d529f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72447
Reviewed-by: Nick Harper <nharper@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>

ssl/d1_both.cc

diff --git a/ssl/d1_both.cc b/ssl/d1_both.cc
index 7f077a3..c460e14 100644
--- a/ssl/d1_both.cc
+++ b/ssl/d1_both.cc
@@ -373,20 +373,6 @@
       return false;
     }
 
-    // The encrypted epoch in DTLS has only one handshake message.
-    //
-    // TODO(crbug.com/42290594): This check doesn't make any sense in DTLS 1.3,
-    // but is currently a no-op because epoch 1 is 0-RTT. Revisit this and
-    // figure out if we need to change anything. See
-    // https://boringssl-review.googlesource.com/c/boringssl/+/8988 for when
-    // this check was added.
-    if (ssl->d1->read_epoch.epoch == 1 &&
-        msg_hdr.seq != ssl->d1->handshake_read_seq) {
-      OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD);
-      *out_alert = SSL_AD_UNEXPECTED_MESSAGE;
-      return false;
-    }
-
     if (msg_hdr.seq < ssl->d1->handshake_read_seq ||
         msg_hdr.seq >
             (unsigned)ssl->d1->handshake_read_seq + SSL_MAX_HANDSHAKE_FLIGHT) {

ssl/test/runner/runner.go

diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 8a4dd55..4c6590f 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -3231,23 +3231,22 @@
 			},
 			resumeSession: true,
 		},
-		// TODO(crbug.com/42290594): This test and the next shouldn't be
-		// restricted to a max version of TLS 1.2, but they're broken in DTLS 1.3.
 		{
 			protocol: dtls,
-			name:     "DTLS-SendExtraFinished",
+			name:     "DTLS12-SendExtraFinished",
 			config: Config{
 				MaxVersion: VersionTLS12,
 				Bugs: ProtocolBugs{
 					SendExtraFinished: true,
 				},
 			},
-			shouldFail:    true,
-			expectedError: ":UNEXPECTED_RECORD:",
+			shouldFail:         true,
+			expectedError:      ":UNEXPECTED_RECORD:",
+			expectedLocalError: "remote error: unexpected message",
 		},
 		{
 			protocol: dtls,
-			name:     "DTLS-SendExtraFinished-Reordered",
+			name:     "DTLS12-SendExtraFinished-Reordered",
 			config: Config{
 				MaxVersion: VersionTLS12,
 				Bugs: ProtocolBugs{
@@ -3256,8 +3255,69 @@
 					SendExtraFinished:         true,
 				},
 			},
-			shouldFail:    true,
-			expectedError: ":UNEXPECTED_RECORD:",
+			shouldFail:         true,
+			expectedError:      ":EXCESS_HANDSHAKE_DATA:",
+			expectedLocalError: "remote error: unexpected message",
+		},
+		{
+			protocol: dtls,
+			name:     "DTLS12-SendExtraFinished-Packed",
+			config: Config{
+				MaxVersion: VersionTLS12,
+				Bugs: ProtocolBugs{
+					SendExtraFinished:      true,
+					PackHandshakeFragments: 1000,
+				},
+			},
+			shouldFail:         true,
+			expectedError:      ":EXCESS_HANDSHAKE_DATA:",
+			expectedLocalError: "remote error: unexpected message",
+		},
+		{
+			protocol: dtls,
+			name:     "DTLS13-SendExtraFinished",
+			config: Config{
+				MaxVersion: VersionTLS13,
+				Bugs: ProtocolBugs{
+					SendExtraFinished: true,
+				},
+			},
+			// TODO(crbug.com/42290594): When not reordered or packed, the extra
+			// Finished in epoch 2 does not arrive until after we've switched to
+			// epoch 3, so the record is simply dropped right now. When we defer
+			// epoch changes to the first record, this will change and we'll
+			// notice this, if no epoch 3 records arrive in the meantime. In
+			// general, a DTLS implementation may or may not notice invalid
+			// messages across key changes.
+		},
+		{
+			protocol: dtls,
+			name:     "DTLS13-SendExtraFinished-Reordered",
+			config: Config{
+				MaxVersion: VersionTLS13,
+				Bugs: ProtocolBugs{
+					MaxHandshakeRecordLength:  2,
+					ReorderHandshakeFragments: true,
+					SendExtraFinished:         true,
+				},
+			},
+			shouldFail:         true,
+			expectedError:      ":EXCESS_HANDSHAKE_DATA:",
+			expectedLocalError: "remote error: unexpected message",
+		},
+		{
+			protocol: dtls,
+			name:     "DTLS13-SendExtraFinished-Packed",
+			config: Config{
+				MaxVersion: VersionTLS13,
+				Bugs: ProtocolBugs{
+					SendExtraFinished:      true,
+					PackHandshakeFragments: 1000,
+				},
+			},
+			shouldFail:         true,
+			expectedError:      ":EXCESS_HANDSHAKE_DATA:",
+			expectedLocalError: "remote error: unexpected message",
 		},
 		{
 			testType: serverTest,