Align TLS 1.2 and 1.3 server session validity checks.
Having that logic in two different places is a nuisance when we go to
add new checks like resumption stuff. Along the way, this adds missing
tests for the ClientHello cipher/session consistency check. (We'll
eventually get it for free once the cipher/resumption change is
unblocked, but get this working in the meantime.)
This also fixes a bug where the session validity checks happened in the
wrong order relative to whether tickets_supported or renew_ticket was
looked at. Fix that by lifting that logic closer to the handshake.
Change-Id: I3f4b59cfe01064f9125277dc5834e62a36e64aae
Reviewed-on: https://boringssl-review.googlesource.com/12230
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/handshake_server.c b/ssl/handshake_server.c
index c8208ad..4c12d09 100644
--- a/ssl/handshake_server.c
+++ b/ssl/handshake_server.c
@@ -730,9 +730,9 @@
client_hello.random_len);
/* Determine whether we are doing session resumption. */
- int send_new_ticket = 0;
- switch (
- ssl_get_prev_session(ssl, &session, &send_new_ticket, &client_hello)) {
+ int tickets_supported = 0, renew_ticket = 0;
+ switch (ssl_get_prev_session(ssl, &session, &tickets_supported,
+ &renew_ticket, &client_hello)) {
case ssl_session_success:
break;
case ssl_session_error:
@@ -741,7 +741,6 @@
ssl->rwstate = SSL_PENDING_SESSION;
goto err;
}
- ssl->s3->hs->ticket_expected = send_new_ticket;
/* The EMS state is needed when making the resumption decision, but
* extensions are not normally parsed until later. This detects the EMS
@@ -754,7 +753,6 @@
TLSEXT_TYPE_extended_master_secret) &&
CBS_len(&ems) == 0;
- int has_session = 0;
if (session != NULL) {
if (session->extended_master_secret &&
!have_extended_master_secret) {
@@ -765,24 +763,23 @@
goto f_err;
}
- has_session =
- /* Only resume if the session's version matches the negotiated
- * version: most clients do not accept a mismatch. */
- ssl->version == session->ssl_version &&
+ if (!ssl_session_is_resumable(ssl, session) ||
/* If the client offers the EMS extension, but the previous session
* didn't use it, then negotiate a new session. */
- have_extended_master_secret == session->extended_master_secret &&
- /* Only resume if the session's cipher is still valid under the
- * current configuration. */
- ssl_is_valid_cipher(ssl, session->cipher);
+ have_extended_master_secret != session->extended_master_secret) {
+ SSL_SESSION_free(session);
+ session = NULL;
+ }
}
- if (has_session) {
+ if (session != NULL) {
/* Use the old session. */
+ ssl->s3->hs->ticket_expected = renew_ticket;
ssl->session = session;
session = NULL;
ssl->s3->session_reused = 1;
} else {
+ ssl->s3->hs->ticket_expected = tickets_supported;
ssl_set_session(ssl, NULL);
if (!ssl_get_new_session(ssl, 1 /* server */)) {
goto err;
diff --git a/ssl/internal.h b/ssl/internal.h
index c964a83..8f4300e 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1651,6 +1651,10 @@
* it has expired. */
int ssl_session_is_time_valid(const SSL *ssl, const SSL_SESSION *session);
+/* ssl_session_is_resumable returns one if |session| is resumable for |ssl| and
+ * zero otherwise. */
+int ssl_session_is_resumable(const SSL *ssl, const SSL_SESSION *session);
+
void ssl_set_session(SSL *ssl, SSL_SESSION *session);
enum ssl_session_result_t {
@@ -1660,14 +1664,13 @@
};
/* ssl_get_prev_session looks up the previous session based on |ctx|. On
- * success, it sets |*out_session| to the session or NULL if none was found. It
- * sets |*out_send_ticket| to whether a ticket should be sent at the end of the
- * handshake. If the session could not be looked up synchronously, it returns
+ * success, it sets |*out_session| to the session or NULL if none was found. If
+ * the session could not be looked up synchronously, it returns
* |ssl_session_retry| and should be called again. Otherwise, it returns
* |ssl_session_error|. */
enum ssl_session_result_t ssl_get_prev_session(
- SSL *ssl, SSL_SESSION **out_session, int *out_send_ticket,
- const struct ssl_early_callback_ctx *ctx);
+ SSL *ssl, SSL_SESSION **out_session, int *out_tickets_supported,
+ int *out_renew_ticket, const struct ssl_early_callback_ctx *ctx);
/* The following flags determine which parts of the session are duplicated. */
#define SSL_SESSION_DUP_AUTH_ONLY 0x0
diff --git a/ssl/ssl_session.c b/ssl/ssl_session.c
index 9f61a5d..faf155c 100644
--- a/ssl/ssl_session.c
+++ b/ssl/ssl_session.c
@@ -633,6 +633,18 @@
return session->timeout > (long)now.tv_sec - session->time;
}
+int ssl_session_is_resumable(const SSL *ssl, const SSL_SESSION *session) {
+ return ssl_session_is_context_valid(ssl, session) &&
+ /* The session must not be expired. */
+ ssl_session_is_time_valid(ssl, session) &&
+ /* Only resume if the session's version matches the negotiated
+ * version. */
+ ssl->version == session->ssl_version &&
+ /* Only resume if the session's cipher is still valid under the
+ * current configuration. */
+ ssl_is_valid_cipher(ssl, session->cipher);
+}
+
/* ssl_lookup_session looks up |session_id| in the session cache and sets
* |*out_session| to an |SSL_SESSION| object if found. The caller takes
* ownership of the result. */
@@ -693,15 +705,8 @@
}
}
- if (session == NULL) {
- return ssl_session_success;
- }
-
- if (!ssl_session_is_context_valid(ssl, session)) {
- /* The client did not offer a suitable ticket or session ID. */
- SSL_SESSION_free(session);
- session = NULL;
- } else if (!ssl_session_is_time_valid(ssl, session)) {
+ if (session != NULL &&
+ !ssl_session_is_time_valid(ssl, session)) {
/* The session was from the cache, so remove it. */
SSL_CTX_remove_session(ssl->initial_ctx, session);
SSL_SESSION_free(session);
@@ -713,8 +718,8 @@
}
enum ssl_session_result_t ssl_get_prev_session(
- SSL *ssl, SSL_SESSION **out_session, int *out_send_ticket,
- const struct ssl_early_callback_ctx *ctx) {
+ SSL *ssl, SSL_SESSION **out_session, int *out_tickets_supported,
+ int *out_renew_ticket, const struct ssl_early_callback_ctx *ctx) {
/* This is used only by servers. */
assert(ssl->server);
SSL_SESSION *session = NULL;
@@ -743,11 +748,8 @@
}
*out_session = session;
- if (session != NULL) {
- *out_send_ticket = renew_ticket;
- } else {
- *out_send_ticket = tickets_supported;
- }
+ *out_tickets_supported = tickets_supported;
+ *out_renew_ticket = renew_ticket;
return ssl_session_success;
}
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 24e4548..c318a9b 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -3133,12 +3133,6 @@
memcpy(session->session_id, session_id, session_id_len);
session->session_id_length = session_id_len;
- if (!ssl_session_is_context_valid(ssl, session) ||
- !ssl_session_is_time_valid(ssl, session)) {
- SSL_SESSION_free(session);
- session = NULL;
- }
-
*out_session = session;
done:
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index 446c8dc..c3b6b04 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -769,6 +769,11 @@
// the server believes it has actually negotiated.
SendCipherSuite uint16
+ // SendCipherSuites, if not nil, is the cipher suite list that the
+ // client will send in the ClientHello. This does not affect the cipher
+ // the client believes it has actually offered.
+ SendCipherSuites []uint16
+
// AppDataBeforeHandshake, if not nil, causes application data to be
// sent immediately before the first handshake message.
AppDataBeforeHandshake []byte
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go
index 3f377f3..36dc1e0 100644
--- a/ssl/test/runner/handshake_client.go
+++ b/ssl/test/runner/handshake_client.go
@@ -309,6 +309,10 @@
hello.vers = c.config.Bugs.SendClientVersion
}
+ if c.config.Bugs.SendCipherSuites != nil {
+ hello.cipherSuites = c.config.Bugs.SendCipherSuites
+ }
+
var helloBytes []byte
if c.config.Bugs.SendV2ClientHello {
// Test that the peer left-pads random.
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 8f43e5c..717c420 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -5633,6 +5633,7 @@
config: Config{
MaxVersion: VersionTLS12,
Bugs: ProtocolBugs{
+ ExpectNewTicket: true,
FilterTicket: func(in []byte) ([]byte, error) {
return SetShimTicketVersion(in, VersionTLS13)
},
@@ -5672,6 +5673,7 @@
config: Config{
MaxVersion: VersionTLS12,
Bugs: ProtocolBugs{
+ ExpectNewTicket: true,
FilterTicket: func(in []byte) ([]byte, error) {
return SetShimTicketCipherSuite(in, TLS_AES_128_GCM_SHA256)
},
@@ -5691,6 +5693,7 @@
config: Config{
MaxVersion: VersionTLS12,
Bugs: ProtocolBugs{
+ ExpectNewTicket: true,
FilterTicket: func(in []byte) ([]byte, error) {
return SetShimTicketCipherSuite(in, TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384)
},
@@ -5723,6 +5726,46 @@
expectResumeRejected: true,
})
+ // Clients must not advertise a session without also advertising the
+ // cipher.
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "Resume-Server-UnofferedCipher",
+ resumeSession: true,
+ config: Config{
+ MaxVersion: VersionTLS12,
+ CipherSuites: []uint16{TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256},
+ },
+ resumeConfig: &Config{
+ MaxVersion: VersionTLS12,
+ CipherSuites: []uint16{TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256},
+ Bugs: ProtocolBugs{
+ SendCipherSuites: []uint16{TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256},
+ },
+ },
+ shouldFail: true,
+ expectedError: ":REQUIRED_CIPHER_MISSING:",
+ })
+
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "Resume-Server-UnofferedCipher-TLS13",
+ resumeSession: true,
+ config: Config{
+ MaxVersion: VersionTLS13,
+ CipherSuites: []uint16{TLS_CHACHA20_POLY1305_SHA256},
+ },
+ resumeConfig: &Config{
+ MaxVersion: VersionTLS13,
+ CipherSuites: []uint16{TLS_CHACHA20_POLY1305_SHA256},
+ Bugs: ProtocolBugs{
+ SendCipherSuites: []uint16{TLS_AES_128_GCM_SHA256},
+ },
+ },
+ shouldFail: true,
+ expectedError: ":REQUIRED_CIPHER_MISSING:",
+ })
+
// Sessions may not be resumed at a different cipher.
testCases = append(testCases, testCase{
name: "Resume-Client-CipherMismatch",
diff --git a/ssl/tls13_server.c b/ssl/tls13_server.c
index 0db16d9..c2f8bc5 100644
--- a/ssl/tls13_server.c
+++ b/ssl/tls13_server.c
@@ -143,11 +143,7 @@
}
if (session != NULL &&
- /* Only resume if the session's version matches. */
- (session->ssl_version != ssl->version ||
- !ssl_client_cipher_list_contains_cipher(
- &client_hello, (uint16_t)SSL_CIPHER_get_id(session->cipher)) ||
- !ssl_is_valid_cipher(ssl, session->cipher))) {
+ !ssl_session_is_resumable(ssl, session)) {
SSL_SESSION_free(session);
session = NULL;
}
@@ -268,7 +264,16 @@
return ssl_hs_error;
}
- if (!ssl->s3->session_reused) {
+ if (ssl->s3->session_reused) {
+ /* Clients may not offer sessions containing unsupported ciphers. */
+ if (!ssl_client_cipher_list_contains_cipher(
+ &client_hello,
+ (uint16_t)SSL_CIPHER_get_id(ssl->s3->new_session->cipher))) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_REQUIRED_CIPHER_MISSING);
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
+ return ssl_hs_error;
+ }
+ } else {
const SSL_CIPHER *cipher = choose_tls13_cipher(ssl, &client_hello);
if (cipher == NULL) {
OPENSSL_PUT_ERROR(SSL, SSL_R_NO_SHARED_CIPHER);
