Manage Channel ID handshake state better.
The channel_id_valid bit is both used for whether channel_id is filled
in (SSL_get_tls_channel_id), and whether this particular handshake will
eventually negotiate Channel ID.
The former means that, if SSL_get_tls_channel_id is called on the
client, we'll return all zeros. Apparently we never fill in channel_id
on the client at all. The latter means the state needs to be reset on
renegotiation because we do not currently forbid renegotiation with
Channel ID (we probably should...), which is the last use of the init
callback for extensions.
Instead, split this into a bit for the handshake and a bit for the
connection. Note this means we actually do not expose or even retain
whether Channel ID was used on the client.
This requires a tweak to the handoff logic, but it should be compatible.
The serialized ssl->s3->channel_id was always a no-op: the handback
happens before the ChannelID message, except in RSA key exchange. But we
forbid Channel ID in RSA key exchange anyway.
Update-Note: SSL_get_tls_channel_id will no longer return all zeros
during the handshake or on the client. I did not find any callers
relying on this.
Change-Id: Icd4b78dd3f311d1c7dfc1cae7d2b86dc7e327a99
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47906
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 0981f15..cad9750 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -2996,11 +2996,12 @@
// success and zero on error.
OPENSSL_EXPORT int SSL_set1_tls_channel_id(SSL *ssl, EVP_PKEY *private_key);
-// SSL_get_tls_channel_id gets the client's TLS Channel ID from a server |SSL*|
+// SSL_get_tls_channel_id gets the client's TLS Channel ID from a server |SSL|
// and copies up to the first |max_out| bytes into |out|. The Channel ID
// consists of the client's P-256 public key as an (x,y) pair where each is a
// 32-byte, big-endian field element. It returns 0 if the client didn't offer a
-// Channel ID and the length of the complete Channel ID otherwise.
+// Channel ID and the length of the complete Channel ID otherwise. This function
+// always returns zero if |ssl| is a client.
OPENSSL_EXPORT size_t SSL_get_tls_channel_id(SSL *ssl, uint8_t *out,
size_t max_out);
diff --git a/ssl/handoff.cc b/ssl/handoff.cc
index 3ac29c6..5f836fa 100644
--- a/ssl/handoff.cc
+++ b/ssl/handoff.cc
@@ -338,6 +338,7 @@
} else {
session = s3->session_reused ? ssl->session.get() : hs->new_session.get();
}
+ static const uint8_t kUnusedChannelID[64] = {0};
if (!CBB_add_asn1(out, &seq, CBS_ASN1_SEQUENCE) ||
!CBB_add_asn1_uint64(&seq, kHandbackVersion) ||
!CBB_add_asn1_uint64(&seq, type) ||
@@ -352,7 +353,7 @@
!CBB_add_asn1_octet_string(&seq, read_iv, read_iv_len) ||
!CBB_add_asn1_octet_string(&seq, write_iv, write_iv_len) ||
!CBB_add_asn1_bool(&seq, s3->session_reused) ||
- !CBB_add_asn1_bool(&seq, s3->channel_id_valid) ||
+ !CBB_add_asn1_bool(&seq, hs->channel_id_negotiated) ||
!ssl_session_serialize(session, &seq) ||
!CBB_add_asn1_octet_string(&seq, s3->next_proto_negotiated.data(),
s3->next_proto_negotiated.size()) ||
@@ -361,8 +362,8 @@
!CBB_add_asn1_octet_string(
&seq, reinterpret_cast<uint8_t *>(s3->hostname.get()),
hostname_len) ||
- !CBB_add_asn1_octet_string(&seq, s3->channel_id,
- sizeof(s3->channel_id)) ||
+ !CBB_add_asn1_octet_string(&seq, kUnusedChannelID,
+ sizeof(kUnusedChannelID)) ||
// These two fields were historically |token_binding_negotiated| and
// |negotiated_token_binding_param|.
!CBB_add_asn1_bool(&seq, 0) ||
@@ -448,9 +449,10 @@
uint64_t handback_version, unused_token_binding_param, cipher, type_u64;
CBS seq, read_seq, write_seq, server_rand, client_rand, read_iv, write_iv,
- next_proto, alpn, hostname, channel_id, transcript, key_share;
- int session_reused, channel_id_valid, cert_request, extended_master_secret,
- ticket_expected, unused_token_binding, next_proto_neg_seen;
+ next_proto, alpn, hostname, unused_channel_id, transcript, key_share;
+ int session_reused, channel_id_negotiated, cert_request,
+ extended_master_secret, ticket_expected, unused_token_binding,
+ next_proto_neg_seen;
SSL_SESSION *session = nullptr;
CBS handback_cbs(handback);
@@ -478,7 +480,7 @@
!CBS_get_asn1(&seq, &read_iv, CBS_ASN1_OCTETSTRING) ||
!CBS_get_asn1(&seq, &write_iv, CBS_ASN1_OCTETSTRING) ||
!CBS_get_asn1_bool(&seq, &session_reused) ||
- !CBS_get_asn1_bool(&seq, &channel_id_valid)) {
+ !CBS_get_asn1_bool(&seq, &channel_id_negotiated)) {
return false;
}
@@ -497,10 +499,7 @@
if (!session || !CBS_get_asn1(&seq, &next_proto, CBS_ASN1_OCTETSTRING) ||
!CBS_get_asn1(&seq, &alpn, CBS_ASN1_OCTETSTRING) ||
!CBS_get_asn1(&seq, &hostname, CBS_ASN1_OCTETSTRING) ||
- !CBS_get_asn1(&seq, &channel_id, CBS_ASN1_OCTETSTRING) ||
- CBS_len(&channel_id) != sizeof(s3->channel_id) ||
- !CBS_copy_bytes(&channel_id, s3->channel_id,
- sizeof(s3->channel_id)) ||
+ !CBS_get_asn1(&seq, &unused_channel_id, CBS_ASN1_OCTETSTRING) ||
!CBS_get_asn1_bool(&seq, &unused_token_binding) ||
!CBS_get_asn1_uint64(&seq, &unused_token_binding_param) ||
!CBS_get_asn1_bool(&seq, &next_proto_neg_seen) ||
@@ -616,7 +615,7 @@
return false;
}
s3->session_reused = session_reused;
- s3->channel_id_valid = channel_id_valid;
+ hs->channel_id_negotiated = channel_id_negotiated;
s3->next_proto_negotiated.CopyFrom(next_proto);
s3->alpn_selected.CopyFrom(alpn);
diff --git a/ssl/handshake.cc b/ssl/handshake.cc
index e628000..ca2238f 100644
--- a/ssl/handshake.cc
+++ b/ssl/handshake.cc
@@ -152,7 +152,8 @@
hints_requested(false),
cert_compression_negotiated(false),
apply_jdk11_workaround(false),
- can_release_private_key(false) {
+ can_release_private_key(false),
+ channel_id_negotiated(false) {
assert(ssl);
}
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index 7e2fbb5..b7432ec 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -1481,7 +1481,7 @@
SSL *const ssl = hs->ssl;
hs->can_release_private_key = true;
// Resolve Channel ID first, before any non-idempotent operations.
- if (ssl->s3->channel_id_valid) {
+ if (hs->channel_id_negotiated) {
if (!ssl_do_channel_id_callback(hs)) {
return ssl_hs_error;
}
@@ -1516,7 +1516,7 @@
}
}
- if (ssl->s3->channel_id_valid) {
+ if (hs->channel_id_negotiated) {
ScopedCBB cbb;
CBB body;
if (!ssl->method->init_message(ssl, cbb.get(), &body, SSL3_MT_CHANNEL_ID) ||
diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc
index 24dbeae..2d1cc38 100644
--- a/ssl/handshake_server.cc
+++ b/ssl/handshake_server.cc
@@ -926,7 +926,7 @@
hs->cert_request = !!(hs->config->verify_mode & SSL_VERIFY_PEER);
// Only request a certificate if Channel ID isn't negotiated.
if ((hs->config->verify_mode & SSL_VERIFY_PEER_IF_NO_OBC) &&
- ssl->s3->channel_id_valid) {
+ hs->channel_id_negotiated) {
hs->cert_request = false;
}
// CertificateRequest may only be sent in certificate-based ciphers.
@@ -980,9 +980,9 @@
// We only accept ChannelIDs on connections with ECDHE in order to avoid a
// known attack while we fix ChannelID itself.
- if (ssl->s3->channel_id_valid &&
+ if (hs->channel_id_negotiated &&
(hs->new_cipher->algorithm_mkey & SSL_kECDHE) == 0) {
- ssl->s3->channel_id_valid = false;
+ hs->channel_id_negotiated = false;
}
// If this is a resumption and the original handshake didn't support
@@ -990,7 +990,7 @@
// session and so cannot resume with ChannelIDs.
if (ssl->session != NULL &&
ssl->session->original_handshake_hash_len == 0) {
- ssl->s3->channel_id_valid = false;
+ hs->channel_id_negotiated = false;
}
struct OPENSSL_timeval now;
@@ -1681,7 +1681,7 @@
static enum ssl_hs_wait_t do_read_channel_id(SSL_HANDSHAKE *hs) {
SSL *const ssl = hs->ssl;
- if (!ssl->s3->channel_id_valid) {
+ if (!hs->channel_id_negotiated) {
hs->state = state12_read_client_finished;
return ssl_hs_ok;
}
diff --git a/ssl/internal.h b/ssl/internal.h
index b572931..7be766d 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1958,6 +1958,10 @@
// in this handshake.
bool can_release_private_key : 1;
+ // channel_id_negotiated is true if Channel ID should be used in this
+ // handshake.
+ bool channel_id_negotiated : 1;
+
// client_version is the value sent or received in the ClientHello version.
uint16_t client_version = 0;
@@ -2562,9 +2566,8 @@
bool send_connection_binding : 1;
- // In a client, this means that the server supported Channel ID and that a
- // Channel ID was sent. In a server it means that we echoed support for
- // Channel IDs and that |channel_id| will be valid after the handshake.
+ // channel_id_valid is true if, on the server, the client has negotiated a
+ // Channel ID and the |channel_id| field is filled in.
bool channel_id_valid : 1;
// key_update_pending is true if we have a KeyUpdate acknowledgment
diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc
index f6ba368..a63ca5d 100644
--- a/ssl/t1_lib.cc
+++ b/ssl/t1_lib.cc
@@ -1641,10 +1641,6 @@
//
// https://tools.ietf.org/html/draft-balfanz-tls-channelid-01
-static void ext_channel_id_init(SSL_HANDSHAKE *hs) {
- hs->ssl->s3->channel_id_valid = false;
-}
-
static bool ext_channel_id_add_clienthello(SSL_HANDSHAKE *hs, CBB *out) {
SSL *const ssl = hs->ssl;
if (!hs->config->channel_id_enabled || SSL_is_dtls(ssl)) {
@@ -1662,19 +1658,18 @@
static bool ext_channel_id_parse_serverhello(SSL_HANDSHAKE *hs,
uint8_t *out_alert,
CBS *contents) {
- SSL *const ssl = hs->ssl;
if (contents == NULL) {
return true;
}
- assert(!SSL_is_dtls(ssl));
+ assert(!SSL_is_dtls(hs->ssl));
assert(hs->config->channel_id_enabled);
if (CBS_len(contents) != 0) {
return false;
}
- ssl->s3->channel_id_valid = true;
+ hs->channel_id_negotiated = true;
return true;
}
@@ -1690,13 +1685,12 @@
return false;
}
- ssl->s3->channel_id_valid = true;
+ hs->channel_id_negotiated = true;
return true;
}
static bool ext_channel_id_add_serverhello(SSL_HANDSHAKE *hs, CBB *out) {
- SSL *const ssl = hs->ssl;
- if (!ssl->s3->channel_id_valid) {
+ if (!hs->channel_id_negotiated) {
return true;
}
@@ -3195,7 +3189,7 @@
},
{
TLSEXT_TYPE_channel_id,
- ext_channel_id_init,
+ NULL,
ext_channel_id_add_clienthello,
ext_channel_id_parse_serverhello,
ext_channel_id_parse_clienthello,
@@ -4080,11 +4074,11 @@
if (!sig_ok) {
OPENSSL_PUT_ERROR(SSL, SSL_R_CHANNEL_ID_SIGNATURE_INVALID);
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECRYPT_ERROR);
- ssl->s3->channel_id_valid = false;
return false;
}
OPENSSL_memcpy(ssl->s3->channel_id, p, 64);
+ ssl->s3->channel_id_valid = true;
return true;
}
diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc
index 8ba6b4d..bac37ff 100644
--- a/ssl/tls13_client.cc
+++ b/ssl/tls13_client.cc
@@ -503,7 +503,7 @@
}
// Channel ID is incompatible with 0-RTT. The ALPS extension should be
// negotiated implicitly.
- if (ssl->s3->channel_id_valid ||
+ if (hs->channel_id_negotiated ||
hs->new_session->has_application_settings) {
OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_EXTENSION_ON_EARLY_DATA);
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
@@ -820,7 +820,7 @@
hs->can_release_private_key = true;
// Send a Channel ID assertion if necessary.
- if (ssl->s3->channel_id_valid) {
+ if (hs->channel_id_negotiated) {
if (!ssl_do_channel_id_callback(hs)) {
hs->tls13_state = state_complete_second_flight;
return ssl_hs_error;
diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc
index c2a66a8..2dc2856 100644
--- a/ssl/tls13_server.cc
+++ b/ssl/tls13_server.cc
@@ -445,7 +445,7 @@
ssl->s3->early_data_reason = ssl_early_data_unsupported_for_session;
} else if (!hs->early_data_offered) {
ssl->s3->early_data_reason = ssl_early_data_peer_declined;
- } else if (ssl->s3->channel_id_valid) {
+ } else if (hs->channel_id_negotiated) {
// Channel ID is incompatible with 0-RTT.
ssl->s3->early_data_reason = ssl_early_data_channel_id;
} else if (MakeConstSpan(ssl->s3->alpn_selected) != session->early_alpn) {
@@ -820,7 +820,7 @@
hs->cert_request = !!(hs->config->verify_mode & SSL_VERIFY_PEER);
// Only request a certificate if Channel ID isn't negotiated.
if ((hs->config->verify_mode & SSL_VERIFY_PEER_IF_NO_OBC) &&
- ssl->s3->channel_id_valid) {
+ hs->channel_id_negotiated) {
hs->cert_request = false;
}
}
@@ -1156,7 +1156,7 @@
static enum ssl_hs_wait_t do_read_channel_id(SSL_HANDSHAKE *hs) {
SSL *const ssl = hs->ssl;
- if (!ssl->s3->channel_id_valid) {
+ if (!hs->channel_id_negotiated) {
hs->tls13_state = state13_read_client_finished;
return ssl_hs_ok;
}
