commit e4c19175abe1c1a113ee95ad0c8b3fc4239b5ccb
author David Benjamin <davidben@google.com> Mon Apr 12 18:33:12 2021 -0400
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Tue Apr 13 17:25:16 2021 +0000
tree f8bde942fee928dd1ee1f50ef72a18bfabfd99f2
parent 3b8c5ec1f6dfac6b1aa5a060990216819ff298a7

Don't duplicate ServerHello construction code.

This also fixes a minor bug (that doesn't matter because we don't
implement DTLS 1.3). init_message must be paired with finish_message to
correctly handle the DTLS header.

Change-Id: I4b65c82d4b691d5b77d9e20513983145098d6f8f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46785
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

ssl/tls13_server.cc

diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc
index b4f5297..21f3661 100644
--- a/ssl/tls13_server.cc
+++ b/ssl/tls13_server.cc
@@ -708,6 +708,28 @@
   return ssl_hs_ok;
 }
 
+static bool make_server_hello(SSL_HANDSHAKE *hs, Array<uint8_t> *out) {
+  SSL *const ssl = hs->ssl;
+  ScopedCBB cbb;
+  CBB body, extensions, session_id;
+  if (!ssl->method->init_message(ssl, cbb.get(), &body, SSL3_MT_SERVER_HELLO) ||
+      !CBB_add_u16(&body, TLS1_2_VERSION) ||
+      !CBB_add_bytes(&body, ssl->s3->server_random,
+                     sizeof(ssl->s3->server_random)) ||
+      !CBB_add_u8_length_prefixed(&body, &session_id) ||
+      !CBB_add_bytes(&session_id, hs->session_id, hs->session_id_len) ||
+      !CBB_add_u16(&body, SSL_CIPHER_get_protocol_id(hs->new_cipher)) ||
+      !CBB_add_u8(&body, 0) ||
+      !CBB_add_u16_length_prefixed(&body, &extensions) ||
+      !ssl_ext_pre_shared_key_add_serverhello(hs, &extensions) ||
+      !ssl_ext_key_share_add_serverhello(hs, &extensions) ||
+      !ssl_ext_supported_versions_add_serverhello(hs, &extensions) ||
+      !ssl->method->finish_message(ssl, cbb.get(), out)) {
+    return false;
+  }
+  return true;
+}
+
 static enum ssl_hs_wait_t do_send_server_hello(SSL_HANDSHAKE *hs) {
   SSL *const ssl = hs->ssl;
 
@@ -722,48 +744,17 @@
     Span<uint8_t> random_suffix = random.subspan(24);
     OPENSSL_memset(random_suffix.data(), 0, random_suffix.size());
 
-    ScopedCBB cbb;
-    CBB body, extensions, session_id;
-    if (!ssl->method->init_message(ssl, cbb.get(), &body,
-                                   SSL3_MT_SERVER_HELLO) ||
-        !CBB_add_u16(&body, TLS1_2_VERSION) ||
-        !CBB_add_bytes(&body, random.data(), random.size()) ||
-        !CBB_add_u8_length_prefixed(&body, &session_id) ||
-        !CBB_add_bytes(&session_id, hs->session_id, hs->session_id_len) ||
-        !CBB_add_u16(&body, SSL_CIPHER_get_protocol_id(hs->new_cipher)) ||
-        !CBB_add_u8(&body, 0) ||
-        !CBB_add_u16_length_prefixed(&body, &extensions) ||
-        !ssl_ext_pre_shared_key_add_serverhello(hs, &extensions) ||
-        !ssl_ext_key_share_add_serverhello(hs, &extensions) ||
-        !ssl_ext_supported_versions_add_serverhello(hs, &extensions) ||
-        !CBB_flush(cbb.get())) {
-      return ssl_hs_error;
-    }
-
-    // Note that |cbb| includes the message type and length fields, but not the
-    // record layer header.
-    if (!tls13_ech_accept_confirmation(
-            hs, random_suffix,
-            bssl::MakeConstSpan(CBB_data(cbb.get()), CBB_len(cbb.get())))) {
+    Array<uint8_t> server_hello_ech_conf;
+    if (!make_server_hello(hs, &server_hello_ech_conf) ||
+        !tls13_ech_accept_confirmation(hs, random_suffix,
+                                       server_hello_ech_conf)) {
       return ssl_hs_error;
     }
   }
 
-  // Send a ServerHello.
-  ScopedCBB cbb;
-  CBB body, extensions, session_id;
-  if (!ssl->method->init_message(ssl, cbb.get(), &body, SSL3_MT_SERVER_HELLO) ||
-      !CBB_add_u16(&body, TLS1_2_VERSION) ||
-      !CBB_add_bytes(&body, random.data(), random.size()) ||
-      !CBB_add_u8_length_prefixed(&body, &session_id) ||
-      !CBB_add_bytes(&session_id, hs->session_id, hs->session_id_len) ||
-      !CBB_add_u16(&body, SSL_CIPHER_get_protocol_id(hs->new_cipher)) ||
-      !CBB_add_u8(&body, 0) ||
-      !CBB_add_u16_length_prefixed(&body, &extensions) ||
-      !ssl_ext_pre_shared_key_add_serverhello(hs, &extensions) ||
-      !ssl_ext_key_share_add_serverhello(hs, &extensions) ||
-      !ssl_ext_supported_versions_add_serverhello(hs, &extensions) ||
-      !ssl_add_message_cbb(ssl, cbb.get())) {
+  Array<uint8_t> server_hello;
+  if (!make_server_hello(hs, &server_hello) ||
+      !ssl->method->add_message(ssl, std::move(server_hello))) {
     return ssl_hs_error;
   }
 
@@ -782,6 +773,8 @@
   }
 
   // Send EncryptedExtensions.
+  ScopedCBB cbb;
+  CBB body;
   if (!ssl->method->init_message(ssl, cbb.get(), &body,
                                  SSL3_MT_ENCRYPTED_EXTENSIONS) ||
       !ssl_add_serverhello_tlsext(hs, &body) ||