Require handshake flights end at record boundaries.

The TLS handshake runs over a sequence of messages, serialized onto a
stream, which is then packetized into records, with no correlation to
message boundaries.

TLS messages may span records, so a TLS implementation will buffer up
excess data in a record for the next message. If not checked, that next
message may a round-trip or even a key change later. Carrying data
across a key change has security consequences, so we reject any excess
data across key changes (see ChangeCipherSpec synchronization tests and
(d)tls_set_read_state). However, we do not currently check it across
network round trips that do not change keys.

For instance, a TLS 1.2 client may pack part of ClientKeyExchange (the
first byte, at least, is deterministic) into the ClientHello record,
before even receiving ServerHello. Most TLS implementations will accept
this.

However, the handback logic does *not* serialize excess data in hs_buf.
There shouldn't be any, but if the peer is doing strange things as
above, that data will get silently dropped. The way TLS 1.3 0-RTT
handback logic works (the key isn't installed until after handback),
this data is even silently dropped though there is a key change.

To keep all our behavior consistent, check for unprocessed handshake
data at the end of each flight and reject it. Add a bunch of tests.

Update-Note: If the peer packs data across handshake flights, or packs
HelloRequest into the same record as Finished, this will now be an
error. (The former is pathologically odd behavior. The latter is also
rejected by Schannel and also odd.)

Change-Id: I9412bbdea09ee7fdcfeb78d3456329505a190641
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/39987
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata
index 132c9e0..b2a6146 100644
--- a/crypto/err/ssl.errordata
+++ b/crypto/err/ssl.errordata
@@ -23,7 +23,6 @@
 SSL,119,BIO_NOT_SET
 SSL,261,BLOCK_CIPHER_PAD_IS_WRONG
 SSL,120,BN_LIB
-SSL,255,BUFFERED_MESSAGES_ON_CIPHER_CHANGE
 SSL,121,BUFFER_TOO_SMALL
 SSL,275,CANNOT_HAVE_BOTH_PRIVKEY_AND_METHOD
 SSL,272,CANNOT_PARSE_LEAF_CERT
@@ -64,6 +63,7 @@
 SSL,148,ERROR_IN_RECEIVED_CIPHER_LIST
 SSL,149,ERROR_PARSING_EXTENSION
 SSL,150,EXCESSIVE_MESSAGE_SIZE
+SSL,255,EXCESS_HANDSHAKE_DATA
 SSL,151,EXTRA_DATA_IN_MESSAGE
 SSL,152,FRAGMENT_MISMATCH
 SSL,153,GOT_NEXT_PROTO_WITHOUT_EXTENSION
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 6713d52..9847be1 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -5021,7 +5021,7 @@
 #define SSL_R_UNSUPPORTED_PROTOCOL_FOR_CUSTOM_KEY 252
 #define SSL_R_NO_COMMON_SIGNATURE_ALGORITHMS 253
 #define SSL_R_DOWNGRADE_DETECTED 254
-#define SSL_R_BUFFERED_MESSAGES_ON_CIPHER_CHANGE 255
+#define SSL_R_EXCESS_HANDSHAKE_DATA 255
 #define SSL_R_INVALID_COMPRESSION_LIST 256
 #define SSL_R_DUPLICATE_EXTENSION 257
 #define SSL_R_MISSING_KEY_SHARE 258
diff --git a/ssl/dtls_method.cc b/ssl/dtls_method.cc
index 4b369b6..620a2e1 100644
--- a/ssl/dtls_method.cc
+++ b/ssl/dtls_method.cc
@@ -80,7 +80,7 @@
 static bool dtls1_set_read_state(SSL *ssl, UniquePtr<SSLAEADContext> aead_ctx) {
   // Cipher changes are forbidden if the current epoch has leftover data.
   if (dtls_has_unprocessed_handshake_data(ssl)) {
-    OPENSSL_PUT_ERROR(SSL, SSL_R_BUFFERED_MESSAGES_ON_CIPHER_CHANGE);
+    OPENSSL_PUT_ERROR(SSL, SSL_R_EXCESS_HANDSHAKE_DATA);
     ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
     return false;
   }
@@ -112,6 +112,7 @@
     dtls1_free,
     dtls1_get_message,
     dtls1_next_message,
+    dtls_has_unprocessed_handshake_data,
     dtls1_open_handshake,
     dtls1_open_change_cipher_spec,
     dtls1_open_app_data,
diff --git a/ssl/handshake.cc b/ssl/handshake.cc
index f6460d3..4981fc4 100644
--- a/ssl/handshake.cc
+++ b/ssl/handshake.cc
@@ -471,6 +471,13 @@
     ssl->s3->previous_server_finished_len = finished_len;
   }
 
+  // The Finished message should be the end of a flight.
+  if (ssl->method->has_unprocessed_handshake_data(ssl)) {
+    ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
+    OPENSSL_PUT_ERROR(SSL, SSL_R_EXCESS_HANDSHAKE_DATA);
+    return ssl_hs_error;
+  }
+
   ssl->method->next_message(ssl);
   return ssl_hs_ok;
 }
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index 25f96d0..a970a3c 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -1201,6 +1201,13 @@
     return ssl_hs_error;
   }
 
