commit fb4d2571f3faa85ce14bbc086f3fcfbb7043f944
author David Benjamin <davidben@google.com> Tue Jun 01 15:58:30 2021 -0400
committer Adam Langley <agl@google.com> Thu Jun 10 16:29:35 2021 +0000
tree 20e8030b13ed36d82fa1ab6770df75934acbcb02
parent 9052286dab3508855c130a5e8a9fd2c0857e7869

Shift some complexity out of ssl_add_clienthello_tlsext.

ssl_add_clienthello_tlsext is about to get kinda messy with ECH. Move
the padding and GREASE extensions into a few helpers.

Bug: 275
Change-Id: I3bb702fb79dce4be68490c4a8fd889121ecdae58
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47995
Reviewed-by: Adam Langley <agl@google.com>

ssl/handshake.cc

diff --git a/ssl/handshake.cc b/ssl/handshake.cc
index 1994465..07c9e3d 100644
--- a/ssl/handshake.cc
+++ b/ssl/handshake.cc
@@ -439,8 +439,8 @@
   return ret;
 }
 
-uint16_t ssl_get_grease_value(const SSL_HANDSHAKE *hs,
-                              enum ssl_grease_index_t index) {
+static uint16_t grease_index_to_value(const SSL_HANDSHAKE *hs,
+                                      enum ssl_grease_index_t index) {
   // This generates a random value of the form 0xωaωa, for all 0 ≤ ω < 16.
   uint16_t ret = hs->grease_seed[index];
   ret = (ret & 0xf0) | 0x0a;
@@ -448,6 +448,19 @@
   return ret;
 }
 
+uint16_t ssl_get_grease_value(const SSL_HANDSHAKE *hs,
+                              enum ssl_grease_index_t index) {
+  uint16_t ret = grease_index_to_value(hs, index);
+  if (index == ssl_grease_extension2 &&
+      ret == grease_index_to_value(hs, ssl_grease_extension1)) {
+    // The two fake extensions must not have the same value. GREASE values are
+    // of the form 0x1a1a, 0x2a2a, 0x3a3a, etc., so XOR to generate a different
+    // one.
+    ret ^= 0x1010;
+  }
+  return ret;
+}
+
 enum ssl_hs_wait_t ssl_get_finished(SSL_HANDSHAKE *hs) {
   SSL *const ssl = hs->ssl;
   SSLMessage msg;

ssl/t1_lib.cc

diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc
index 7f2c244..e780677 100644
--- a/ssl/t1_lib.cc
+++ b/ssl/t1_lib.cc
@@ -3247,6 +3247,19 @@
   return NULL;
 }
 
+static bool add_padding_extension(CBB *cbb, uint16_t ext, size_t len) {
+  CBB child;
+  uint8_t *ptr;
+  if (!CBB_add_u16(cbb, ext) ||  //
+      !CBB_add_u16_length_prefixed(cbb, &child) ||
+      !CBB_add_space(&child, &ptr, len)) {
+    OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
+    return false;
+  }
+  OPENSSL_memset(ptr, 0, len);
+  return CBB_flush(cbb);
+}
+
 bool ssl_add_clienthello_tlsext(SSL_HANDSHAKE *hs, CBB *out,
                                 bool *out_needs_psk_binder, size_t header_len) {
   SSL *const ssl = hs->ssl;
@@ -3262,15 +3275,11 @@
   // important to reset this value.
   hs->extensions.sent = 0;
 
-  uint16_t grease_ext1 = 0;
-  if (ssl->ctx->grease_enabled) {
-    // Add a fake empty extension. See RFC 8701.
-    grease_ext1 = ssl_get_grease_value(hs, ssl_grease_extension1);
-    if (!CBB_add_u16(&extensions, grease_ext1) ||
-        !CBB_add_u16(&extensions, 0 /* zero length */)) {
-      OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
-      return false;
-    }
+  // Add a fake empty extension. See RFC 8701.
+  if (ssl->ctx->grease_enabled &&
+      !add_padding_extension(
+          &extensions, ssl_get_grease_value(hs, ssl_grease_extension1), 0)) {
+    return false;
   }
 
   bool last_was_empty = false;
@@ -3293,22 +3302,10 @@
 
   if (ssl->ctx->grease_enabled) {
     // Add a fake non-empty extension. See RFC 8701.
-    uint16_t grease_ext2 = ssl_get_grease_value(hs, ssl_grease_extension2);
-
-    // The two fake extensions must not have the same value. GREASE values are
-    // of the form 0x1a1a, 0x2a2a, 0x3a3a, etc., so XOR to generate a different
-    // one.
-    if (grease_ext1 == grease_ext2) {
-      grease_ext2 ^= 0x1010;
-    }
-
-    if (!CBB_add_u16(&extensions, grease_ext2) ||
-        !CBB_add_u16(&extensions, 1 /* one byte length */) ||
-        !CBB_add_u8(&extensions, 0 /* single zero byte as contents */)) {
-      OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
+    if (!add_padding_extension(
+            &extensions, ssl_get_grease_value(hs, ssl_grease_extension2), 1)) {
       return false;
     }
-
     last_was_empty = false;
   }
 
@@ -3348,16 +3345,9 @@
       }
     }
 
-    if (padding_len != 0) {
-      uint8_t *padding_bytes;
-      if (!CBB_add_u16(&extensions, TLSEXT_TYPE_padding) ||
-          !CBB_add_u16(&extensions, padding_len) ||
-          !CBB_add_space(&extensions, &padding_bytes, padding_len)) {
-        OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
-        return false;
-      }
-
-      OPENSSL_memset(padding_bytes, 0, padding_len);
+    if (padding_len != 0 &&
+        !add_padding_extension(&extensions, TLSEXT_TYPE_padding, padding_len)) {
+      return false;
     }
   }