Fix client handling of 0-RTT rejects with cipher mismatch.
Servers can only accept 0-RTT if the ciphers match. If they reject
0-RTT, however, they may change the cipher suite and even the PRF hash.
This is tricky, however, because the 0-RTT accept or reject signal comes
in EncryptedExtensions, which is *after* the new cipher suite is
installed. (Although a client could infer 0-RTT is rejected based on the
cipher suite if it wanted.)
While we correctly handled the PRF hash switch, we get the cipher suite
mixed up due to dependency on SSL_get_session and incorrectly decrypt
EncryptedExtensions. Fix this and add some tests.
Change-Id: Ia20f2ed665cf601d30a38f0c8d4786c4c111019f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/40005
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata
index b2a6146..27ad65c 100644
--- a/crypto/err/ssl.errordata
+++ b/crypto/err/ssl.errordata
@@ -36,6 +36,7 @@
SSL,127,CERT_LENGTH_MISMATCH
SSL,128,CHANNEL_ID_NOT_P256
SSL,129,CHANNEL_ID_SIGNATURE_INVALID
+SSL,304,CIPHER_MISMATCH_ON_EARLY_DATA
SSL,130,CIPHER_OR_HASH_UNAVAILABLE
SSL,131,CLIENTHELLO_PARSE_FAILED
SSL,132,CLIENTHELLO_TLSEXT
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 9847be1..1fedaf8 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -3192,6 +3192,11 @@
// This function should use |SSL_get_current_cipher| to determine the TLS
// cipher suite.
//
+ // TODO(davidben): The advice to use |SSL_get_current_cipher| does not work
+ // for 0-RTT rejects on the client. As part of the fix to
+ // https://crbug.com/boringssl/303, we will add an explicit cipher suite
+ // parameter.
+ //
// It returns one on success and zero on error.
int (*set_encryption_secrets)(SSL *ssl, enum ssl_encryption_level_t level,
const uint8_t *read_secret,
@@ -5070,6 +5075,7 @@
#define SSL_R_INVALID_DELEGATED_CREDENTIAL 301
#define SSL_R_KEY_USAGE_BIT_INCORRECT 302
#define SSL_R_INCONSISTENT_CLIENT_HELLO 303
+#define SSL_R_CIPHER_MISMATCH_ON_EARLY_DATA 304
#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/handoff.cc b/ssl/handoff.cc
index a9da05e..fdbac18 100644
--- a/ssl/handoff.cc
+++ b/ssl/handoff.cc
@@ -654,6 +654,7 @@
// none of the read keys. The read keys are installed in the state machine
// immediately after processing handback.
if (!tls13_set_traffic_key(ssl, ssl_encryption_application, evp_aead_seal,
+ hs->new_session.get(),
hs->server_traffic_secret_0())) {
return false;
}
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index a970a3c..fa692c5 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -463,7 +463,7 @@
}
if (ssl->quic_method == nullptr &&
!tls13_set_traffic_key(ssl, ssl_encryption_early_data, evp_aead_seal,
- hs->early_traffic_secret())) {
+ ssl->session.get(), hs->early_traffic_secret())) {
return ssl_hs_error;
}
diff --git a/ssl/internal.h b/ssl/internal.h
index 4a93e15..a6b58c5 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1356,9 +1356,11 @@
bool tls13_advance_key_schedule(SSL_HANDSHAKE *hs, Span<const uint8_t> in);
// tls13_set_traffic_key sets the read or write traffic keys to
-// |traffic_secret|. It returns true on success and false on error.
+// |traffic_secret|. The version and cipher suite are determined from |session|.
+// It returns true on success and false on error.
bool tls13_set_traffic_key(SSL *ssl, enum ssl_encryption_level_t level,
enum evp_aead_direction_t direction,
+ const SSL_SESSION *session,
Span<const uint8_t> traffic_secret);
// tls13_derive_early_secret derives the early traffic secret. It returns true
diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc
index 77d3f4c..6635703 100644
--- a/ssl/ssl_session.cc
+++ b/ssl/ssl_session.cc
@@ -624,10 +624,14 @@
ssl->server == session->is_server &&
// The session must not be expired.
ssl_session_is_time_valid(ssl, session) &&
- /* Only resume if the session's version matches the negotiated
- * version. */
+ // Only resume if the session's version matches the negotiated
+ // version.
ssl->version == session->ssl_version &&
- // Only resume if the session's cipher matches the negotiated one.
+ // Only resume if the session's cipher matches the negotiated one. This
+ // is stricter than necessary for TLS 1.3, which allows cross-cipher
+ // resumption if the PRF hashes match. We require an exact match for
+ // simplicity. If loosening this, the 0-RTT accept logic must be
+ // updated to check the cipher.
hs->new_cipher == session->cipher &&
// If the session contains a client certificate (either the full
// certificate or just the hash) then require that the form of the
diff --git a/ssl/test/runner/fuzzer_mode.json b/ssl/test/runner/fuzzer_mode.json
index 0bdee88..940415d 100644
--- a/ssl/test/runner/fuzzer_mode.json
+++ b/ssl/test/runner/fuzzer_mode.json
@@ -42,7 +42,7 @@
"EarlyData-ALPNOmitted1-Server-*": "Trial decryption does not work with the NULL cipher.",
"EarlyData-ALPNOmitted2-Server-*": "Trial decryption does not work with the NULL cipher.",
"*-EarlyData-RejectUnfinishedWrite-Client-*": "Trial decryption does not work with the NULL cipher.",
- "EarlyData-Reject*-Client-*": "Trial decryption does not work with the NULL cipher.",
+ "EarlyData-Reject*-Client*": "Trial decryption does not work with the NULL cipher.",
"CustomExtensions-Server-EarlyDataOffered": "Trial decryption does not work with the NULL cipher.",
"*-TicketAgeSkew-*-Reject*": "Trial decryption does not work with the NULL cipher.",
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 4ec8bd8..255cd9e 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -13565,8 +13565,9 @@
"-on-resume-expect-alpn", "foo",
"-on-retry-expect-alpn", "bar",
},
- shouldFail: true,
- expectedError: ":ALPN_MISMATCH_ON_EARLY_DATA:",
+ shouldFail: true,
+ expectedError: ":ALPN_MISMATCH_ON_EARLY_DATA:",
+ expectedLocalError: "remote error: illegal parameter",
})
// Test that the client does not offer early data if it is incompatible
@@ -13723,10 +13724,11 @@
MaxEarlyDataSize: 16384,
RequestChannelID: true,
},
- resumeSession: true,
- expectChannelID: true,
- shouldFail: true,
- expectedError: ":UNEXPECTED_EXTENSION_ON_EARLY_DATA:",
+ resumeSession: true,
+ expectChannelID: true,
+ shouldFail: true,
+ expectedError: ":UNEXPECTED_EXTENSION_ON_EARLY_DATA:",
+ expectedLocalError: "remote error: illegal parameter",
flags: []string{
"-enable-early-data",
"-expect-ticket-supports-early-data",
@@ -14042,6 +14044,86 @@
"-expect-early-data-reason", "protocol_version",
},
})
+
+ // On 0-RTT reject, the server may end up negotiating a cipher suite with a
+ // different PRF hash. Test that the client handles this correctly.
+ testCases = append(testCases, testCase{
+ testType: clientTest,
+ name: "EarlyData-Reject0RTT-DifferentPRF-Client",
+ config: Config{
+ MaxVersion: VersionTLS13,
+ CipherSuites: []uint16{TLS_AES_128_GCM_SHA256},
+ MaxEarlyDataSize: 16384,
+ },
+ resumeConfig: &Config{
+ MaxVersion: VersionTLS13,
+ MaxEarlyDataSize: 16384,
+ CipherSuites: []uint16{TLS_AES_256_GCM_SHA384},
+ },
+ resumeSession: true,
+ expectResumeRejected: true,
+ flags: []string{
+ "-enable-early-data",
+ "-expect-reject-early-data",
+ "-expect-ticket-supports-early-data",
+ "-on-resume-shim-writes-first",
+ },
+ })
+ testCases = append(testCases, testCase{
+ testType: clientTest,
+ name: "EarlyData-Reject0RTT-DifferentPRF-HRR-Client",
+ config: Config{
+ MaxVersion: VersionTLS13,
+ CipherSuites: []uint16{TLS_AES_128_GCM_SHA256},
+ MaxEarlyDataSize: 16384,
+ },
+ resumeConfig: &Config{
+ MaxVersion: VersionTLS13,
+ MaxEarlyDataSize: 16384,
+ CipherSuites: []uint16{TLS_AES_256_GCM_SHA384},
+ // P-384 requires a HelloRetryRequest against BoringSSL's default
+ // configuration. Assert this with ExpectMissingKeyShare.
+ CurvePreferences: []CurveID{CurveP384},
+ Bugs: ProtocolBugs{
+ ExpectMissingKeyShare: true,
+ },
+ },
+ resumeSession: true,
+ expectResumeRejected: true,
+ flags: []string{
+ "-enable-early-data",
+ "-expect-reject-early-data",
+ "-expect-ticket-supports-early-data",
+ "-on-resume-shim-writes-first",
+ },
+ })
+
+ // Test that the client enforces cipher suite match on 0-RTT accept.
+ testCases = append(testCases, testCase{
+ testType: clientTest,
+ name: "EarlyData-CipherMismatch-Client-TLS13",
+ config: Config{
+ MaxVersion: VersionTLS13,
+ MaxEarlyDataSize: 16384,
+ CipherSuites: []uint16{TLS_AES_128_GCM_SHA256},
+ },
+ resumeConfig: &Config{
+ MaxVersion: VersionTLS13,
+ MaxEarlyDataSize: 16384,
+ CipherSuites: []uint16{TLS_CHACHA20_POLY1305_SHA256},
+ Bugs: ProtocolBugs{
+ AlwaysAcceptEarlyData: true,
+ },
+ },
+ resumeSession: true,
+ flags: []string{
+ "-enable-early-data",
+ "-expect-ticket-supports-early-data",
+ },
+ shouldFail: true,
+ expectedError: ":CIPHER_MISMATCH_ON_EARLY_DATA:",
+ expectedLocalError: "remote error: illegal parameter",
+ })
}
func addTLS13CipherPreferenceTests() {
diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc
index f48d1da..e22c1e1 100644
--- a/ssl/tls13_client.cc
+++ b/ssl/tls13_client.cc
@@ -400,6 +400,7 @@
!ssl_hash_message(hs, msg) ||
!tls13_derive_handshake_secrets(hs) ||
!tls13_set_traffic_key(ssl, ssl_encryption_handshake, evp_aead_open,
+ hs->new_session.get(),
hs->server_handshake_secret())) {
return ssl_hs_error;
}
@@ -408,6 +409,7 @@
// If not sending early data, set client traffic keys now so that alerts are
// encrypted.
if (!tls13_set_traffic_key(ssl, ssl_encryption_handshake, evp_aead_seal,
+ hs->new_session.get(),
hs->client_handshake_secret())) {
return ssl_hs_error;
}
@@ -446,14 +448,20 @@
}
if (ssl->s3->early_data_accepted) {
- if (hs->early_session->cipher != hs->new_session->cipher ||
- MakeConstSpan(hs->early_session->early_alpn) !=
- ssl->s3->alpn_selected) {
+ if (hs->early_session->cipher != hs->new_session->cipher) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_CIPHER_MISMATCH_ON_EARLY_DATA);
+ ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
+ return ssl_hs_error;
+ }
+ if (MakeConstSpan(hs->early_session->early_alpn) !=
+ ssl->s3->alpn_selected) {
OPENSSL_PUT_ERROR(SSL, SSL_R_ALPN_MISMATCH_ON_EARLY_DATA);
+ ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
return ssl_hs_error;
}
if (ssl->s3->channel_id_valid || ssl->s3->token_binding_negotiated) {
OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_EXTENSION_ON_EARLY_DATA);
+ ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
return ssl_hs_error;
}
}
@@ -661,6 +669,7 @@
if (hs->early_data_offered) {
if (!tls13_set_traffic_key(ssl, ssl_encryption_handshake, evp_aead_seal,
+ hs->new_session.get(),
hs->client_handshake_secret())) {
return ssl_hs_error;
}
@@ -756,8 +765,10 @@
// Derive the final keys and enable them.
if (!tls13_set_traffic_key(ssl, ssl_encryption_application, evp_aead_open,
+ hs->new_session.get(),
hs->server_traffic_secret_0()) ||
!tls13_set_traffic_key(ssl, ssl_encryption_application, evp_aead_seal,
+ hs->new_session.get(),
hs->client_traffic_secret_0()) ||
!tls13_derive_resumption_secret(hs)) {
return ssl_hs_error;
diff --git a/ssl/tls13_enc.cc b/ssl/tls13_enc.cc
index 1f6ff64..d0c27b6 100644
--- a/ssl/tls13_enc.cc
+++ b/ssl/tls13_enc.cc
@@ -139,8 +139,8 @@
bool tls13_set_traffic_key(SSL *ssl, enum ssl_encryption_level_t level,
enum evp_aead_direction_t direction,
+ const SSL_SESSION *session,
Span<const uint8_t> traffic_secret) {
- const SSL_SESSION *session = SSL_get_session(ssl);
uint16_t version = ssl_session_protocol_version(session);
UniquePtr<SSLAEADContext> traffic_aead;
@@ -341,11 +341,12 @@
ssl->s3->write_traffic_secret_len);
}
- const EVP_MD *digest = ssl_session_get_digest(SSL_get_session(ssl));
+ const SSL_SESSION *session = SSL_get_session(ssl);
+ const EVP_MD *digest = ssl_session_get_digest(session);
return hkdf_expand_label(secret, digest, secret,
label_to_span(kTLS13LabelApplicationTraffic), {}) &&
tls13_set_traffic_key(ssl, ssl_encryption_application, direction,
- secret);
+ session, secret);
}
static const char kTLS13LabelResumption[] = "res master";
diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc
index fda0bca..f796260 100644
--- a/ssl/tls13_server.cc
+++ b/ssl/tls13_server.cc
@@ -352,6 +352,10 @@
return ssl_hs_error;
}
+ // |ssl_session_is_resumable| forbids cross-cipher resumptions even if the
+ // PRF hashes match.
+ assert(hs->new_cipher == session->cipher);
+
if (!ssl->enable_early_data) {
ssl->s3->early_data_reason = ssl_early_data_disabled;
} else if (session->ticket_max_early_data == 0) {
@@ -600,6 +604,7 @@
// Derive and enable the handshake traffic secrets.
if (!tls13_derive_handshake_secrets(hs) ||
!tls13_set_traffic_key(ssl, ssl_encryption_handshake, evp_aead_seal,
+ hs->new_session.get(),
hs->server_handshake_secret())) {
return ssl_hs_error;
}
@@ -700,6 +705,7 @@
hs, MakeConstSpan(kZeroes, hs->transcript.DigestLen())) ||
!tls13_derive_application_secrets(hs) ||
!tls13_set_traffic_key(ssl, ssl_encryption_application, evp_aead_seal,
+ hs->new_session.get(),
hs->server_traffic_secret_0())) {
return ssl_hs_error;
}
@@ -776,6 +782,7 @@
// QUIC never receives handshake messages under 0-RTT keys.
if (ssl->quic_method == nullptr &&
!tls13_set_traffic_key(ssl, ssl_encryption_early_data, evp_aead_open,
+ hs->new_session.get(),
hs->early_traffic_secret())) {
return ssl_hs_error;
}
@@ -789,6 +796,7 @@
// return.
if (ssl->quic_method != nullptr) {
if (!tls13_set_traffic_key(ssl, ssl_encryption_handshake, evp_aead_open,
+ hs->new_session.get(),
hs->client_handshake_secret())) {
return ssl_hs_error;
}
@@ -821,6 +829,7 @@
ssl->method->next_message(ssl);
}
if (!tls13_set_traffic_key(ssl, ssl_encryption_handshake, evp_aead_open,
+ hs->new_session.get(),
hs->client_handshake_secret())) {
return ssl_hs_error;
}
@@ -931,6 +940,7 @@
!tls13_process_finished(hs, msg, ssl->s3->early_data_accepted) ||
// evp_aead_seal keys have already been switched.
!tls13_set_traffic_key(ssl, ssl_encryption_application, evp_aead_open,
+ hs->new_session.get(),
hs->client_traffic_secret_0())) {
return ssl_hs_error;
}