+  // ServerHelloDone should be the end of the flight.
+  if (ssl->method->has_unprocessed_handshake_data(ssl)) {
+    ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
+    OPENSSL_PUT_ERROR(SSL, SSL_R_EXCESS_HANDSHAKE_DATA);
+    return ssl_hs_error;
+  }
+
   ssl->method->next_message(ssl);
   hs->state = state_send_client_certificate;
   return ssl_hs_ok;
diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc
index 386ed6a..924701f 100644
--- a/ssl/handshake_server.cc
+++ b/ssl/handshake_server.cc
@@ -569,6 +569,14 @@
     return ssl_hs_error;
   }
 
+  // ClientHello should be the end of the flight. We check this early to cover
+  // all protocol versions.
+  if (ssl->method->has_unprocessed_handshake_data(ssl)) {
+    ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
+    OPENSSL_PUT_ERROR(SSL, SSL_R_EXCESS_HANDSHAKE_DATA);
+    return ssl_hs_error;
+  }
+
   if (hs->config->handoff) {
     return ssl_hs_handoff;
   }
diff --git a/ssl/internal.h b/ssl/internal.h
index 257f7ec..32303cb 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -2110,6 +2110,9 @@
   bool (*get_message)(const SSL *ssl, SSLMessage *out);
   // next_message is called to release the current handshake message.
   void (*next_message)(SSL *ssl);
+  // has_unprocessed_handshake_data returns whether there is buffered
+  // handshake data that has not been consumed by |get_message|.
+  bool (*has_unprocessed_handshake_data)(const SSL *ssl);
   // Use the |ssl_open_handshake| wrapper.
   ssl_open_record_t (*open_handshake)(SSL *ssl, size_t *out_consumed,
                                       uint8_t *out_alert, Span<uint8_t> in);
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 0105a8b..62d4b09 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -5536,7 +5536,7 @@
   ASSERT_EQ(SSL_get_error(client_.get(), -1), SSL_ERROR_SSL);
   uint32_t err = ERR_get_error();
   EXPECT_EQ(ERR_GET_LIB(err), ERR_LIB_SSL);
-  EXPECT_EQ(ERR_GET_REASON(err), SSL_R_BUFFERED_MESSAGES_ON_CIPHER_CHANGE);
+  EXPECT_EQ(ERR_GET_REASON(err), SSL_R_EXCESS_HANDSHAKE_DATA);
 
   // The client sends an alert in response to this.
   ASSERT_TRUE(transport_->client()->has_alert());
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index 26fc440..26f4885 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -657,11 +657,49 @@
 	// in the same record as ServerHello.
 	PartialEncryptedExtensionsWithServerHello bool
 
-	// PartialClientFinishedWithClientHello, if true, causes the TLS 1.3
-	// client to send part of Finished unencrypted in the same record as
-	// ClientHello.
+	// PartialClientFinishedWithClientHello, if true, causes the TLS 1.2
+	// or TLS 1.3 client to send part of Finished unencrypted in the same
+	// record as ClientHello.
 	PartialClientFinishedWithClientHello bool
 
