commit 350fe3bf32dcf87f281685b1e7fc8df876f396a8
author David Benjamin <davidben@google.com> Wed Jun 02 17:58:53 2021 -0400
committer Adam Langley <agl@google.com> Thu Jun 10 16:41:40 2021 +0000
tree 30d611f851d5c9d0cde4a42b816a82c433675e03
parent b32aa05538113798a07e086102ffb9d57dcdc6a8

Fix ext_pre_shared_key_clienthello_length calculation.

If we're dropping the PSK extension due to an HRR with mismatched hash
(looking back at that, we could easily have avoided that nuisance...
I've filed [0] on rfc8446bis), we don't predict the length correctly.
The consequence is we don't pad the second ClientHello to the desired
range. Fix this and add an assert.

[0] https://github.com/tlswg/tls13-spec/issues/1227

Change-Id: I47d880b6bdafa95840f7513b6b7718851f22554d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47998
Reviewed-by: Adam Langley <agl@google.com>

ssl/t1_lib.cc

diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc
index da89afb..33621a3 100644
--- a/ssl/t1_lib.cc
+++ b/ssl/t1_lib.cc
@@ -1937,10 +1937,27 @@
 //
 // https://tools.ietf.org/html/rfc8446#section-4.2.11
 
-static size_t ext_pre_shared_key_clienthello_length(SSL_HANDSHAKE *hs) {
-  SSL *const ssl = hs->ssl;
+static bool should_offer_psk(const SSL_HANDSHAKE *hs) {
+  const SSL *const ssl = hs->ssl;
   if (hs->max_version < TLS1_3_VERSION || ssl->session == nullptr ||
       ssl_session_protocol_version(ssl->session.get()) < TLS1_3_VERSION) {
+    return false;
+  }
+
+  // Per RFC 8446 section 4.1.4, skip offering the session if the selected
+  // cipher in HelloRetryRequest does not match. This avoids performing the
+  // transcript hash transformation for multiple hashes.
+  if (ssl->s3->used_hello_retry_request &&
+      ssl->session->cipher->algorithm_prf != hs->new_cipher->algorithm_prf) {
+    return false;
+  }
+
+  return true;
+}
+
+static size_t ext_pre_shared_key_clienthello_length(const SSL_HANDSHAKE *hs) {
+  const SSL *const ssl = hs->ssl;
+  if (!should_offer_psk(hs)) {
     return 0;
   }
 
@@ -1953,16 +1970,7 @@
                                                bool *out_needs_binder) {
   const SSL *const ssl = hs->ssl;
   *out_needs_binder = false;
-  if (hs->max_version < TLS1_3_VERSION || ssl->session == nullptr ||
-      ssl_session_protocol_version(ssl->session.get()) < TLS1_3_VERSION) {
-    return true;
-  }
-
-  // Per RFC 8446 section 4.1.4, skip offering the session if the selected
-  // cipher in HelloRetryRequest does not match. This avoids performing the
-  // transcript hash transformation for multiple hashes.
-  if (ssl->s3->used_hello_retry_request &&
-      ssl->session->cipher->algorithm_prf != hs->new_cipher->algorithm_prf) {
+  if (!should_offer_psk(hs)) {
     return true;
   }
 
@@ -3309,8 +3317,8 @@
     last_was_empty = false;
   }
 
+  size_t psk_extension_len = ext_pre_shared_key_clienthello_length(hs);
   if (!SSL_is_dtls(ssl) && !ssl->quic_method) {
-    size_t psk_extension_len = ext_pre_shared_key_clienthello_length(hs);
     header_len +=
         SSL3_HM_HEADER_LENGTH + 2 + CBB_len(&extensions) + psk_extension_len;
     size_t padding_len = 0;
@@ -3353,11 +3361,14 @@
   }
 
   // The PSK extension must be last, including after the padding.
+  const size_t len_before = CBB_len(&extensions);
   if (!ext_pre_shared_key_add_clienthello(hs, &extensions,
                                           out_needs_psk_binder)) {
     OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
     return false;
   }
+  assert(psk_extension_len == CBB_len(&extensions) - len_before);
+  (void)len_before;  // |assert| is omitted in release builds.
 
   // Discard empty extensions blocks.
   if (CBB_len(&extensions) == 0) {