Forbid interleaving app data in a HelloRequest.

We already forbid renego/app-data interleave. Forbid it within a
HelloRequest too because that's nonsense. No one would ever send:

   [hs:HelloReq-] [app:Hello world] [hs:-uest]

Add tests for this case.

This is in preparation for our more complex TLS 1.3 post-handshake logic
which is going to go through the usual handshake reassembly logic and,
for sanity, will want to enforce this anyway.

BUG=83

Change-Id: I80eb9f3333da3d751f98f25d9469860d1993a97a
Reviewed-on: https://boringssl-review.googlesource.com/9000
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

3 files changed, 37 insertions, 1 deletions

diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
index 060ef691..3e674e6f 100644
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -411,7 +411,8 @@ start:
 
   /* we now have a packet which can be read and processed */
 
-  if (type == rr->type) {
+  /* Do not allow interleaving application data and HelloRequest. */
+  if (type == rr->type && ssl->s3->hello_request_len == 0) {
     /* Discard empty records. */
     if (rr->length == 0) {
       goto start;
diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go
index cefdde33..d01643c9 100644
--- a/ssl/test/runner/conn.go
+++ b/ssl/test/runner/conn.go
@@ -1274,6 +1274,20 @@ func (c *Conn) simulatePacketLoss(resendFunc func()) error {
 	return nil
 }
 
+func (c *Conn) SendHalfHelloRequest() error {
+	if err := c.Handshake(); err != nil {
+		return err
+	}
+
+	c.out.Lock()
+	defer c.out.Unlock()
+
+	if _, err := c.writeRecord(recordTypeHandshake, []byte{typeHelloRequest, 0}); err != nil {
+		return err
+	}
+	return c.flushHandshake()
+}
+
 // Write writes data to the connection.
 func (c *Conn) Write(b []byte) (int, error) {
 	if err := c.Handshake(); err != nil {
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index bfd9faf3..35fe585e 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -301,6 +301,9 @@ type testCase struct {
 	// renegotiate indicates the number of times the connection should be
 	// renegotiated during the exchange.
 	renegotiate int
+	// sendHalfHelloRequest, if true, causes the server to send half a
+	// HelloRequest when the handshake completes.
+	sendHalfHelloRequest bool
 	// renegotiateCiphers is a list of ciphersuite ids that will be
 	// switched in just before renegotiation.
 	renegotiateCiphers []uint16
@@ -572,6 +575,10 @@ func doExchange(test *testCase, config *Config, conn net.Conn, isResume bool) er
 		tlsConn.SendAlert(alertLevelWarning, alertUnexpectedMessage)
 	}
 
+	if test.sendHalfHelloRequest {
+		tlsConn.SendHalfHelloRequest()
+	}
+
 	if test.renegotiate > 0 {
 		if test.renegotiateCiphers != nil {
 			config.CipherSuites = test.renegotiateCiphers
@@ -3383,6 +3390,20 @@ func addStateMachineCoverageTests(config stateMachineTestConfig) {
 			},
 		})
 
+		tests = append(tests, testCase{
+			name: "SendHalfHelloRequest",
+			config: Config{
+				MaxVersion: VersionTLS12,
+				Bugs: ProtocolBugs{
+					PackHelloRequestWithFinished: config.packHandshakeFlight,
+				},
+			},
+			sendHalfHelloRequest: true,
+			flags:                []string{"-renegotiate-ignore"},
+			shouldFail:           true,
+			expectedError:        ":UNEXPECTED_RECORD:",
+		})
+
 		// NPN on client and server; results in post-handshake message.
 		tests = append(tests, testCase{
 			name: "NPN-Client",