+	// PartialClientFinishedWithSecondClientHello, if true, causes the
+	// TLS 1.3 client to send part of Finished unencrypted in the same
+	// record as the second ClientHello.
+	PartialClientFinishedWithSecondClientHello bool
+
+	// PartialEndOfEarlyDataWithClientHello, if true, causes the TLS 1.3
+	// client to send part of EndOfEarlyData unencrypted in the same record
+	// as ClientHello.
+	PartialEndOfEarlyDataWithClientHello bool
+
+	// PartialSecondClientHelloAfterFirst, if true, causes the TLS 1.3 client
+	// to send part of the second ClientHello in the same record as the first
+	// one.
+	PartialSecondClientHelloAfterFirst bool
+
+	// PartialClientKeyExchangeWithClientHello, if true, causes the TLS 1.2
+	// client to send part of the ClientKeyExchange in the same record as
+	// the ClientHello.
+	PartialClientKeyExchangeWithClientHello bool
+
+	// PartialNewSessionTicketWithServerHelloDone, if true, causes the TLS 1.2
+	// server to send part of the NewSessionTicket in the same record as
+	// ServerHelloDone.
+	PartialNewSessionTicketWithServerHelloDone bool
+
+	// PartialNewSessionTicketWithServerHelloDone, if true, causes the TLS 1.2
+	// server to send part of the Finshed in the same record as ServerHelloDone.
+	PartialFinishedWithServerHelloDone bool
+
+	// PartialServerHelloWithHelloRetryRequest, if true, causes the TLS 1.3
+	// server to send part of the ServerHello in the same record as
+	// HelloRetryRequest.
+	PartialServerHelloWithHelloRetryRequest bool
+
+	// TrailingDataWithFinished, if true, causes the record containing the
+	// Finished message to include an extra byte of data at the end.
+	TrailingDataWithFinished bool
+
 	// SendV2ClientHello causes the client to send a V2ClientHello
 	// instead of a normal ClientHello.
 	SendV2ClientHello bool
diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go
index ad551c5..e24cf5a 100644
--- a/ssl/test/runner/conn.go
+++ b/ssl/test/runner/conn.go
@@ -1126,18 +1126,14 @@
 			msgType = typeHelloRetryRequest
 		}
 		if msgType != data[0] {
-			newData := make([]byte, len(data))
-			copy(newData, data)
-			newData[0] = msgType
-			data = newData
+			data = append([]byte{msgType}, data[1:]...)
 		}
 
 		if c.config.Bugs.SendTrailingMessageData != 0 && msgType == c.config.Bugs.SendTrailingMessageData {
-			newData := make([]byte, len(data))
+			// Add a 0 to the body.
+			newData := make([]byte, len(data)+1)
 			copy(newData, data)
 
-			// Add a 0 to the body.
-			newData = append(newData, 0)
 			// Fix the header.
 			newLen := len(newData) - 4
 			newData[1] = byte(newLen >> 16)
@@ -1146,6 +1142,12 @@
 
 			data = newData
 		}
+
+		if c.config.Bugs.TrailingDataWithFinished && msgType == typeFinished {
+			// Add a 0 to the record. Note unused bytes in |data| may be owned by the
+			// caller, so we force a new allocation.
+			data = append(data[:len(data):len(data)], 0)
+		}
 	}
 
 	if c.isDTLS {
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go
index df69505..27e9a95 100644
--- a/ssl/test/runner/handshake_client.go
+++ b/ssl/test/runner/handshake_client.go
@@ -441,15 +441,18 @@
 			helloBytes = hello.marshal()
 		}
 
+		var appendToHello byte
 		if c.config.Bugs.PartialClientFinishedWithClientHello {
-			// Include one byte of Finished. We can compute it
-			// without completing the handshake. This assumes we
-			// negotiate TLS 1.3 with no HelloRetryRequest or
-			// CertificateRequest.
-			toWrite := make([]byte, 0, len(helloBytes)+1)
-			toWrite = append(toWrite, helloBytes...)
-			toWrite = append(toWrite, typeFinished)
-			c.writeRecord(recordTypeHandshake, toWrite)
+			appendToHello = typeFinished
+		} else if c.config.Bugs.PartialEndOfEarlyDataWithClientHello {
+			appendToHello = typeEndOfEarlyData
+		} else if c.config.Bugs.PartialSecondClientHelloAfterFirst {
+			appendToHello = typeClientHello
+		} else if c.config.Bugs.PartialClientKeyExchangeWithClientHello {
+			appendToHello = typeClientKeyExchange
+		}
+		if appendToHello != 0 {
+			c.writeRecord(recordTypeHandshake, append(helloBytes[:len(helloBytes):len(helloBytes)], appendToHello))
 		} else {
 			c.writeRecord(recordTypeHandshake, helloBytes)
 		}
@@ -613,14 +616,25 @@
 			generatePSKBinders(c.wireVersion, hello, pskCipherSuite, session.masterSecret, helloBytes, helloRetryRequest.marshal(), c.config)
 		}
 		secondHelloBytes = hello.marshal()
