Compute the ECH GREASE payload outside of the callbacks.
This is kinda annoying and, like the grease_seed, demonstrates a
shortcoming with the idea of making add_clienthello completely const.
Strictly speaking, we only need idempotence. But I think this is okay:
const is much easier to enforce than idempotence, and we'll likely need
to do this anyway:
- While not in the current draft, I expect the draft's eventual HRR
resolution to retain the ECH extension, GREASE or not, on ECH reject.
Things are somewhat violating RFC8446 HRR rules right now. That means
we'll need to stash the ECH payload regardless.
- ECH binds all the other extensions in the outer ClientHello, so
computing the payload will need to move outside the callback system
anyway.
In some sense, all this is shifting our ClientHello output from the
"immediate mode" end of the spectrum (as we usually use) to the
"retained mode" end, which I suppose makes sense as this message becomes
more intricate.
Bug: 275
Change-Id: I9eb8cd1cde2ce264345b6ed3ee526d4eab81e911
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47990
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index 7607d56..710b036 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -498,6 +498,7 @@
}
if (!ssl_setup_key_shares(hs, /*override_group_id=*/0) ||
+ !ssl_setup_ech_grease(hs) ||
!ssl_write_client_hello(hs)) {
return ssl_hs_error;
}
diff --git a/ssl/internal.h b/ssl/internal.h
index b3e702b..37a6b2c 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1505,6 +1505,10 @@
SSL_HANDSHAKE *hs, bssl::Span<uint8_t> out,
bssl::Span<const uint8_t> server_hello_ech_conf);
+// ssl_setup_ech_grease computes a GREASE ECH extension to offer instead of a
+// real one. It returns true on success and false otherwise.
+bool ssl_setup_ech_grease(SSL_HANDSHAKE *hs);
+
// Delegated credentials.
@@ -1757,8 +1761,7 @@
// cookie is the value of the cookie received from the server, if any.
Array<uint8_t> cookie;
- // ech_grease contains the bytes of the GREASE ECH extension that was sent in
- // the first ClientHello.
+ // ech_grease contains the GREASE ECH extension to send in the ClientHello.
Array<uint8_t> ech_grease;
// ech_client_hello_buf, on the server, contains the bytes of the
diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc
index 865d33a..57c9fd5 100644
--- a/ssl/t1_lib.cc
+++ b/ssl/t1_lib.cc
@@ -605,18 +605,8 @@
return value % (max - min + 1) + min;
}
-static bool ext_ech_add_clienthello_grease(SSL_HANDSHAKE *hs, CBB *out) {
- // If we are responding to the server's HelloRetryRequest, we repeat the bytes
- // of the first ECH GREASE extension.
- if (hs->ssl->s3->used_hello_retry_request) {
- CBB ech_body;
- if (!CBB_add_u16(out, TLSEXT_TYPE_encrypted_client_hello) ||
- !CBB_add_u16_length_prefixed(out, &ech_body) ||
- !CBB_add_bytes(&ech_body, hs->ech_grease.data(),
- hs->ech_grease.size()) ||
- !CBB_flush(out)) {
- return false;
- }
+bool ssl_setup_ech_grease(SSL_HANDSHAKE *hs) {
+ if (hs->max_version < TLS1_3_VERSION || !hs->config->ech_grease_enabled) {
return true;
}
@@ -650,32 +640,23 @@
// TODO(davidben): If the padding scheme changes to also round the entire
// payload, adjust this to match. See
// https://github.com/tlswg/draft-ietf-tls-esni/issues/433
- uint8_t payload[196 + kAEADOverhead];
const size_t payload_len = random_size(128, 196) + kAEADOverhead;
- assert(payload_len <= sizeof(payload));
- RAND_bytes(payload, payload_len);
-
- // Inside the TLS extension contents, write a serialized ClientEncryptedCH.
- CBB ech_body, enc_cbb, payload_cbb;
- if (!CBB_add_u16(out, TLSEXT_TYPE_encrypted_client_hello) ||
- !CBB_add_u16_length_prefixed(out, &ech_body) ||
- !CBB_add_u16(&ech_body, kdf_id) || //
- !CBB_add_u16(&ech_body, aead_id) ||
- !CBB_add_u8(&ech_body, ech_config_id) ||
- !CBB_add_u16_length_prefixed(&ech_body, &enc_cbb) ||
+ bssl::ScopedCBB cbb;
+ CBB enc_cbb, payload_cbb;
+ uint8_t *payload;
+ if (!CBB_init(cbb.get(), 64 + payload_len) ||
+ !CBB_add_u16(cbb.get(), kdf_id) || //
+ !CBB_add_u16(cbb.get(), aead_id) ||
+ !CBB_add_u8(cbb.get(), ech_config_id) ||
+ !CBB_add_u16_length_prefixed(cbb.get(), &enc_cbb) ||
!CBB_add_bytes(&enc_cbb, ech_enc, OPENSSL_ARRAY_SIZE(ech_enc)) ||
- !CBB_add_u16_length_prefixed(&ech_body, &payload_cbb) ||
- !CBB_add_bytes(&payload_cbb, payload, payload_len) || //
- !CBB_flush(&ech_body)) {
+ !CBB_add_u16_length_prefixed(cbb.get(), &payload_cbb) ||
+ !CBB_add_space(&payload_cbb, &payload, payload_len) ||
+ !RAND_bytes(payload, payload_len) ||
+ !CBBFinishArray(cbb.get(), &hs->ech_grease)) {
return false;
}
- // Save the bytes of the newly-generated extension in case the server sends
- // a HelloRetryRequest.
- if (!hs->ech_grease.CopyFrom(
- MakeConstSpan(CBB_data(&ech_body), CBB_len(&ech_body)))) {
- return false;
- }
- return CBB_flush(out);
+ return true;
}
static bool ext_ech_add_clienthello(SSL_HANDSHAKE *hs, CBB *out) {
@@ -683,7 +664,16 @@
return true;
}
if (hs->config->ech_grease_enabled) {
- return ext_ech_add_clienthello_grease(hs, out);
+ assert(!hs->ech_grease.empty());
+ CBB ech_body;
+ if (!CBB_add_u16(out, TLSEXT_TYPE_encrypted_client_hello) ||
+ !CBB_add_u16_length_prefixed(out, &ech_body) ||
+ !CBB_add_bytes(&ech_body, hs->ech_grease.data(),
+ hs->ech_grease.size()) ||
+ !CBB_flush(out)) {
+ return false;
+ }
+ return true;
}
// Nothing to do, since we don't yet implement the non-GREASE parts of ECH.
return true;
