Enforce that sessions are resumed at the version they're created.

After sharding the session cache for fallbacks, the numbers have been pretty
good; 0.03% on dev and 0.02% on canary. Stable is at 0.06% but does not have
the sharded session cache. Before sharding, stable, beta, and dev had been
fairly closely aligned. Between 0.03% being low and the fallback saving us in
all but extremely contrived cases, I think this should be fairly safe.

Add tests for both the cipher suite and protocol version mismatch checks.

BUG=441456

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

crypto/err/ssl.errordata

@@ -267,6 +267,7 @@ SSL,reason,195,NO_SRTP_PROFILES
 SSL,reason,196,NULL_SSL_CTX
 SSL,reason,197,NULL_SSL_METHOD_PASSED
 SSL,reason,198,OLD_SESSION_CIPHER_NOT_RETURNED
+SSL,reason,273,OLD_SESSION_VERSION_NOT_RETURNED
 SSL,reason,199,PACKET_LENGTH_TOO_LONG
 SSL,reason,200,PARSE_TLSEXT
 SSL,reason,201,PATH_TOO_LONG

include/openssl/ssl.h

@@ -2620,6 +2620,7 @@ OPENSSL_EXPORT const char *SSLeay_version(int unused);
 #define SSL_R_X509_VERIFICATION_SETUP_PROBLEMS 270
 #define SSL_R_FRAGMENT_MISMATCH 271
 #define SSL_R_BUFFER_TOO_SMALL 272
+#define SSL_R_OLD_SESSION_VERSION_NOT_RETURNED 273
 #define SSL_R_SSLV3_ALERT_CLOSE_NOTIFY 1000
 #define SSL_R_SSLV3_ALERT_UNEXPECTED_MESSAGE 1010
 #define SSL_R_SSLV3_ALERT_BAD_RECORD_MAC 1020

ssl/s3_clnt.c

@@ -855,24 +855,22 @@ int ssl3_get_server_hello(SSL *s) {
     goto f_err;
   }
 
-  if (s->hit && s->session->cipher != c) {
-    al = SSL_AD_ILLEGAL_PARAMETER;
-    OPENSSL_PUT_ERROR(SSL, ssl3_get_server_hello,
-                      SSL_R_OLD_SESSION_CIPHER_NOT_RETURNED);
-    goto f_err;
+  if (s->hit) {
+    if (s->session->cipher != c) {
+      al = SSL_AD_ILLEGAL_PARAMETER;
+      OPENSSL_PUT_ERROR(SSL, ssl3_get_server_hello,
+                        SSL_R_OLD_SESSION_CIPHER_NOT_RETURNED);
+      goto f_err;
+    }
+    if (s->session->ssl_version != s->version) {
+      al = SSL_AD_ILLEGAL_PARAMETER;
+      OPENSSL_PUT_ERROR(SSL, ssl3_get_server_hello,
+                        SSL_R_OLD_SESSION_VERSION_NOT_RETURNED);
+      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. */
   if (!SSL_USE_SIGALGS(s) &&

ssl/test/runner/handshake_server.go

@@ -443,6 +443,9 @@ func (hs *serverHandshakeState) doResumeHandshake() error {
 	c := hs.c
 
 	hs.hello.cipherSuite = hs.suite.id
+	if c.config.Bugs.SendCipherSuite != 0 {
+		hs.hello.cipherSuite = c.config.Bugs.SendCipherSuite
+	}
 	// We echo the client's session ID in the ServerHello to let it know
 	// that we're doing a resumption.
 	hs.hello.sessionId = hs.clientHello.sessionId

ssl/test/runner/runner.go

@@ -2702,27 +2702,40 @@ func addResumptionVersionTests() {
 					suffix += "-DTLS"
 				}
 
-				testCases = append(testCases, testCase{
-					protocol:      protocol,
-					name:          "Resume-Client" + suffix,
-					resumeSession: true,
-					config: Config{
-						MaxVersion:   sessionVers.version,
-						CipherSuites: []uint16{TLS_RSA_WITH_AES_128_CBC_SHA},
-						Bugs: ProtocolBugs{
-							AllowSessionVersionMismatch: true,
+				if sessionVers.version == resumeVers.version {
+					testCases = append(testCases, testCase{
+						protocol:      protocol,
+						name:          "Resume-Client" + suffix,
+						resumeSession: true,
+						config: Config{
+							MaxVersion:   sessionVers.version,
+							CipherSuites: []uint16{TLS_RSA_WITH_AES_128_CBC_SHA},
 						},
-					},
-					expectedVersion: sessionVers.version,
-					resumeConfig: &Config{
-						MaxVersion:   resumeVers.version,
-						CipherSuites: []uint16{TLS_RSA_WITH_AES_128_CBC_SHA},
-						Bugs: ProtocolBugs{
-							AllowSessionVersionMismatch: true,
+						expectedVersion:       sessionVers.version,
+						expectedResumeVersion: resumeVers.version,
+					})
+				} else {
+					testCases = append(testCases, testCase{
+						protocol:      protocol,
+						name:          "Resume-Client-Mismatch" + suffix,
+						resumeSession: true,
+						config: Config{
+							MaxVersion:   sessionVers.version,
+							CipherSuites: []uint16{TLS_RSA_WITH_AES_128_CBC_SHA},
 						},
-					},
-					expectedResumeVersion: resumeVers.version,
-				})
+						expectedVersion: sessionVers.version,
+						resumeConfig: &Config{
+							MaxVersion:   resumeVers.version,
+							CipherSuites: []uint16{TLS_RSA_WITH_AES_128_CBC_SHA},
+							Bugs: ProtocolBugs{
+								AllowSessionVersionMismatch: true,
+							},
+						},
+						expectedResumeVersion: resumeVers.version,
+						shouldFail:            true,
+						expectedError:         ":OLD_SESSION_VERSION_NOT_RETURNED:",
+					})
+				}
 
 				testCases = append(testCases, testCase{
 					protocol:      protocol,
@@ -2766,6 +2779,22 @@ func addResumptionVersionTests() {
 			}
 		}
 	}
+
+	testCases = append(testCases, testCase{
+		name:          "Resume-Client-CipherMismatch",
+		resumeSession: true,
+		config: Config{
+			CipherSuites: []uint16{TLS_RSA_WITH_AES_128_GCM_SHA256},
+		},
+		resumeConfig: &Config{
+			CipherSuites: []uint16{TLS_RSA_WITH_AES_128_GCM_SHA256},
+			Bugs: ProtocolBugs{
+				SendCipherSuite: TLS_RSA_WITH_AES_128_CBC_SHA,
+			},
+		},
+		shouldFail:    true,
+		expectedError: ":OLD_SESSION_CIPHER_NOT_RETURNED:",
+	})
 }
 
 func addRenegotiationTests() {