+		secondHelloBytesToWrite := secondHelloBytes
+
+		if c.config.Bugs.PartialSecondClientHelloAfterFirst {
+			// The first byte has already been sent.
+			secondHelloBytesToWrite = secondHelloBytesToWrite[1:]
+		}
 
 		if c.config.Bugs.InterleaveEarlyData {
 			c.sendFakeEarlyData(4)
-			c.writeRecord(recordTypeHandshake, secondHelloBytes[:16])
+			c.writeRecord(recordTypeHandshake, secondHelloBytesToWrite[:16])
 			c.sendFakeEarlyData(4)
-			c.writeRecord(recordTypeHandshake, secondHelloBytes[16:])
+			c.writeRecord(recordTypeHandshake, secondHelloBytesToWrite[16:])
+		} else if c.config.Bugs.PartialClientFinishedWithSecondClientHello {
+			toWrite := make([]byte, len(secondHelloBytesToWrite)+1)
+			copy(toWrite, secondHelloBytesToWrite)
+			toWrite[len(secondHelloBytesToWrite)] = typeFinished
+			c.writeRecord(recordTypeHandshake, toWrite)
 		} else {
-			c.writeRecord(recordTypeHandshake, secondHelloBytes)
+			c.writeRecord(recordTypeHandshake, secondHelloBytesToWrite)
 		}
 		c.flushHandshake()
 
@@ -1077,7 +1091,12 @@
 		}
 		endOfEarlyData := new(endOfEarlyDataMsg)
 		endOfEarlyData.nonEmpty = c.config.Bugs.NonEmptyEndOfEarlyData
-		c.writeRecord(recordTypeHandshake, endOfEarlyData.marshal())
+		if c.config.Bugs.PartialEndOfEarlyDataWithClientHello {
+			// The first byte has already been sent.
+			c.writeRecord(recordTypeHandshake, endOfEarlyData.marshal()[1:])
+		} else {
+			c.writeRecord(recordTypeHandshake, endOfEarlyData.marshal())
+		}
 		hs.writeClientHash(endOfEarlyData.marshal())
 	}
 
@@ -1312,7 +1331,12 @@
 		if c.config.Bugs.EarlyChangeCipherSpec < 2 {
 			hs.writeClientHash(ckx.marshal())
 		}
-		c.writeRecord(recordTypeHandshake, ckx.marshal())
+		if c.config.Bugs.PartialClientKeyExchangeWithClientHello {
+			// The first byte was already written.
+			c.writeRecord(recordTypeHandshake, ckx.marshal()[1:])
+		} else {
+			c.writeRecord(recordTypeHandshake, ckx.marshal())
+		}
 	}
 
 	if hs.serverHello.extensions.extendedMasterSecret && c.vers >= VersionTLS10 {
@@ -1843,7 +1867,12 @@
 	c.clientVerify = append(c.clientVerify[:0], finished.verifyData...)
 	hs.finishedBytes = finished.marshal()
 	hs.writeHash(hs.finishedBytes, seqno)
-	postCCSMsgs = append(postCCSMsgs, hs.finishedBytes)
+	if c.config.Bugs.PartialClientFinishedWithClientHello {
+		// The first byte has already been written.
+		postCCSMsgs = append(postCCSMsgs, hs.finishedBytes[1:])
+	} else {
+		postCCSMsgs = append(postCCSMsgs, hs.finishedBytes)
+	}
 
 	if c.config.Bugs.FragmentAcrossChangeCipherSpec {
 		c.writeRecord(recordTypeHandshake, postCCSMsgs[0][:5])
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go
index 48dd239..dc4b744 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -608,7 +608,12 @@
 
 		oldClientHelloBytes := hs.clientHello.marshal()
 		hs.writeServerHash(helloRetryRequest.marshal())
-		c.writeRecord(recordTypeHandshake, helloRetryRequest.marshal())
+		if c.config.Bugs.PartialServerHelloWithHelloRetryRequest {
+			data := helloRetryRequest.marshal()
+			c.writeRecord(recordTypeHandshake, append(data[:len(data):len(data)], typeServerHello))
+		} else {
+			c.writeRecord(recordTypeHandshake, helloRetryRequest.marshal())
+		}
 		c.flushHandshake()
 
 		if !c.config.Bugs.SkipChangeCipherSpec {
@@ -797,15 +802,16 @@
 	}
 
 	// Send unencrypted ServerHello.
-	hs.writeServerHash(hs.hello.marshal())
+	helloBytes := hs.hello.marshal()
+	hs.writeServerHash(helloBytes)
+	if config.Bugs.PartialServerHelloWithHelloRetryRequest {
+		// The first byte has already been written.
+		helloBytes = helloBytes[1:]
+	}
 	if config.Bugs.PartialEncryptedExtensionsWithServerHello {
-		helloBytes := hs.hello.marshal()
-		toWrite := make([]byte, 0, len(helloBytes)+1)
-		toWrite = append(toWrite, helloBytes...)
-		toWrite = append(toWrite, typeEncryptedExtensions)
-		c.writeRecord(recordTypeHandshake, toWrite)
+		c.writeRecord(recordTypeHandshake, append(helloBytes[:len(helloBytes):len(helloBytes)], typeEncryptedExtensions))
 	} else {
-		c.writeRecord(recordTypeHandshake, hs.hello.marshal())
+		c.writeRecord(recordTypeHandshake, helloBytes)
 	}
 	c.flushHandshake()
 
@@ -1677,8 +1683,19 @@
 	}
 
 	helloDone := new(serverHelloDoneMsg)
-	hs.writeServerHash(helloDone.marshal())
-	c.writeRecord(recordTypeHandshake, helloDone.marshal())
+	helloDoneBytes := helloDone.marshal()
+	hs.writeServerHash(helloDoneBytes)
+	var toAppend byte
+	if config.Bugs.PartialNewSessionTicketWithServerHelloDone {
+		toAppend = typeNewSessionTicket
+	} else if config.Bugs.PartialFinishedWithServerHelloDone {
+		toAppend = typeFinished
+	}
+	if toAppend != 0 {
+		c.writeRecord(recordTypeHandshake, append(helloDoneBytes[:len(helloDoneBytes):len(helloDoneBytes)], toAppend))
+	} else {
+		c.writeRecord(recordTypeHandshake, helloDoneBytes)
+	}
 	c.flushHandshake()
 
 	var pub crypto.PublicKey // public key for client auth, if any
@@ -1937,7 +1954,12 @@
 	}
 
 	hs.writeServerHash(m.marshal())
