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 tlswg/draft-ietf-tls-esni#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 <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com>
Tw3akG33k · May 20, 2021 · 49ee62f · 49ee62f
1 parent 5e72294
commit 49ee62f
Show file tree

Hide file tree

Showing 5 changed files with 23 additions and 45 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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.