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
30ddb434bfb845356fbacb6b2bd51f8814c7043c, 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>
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index aeb2604..bade13b 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -974,6 +974,17 @@
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.
*/
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index 5f861a7..d591e19 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -881,21 +881,27 @@
}
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;
}
}
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index f8649d3..9172223 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -1745,13 +1745,6 @@
}
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 @@
},
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,
+ })
}
}
}