-	c.writeRecord(recordTypeHandshake, m.marshal())
+	if c.config.Bugs.PartialNewSessionTicketWithServerHelloDone {
+		// The first byte was already sent.
+		c.writeRecord(recordTypeHandshake, m.marshal()[1:])
+	} else {
+		c.writeRecord(recordTypeHandshake, m.marshal())
+	}
 
 	return nil
 }
@@ -1955,6 +1977,10 @@
 	hs.finishedBytes = finished.marshal()
 	hs.writeServerHash(hs.finishedBytes)
 	postCCSBytes := hs.finishedBytes
+	if c.config.Bugs.PartialFinishedWithServerHelloDone {
+		// The first byte has already been sent.
+		postCCSBytes = postCCSBytes[1:]
+	}
 
 	if c.config.Bugs.FragmentAcrossChangeCipherSpec {
 		c.writeRecord(recordTypeHandshake, postCCSBytes[:5])
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 86aa61e..4ec8bd8 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -5359,6 +5359,12 @@
 			},
 		})
 
+		halfHelloRequestError := ":UNEXPECTED_RECORD:"
+		if config.packHandshake {
+			// If the HelloRequest is sent in the same record as the server Finished,
+			// BoringSSL rejects it before the handshake completes.
+			halfHelloRequestError = ":EXCESS_HANDSHAKE_DATA:"
+		}
 		tests = append(tests, testCase{
 			name: "SendHalfHelloRequest",
 			config: Config{
@@ -5370,7 +5376,7 @@
 			sendHalfHelloRequest: true,
 			flags:                []string{"-renegotiate-ignore"},
 			shouldFail:           true,
-			expectedError:        ":UNEXPECTED_RECORD:",
+			expectedError:        halfHelloRequestError,
 		})
 
 		// NPN on client and server; results in post-handshake message.
@@ -8658,8 +8664,8 @@
 		expectedError: ":UNEXPECTED_MESSAGE:",
 	})
 
-	// Test renegotiation works if HelloRequest and server Finished come in
-	// the same record.
+	// Test that HelloRequest is rejected if it comes in the same record as the
+	// server Finished.
 	testCases = append(testCases, testCase{
 		name: "Renegotiate-Client-Packed",
 		config: Config{
@@ -8669,11 +8675,11 @@
 				PackHelloRequestWithFinished: true,
 			},
 		},
-		renegotiate: 1,
-		flags: []string{
-			"-renegotiate-freely",
-			"-expect-total-renegotiations", "1",
-		},
+		renegotiate:        1,
+		flags:              []string{"-renegotiate-freely"},
+		shouldFail:         true,
+		expectedError:      ":EXCESS_HANDSHAKE_DATA:",
+		expectedLocalError: "remote error: unexpected message",
 	})
 
 	// Renegotiation is forbidden in TLS 1.3.
