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>
diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata
index 38aa7ed..bd34db5 100644
--- a/crypto/err/ssl.errordata
+++ b/crypto/err/ssl.errordata
@@ -267,6 +267,7 @@
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
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 52de0bb..831b511 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -2620,6 +2620,7 @@
#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
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index 71cc4c7..a6e76c9 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -855,24 +855,22 @@
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) &&
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go
index 77fd0a5..46e0fb0 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -443,6 +443,9 @@
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
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 6e80f94..3b8aa9e 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2702,27 +2702,40 @@
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 @@
}
}
}
+
+ 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() {
