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>
diff --git a/ssl/d1_both.cc b/ssl/d1_both.cc
index ab499df..c2f6479 100644
--- a/ssl/d1_both.cc
+++ b/ssl/d1_both.cc
@@ -535,19 +535,17 @@
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 @@
// 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_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 @@
// 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 @@
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 @@
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) {
diff --git a/ssl/handshake.cc b/ssl/handshake.cc
index d95677f..ffd4a92 100644
--- a/ssl/handshake.cc
+++ b/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() {
- 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_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;
}
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index 9d2a1fb..2c42698 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -338,21 +338,19 @@
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,
diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc
index 002e5bb..5507250 100644
--- a/ssl/handshake_server.cc
+++ b/ssl/handshake_server.cc
@@ -818,7 +818,7 @@
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_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 @@
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 @@
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 @@
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;
diff --git a/ssl/internal.h b/ssl/internal.h
index 31cbdeb..ad2e957 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -331,6 +331,9 @@
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 @@
// 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 @@
// 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 @@
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 @@
// 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);
diff --git a/ssl/s3_both.cc b/ssl/s3_both.cc
index dfa8bfa..f63ed26 100644
--- a/ssl/s3_both.cc
+++ b/ssl/s3_both.cc
@@ -176,23 +176,16 @@
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 @@
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) {
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc
index 262f9b2..9c551c2 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -187,6 +187,17 @@
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.
diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc
index 7f3308b..7a83ed4 100644
--- a/ssl/t1_lib.cc
+++ b/ssl/t1_lib.cc
@@ -2225,8 +2225,7 @@
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 @@
!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;