@@ -11504,6 +11510,47 @@
 		})
 	}
 
+	// In TLS 1.2 resumptions, the client sends ClientHello in the first flight
+	// and ChangeCipherSpec + Finished in the second flight. Test the server's
+	// behavior when the Finished message is fragmented across not only
+	// ChangeCipherSpec but also the flight boundary.
+	testCases = append(testCases, testCase{
+		testType: serverTest,
+		name:     "PartialClientFinishedWithClientHello-TLS12-Resume",
+		config: Config{
+			MaxVersion: VersionTLS12,
+		},
+		resumeConfig: &Config{
+			MaxVersion: VersionTLS12,
+			Bugs: ProtocolBugs{
+				PartialClientFinishedWithClientHello: true,
+			},
+		},
+		resumeSession:      true,
+		shouldFail:         true,
+		expectedError:      ":EXCESS_HANDSHAKE_DATA:",
+		expectedLocalError: "remote error: unexpected message",
+	})
+
+	// In TLS 1.2 full handshakes without tickets, the server's first flight ends
+	// with ServerHelloDone and the second flight is ChangeCipherSpec + Finished.
+	// Test the client's behavior when the Finished message is fragmented across
+	// not only ChangeCipherSpec but also the flight boundary.
+	testCases = append(testCases, testCase{
+		testType: clientTest,
+		name:     "PartialFinishedWithServerHelloDone",
+		config: Config{
+			MaxVersion:             VersionTLS12,
+			SessionTicketsDisabled: true,
+			Bugs: ProtocolBugs{
+				PartialFinishedWithServerHelloDone: true,
+			},
+		},
+		shouldFail:         true,
+		expectedError:      ":EXCESS_HANDSHAKE_DATA:",
+		expectedLocalError: "remote error: unexpected message",
+	})
+
 	// Test that, in DTLS, ChangeCipherSpec is not allowed when there are
 	// messages in the handshake queue. Do this by testing the server
 	// reading the client Finished, reversing the flight so Finished comes
@@ -11520,7 +11567,7 @@
 			},
 		},
 		shouldFail:    true,
-		expectedError: ":BUFFERED_MESSAGES_ON_CIPHER_CHANGE:",
+		expectedError: ":EXCESS_HANDSHAKE_DATA:",
 	})
 
 	// Test synchronization between encryption changes and the handshake in
@@ -11534,7 +11581,7 @@
 			},
 		},
 		shouldFail:    true,
-		expectedError: ":BUFFERED_MESSAGES_ON_CIPHER_CHANGE:",
+		expectedError: ":EXCESS_HANDSHAKE_DATA:",
 	})
 	testCases = append(testCases, testCase{
 		testType: serverTest,
@@ -11546,7 +11593,39 @@
 			},
 		},
 		shouldFail:    true,
-		expectedError: ":BUFFERED_MESSAGES_ON_CIPHER_CHANGE:",
+		expectedError: ":EXCESS_HANDSHAKE_DATA:",
+	})
+	testCases = append(testCases, testCase{
+		testType: serverTest,
+		name:     "PartialClientFinishedWithSecondClientHello",
+		config: Config{
+			MaxVersion: VersionTLS13,
+			// Trigger a curve-based HelloRetryRequest.
+			DefaultCurves: []CurveID{},
+			Bugs: ProtocolBugs{
+				PartialClientFinishedWithSecondClientHello: true,
+			},
+		},
+		shouldFail:    true,
+		expectedError: ":EXCESS_HANDSHAKE_DATA:",
+	})
+	testCases = append(testCases, testCase{
+		testType: serverTest,
+		name:     "PartialEndOfEarlyDataWithClientHello",
+		config: Config{
+			MaxVersion: VersionTLS13,
+		},
+		resumeConfig: &Config{
+			Bugs: ProtocolBugs{
+				PartialEndOfEarlyDataWithClientHello: true,
+				SendEarlyData:                        [][]byte{{1, 2, 3, 4}},
+				ExpectEarlyDataAccepted:              true,
+			},
+		},
+		resumeSession: true,
+		flags:         []string{"-enable-early-data"},
+		shouldFail:    true,
+		expectedError: ":EXCESS_HANDSHAKE_DATA:",
 	})
 
 	// Test that early ChangeCipherSpecs are handled correctly.
