Fold ssl_decode_client_hello_inner into ssl_client_hello_decrypt.

Both call sites end up calling them in succession. This saves a little
bit of code.

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

ssl/encrypted_client_hello.cc

@@ -258,8 +258,8 @@ bool ssl_decode_client_hello_inner(
   return true;
 }
 
-bool ssl_client_hello_decrypt(EVP_HPKE_CTX *hpke_ctx, Array<uint8_t> *out,
-                              bool *out_is_decrypt_error,
+bool ssl_client_hello_decrypt(SSL_HANDSHAKE *hs, uint8_t *out_alert,
+                              bool *out_is_decrypt_error, Array<uint8_t> *out,
                               const SSL_CLIENT_HELLO *client_hello_outer,
                               Span<const uint8_t> payload) {
   *out_is_decrypt_error = false;
@@ -270,6 +270,7 @@ bool ssl_client_hello_decrypt(EVP_HPKE_CTX *hpke_ctx, Array<uint8_t> *out,
   Array<uint8_t> aad;
   if (!aad.CopyFrom(MakeConstSpan(client_hello_outer->client_hello,
                                   client_hello_outer->client_hello_len))) {
+    *out_alert = SSL_AD_INTERNAL_ERROR;
     return false;
   }
 
@@ -284,36 +285,42 @@ bool ssl_client_hello_decrypt(EVP_HPKE_CTX *hpke_ctx, Array<uint8_t> *out,
       payload.data() - client_hello_outer->client_hello, payload.size());
   OPENSSL_memset(payload_aad.data(), 0, payload_aad.size());
 
+  // Decrypt the EncodedClientHelloInner.
+  Array<uint8_t> encoded;
 #if defined(BORINGSSL_UNSAFE_FUZZER_MODE)
   // In fuzzer mode, disable encryption to improve coverage. We reserve a short
   // input to signal decryption failure, so the fuzzer can explore fallback to
   // ClientHelloOuter.
   const uint8_t kBadPayload[] = {0xff};
   if (payload == kBadPayload) {
+    *out_alert = SSL_AD_DECRYPT_ERROR;
     *out_is_decrypt_error = true;
     OPENSSL_PUT_ERROR(SSL, SSL_R_DECRYPTION_FAILED);
     return false;
   }
-  if (!out->CopyFrom(payload)) {
+  if (!encoded.CopyFrom(payload)) {
+    *out_alert = SSL_AD_INTERNAL_ERROR;
     return false;
   }
 #else
-  // Attempt to decrypt into |out|.
-  if (!out->Init(payload.size())) {
-    OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
+  if (!encoded.Init(payload.size())) {
+    *out_alert = SSL_AD_INTERNAL_ERROR;
     return false;
   }
   size_t len;
-  if (!EVP_HPKE_CTX_open(hpke_ctx, out->data(), &len, out->size(),
-                         payload.data(), payload.size(), aad.data(),
-                         aad.size())) {
+  if (!EVP_HPKE_CTX_open(hs->ech_hpke_ctx.get(), encoded.data(), &len,
+                         encoded.size(), payload.data(), payload.size(),
+                         aad.data(), aad.size())) {
+    *out_alert = SSL_AD_DECRYPT_ERROR;
     *out_is_decrypt_error = true;
     OPENSSL_PUT_ERROR(SSL, SSL_R_DECRYPTION_FAILED);
     return false;
   }
-  out->Shrink(len);
+  encoded.Shrink(len);
 #endif
-  return true;
+
+  return ssl_decode_client_hello_inner(hs->ssl, out_alert, out, encoded,
+                                       client_hello_outer);
 }
 
 static bool is_hex_component(Span<const uint8_t> in) {

ssl/handshake_server.cc

@@ -554,29 +554,22 @@ static bool decrypt_ech(SSL_HANDSHAKE *hs, uint8_t *out_alert,
       ERR_clear_error();
       continue;
     }
-    Array<uint8_t> encoded_client_hello_inner;
     bool is_decrypt_error;
-    if (!ssl_client_hello_decrypt(hs->ech_hpke_ctx.get(),
-                                  &encoded_client_hello_inner,
-                                  &is_decrypt_error, client_hello, payload)) {
+    if (!ssl_client_hello_decrypt(hs, out_alert, &is_decrypt_error,
+                                  &hs->ech_client_hello_buf, client_hello,
+                                  payload)) {
       if (is_decrypt_error) {
         // Ignore the error and try another ECHConfig.
         ERR_clear_error();
+        // The |out_alert| calling convention currently relies on a default of
+        // |SSL_AD_DECODE_ERROR|. https://crbug.com/boringssl/373 tracks
+        // switching to sum types, which avoids this.
+        *out_alert = SSL_AD_DECODE_ERROR;
         continue;
       }
       OPENSSL_PUT_ERROR(SSL, SSL_R_DECRYPTION_FAILED);
       return false;
     }
-
-    // Recover the ClientHelloInner from the EncodedClientHelloInner.
-    bssl::Array<uint8_t> client_hello_inner;
-    if (!ssl_decode_client_hello_inner(ssl, out_alert, &client_hello_inner,
-                                       encoded_client_hello_inner,
-                                       client_hello)) {
-      OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
-      return false;
-    }
-    hs->ech_client_hello_buf = std::move(client_hello_inner);
     hs->ech_config_id = config_id;
     ssl->s3->ech_status = ssl_ech_accepted;
     return true;

ssl/internal.h

@@ -1498,17 +1498,19 @@ enum ssl_client_hello_type_t {
 // ClientHelloOuter |client_hello_outer|. If successful, it writes the recovered
 // ClientHelloInner to |out_client_hello_inner|. It returns true on success and
 // false on failure.
+//
+// This function is exported for fuzzing.
 OPENSSL_EXPORT bool ssl_decode_client_hello_inner(
     SSL *ssl, uint8_t *out_alert, Array<uint8_t> *out_client_hello_inner,
     Span<const uint8_t> encoded_client_hello_inner,
     const SSL_CLIENT_HELLO *client_hello_outer);
 
-// ssl_client_hello_decrypt attempts to decrypt the |payload| and writes the
-// result to |*out|. |payload| must point into |client_hello_outer|. It returns
-// true on success and false on error. On error, it sets |*out_is_decrypt_error|
-// to whether the failure was due to a bad ciphertext.
-bool ssl_client_hello_decrypt(EVP_HPKE_CTX *hpke_ctx, Array<uint8_t> *out,
-                              bool *out_is_decrypt_error,
+// ssl_client_hello_decrypt attempts to decrypt and decode the |payload|. It
+// writes the result to |*out|. |payload| must point into |client_hello_outer|.
+// It returns true on success and false on error. On error, it sets
+// |*out_is_decrypt_error| to whether the failure was due to a bad ciphertext.
+bool ssl_client_hello_decrypt(SSL_HANDSHAKE *hs, uint8_t *out_alert,
+                              bool *out_is_decrypt_error, Array<uint8_t> *out,
                               const SSL_CLIENT_HELLO *client_hello_outer,
                               Span<const uint8_t> payload);
 

ssl/tls13_server.cc

@@ -658,28 +658,16 @@ static enum ssl_hs_wait_t do_read_second_client_hello(SSL_HANDSHAKE *hs) {
     }
 
     // Decrypt the payload with the HPKE context from the first ClientHello.
-    Array<uint8_t> encoded_client_hello_inner;
+    uint8_t alert = SSL_AD_DECODE_ERROR;
     bool unused;
-    if (!ssl_client_hello_decrypt(hs->ech_hpke_ctx.get(),
-                                  &encoded_client_hello_inner, &unused,
-                                  &client_hello, payload)) {
+    if (!ssl_client_hello_decrypt(hs, &alert, &unused,
+                                  &hs->ech_client_hello_buf, &client_hello,
+                                  payload)) {
       // Decryption failure is fatal in the second ClientHello.
       OPENSSL_PUT_ERROR(SSL, SSL_R_DECRYPTION_FAILED);
-      ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECRYPT_ERROR);
-      return ssl_hs_error;
-    }
-
-    // Recover the ClientHelloInner from the EncodedClientHelloInner.
-    uint8_t alert = SSL_AD_DECODE_ERROR;
-    bssl::Array<uint8_t> client_hello_inner;
-    if (!ssl_decode_client_hello_inner(ssl, &alert, &client_hello_inner,
-                                       encoded_client_hello_inner,
-                                       &client_hello)) {
-      OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
       ssl_send_alert(ssl, SSL3_AL_FATAL, alert);
       return ssl_hs_error;
     }
-    hs->ech_client_hello_buf = std::move(client_hello_inner);
 
     // Reparse |client_hello| from the buffer owned by |hs|.
     if (!hs->GetClientHello(&msg, &client_hello)) {