Switch more things to Array.

This adds a CBBFinishArray helper since we need to do that fairly often.

Bug: 132
Change-Id: I7ec0720de0e6ea31caa90c316041bb5f66661cd3
Reviewed-on: https://boringssl-review.googlesource.com/20671
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

ssl/d1_both.cc

@@ -535,19 +535,17 @@ int dtls1_init_message(SSL *ssl, CBB *cbb, CBB *body, uint8_t type) {
   return 1;
 }
 
-int dtls1_finish_message(SSL *ssl, CBB *cbb, uint8_t **out_msg,
-                         size_t *out_len) {
-  *out_msg = NULL;
-  if (!CBB_finish(cbb, out_msg, out_len) ||
-      *out_len < DTLS1_HM_HEADER_LENGTH) {
+int dtls1_finish_message(SSL *ssl, CBB *cbb, Array<uint8_t> *out_msg) {
+  if (!CBBFinishArray(cbb, out_msg) ||
+      out_msg->size() < DTLS1_HM_HEADER_LENGTH) {
     OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
-    OPENSSL_free(*out_msg);
     return 0;
   }
 
   // Fix up the header. Copy the fragment length into the total message
   // length.
-  OPENSSL_memcpy(*out_msg + 1, *out_msg + DTLS1_HM_HEADER_LENGTH - 3, 3);
+  OPENSSL_memcpy(out_msg->data() + 1,
+                 out_msg->data() + DTLS1_HM_HEADER_LENGTH - 3, 3);
   return 1;
 }
 
@@ -555,7 +553,7 @@ int dtls1_finish_message(SSL *ssl, CBB *cbb, uint8_t **out_msg,
 // outgoing flight. It returns one on success and zero on error. In both cases,
 // it takes ownership of |data| and releases it with |OPENSSL_free| when
 // done.
-static int add_outgoing(SSL *ssl, int is_ccs, uint8_t *data, size_t len) {
+static int add_outgoing(SSL *ssl, int is_ccs, Array<uint8_t> data) {
   if (ssl->d1->outgoing_messages_complete) {
     // If we've begun writing a new flight, we received the peer flight. Discard
     // the timer and the our flight.
@@ -566,10 +564,10 @@ static int add_outgoing(SSL *ssl, int is_ccs, uint8_t *data, size_t len) {
   static_assert(SSL_MAX_HANDSHAKE_FLIGHT <
                     (1 << 8 * sizeof(ssl->d1->outgoing_messages_len)),
                 "outgoing_messages_len is too small");
-  if (ssl->d1->outgoing_messages_len >= SSL_MAX_HANDSHAKE_FLIGHT) {
+  if (ssl->d1->outgoing_messages_len >= SSL_MAX_HANDSHAKE_FLIGHT ||
+      data.size() > 0xffffffff) {
     assert(0);
     OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
-    OPENSSL_free(data);
     return 0;
   }
 
@@ -577,9 +575,8 @@ static int add_outgoing(SSL *ssl, int is_ccs, uint8_t *data, size_t len) {
     // TODO(svaldez): Move this up a layer to fix abstraction for SSLTranscript
     // on hs.
     if (ssl->s3->hs != NULL &&
-        !ssl->s3->hs->transcript.Update(data, len)) {
+        !ssl->s3->hs->transcript.Update(data.data(), data.size())) {
       OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
-      OPENSSL_free(data);
       return 0;
     }
     ssl->d1->handshake_write_seq++;
@@ -587,7 +584,8 @@ static int add_outgoing(SSL *ssl, int is_ccs, uint8_t *data, size_t len) {
 
   DTLS_OUTGOING_MESSAGE *msg =
       &ssl->d1->outgoing_messages[ssl->d1->outgoing_messages_len];
-  msg->data = data;
+  size_t len;
+  data.Release(&msg->data, &len);
   msg->len = len;
   msg->epoch = ssl->d1->w_epoch;
   msg->is_ccs = is_ccs;
@@ -596,12 +594,12 @@ static int add_outgoing(SSL *ssl, int is_ccs, uint8_t *data, size_t len) {
   return 1;
 }
 
-int dtls1_add_message(SSL *ssl, uint8_t *data, size_t len) {
-  return add_outgoing(ssl, 0 /* handshake */, data, len);
+int dtls1_add_message(SSL *ssl, Array<uint8_t> data) {
+  return add_outgoing(ssl, 0 /* handshake */, std::move(data));
 }
 
 int dtls1_add_change_cipher_spec(SSL *ssl) {
-  return add_outgoing(ssl, 1 /* ChangeCipherSpec */, NULL, 0);
+  return add_outgoing(ssl, 1 /* ChangeCipherSpec */, Array<uint8_t>());
 }
 
 int dtls1_add_alert(SSL *ssl, uint8_t level, uint8_t desc) {

ssl/handshake.cc

@@ -114,6 +114,8 @@
 
 #include <assert.h>
 
+#include <utility>
+
 #include "../crypto/internal.h"
 #include "internal.h"
 
@@ -144,9 +146,7 @@ SSL_HANDSHAKE::SSL_HANDSHAKE(SSL *ssl_arg)
 }
 
 SSL_HANDSHAKE::~SSL_HANDSHAKE() {
-  OPENSSL_free(ecdh_public_key);
   OPENSSL_free(peer_sigalgs);
-  OPENSSL_free(server_params);
   ssl->ctx->x509_method->hs_flush_cached_ca_names(this);
   OPENSSL_free(key_block);
 }
@@ -174,10 +174,9 @@ int ssl_check_message_type(SSL *ssl, const SSLMessage &msg, int type) {
 }
 
 int ssl_add_message_cbb(SSL *ssl, CBB *cbb) {
-  uint8_t *msg;
-  size_t len;
-  if (!ssl->method->finish_message(ssl, cbb, &msg, &len) ||
-      !ssl->method->add_message(ssl, msg, len)) {
+  Array<uint8_t> msg;
+  if (!ssl->method->finish_message(ssl, cbb, &msg) ||
+      !ssl->method->add_message(ssl, std::move(msg))) {
     return 0;
   }
 

ssl/handshake_client.cc

@@ -338,21 +338,19 @@ int ssl_write_client_hello(SSL_HANDSHAKE *hs) {
     return 0;
   }
 
-  uint8_t *msg = NULL;
-  size_t len;
-  if (!ssl->method->finish_message(ssl, cbb.get(), &msg, &len)) {
+  Array<uint8_t> msg;
+  if (!ssl->method->finish_message(ssl, cbb.get(), &msg)) {
     return 0;
   }
 
   // Now that the length prefixes have been computed, fill in the placeholder
   // PSK binder.
   if (hs->needs_psk_binder &&
-      !tls13_write_psk_binder(hs, msg, len)) {
-    OPENSSL_free(msg);
+      !tls13_write_psk_binder(hs, msg.data(), msg.size())) {
     return 0;
   }
 
-  return ssl->method->add_message(ssl, msg, len);
+  return ssl->method->add_message(ssl, std::move(msg));
 }
 
 static int parse_server_version(SSL_HANDSHAKE *hs, uint16_t *out,

ssl/handshake_server.cc

@@ -818,7 +818,7 @@ static enum ssl_hs_wait_t do_send_server_certificate(SSL_HANDSHAKE *hs) {
       assert(alg_k & SSL_kPSK);
     }
 
-    if (!CBB_finish(cbb.get(), &hs->server_params, &hs->server_params_len)) {
+    if (!CBBFinishArray(cbb.get(), &hs->server_params)) {
       return ssl_hs_error;
     }
   }
@@ -830,7 +830,7 @@ static enum ssl_hs_wait_t do_send_server_certificate(SSL_HANDSHAKE *hs) {
 static enum ssl_hs_wait_t do_send_server_key_exchange(SSL_HANDSHAKE *hs) {
   SSL *const ssl = hs->ssl;
 
-  if (hs->server_params_len == 0) {
+  if (hs->server_params.size() == 0) {
     hs->state = state_send_server_hello_done;
     return ssl_hs_ok;
   }
@@ -840,9 +840,9 @@ static enum ssl_hs_wait_t do_send_server_key_exchange(SSL_HANDSHAKE *hs) {
   if (!ssl->method->init_message(ssl, cbb.get(), &body,
                                  SSL3_MT_SERVER_KEY_EXCHANGE) ||
       // |hs->server_params| contains a prefix for signing.
-      hs->server_params_len < 2 * SSL3_RANDOM_SIZE ||
-      !CBB_add_bytes(&body, hs->server_params + 2 * SSL3_RANDOM_SIZE,
-                     hs->server_params_len - 2 * SSL3_RANDOM_SIZE)) {
+      hs->server_params.size() < 2 * SSL3_RANDOM_SIZE ||
+      !CBB_add_bytes(&body, hs->server_params.data() + 2 * SSL3_RANDOM_SIZE,
+                     hs->server_params.size() - 2 * SSL3_RANDOM_SIZE)) {
     return ssl_hs_error;
   }
 
@@ -876,8 +876,8 @@ static enum ssl_hs_wait_t do_send_server_key_exchange(SSL_HANDSHAKE *hs) {
 
     size_t sig_len;
     switch (ssl_private_key_sign(hs, ptr, &sig_len, max_sig_len,
-                                 signature_algorithm, hs->server_params,
-                                 hs->server_params_len)) {
+                                 signature_algorithm, hs->server_params.data(),
+                                 hs->server_params.size())) {
       case ssl_private_key_success:
         if (!CBB_did_write(&child, sig_len)) {
           return ssl_hs_error;
@@ -894,9 +894,7 @@ static enum ssl_hs_wait_t do_send_server_key_exchange(SSL_HANDSHAKE *hs) {
     return ssl_hs_error;
   }
 
-  OPENSSL_free(hs->server_params);
-  hs->server_params = NULL;
-  hs->server_params_len = 0;
+  hs->server_params.Reset();
 
   hs->state = state_send_server_hello_done;
   return ssl_hs_ok;

ssl/internal.h

@@ -331,6 +331,9 @@ class Array {
   size_t size_ = 0;
 };
 
+// CBBFinishArray behaves like |CBB_finish| but stores the result in an Array.
+bool CBBFinishArray(CBB *cbb, Array<uint8_t> *out);
+
 
 // Protocol versions.
 //
@@ -1304,8 +1307,7 @@ struct SSL_HANDSHAKE {
 
   // ecdh_public_key, for servers, is the key share to be sent to the client in
   // TLS 1.3.
-  uint8_t *ecdh_public_key = nullptr;
-  size_t ecdh_public_key_len = 0;
+  Array<uint8_t> ecdh_public_key;
 
   // peer_sigalgs are the signature algorithms that the peer supports. These are
   // taken from the contents of the signature algorithms extension for a server
@@ -1325,8 +1327,7 @@ struct SSL_HANDSHAKE {
   // server_params, in a TLS 1.2 server, stores the ServerKeyExchange
   // parameters. It has client and server randoms prepended for signing
   // convenience.
-  uint8_t *server_params = nullptr;
-  size_t server_params_len = 0;
+  Array<uint8_t> server_params;
 
   // peer_psk_identity_hint, on the client, is the psk_identity_hint sent by the
   // server when using a TLS 1.2 PSK key exchange.
@@ -2309,16 +2310,15 @@ int ssl3_new(SSL *ssl);
 void ssl3_free(SSL *ssl);
 
 int ssl3_init_message(SSL *ssl, CBB *cbb, CBB *body, uint8_t type);
-int ssl3_finish_message(SSL *ssl, CBB *cbb, uint8_t **out_msg, size_t *out_len);
-int ssl3_add_message(SSL *ssl, uint8_t *msg, size_t len);
+int ssl3_finish_message(SSL *ssl, CBB *cbb, Array<uint8_t> *out_msg);
+int ssl3_add_message(SSL *ssl, Array<uint8_t> msg);
 int ssl3_add_change_cipher_spec(SSL *ssl);
 int ssl3_add_alert(SSL *ssl, uint8_t level, uint8_t desc);
 int ssl3_flush_flight(SSL *ssl);
 
 int dtls1_init_message(SSL *ssl, CBB *cbb, CBB *body, uint8_t type);
-int dtls1_finish_message(SSL *ssl, CBB *cbb, uint8_t **out_msg,
-                         size_t *out_len);
-int dtls1_add_message(SSL *ssl, uint8_t *msg, size_t len);
+int dtls1_finish_message(SSL *ssl, CBB *cbb, Array<uint8_t> *out_msg);
+int dtls1_add_message(SSL *ssl, Array<uint8_t> msg);
 int dtls1_add_change_cipher_spec(SSL *ssl);
 int dtls1_add_alert(SSL *ssl, uint8_t level, uint8_t desc);
 int dtls1_flush_flight(SSL *ssl);
@@ -2525,15 +2525,12 @@ struct ssl_protocol_method_st {
   // root CBB to be passed into |finish_message|. |*body| is set to a child CBB
   // the caller should write to. It returns one on success and zero on error.
   int (*init_message)(SSL *ssl, CBB *cbb, CBB *body, uint8_t type);
-  // finish_message finishes a handshake message. It sets |*out_msg| to a
-  // newly-allocated buffer with the serialized message. The caller must
-  // release it with |OPENSSL_free| when done. It returns one on success and
-  // zero on error.
-  int (*finish_message)(SSL *ssl, CBB *cbb, uint8_t **out_msg, size_t *out_len);
+  // finish_message finishes a handshake message. It sets |*out_msg| to the
+  // serialized message. It returns one on success and zero on error.
+  int (*finish_message)(SSL *ssl, CBB *cbb, bssl::Array<uint8_t> *out_msg);
   // add_message adds a handshake message to the pending flight. It returns one
-  // on success and zero on error. In either case, it takes ownership of |msg|
-  // and releases it with |OPENSSL_free| when done.
-  int (*add_message)(SSL *ssl, uint8_t *msg, size_t len);
+  // on success and zero on error.
+  int (*add_message)(SSL *ssl, bssl::Array<uint8_t> msg);
   // add_change_cipher_spec adds a ChangeCipherSpec record to the pending
   // flight. It returns one on success and zero on error.
   int (*add_change_cipher_spec)(SSL *ssl);

ssl/s3_both.cc

@@ -176,23 +176,16 @@ int ssl3_init_message(SSL *ssl, CBB *cbb, CBB *body, uint8_t type) {
   return 1;
 }
 
-int ssl3_finish_message(SSL *ssl, CBB *cbb, uint8_t **out_msg,
-                        size_t *out_len) {
-  if (!CBB_finish(cbb, out_msg, out_len)) {
-    OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
-    return 0;
-  }
-
-  return 1;
+int ssl3_finish_message(SSL *ssl, CBB *cbb, Array<uint8_t> *out_msg) {
+  return CBBFinishArray(cbb, out_msg);
 }
 
-int ssl3_add_message(SSL *ssl, uint8_t *msg, size_t len) {
+int ssl3_add_message(SSL *ssl, Array<uint8_t> msg) {
   // Add the message to the current flight, splitting into several records if
   // needed.
-  int ret = 0;
   size_t added = 0;
   do {
-    size_t todo = len - added;
+    size_t todo = msg.size() - added;
     if (todo > ssl->max_send_fragment) {
       todo = ssl->max_send_fragment;
     }
@@ -205,24 +198,21 @@ int ssl3_add_message(SSL *ssl, uint8_t *msg, size_t len) {
       type = SSL3_RT_PLAINTEXT_HANDSHAKE;
     }
 
-    if (!add_record_to_flight(ssl, type, msg + added, todo)) {
-      goto err;
+    if (!add_record_to_flight(ssl, type, msg.data() + added, todo)) {
+      return 0;
     }
     added += todo;
-  } while (added < len);
+  } while (added < msg.size());
 
-  ssl_do_msg_callback(ssl, 1 /* write */, SSL3_RT_HANDSHAKE, msg, len);
+  ssl_do_msg_callback(ssl, 1 /* write */, SSL3_RT_HANDSHAKE, msg.data(),
+                      msg.size());
   // TODO(svaldez): Move this up a layer to fix abstraction for SSLTranscript on
   // hs.
   if (ssl->s3->hs != NULL &&
-      !ssl->s3->hs->transcript.Update(msg, len)) {
-    goto err;
+      !ssl->s3->hs->transcript.Update(msg.data(), msg.size())) {
+    return 0;
   }
-  ret = 1;
-
-err:
-  OPENSSL_free(msg);
-  return ret;
+  return 1;
 }
 
 int ssl3_add_change_cipher_spec(SSL *ssl) {

ssl/ssl_lib.cc

@@ -187,6 +187,17 @@ static CRYPTO_EX_DATA_CLASS g_ex_data_class_ssl =
 static CRYPTO_EX_DATA_CLASS g_ex_data_class_ssl_ctx =
     CRYPTO_EX_DATA_CLASS_INIT_WITH_APP_DATA;
 
+bool CBBFinishArray(CBB *cbb, Array<uint8_t> *out) {
+  uint8_t *ptr;
+  size_t len;
+  if (!CBB_finish(cbb, &ptr, &len)) {
+    OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
+    return false;
+  }
+  out->Reset(ptr, len);
+  return true;
+}
+
 void ssl_reset_error_state(SSL *ssl) {
   // Functions which use |SSL_get_error| must reset I/O and error state on
   // entry.

ssl/t1_lib.cc

@@ -2225,8 +2225,7 @@ int ssl_ext_key_share_parse_clienthello(SSL_HANDSHAKE *hs, bool *out_found,
   if (!key_share ||
       !CBB_init(public_key.get(), 32) ||
       !key_share->Accept(public_key.get(), &secret, out_alert, peer_key) ||
-      !CBB_finish(public_key.get(), &hs->ecdh_public_key,
-                  &hs->ecdh_public_key_len)) {
+      !CBBFinishArray(public_key.get(), &hs->ecdh_public_key)) {
     *out_alert = SSL_AD_ILLEGAL_PARAMETER;
     return 0;
   }
@@ -2244,15 +2243,13 @@ int ssl_ext_key_share_add_serverhello(SSL_HANDSHAKE *hs, CBB *out) {
       !CBB_add_u16_length_prefixed(out, &kse_bytes) ||
       !CBB_add_u16(&kse_bytes, group_id) ||
       !CBB_add_u16_length_prefixed(&kse_bytes, &public_key) ||
-      !CBB_add_bytes(&public_key, hs->ecdh_public_key,
-                     hs->ecdh_public_key_len) ||
+      !CBB_add_bytes(&public_key, hs->ecdh_public_key.data(),
+                     hs->ecdh_public_key.size()) ||
       !CBB_flush(out)) {
     return 0;
   }
 
-  OPENSSL_free(hs->ecdh_public_key);
-  hs->ecdh_public_key = NULL;
-  hs->ecdh_public_key_len = 0;
+  hs->ecdh_public_key.Reset();
 
   hs->new_session->group_id = group_id;
   return 1;