@@ -11662,6 +11741,129 @@
 	})
 }
 
+// addEndOfFlightTests adds tests where the runner adds extra data in the final
+// record of each handshake flight. Depending on the implementation strategy,
+// this data may be carried over to the next flight (assuming no key change) or
+// may be rejected. To avoid differences with split handshakes and generally
+// reject misbehavior, BoringSSL treats this as an error. When possible, these
+// tests pull the extra data from the subsequent flight to distinguish the data
+// being carried over from a general syntax error.
+//
+// These tests are similar to tests in |addChangeCipherSpecTests| that send
+// extra data at key changes. Not all key changes are at the end of a flight and
+// not all flights end at a key change.
+func addEndOfFlightTests() {
+	// TLS 1.3 client handshakes.
+	//
+	// Data following the second TLS 1.3 ClientHello is covered by
+	// PartialClientFinishedWithClientHello,
+	// PartialClientFinishedWithSecondClientHello, and
+	// PartialEndOfEarlyDataWithClientHello in |addChangeCipherSpecTests|.
+	testCases = append(testCases, testCase{
+		testType: serverTest,
+		name:     "PartialSecondClientHelloAfterFirst",
+		config: Config{
+			MaxVersion: VersionTLS13,
+			// Trigger a curve-based HelloRetryRequest.
+			DefaultCurves: []CurveID{},
+			Bugs: ProtocolBugs{
+				PartialSecondClientHelloAfterFirst: true,
+			},
+		},
+		shouldFail:         true,
+		expectedError:      ":EXCESS_HANDSHAKE_DATA:",
+		expectedLocalError: "remote error: unexpected message",
+	})
+
+	// TLS 1.3 server handshakes.
+	testCases = append(testCases, testCase{
+		testType: clientTest,
+		name:     "PartialServerHelloWithHelloRetryRequest",
+		config: Config{
+			MaxVersion: VersionTLS13,
+			// P-384 requires HelloRetryRequest in BoringSSL.
+			CurvePreferences: []CurveID{CurveP384},
+			Bugs: ProtocolBugs{
+				PartialServerHelloWithHelloRetryRequest: true,
+			},
+		},
+		shouldFail:         true,
+		expectedError:      ":EXCESS_HANDSHAKE_DATA:",
+		expectedLocalError: "remote error: unexpected message",
+	})
+
+	// TLS 1.2 client handshakes.
+	testCases = append(testCases, testCase{
+		testType: serverTest,
+		name:     "PartialClientKeyExchangeWithClientHello",
+		config: Config{
+			MaxVersion: VersionTLS12,
+			Bugs: ProtocolBugs{
+				PartialClientKeyExchangeWithClientHello: true,
+			},
+		},
+		shouldFail:         true,
+		expectedError:      ":EXCESS_HANDSHAKE_DATA:",
+		expectedLocalError: "remote error: unexpected message",
+	})
+
+	// TLS 1.2 server handshakes.
+	testCases = append(testCases, testCase{
+		testType: clientTest,
+		name:     "PartialNewSessionTicketWithServerHelloDone",
+		config: Config{
+			MaxVersion: VersionTLS12,
+			Bugs: ProtocolBugs{
+				PartialNewSessionTicketWithServerHelloDone: true,
+			},
+		},
+		shouldFail:         true,
+		expectedError:      ":EXCESS_HANDSHAKE_DATA:",
+		expectedLocalError: "remote error: unexpected message",
+	})
+
+	for _, vers := range tlsVersions {
+		for _, testType := range []testType{clientTest, serverTest} {
+			suffix := "-Client"
+			if testType == serverTest {
+				suffix = "-Server"
+			}
+			suffix += "-" + vers.name
+
+			testCases = append(testCases, testCase{
+				testType: testType,
+				name:     "TrailingDataWithFinished" + suffix,
+				config: Config{
+					MaxVersion: vers.version,
+					Bugs: ProtocolBugs{
+						TrailingDataWithFinished: true,
+					},
+				},
+				shouldFail:         true,
+				expectedError:      ":EXCESS_HANDSHAKE_DATA:",
+				expectedLocalError: "remote error: unexpected message",
+			})
+			testCases = append(testCases, testCase{
+				testType: testType,
+				name:     "TrailingDataWithFinished-Resume" + suffix,
+				config: Config{
+					MaxVersion: vers.version,
+				},
+				resumeConfig: &Config{
+					MaxVersion: vers.version,
+					Bugs: ProtocolBugs{
+						TrailingDataWithFinished: true,
+					},
+				},
+				resumeSession:      true,
+				shouldFail:         true,
+				expectedError:      ":EXCESS_HANDSHAKE_DATA:",
+				expectedLocalError: "remote error: unexpected message",
+			})
+		}
+	}
+}
+
 type perMessageTest struct {
 	messageType uint8
 	test        testCase
@@ -15290,6 +15492,7 @@
 	addTLS13RecordTests()
 	addAllStateMachineCoverageTests()
 	addChangeCipherSpecTests()
+	addEndOfFlightTests()
 	addWrongMessageTypeTests()
 	addTrailingMessageDataTests()
 	addTLS13HandshakeTests()
diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc
index 8bb3339..f48d1da 100644
--- a/ssl/tls13_client.cc
+++ b/ssl/tls13_client.cc
@@ -183,6 +183,13 @@
     return ssl_hs_error;
   }
 
