Fix handshake check when False Start is used with implicit read.

It may take up to two iterations of s->handshake_func before it is safe to
continue. Fortunately, even if anything was using False Start this way
(Chromium doesn't), we don't inherit NSS's security bug. The "redundant" check
in the type match case later on in this function saves us.

Amusingly, the success case still worked before this fix. Even though we fall
through to the post-handshake codepath and get a handshake record while
"expecting" app data, the handshake state machine is still pumped thanks to a
codepath meant for renego!

Change-Id: Ie129d83ac1451ad4947c4f86380879db8a3fd924
Reviewed-on: https://boringssl-review.googlesource.com/3335
Reviewed-by: Adam Langley <agl@google.com>

ssl/s3_pkt.c

@@ -772,8 +772,13 @@ int ssl3_read_bytes(SSL *s, int type, uint8_t *buf, int len, int peek) {
 
   /* Now s->s3->handshake_fragment_len == 0 if type == SSL3_RT_HANDSHAKE. */
 
-  if (!s->in_handshake && SSL_in_init(s)) {
-    /* type == SSL3_RT_APPLICATION_DATA */
+  /* This may require multiple iterations. False Start will cause
+   * |s->handshake_func| to signal success one step early, but the handshake
+   * must be completely finished before other modes are accepted.
+   *
+   * TODO(davidben): Move this check up to a higher level. */
+  while (!s->in_handshake && SSL_in_init(s)) {
+    assert(type == SSL3_RT_APPLICATION_DATA);
     i = s->handshake_func(s);
     if (i < 0) {
       return i;

ssl/test/runner/runner.go

@@ -759,6 +759,25 @@ var testCases = []testCase{
 		shouldFail:      true,
 		expectedError:   ":UNEXPECTED_RECORD:",
 	},
+	{
+		name: "FalseStart-SkipServerSecondLeg-Implicit",
+		config: Config{
+			CipherSuites: []uint16{TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256},
+			NextProtos:   []string{"foo"},
+			Bugs: ProtocolBugs{
+				SkipNewSessionTicket: true,
+				SkipChangeCipherSpec: true,
+				SkipFinished:         true,
+			},
+		},
+		flags: []string{
+			"-implicit-handshake",
+			"-false-start",
+			"-advertise-alpn", "\x03foo",
+		},
+		shouldFail:    true,
+		expectedError: ":UNEXPECTED_RECORD:",
+	},
 }
 
 func doExchange(test *testCase, config *Config, conn net.Conn, messageLen int, isResume bool) error {
@@ -1758,6 +1777,24 @@ func addStateMachineCoverageTests(async, splitHandshake bool, protocol protocol)
 			resumeSession:   true,
 		})
 
+		// Client does False Start but doesn't explicitly call
+		// SSL_connect.
+		testCases = append(testCases, testCase{
+			protocol: protocol,
+			name:     "FalseStart-Implicit" + suffix,
+			config: Config{
+				CipherSuites: []uint16{TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256},
+				NextProtos:   []string{"foo"},
+				Bugs: ProtocolBugs{
+					MaxHandshakeRecordLength: maxHandshakeRecordLength,
+				},
+			},
+			flags: append(flags,
+				"-implicit-handshake",
+				"-false-start",
+				"-advertise-alpn", "\x03foo"),
+		})
+
 		// False Start without session tickets.
 		testCases = append(testCases, testCase{
 			name: "FalseStart-SessionTicketsDisabled",