Negotiate ciphers before resumption.
This changes our resumption strategy. Before, we would negotiate ciphers
only on fresh handshakes. On resumption, we would blindly use whatever
was in the session.
Instead, evaluate cipher suite preferences on every handshake.
Resumption requires that the saved cipher suite match the one that would
have been negotiated anyway. If client or server preferences changed
sufficiently, we decline the session.
This is much easier to reason about (we always pick the best cipher
suite), simpler, and avoids getting stuck under old preferences if
tickets are continuously renewed. Notably, although TLS 1.2 ticket
renewal does not work in practice, TLS 1.3 will renew tickets like
there's no tomorrow.
It also means we don't need dedicated code to avoid resuming a cipher
which has since been disabled. (That dedicated code was a little odd
anyway since the mask_k, etc., checks didn't occur. When cert_cb was
skipped on resumption, one could resume without ever configuring a
certificate! So we couldn't know whether to mask off RSA or ECDSA cipher
suites.)
Add tests which assert on this new arrangement.
BUG=116
Change-Id: Id40d851ccd87e06c46c6ec272527fd8ece8abfc6
Reviewed-on: https://boringssl-review.googlesource.com/11847
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/ssl/handshake_server.c b/ssl/handshake_server.c
index ff91697..a0562d8 100644
--- a/ssl/handshake_server.c
+++ b/ssl/handshake_server.c
@@ -762,6 +762,16 @@
}
}
+ /* Negotiate the cipher suite. This must be done after |cert_cb| so the
+ * certificate is finalized. */
+ ssl->s3->tmp.new_cipher =
+ ssl3_choose_cipher(ssl, &client_hello, ssl_get_cipher_preferences(ssl));
+ if (ssl->s3->tmp.new_cipher == NULL) {
+ al = SSL_AD_HANDSHAKE_FAILURE;
+ OPENSSL_PUT_ERROR(SSL, SSL_R_NO_SHARED_CIPHER);
+ goto f_err;
+ }
+
ssl->state = SSL3_ST_SR_CLNT_HELLO_E;
}
@@ -827,27 +837,8 @@
goto f_err;
}
- if (ssl->session != NULL) {
- /* Check that the cipher is in the list. */
- if (!ssl_client_cipher_list_contains_cipher(
- &client_hello, (uint16_t)ssl->session->cipher->id)) {
- al = SSL_AD_ILLEGAL_PARAMETER;
- OPENSSL_PUT_ERROR(SSL, SSL_R_REQUIRED_CIPHER_MISSING);
- goto f_err;
- }
-
- ssl->s3->tmp.new_cipher = ssl->session->cipher;
- } else {
- const SSL_CIPHER *c =
- ssl3_choose_cipher(ssl, &client_hello, ssl_get_cipher_preferences(ssl));
- if (c == NULL) {
- al = SSL_AD_HANDSHAKE_FAILURE;
- OPENSSL_PUT_ERROR(SSL, SSL_R_NO_SHARED_CIPHER);
- goto f_err;
- }
-
- ssl->s3->new_session->cipher = c;
- ssl->s3->tmp.new_cipher = c;
+ if (ssl->session == NULL) {
+ ssl->s3->new_session->cipher = ssl->s3->tmp.new_cipher;
/* On new sessions, stash the SNI value in the session. */
if (ssl->s3->hs->hostname != NULL) {
@@ -877,13 +868,13 @@
}
}
- /* Resolve ALPN after the cipher suite is selected. HTTP/2 negotiation depends
- * on the cipher suite. */
+ /* HTTP/2 negotiation depends on the cipher suite, so ALPN negotiation was
+ * deferred. Complete it now. */
if (!ssl_negotiate_alpn(ssl, &al, &client_hello)) {
goto f_err;
}
- /* Now that the cipher is known, initialize the handshake hash. */
+ /* Now that all parameters are known, initialize the handshake hash. */
if (!ssl3_init_handshake_hash(ssl)) {
goto f_err;
}
diff --git a/ssl/internal.h b/ssl/internal.h
index 8860d63..d44179b 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1743,10 +1743,6 @@
int ssl3_write_bytes(SSL *ssl, int type, const void *buf, int len);
int ssl3_output_cert_chain(SSL *ssl);
-/* ssl_is_valid_cipher checks that |cipher| is valid according to the current
- * server configuration in |ssl|. It returns 1 if valid, and 0 otherwise. */
-int ssl_is_valid_cipher(const SSL *ssl, const SSL_CIPHER *cipher);
-
const SSL_CIPHER *ssl3_choose_cipher(
SSL *ssl, const struct ssl_early_callback_ctx *client_hello,
const struct ssl_cipher_preference_list_st *srvr);
diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c
index 9cc0d9d..901b8af 100644
--- a/ssl/s3_lib.c
+++ b/ssl/s3_lib.c
@@ -240,23 +240,6 @@
return NULL;
}
-int ssl_is_valid_cipher(const SSL *ssl, const SSL_CIPHER *cipher) {
- /* Check the TLS version. */
- uint16_t version = ssl3_protocol_version(ssl);
- if (SSL_CIPHER_get_min_version(cipher) > version ||
- SSL_CIPHER_get_max_version(cipher) < version) {
- return 0;
- }
-
- /* TLS 1.3 ciphers are not configurable. */
- if (version >= TLS1_3_VERSION) {
- return 1;
- }
-
- return sk_SSL_CIPHER_find(ssl_get_cipher_preferences(ssl)->ciphers, NULL,
- cipher);
-}
-
const SSL_CIPHER *ssl3_choose_cipher(
SSL *ssl, const struct ssl_early_callback_ctx *client_hello,
const struct ssl_cipher_preference_list_st *server_pref) {
diff --git a/ssl/ssl_session.c b/ssl/ssl_session.c
index 4a1f88d..a28876b 100644
--- a/ssl/ssl_session.c
+++ b/ssl/ssl_session.c
@@ -640,9 +640,8 @@
/* 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);
+ /* Only resume if the session's cipher matches the negotiated one. */
+ ssl->s3->tmp.new_cipher == session->cipher;
}
/* ssl_lookup_session looks up |session_id| in the session cache and sets
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index f55bf37..c6b18b1 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -5729,6 +5729,54 @@
expectResumeRejected: true,
})
+ // Sessions are not resumed if they do not use the preferred cipher.
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "Resume-Server-CipherNotPreferred",
+ resumeSession: true,
+ config: Config{
+ MaxVersion: VersionTLS12,
+ Bugs: ProtocolBugs{
+ ExpectNewTicket: true,
+ FilterTicket: func(in []byte) ([]byte, error) {
+ return SetShimTicketCipherSuite(in, TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA)
+ },
+ },
+ },
+ flags: []string{
+ "-ticket-key",
+ base64.StdEncoding.EncodeToString(TestShimTicketKey),
+ },
+ shouldFail: false,
+ expectResumeRejected: true,
+ })
+
+ // TLS 1.3 allows sessions to be resumed at a different cipher if their
+ // PRF hashes match, but BoringSSL will always decline such resumptions.
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "Resume-Server-CipherNotPreferred-TLS13",
+ resumeSession: true,
+ config: Config{
+ MaxVersion: VersionTLS13,
+ CipherSuites: []uint16{TLS_CHACHA20_POLY1305_SHA256, TLS_AES_128_GCM_SHA256},
+ Bugs: ProtocolBugs{
+ FilterTicket: func(in []byte) ([]byte, error) {
+ // If the client (runner) offers ChaCha20-Poly1305 first, the
+ // server (shim) always prefers it. Switch it to AES-GCM.
+ return SetShimTicketCipherSuite(in, TLS_AES_128_GCM_SHA256)
+ },
+ },
+ },
+ flags: []string{
+ "-ticket-key",
+ base64.StdEncoding.EncodeToString(TestShimTicketKey),
+ },
+ shouldFail: false,
+ expectResumeRejected: true,
+ })
+
+ // Sessions may not be resumed if they contain another version's cipher.
testCases = append(testCases, testCase{
testType: serverTest,
name: "Resume-Server-DeclineBadCipher-TLS13",
@@ -5748,8 +5796,9 @@
expectResumeRejected: true,
})
- // Clients must not advertise a session without also advertising the
- // cipher.
+ // If the client does not offer the cipher from the session, decline to
+ // resume. Clients are forbidden from doing this, but BoringSSL selects
+ // the cipher first, so we only decline.
testCases = append(testCases, testCase{
testType: serverTest,
name: "Resume-Server-UnofferedCipher",
@@ -5765,10 +5814,13 @@
SendCipherSuites: []uint16{TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256},
},
},
- shouldFail: true,
- expectedError: ":REQUIRED_CIPHER_MISSING:",
+ expectResumeRejected: true,
})
+ // In TLS 1.3, clients may advertise a cipher list which does not
+ // include the selected cipher. Test that we tolerate this. Servers may
+ // resume at another cipher if the PRF matches, but BoringSSL will
+ // always decline.
testCases = append(testCases, testCase{
testType: serverTest,
name: "Resume-Server-UnofferedCipher-TLS13",
@@ -5784,8 +5836,7 @@
SendCipherSuites: []uint16{TLS_AES_128_GCM_SHA256},
},
},
- shouldFail: true,
- expectedError: ":REQUIRED_CIPHER_MISSING:",
+ expectResumeRejected: true,
})
// Sessions may not be resumed at a different cipher.
diff --git a/ssl/tls13_server.c b/ssl/tls13_server.c
index 0a8b97b..83ef679 100644
--- a/ssl/tls13_server.c
+++ b/ssl/tls13_server.c
@@ -138,6 +138,7 @@
client_hello->cipher_suites_len);
const int aes_is_fine = EVP_has_aes_hardware();
+ const uint16_t version = ssl3_protocol_version(ssl);
const SSL_CIPHER *best = NULL;
while (CBS_len(&cipher_suites) > 0) {
@@ -146,8 +147,11 @@
return NULL;
}
+ /* Limit to TLS 1.3 ciphers we know about. */
const SSL_CIPHER *candidate = SSL_get_cipher_by_value(cipher_suite);
- if (candidate == NULL || !ssl_is_valid_cipher(ssl, candidate)) {
+ if (candidate == NULL ||
+ SSL_CIPHER_get_min_version(candidate) > version ||
+ SSL_CIPHER_get_max_version(candidate) < version) {
continue;
}
@@ -192,6 +196,15 @@
return ssl_hs_error;
}
+ /* Negotiate the cipher suite. */
+ ssl->s3->tmp.new_cipher = choose_tls13_cipher(ssl, &client_hello);
+ if (ssl->s3->tmp.new_cipher == NULL) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_NO_SHARED_CIPHER);
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);
+ return ssl_hs_error;
+ }
+
+ /* Decode the ticket if we agree on a PSK key exchange mode. */
uint8_t alert = SSL_AD_DECODE_ERROR;
SSL_SESSION *session = NULL;
CBS pre_shared_key, binders;
@@ -220,11 +233,24 @@
session = NULL;
}
+ /* Set up the new session, either using the original one as a template or
+ * creating a fresh one. */
if (session == NULL) {
if (!ssl_get_new_session(ssl, 1 /* server */)) {
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
return ssl_hs_error;
}
+
+ ssl->s3->new_session->cipher = ssl->s3->tmp.new_cipher;
+
+ /* On new sessions, stash the SNI value in the session. */
+ if (ssl->s3->hs->hostname != NULL) {
+ ssl->s3->new_session->tlsext_hostname = BUF_strdup(ssl->s3->hs->hostname);
+ if (ssl->s3->new_session->tlsext_hostname == NULL) {
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
+ return ssl_hs_error;
+ }
+ }
} else {
/* Check the PSK binder. */
if (!tls13_verify_psk_binder(ssl, session, &binders)) {
@@ -251,40 +277,8 @@
return ssl_hs_error;
}
- 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);
- ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);
- return ssl_hs_error;
- }
-
- ssl->s3->new_session->cipher = cipher;
-
- /* On new sessions, stash the SNI value in the session. */
- if (ssl->s3->hs->hostname != NULL) {
- ssl->s3->new_session->tlsext_hostname = BUF_strdup(ssl->s3->hs->hostname);
- if (ssl->s3->new_session->tlsext_hostname == NULL) {
- ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
- return ssl_hs_error;
- }
- }
- }
-
- ssl->s3->tmp.new_cipher = ssl->s3->new_session->cipher;
- ssl->method->received_flight(ssl);
-
- /* Resolve ALPN after the cipher suite is selected. HTTP/2 negotiation depends
- * on the cipher suite. */
+ /* HTTP/2 negotiation depends on the cipher suite, so ALPN negotiation was
+ * deferred. Complete it now. */
if (!ssl_negotiate_alpn(ssl, &alert, &client_hello)) {
ssl3_send_alert(ssl, SSL3_AL_FATAL, alert);
return ssl_hs_error;
@@ -310,6 +304,8 @@
return ssl_hs_error;
}
+ ssl->method->received_flight(ssl);
+
/* Resolve ECDHE and incorporate it into the secret. */
int need_retry;
if (!resolve_ecdhe_secret(ssl, &need_retry, &client_hello)) {