+  // HelloRetryRequest should be the end of the flight.
+  if (ssl->method->has_unprocessed_handshake_data(ssl)) {
+    ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
+    OPENSSL_PUT_ERROR(SSL, SSL_R_EXCESS_HANDSHAKE_DATA);
+    return ssl_hs_error;
+  }
+
   ssl->method->next_message(ssl);
   ssl->s3->used_hello_retry_request = true;
   hs->tls13_state = state_send_second_client_hello;
@@ -622,6 +629,13 @@
     return ssl_hs_error;
   }
 
+  // Finished should be the end of the flight.
+  if (ssl->method->has_unprocessed_handshake_data(ssl)) {
+    ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
+    OPENSSL_PUT_ERROR(SSL, SSL_R_EXCESS_HANDSHAKE_DATA);
+    return ssl_hs_error;
+  }
+
   ssl->method->next_message(ssl);
   hs->tls13_state = state_send_end_of_early_data;
   return ssl_hs_ok;
diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc
index 39d44bc..baf2a04 100644
--- a/ssl/tls13_server.cc
+++ b/ssl/tls13_server.cc
@@ -558,6 +558,13 @@
     return ssl_hs_error;
   }
 
+  // ClientHello should be the end of the flight.
+  if (ssl->method->has_unprocessed_handshake_data(ssl)) {
+    ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
+    OPENSSL_PUT_ERROR(SSL, SSL_R_EXCESS_HANDSHAKE_DATA);
+    return ssl_hs_error;
+  }
+
   ssl->method->next_message(ssl);
   hs->tls13_state = state13_send_server_hello;
   return ssl_hs_ok;
diff --git a/ssl/tls_method.cc b/ssl/tls_method.cc
index 1ca8bc5..241a3fd 100644
--- a/ssl/tls_method.cc
+++ b/ssl/tls_method.cc
@@ -72,10 +72,11 @@
   assert(!ssl->s3->has_message);
 
   // During the handshake, |hs_buf| is retained. Release if it there is no
-  // excess in it. There may be excess left if there server sent Finished and
-  // HelloRequest in the same record.
-  //
-  // TODO(davidben): SChannel does not support this. Reject this case.
+  // excess in it. There should not be any excess because the handshake logic
+  // rejects unprocessed data after each Finished message. Note this means we do
+  // not allow a TLS 1.2 HelloRequest to be packed into the same record as
+  // Finished. (Schannel also rejects this.)
+  assert(!ssl->s3->hs_buf || ssl->s3->hs_buf->length == 0);
   if (ssl->s3->hs_buf && ssl->s3->hs_buf->length == 0) {
     ssl->s3->hs_buf.reset();
   }
@@ -84,7 +85,7 @@
 static bool tls_set_read_state(SSL *ssl, UniquePtr<SSLAEADContext> aead_ctx) {
   // Cipher changes are forbidden if the current epoch has leftover data.
   if (tls_has_unprocessed_handshake_data(ssl)) {
-    OPENSSL_PUT_ERROR(SSL, SSL_R_BUFFERED_MESSAGES_ON_CIPHER_CHANGE);
+    OPENSSL_PUT_ERROR(SSL, SSL_R_EXCESS_HANDSHAKE_DATA);
     ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
     return false;
   }
@@ -110,6 +111,7 @@
     tls_free,
     tls_get_message,
     tls_next_message,
+    tls_has_unprocessed_handshake_data,
     tls_open_handshake,
     tls_open_change_cipher_spec,
     tls_open_app_data,