From 49ee62fe13adacfc89d3d612f79e3a8425b500c5 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 20 May 2021 10:34:59 -0400 Subject: [PATCH] Update the ECH GREASE size selection. We misread (or maybe it changed?) the draft padding scheme. The current text does not round the whole payload to a multiple of 32, just the server name as a fallback. Switch the GREASE size selection to match. Although, we may want to change the draft here. See also https://github.com/tlswg/draft-ietf-tls-esni/issues/433 While I'm here, update some references from draft-09 to draft-10. Also make the comment less verbose. Bug: 275 Change-Id: I3c9f34159890bc3b7d71f6877f34b895bc7f9b17 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47644 Reviewed-by: David Benjamin Commit-Queue: David Benjamin --- include/openssl/ssl.h | 2 +- ssl/encrypted_client_hello.cc | 2 +- ssl/t1_lib.cc | 60 +++++++++------------------ ssl/test/runner/handshake_messages.go | 2 +- ssl/tls13_enc.cc | 2 +- 5 files changed, 23 insertions(+), 45 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 0d3d944be7..3ef6d8d7e5 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -3573,7 +3573,7 @@ OPENSSL_EXPORT const char *SSL_early_data_reason_string( // // ECH support in BoringSSL is still experimental and under development. // -// See https://tools.ietf.org/html/draft-ietf-tls-esni-09. +// See https://tools.ietf.org/html/draft-ietf-tls-esni-10. // SSL_set_enable_ech_grease configures whether the client may send ECH GREASE // as part of this connection. diff --git a/ssl/encrypted_client_hello.cc b/ssl/encrypted_client_hello.cc index 94179767f8..5aeb1b46ad 100644 --- a/ssl/encrypted_client_hello.cc +++ b/ssl/encrypted_client_hello.cc @@ -273,7 +273,7 @@ bool ssl_client_hello_decrypt( *out_is_decrypt_error = false; // Compute the ClientHello portion of the ClientHelloOuterAAD value. See - // draft-ietf-tls-esni-09, section 5.2. + // draft-ietf-tls-esni-10, section 5.2. ScopedCBB ch_outer_aad_cbb; CBB enc_cbb, outer_hello_cbb, extensions_cbb; if (!CBB_init(ch_outer_aad_cbb.get(), 0) || diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc index 0a7ffb080d..970e7ba440 100644 --- a/ssl/t1_lib.cc +++ b/ssl/t1_lib.cc @@ -593,7 +593,7 @@ static bool ext_sni_add_serverhello(SSL_HANDSHAKE *hs, CBB *out) { // Encrypted ClientHello (ECH) // -// https://tools.ietf.org/html/draft-ietf-tls-esni-09 +// https://tools.ietf.org/html/draft-ietf-tls-esni-10 // random_size returns a random value between |min| and |max|, inclusive. static size_t random_size(size_t min, size_t max) { @@ -629,48 +629,26 @@ static bool ext_ech_add_clienthello_grease(SSL_HANDSHAKE *hs, CBB *out) { uint8_t private_key_unused[X25519_PRIVATE_KEY_LEN]; X25519_keypair(ech_enc, private_key_unused); - // To determine a plausible length for the payload, we first estimate the size - // of a typical EncodedClientHelloInner, with an expected use of - // outer_extensions. To limit the size, we only consider initial ClientHellos - // that do not offer resumption. + // To determine a plausible length for the payload, we estimate the size of a + // typical EncodedClientHelloInner without resumption: // - // Field/Extension Size - // --------------------------------------------------------------------- - // version 2 - // random 32 - // legacy_session_id 1 - // - Has a U8 length prefix, but body is - // always empty string in inner CH. - // cipher_suites 2 (length prefix) - // - Only includes TLS 1.3 ciphers (3). 6 - // - Maybe also include a GREASE suite. 2 - // legacy_compression_methods 2 (length prefix) - // - Always has "null" compression method. 1 - // extensions: 2 (length prefix) - // - encrypted_client_hello (empty). 4 (id + length prefix) - // - supported_versions. 4 (id + length prefix) - // - U8 length prefix 1 - // - U16 protocol version (TLS 1.3) 2 - // - outer_extensions. 4 (id + length prefix) - // - U8 length prefix 1 - // - N extension IDs (2 bytes each): - // - key_share 2 - // - sigalgs 2 - // - sct 2 - // - alpn 2 - // - supported_groups. 2 - // - status_request. 2 - // - psk_key_exchange_modes. 2 - // - compress_certificate. 2 + // 2+32+1+2 version, random, legacy_session_id, legacy_compression_methods + // 2+4*2 cipher_suites (three TLS 1.3 ciphers, GREASE) + // 2 extensions prefix + // 4 ech_is_inner + // 4+1+2*2 supported_versions (TLS 1.3, GREASE) + // 4+1+10*2 outer_extensions (key_share, sigalgs, sct, alpn, + // supported_groups, status_request, psk_key_exchange_modes, + // compress_certificate, GREASE x2) // - // The server_name extension has an overhead of 9 bytes, plus up to an - // estimated 100 bytes of hostname. Rounding up to a multiple of 32 yields a - // range of 96 to 192. Note that this estimate does not fully capture - // optional extensions like GREASE, but the rounding gives some leeway. - - uint8_t payload[kAEADOverhead + 192]; - const size_t payload_len = - kAEADOverhead + 32 * random_size(96 / 32, 192 / 32); + // The server_name extension has an overhead of 9 bytes. For now, arbitrarily + // estimate maximum_name_length to be between 32 and 100 bytes. + // + // 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); diff --git a/ssl/test/runner/handshake_messages.go b/ssl/test/runner/handshake_messages.go index 15c92c8ee8..2d89dbd819 100644 --- a/ssl/test/runner/handshake_messages.go +++ b/ssl/test/runner/handshake_messages.go @@ -297,7 +297,7 @@ func MarshalECHConfigList(configs ...*ECHConfig) []byte { } // The contents of a CH "encrypted_client_hello" extension. -// https://tools.ietf.org/html/draft-ietf-tls-esni-09 +// https://tools.ietf.org/html/draft-ietf-tls-esni-10 type clientECH struct { hpkeKDF uint16 hpkeAEAD uint16 diff --git a/ssl/tls13_enc.cc b/ssl/tls13_enc.cc index cda53ec0b1..9c3063f606 100644 --- a/ssl/tls13_enc.cc +++ b/ssl/tls13_enc.cc @@ -522,7 +522,7 @@ bool tls13_ech_accept_confirmation( return false; } - // Per draft-ietf-tls-esni-09, accept_confirmation is computed with + // Per draft-ietf-tls-esni-10, accept_confirmation is computed with // Derive-Secret, which derives a secret of size Hash.length. That value is // then truncated to the first 8 bytes. Note this differs from deriving an // 8-byte secret because the target length is included in the derivation.