Move ssl->version to ssl->s3->version
The version is not user configuration but a property of the connection
that we learn partway during the handshake. That state needs to be
reset on SSL_clear, so it's cleaner to implement this by just making it
naturally reset as part of getting a new ssl->s3. (That really should
just be called SSLConnection or something and be a common interface
between TLS and DTLS, but we'll get to that one later.)
Change-Id: I6eaa4ab1df5cbc6569732ed2aa01d84900410080
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/71530
Reviewed-by: Nick Harper <nharper@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/ssl/d1_lib.cc b/ssl/d1_lib.cc
index 49aa50a..4caadad 100644
--- a/ssl/d1_lib.cc
+++ b/ssl/d1_lib.cc
@@ -108,13 +108,6 @@
}
ssl->d1 = d1.release();
-
- // Set the version to the highest supported version.
- //
- // TODO(davidben): Move this field into |s3|, have it store the normalized
- // protocol version, and implement this pre-negotiation quirk in |SSL_version|
- // at the API boundary rather than in internal state.
- ssl->version = DTLS1_2_VERSION;
return true;
}
diff --git a/ssl/handoff.cc b/ssl/handoff.cc
index 9417eb9..2032042 100644
--- a/ssl/handoff.cc
+++ b/ssl/handoff.cc
@@ -329,7 +329,7 @@
const uint8_t *write_iv = nullptr;
if ((type == handback_after_session_resumption ||
type == handback_after_handshake) &&
- ssl->version == TLS1_VERSION &&
+ ssl->s3->version == TLS1_VERSION &&
SSL_CIPHER_is_block_cipher(s3->aead_write_ctx->cipher()) &&
!s3->aead_write_ctx->GetIV(&write_iv, &write_iv_len)) {
return false;
@@ -337,7 +337,7 @@
size_t read_iv_len = 0;
const uint8_t *read_iv = nullptr;
if (type == handback_after_handshake &&
- ssl->version == TLS1_VERSION &&
+ ssl->s3->version == TLS1_VERSION &&
SSL_CIPHER_is_block_cipher(s3->aead_read_ctx->cipher()) &&
!s3->aead_read_ctx->GetIV(&read_iv, &read_iv_len)) {
return false;
@@ -637,9 +637,9 @@
s3->early_data_reason = ssl_early_data_protocol_version;
}
- ssl->version = session->ssl_version;
+ ssl->s3->version = session->ssl_version;
s3->have_version = true;
- if (!ssl_method_supports_version(ssl->method, ssl->version) ||
+ if (!ssl_method_supports_version(ssl->method, ssl->s3->version) ||
session->cipher != hs->new_cipher ||
ssl_protocol_version(ssl) < SSL_CIPHER_get_min_version(session->cipher) ||
SSL_CIPHER_get_max_version(session->cipher) < ssl_protocol_version(ssl)) {
@@ -690,7 +690,7 @@
hs->wait = ssl_hs_flush;
hs->extended_master_secret = extended_master_secret;
hs->ticket_expected = ticket_expected;
- s3->aead_write_ctx->SetVersionIfNullCipher(ssl->version);
+ s3->aead_write_ctx->SetVersionIfNullCipher(ssl->s3->version);
hs->cert_request = cert_request;
if (type != handback_after_handshake &&
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index 976e969..be24ea0 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -725,12 +725,12 @@
assert(ssl->s3->have_version == ssl->s3->initial_handshake_complete);
if (!ssl->s3->have_version) {
- ssl->version = server_version;
- // At this point, the connection's version is known and ssl->version is
+ ssl->s3->version = server_version;
+ // At this point, the connection's version is known and ssl->s3->version is
// fixed. Begin enforcing the record-layer version.
ssl->s3->have_version = true;
- ssl->s3->aead_write_ctx->SetVersionIfNullCipher(ssl->version);
- } else if (server_version != ssl->version) {
+ ssl->s3->aead_write_ctx->SetVersionIfNullCipher(ssl->s3->version);
+ } else if (server_version != ssl->s3->version) {
OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_SSL_VERSION);
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_PROTOCOL_VERSION);
return ssl_hs_error;
@@ -829,7 +829,7 @@
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
return ssl_hs_error;
}
- if (ssl->session->ssl_version != ssl->version) {
+ if (ssl->session->ssl_version != ssl->s3->version) {
OPENSSL_PUT_ERROR(SSL, SSL_R_OLD_SESSION_VERSION_NOT_RETURNED);
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
return ssl_hs_error;
diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc
index 148a29c..1c433ce 100644
--- a/ssl/handshake_server.cc
+++ b/ssl/handshake_server.cc
@@ -241,14 +241,14 @@
}
}
- if (!ssl_negotiate_version(hs, out_alert, &ssl->version, &versions)) {
+ if (!ssl_negotiate_version(hs, out_alert, &ssl->s3->version, &versions)) {
return false;
}
- // At this point, the connection's version is known and |ssl->version| is
+ // At this point, the connection's version is known and |ssl->s3->version| is
// fixed. Begin enforcing the record-layer version.
ssl->s3->have_version = true;
- ssl->s3->aead_write_ctx->SetVersionIfNullCipher(ssl->version);
+ ssl->s3->aead_write_ctx->SetVersionIfNullCipher(ssl->s3->version);
// Handle FALLBACK_SCSV.
if (ssl_client_cipher_list_contains_cipher(client_hello,
@@ -1086,7 +1086,7 @@
ScopedCBB cbb;
CBB body, session_id_bytes;
if (!ssl->method->init_message(ssl, cbb.get(), &body, SSL3_MT_SERVER_HELLO) ||
- !CBB_add_u16(&body, ssl->version) ||
+ !CBB_add_u16(&body, ssl->s3->version) ||
!CBB_add_bytes(&body, ssl->s3->server_random, SSL3_RANDOM_SIZE) ||
!CBB_add_u8_length_prefixed(&body, &session_id_bytes) ||
!CBB_add_bytes(&session_id_bytes, session_id.data(), session_id.size()) ||
diff --git a/ssl/internal.h b/ssl/internal.h
index 8038e51..3d6b419 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -2854,6 +2854,11 @@
enum ssl_encryption_level_t read_level = ssl_encryption_initial;
enum ssl_encryption_level_t write_level = ssl_encryption_initial;
+ // version is the protocol version, or zero if the version has not yet been
+ // set.
+ // TODO(davidben): This is redundant with |have_version|.
+ uint16_t version = 0;
+
// early_data_skipped is the amount of early data that has been skipped by the
// record layer.
uint16_t early_data_skipped = 0;
@@ -3950,9 +3955,6 @@
// that instead, and skip the null check.)
bssl::UniquePtr<bssl::SSL_CONFIG> config;
- // version is the protocol version.
- uint16_t version = 0;
-
uint16_t max_send_fragment = 0;
// There are 2 BIO's even though they are normally both the same. This is so
diff --git a/ssl/s3_lib.cc b/ssl/s3_lib.cc
index b8b37b4..8b822e2 100644
--- a/ssl/s3_lib.cc
+++ b/ssl/s3_lib.cc
@@ -196,13 +196,6 @@
}
ssl->s3 = s3.release();
-
- // Set the version to the highest supported version.
- //
- // TODO(davidben): Move this field into |s3|, have it store the normalized
- // protocol version, and implement this pre-negotiation quirk in |SSL_version|
- // at the API boundary rather than in internal state.
- ssl->version = TLS1_2_VERSION;
return true;
}
diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc
index e64243e..92bd822 100644
--- a/ssl/ssl_session.cc
+++ b/ssl/ssl_session.cc
@@ -362,7 +362,7 @@
}
session->is_server = ssl->server;
- session->ssl_version = ssl->version;
+ session->ssl_version = ssl->s3->version;
session->is_quic = ssl->quic_method != nullptr;
// Fill in the time from the |SSL_CTX|'s clock.
@@ -616,7 +616,7 @@
ssl_session_is_time_valid(ssl, session) &&
// Only resume if the session's version matches the negotiated
// version.
- ssl->version == session->ssl_version &&
+ ssl->s3->version == session->ssl_version &&
// 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
diff --git a/ssl/ssl_versions.cc b/ssl/ssl_versions.cc
index c521a61..13ad4d1 100644
--- a/ssl/ssl_versions.cc
+++ b/ssl/ssl_versions.cc
@@ -254,14 +254,19 @@
if (SSL_in_early_data(ssl) && !ssl->server) {
return ssl->s3->hs->early_session->ssl_version;
}
- return ssl->version;
+ if (ssl->s3->have_version) {
+ return ssl->s3->version;
+ }
+ // The TLS versions has not yet been negotiated. Historically, we would return
+ // (D)TLS 1.2, so preserve that behavior.
+ return SSL_is_dtls(ssl) ? DTLS1_2_VERSION : TLS1_2_VERSION;
}
uint16_t ssl_protocol_version(const SSL *ssl) {
assert(ssl->s3->have_version);
uint16_t version;
- if (!ssl_protocol_version_from_wire(&version, ssl->version)) {
- // |ssl->version| will always be set to a valid version.
+ if (!ssl_protocol_version_from_wire(&version, ssl->s3->version)) {
+ // |ssl->s3->version| will always be set to a valid version.
assert(0);
return 0;
}
diff --git a/ssl/t1_enc.cc b/ssl/t1_enc.cc
index a985d38..82d1600 100644
--- a/ssl/t1_enc.cc
+++ b/ssl/t1_enc.cc
@@ -244,7 +244,7 @@
}
UniquePtr<SSLAEADContext> aead_ctx =
- SSLAEADContext::Create(direction, ssl->version, SSL_is_dtls(ssl),
+ SSLAEADContext::Create(direction, ssl->s3->version, SSL_is_dtls(ssl),
session->cipher, key, mac_secret, iv);
if (!aead_ctx) {
return false;
diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc
index 78aa246..644be6a 100644
--- a/ssl/tls13_client.cc
+++ b/ssl/tls13_client.cc
@@ -86,7 +86,7 @@
/*secret_for_quic=*/{})) {
return false;
}
- ssl->s3->aead_write_ctx->SetVersionIfNullCipher(ssl->version);
+ ssl->s3->aead_write_ctx->SetVersionIfNullCipher(ssl->s3->version);
} else {
assert(level == ssl_encryption_handshake);
if (!tls13_set_traffic_key(ssl, ssl_encryption_handshake, evp_aead_seal,
@@ -437,7 +437,7 @@
if (!supported_versions.present ||
!CBS_get_u16(&supported_versions.data, &version) ||
CBS_len(&supported_versions.data) != 0 ||
- version != ssl->version) {
+ version != ssl->s3->version) {
OPENSSL_PUT_ERROR(SSL, SSL_R_SECOND_SERVERHELLO_VERSION_MISMATCH);
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
return ssl_hs_error;
@@ -451,7 +451,7 @@
return ssl_hs_error;
}
- if (ssl->session->ssl_version != ssl->version) {
+ if (ssl->session->ssl_version != ssl->s3->version) {
OPENSSL_PUT_ERROR(SSL, SSL_R_OLD_SESSION_VERSION_NOT_RETURNED);
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
return ssl_hs_error;
diff --git a/ssl/tls13_enc.cc b/ssl/tls13_enc.cc
index 7c193a3..d4193fd 100644
--- a/ssl/tls13_enc.cc
+++ b/ssl/tls13_enc.cc
@@ -404,7 +404,7 @@
size_t context_hash_len;
if (!hs->transcript.GetHash(context_hash, &context_hash_len) ||
!tls13_verify_data(out, out_len, hs->transcript.Digest(),
- hs->ssl->version, traffic_secret,
+ hs->ssl->s3->version, traffic_secret,
MakeConstSpan(context_hash, context_hash_len),
SSL_is_dtls(hs->ssl))) {
return false;
diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc
index c82fdea..e163d70 100644
--- a/ssl/tls13_server.cc
+++ b/ssl/tls13_server.cc
@@ -101,7 +101,7 @@
CBB contents;
if (!CBB_add_u16(out, TLSEXT_TYPE_supported_versions) ||
!CBB_add_u16_length_prefixed(out, &contents) ||
- !CBB_add_u16(&contents, hs->ssl->version) ||
+ !CBB_add_u16(&contents, hs->ssl->s3->version) ||
!CBB_flush(out)) {
return 0;
}
@@ -615,7 +615,7 @@
!CBB_add_u16_length_prefixed(&body, &extensions) ||
!CBB_add_u16(&extensions, TLSEXT_TYPE_supported_versions) ||
!CBB_add_u16(&extensions, 2 /* length */) ||
- !CBB_add_u16(&extensions, ssl->version) ||
+ !CBB_add_u16(&extensions, ssl->s3->version) ||
!CBB_add_u16(&extensions, TLSEXT_TYPE_key_share) ||
!CBB_add_u16(&extensions, 2 /* length */) ||
!CBB_add_u16(&extensions, hs->new_session->group_id)) {
