Don't resume sessions if the negotiated version doesn't match.

All of NSS, upstream OpenSSL, SChannel, and Secure Transport require, on the
client, that the ServerHello version match the session's version on resumption.
OpenSSL's current behavior is incompatible with all of these. Fall back to a
full handshake on the server instead of mismatch.

Add a comment on the client for why we are, as of
30ddb434bf, not currently enforcing the same in
the client.

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

ssl/s3_clnt.c

@@ -974,6 +974,17 @@ int ssl3_get_server_hello(SSL *s)
 		goto f_err;
 		}
 	s->s3->tmp.new_cipher=c;
+
+	/* Most clients also require that the negotiated version match the
+	 * session's version if resuming. However OpenSSL has historically not
+	 * had the corresponding logic on the server, so this may not be
+	 * compatible, depending on other factors. (Whether the ClientHello
+	 * version is clamped to the session's version and whether the session
+	 * cache is keyed on IP address.)
+	 *
+	 * TODO(davidben): See if we can still enforce this? Perhaps for the
+	 * future TLS 1.3 and forward if this is fixed upstream. */
+
 	/* Don't digest cached records if no sigalgs: we may need them for
 	 * client authentication.
 	 */

ssl/s3_srvr.c

@@ -881,21 +881,27 @@ int ssl3_get_client_hello(SSL *s)
 		}
 	else
 		{
-		i=ssl_get_prev_session(s, &early_ctx);
-		if (i == 1)
-			{ /* previous session */
-			s->hit=1;
-			}
-		else if (i == -1)
-			goto err;
-		else if (i == PENDING_SESSION)
+		i = ssl_get_prev_session(s, &early_ctx);
+		if (i == PENDING_SESSION)
 			{
 			ret = PENDING_SESSION;
 			goto err;
 			}
-		else /* i == 0 */
+		else if (i == -1)
 			{
-			if (!ssl_get_new_session(s,1))
+			goto err;
+			}
+
+		/* Only resume if the session's version matches the negotiated
+		 * version: most clients do not accept a mismatch. */
+		if (i == 1 && s->version == s->session->ssl_version)
+			{
+			s->hit = 1;
+			}
+		else
+			{
+			/* No session was found or it was unacceptable. */
+			if (!ssl_get_new_session(s, 1))
 				goto err;
 			}
 		}

ssl/test/runner/runner.go

@@ -1745,13 +1745,6 @@ func addResumptionVersionTests() {
 			}
 			suffix := "-" + sessionVers.name + "-" + resumeVers.name
 
-			// TODO(davidben): Write equivalent tests for the server
-			// and clean up the server's logic. This requires being
-			// able to give the shim a different set of SSL_OP_NO_*
-			// flags between the initial connection and the
-			// resume. Perhaps resumption should be tested by
-			// serializing the SSL_SESSION and starting a second
-			// shim.
 			testCases = append(testCases, testCase{
 				name:          "Resume-Client" + suffix,
 				resumeSession: true,
@@ -1789,6 +1782,27 @@ func addResumptionVersionTests() {
 				},
 				expectedResumeVersion: resumeVers.version,
 			})
+
+			var flags []string
+			if sessionVers.version != resumeVers.version {
+				flags = append(flags, "-expect-session-miss")
+			}
+			testCases = append(testCases, testCase{
+				testType:      serverTest,
+				name:          "Resume-Server" + suffix,
+				flags:         flags,
+				resumeSession: true,
+				config: Config{
+					MaxVersion:   sessionVers.version,
+					CipherSuites: []uint16{TLS_RSA_WITH_RC4_128_SHA},
+				},
+				expectedVersion: sessionVers.version,
+				resumeConfig: &Config{
+					MaxVersion:   resumeVers.version,
+					CipherSuites: []uint16{TLS_RSA_WITH_RC4_128_SHA},
+				},
+				expectedResumeVersion: resumeVers.version,
+			})
 		}
 	}
 }