Introduce bssl::Array<T> and use it in SSLKeyShare.
An Array<T> is an owning Span<T>. It's similar to absl::FixedArray<T>
but plays well with OPENSSL_malloc and doesn't implement inlining. With
OPENSSL_cleanse folded into OPENSSL_free, we could go nuts with
UniquePtr<uint8_t>, but having the pointer and length tied together is
nice for other reasons. Notably, Array<T> plays great with Span<T>.
Also switch the other parameter to a Span.
Bug: 132
Change-Id: I4cdcf810cf2838208c8ba9fcc6215c1e369dffb8
Reviewed-on: https://boringssl-review.googlesource.com/20667
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/handshake.cc b/ssl/handshake.cc
index 6746582..141287e 100644
--- a/ssl/handshake.cc
+++ b/ssl/handshake.cc
@@ -149,7 +149,6 @@
OPENSSL_free(ecdh_public_key);
OPENSSL_free(peer_sigalgs);
OPENSSL_free(peer_supported_group_list);
- OPENSSL_free(peer_key);
OPENSSL_free(server_params);
ssl->ctx->x509_method->hs_flush_cached_ca_names(this);
OPENSSL_free(certificate_types);
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index 3916692..62e15e2 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -976,10 +976,13 @@
// Initialize ECDH and save the peer public key for later.
hs->key_share = SSLKeyShare::Create(group_id);
+ uint8_t *peer_key = nullptr;
+ size_t peer_key_len = 0;
if (!hs->key_share ||
- !CBS_stow(&point, &hs->peer_key, &hs->peer_key_len)) {
+ !CBS_stow(&point, &peer_key, &peer_key_len)) {
return ssl_hs_error;
}
+ hs->peer_key.Reset(peer_key, peer_key_len);
} else if (!(alg_k & SSL_kPSK)) {
OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_MESSAGE);
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
@@ -1229,8 +1232,7 @@
return ssl_hs_error;
}
- uint8_t *pms = NULL;
- size_t pms_len = 0;
+ Array<uint8_t> pms;
uint32_t alg_k = hs->new_cipher->algorithm_mkey;
uint32_t alg_a = hs->new_cipher->algorithm_auth;
@@ -1240,7 +1242,7 @@
if (alg_a & SSL_aPSK) {
if (ssl->psk_client_callback == NULL) {
OPENSSL_PUT_ERROR(SSL, SSL_R_PSK_NO_CLIENT_CB);
- goto err;
+ return ssl_hs_error;
}
char identity[PSK_MAX_IDENTITY_LEN + 1];
@@ -1251,7 +1253,7 @@
if (psk_len == 0) {
OPENSSL_PUT_ERROR(SSL, SSL_R_PSK_IDENTITY_NOT_FOUND);
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);
- goto err;
+ return ssl_hs_error;
}
assert(psk_len <= PSK_MAX_PSK_LEN);
@@ -1259,7 +1261,7 @@
hs->new_session->psk_identity = BUF_strdup(identity);
if (hs->new_session->psk_identity == NULL) {
OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
- goto err;
+ return ssl_hs_error;
}
// Write out psk_identity.
@@ -1268,29 +1270,26 @@
!CBB_add_bytes(&child, (const uint8_t *)identity,
OPENSSL_strnlen(identity, sizeof(identity))) ||
!CBB_flush(&body)) {
- goto err;
+ return ssl_hs_error;
}
}
- // Depending on the key exchange method, compute |pms| and |pms_len|.
+ // Depending on the key exchange method, compute |pms|.
if (alg_k & SSL_kRSA) {
- pms_len = SSL_MAX_MASTER_KEY_LENGTH;
- pms = (uint8_t *)OPENSSL_malloc(pms_len);
- if (pms == NULL) {
- OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
- goto err;
+ if (!pms.Init(SSL_MAX_MASTER_KEY_LENGTH)) {
+ return ssl_hs_error;
}
RSA *rsa = EVP_PKEY_get0_RSA(hs->peer_pubkey.get());
if (rsa == NULL) {
OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
- goto err;
+ return ssl_hs_error;
}
pms[0] = hs->client_version >> 8;
pms[1] = hs->client_version & 0xff;
if (!RAND_bytes(&pms[2], SSL_MAX_MASTER_KEY_LENGTH - 2)) {
- goto err;
+ return ssl_hs_error;
}
CBB child, *enc_pms = &body;
@@ -1298,56 +1297,50 @@
// In TLS, there is a length prefix.
if (ssl->version > SSL3_VERSION) {
if (!CBB_add_u16_length_prefixed(&body, &child)) {
- goto err;
+ return ssl_hs_error;
}
enc_pms = &child;
}
uint8_t *ptr;
if (!CBB_reserve(enc_pms, &ptr, RSA_size(rsa)) ||
- !RSA_encrypt(rsa, &enc_pms_len, ptr, RSA_size(rsa), pms, pms_len,
- RSA_PKCS1_PADDING) ||
+ !RSA_encrypt(rsa, &enc_pms_len, ptr, RSA_size(rsa), pms.data(),
+ pms.size(), RSA_PKCS1_PADDING) ||
!CBB_did_write(enc_pms, enc_pms_len) ||
!CBB_flush(&body)) {
- goto err;
+ return ssl_hs_error;
}
} else if (alg_k & SSL_kECDHE) {
// Generate a keypair and serialize the public half.
CBB child;
if (!CBB_add_u8_length_prefixed(&body, &child)) {
- goto err;
+ return ssl_hs_error;
}
// Compute the premaster.
uint8_t alert = SSL_AD_DECODE_ERROR;
- if (!hs->key_share->Accept(&child, &pms, &pms_len, &alert, hs->peer_key,
- hs->peer_key_len)) {
+ if (!hs->key_share->Accept(&child, &pms, &alert, hs->peer_key)) {
ssl3_send_alert(ssl, SSL3_AL_FATAL, alert);
- goto err;
+ return ssl_hs_error;
}
if (!CBB_flush(&body)) {
- goto err;
+ return ssl_hs_error;
}
// The key exchange state may now be discarded.
hs->key_share.reset();
- OPENSSL_free(hs->peer_key);
- hs->peer_key = NULL;
- hs->peer_key_len = 0;
+ hs->peer_key.Reset();
} else if (alg_k & SSL_kPSK) {
// For plain PSK, other_secret is a block of 0s with the same length as
// the pre-shared key.
- pms_len = psk_len;
- pms = (uint8_t *)OPENSSL_malloc(pms_len);
- if (pms == NULL) {
- OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
- goto err;
+ if (!pms.Init(psk_len)) {
+ return ssl_hs_error;
}
- OPENSSL_memset(pms, 0, pms_len);
+ OPENSSL_memset(pms.data(), 0, pms.size());
} else {
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);
OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
- goto err;
+ return ssl_hs_error;
}
// For a PSK cipher suite, other_secret is combined with the pre-shared
@@ -1358,40 +1351,33 @@
uint8_t *new_pms;
size_t new_pms_len;
- if (!CBB_init(pms_cbb.get(), 2 + psk_len + 2 + pms_len) ||
+ if (!CBB_init(pms_cbb.get(), 2 + psk_len + 2 + pms.size()) ||
!CBB_add_u16_length_prefixed(pms_cbb.get(), &child) ||
- !CBB_add_bytes(&child, pms, pms_len) ||
+ !CBB_add_bytes(&child, pms.data(), pms.size()) ||
!CBB_add_u16_length_prefixed(pms_cbb.get(), &child) ||
!CBB_add_bytes(&child, psk, psk_len) ||
!CBB_finish(pms_cbb.get(), &new_pms, &new_pms_len)) {
OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
- goto err;
+ return ssl_hs_error;
}
- OPENSSL_free(pms);
- pms = new_pms;
- pms_len = new_pms_len;
+ pms.Reset(new_pms, new_pms_len);
}
// The message must be added to the finished hash before calculating the
// master secret.
if (!ssl_add_message_cbb(ssl, cbb.get())) {
- goto err;
+ return ssl_hs_error;
}
hs->new_session->master_key_length = tls1_generate_master_secret(
- hs, hs->new_session->master_key, pms, pms_len);
+ hs, hs->new_session->master_key, pms.data(), pms.size());
if (hs->new_session->master_key_length == 0) {
- goto err;
+ return ssl_hs_error;
}
hs->new_session->extended_master_secret = hs->extended_master_secret;
- OPENSSL_free(pms);
hs->state = state_send_client_certificate_verify;
return ssl_hs_ok;
-
-err:
- OPENSSL_free(pms);
- return ssl_hs_error;
}
static enum ssl_hs_wait_t do_send_client_certificate_verify(SSL_HANDSHAKE *hs) {
diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc
index a38e25f..722b835 100644
--- a/ssl/handshake_server.cc
+++ b/ssl/handshake_server.cc
@@ -1048,12 +1048,6 @@
static enum ssl_hs_wait_t do_read_client_key_exchange(SSL_HANDSHAKE *hs) {
SSL *const ssl = hs->ssl;
-
- ssl_hs_wait_t ret = ssl_hs_error;
- uint8_t *premaster_secret = NULL;
- size_t premaster_secret_len = 0;
- uint8_t *decrypt_buf = NULL;
-
SSLMessage msg;
if (!ssl->method->get_message(ssl, &msg)) {
return ssl_hs_read_message;
@@ -1077,25 +1071,25 @@
((alg_k & SSL_kPSK) && CBS_len(&client_key_exchange) != 0)) {
OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
- goto err;
+ return ssl_hs_error;
}
if (CBS_len(&psk_identity) > PSK_MAX_IDENTITY_LEN ||
CBS_contains_zero_byte(&psk_identity)) {
OPENSSL_PUT_ERROR(SSL, SSL_R_DATA_LENGTH_TOO_LONG);
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
- goto err;
+ return ssl_hs_error;
}
if (!CBS_strdup(&psk_identity, &hs->new_session->psk_identity)) {
OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
- goto err;
+ return ssl_hs_error;
}
}
- // Depending on the key exchange method, compute |premaster_secret| and
- // |premaster_secret_len|.
+ // Depending on the key exchange method, compute |premaster_secret|.
+ Array<uint8_t> premaster_secret;
if (alg_k & SSL_kRSA) {
CBS encrypted_premaster_secret;
if (ssl->version > SSL3_VERSION) {
@@ -1104,63 +1098,56 @@
CBS_len(&client_key_exchange) != 0) {
OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
- goto err;
+ return ssl_hs_error;
}
} else {
encrypted_premaster_secret = client_key_exchange;
}
// Allocate a buffer large enough for an RSA decryption.
- const size_t rsa_size = EVP_PKEY_size(hs->local_pubkey.get());
- decrypt_buf = (uint8_t *)OPENSSL_malloc(rsa_size);
- if (decrypt_buf == NULL) {
- OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
- goto err;
+ Array<uint8_t> decrypt_buf;
+ if (!decrypt_buf.Init(EVP_PKEY_size(hs->local_pubkey.get()))) {
+ return ssl_hs_error;
}
// Decrypt with no padding. PKCS#1 padding will be removed as part of the
// timing-sensitive code below.
size_t decrypt_len;
- switch (ssl_private_key_decrypt(hs, decrypt_buf, &decrypt_len, rsa_size,
+ switch (ssl_private_key_decrypt(hs, decrypt_buf.data(), &decrypt_len,
+ decrypt_buf.size(),
CBS_data(&encrypted_premaster_secret),
CBS_len(&encrypted_premaster_secret))) {
case ssl_private_key_success:
break;
case ssl_private_key_failure:
- goto err;
+ return ssl_hs_error;
case ssl_private_key_retry:
- ret = ssl_hs_private_key_operation;
- goto err;
+ return ssl_hs_private_key_operation;
}
- if (decrypt_len != rsa_size) {
+ if (decrypt_len != decrypt_buf.size()) {
OPENSSL_PUT_ERROR(SSL, SSL_R_DECRYPTION_FAILED);
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECRYPT_ERROR);
- goto err;
+ return ssl_hs_error;
}
// Prepare a random premaster, to be used on invalid padding. See RFC 5246,
// section 7.4.7.1.
- premaster_secret_len = SSL_MAX_MASTER_KEY_LENGTH;
- premaster_secret = (uint8_t *)OPENSSL_malloc(premaster_secret_len);
- if (premaster_secret == NULL) {
- OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
- goto err;
- }
- if (!RAND_bytes(premaster_secret, premaster_secret_len)) {
- goto err;
+ if (!premaster_secret.Init(SSL_MAX_MASTER_KEY_LENGTH) ||
+ !RAND_bytes(premaster_secret.data(), premaster_secret.size())) {
+ return ssl_hs_error;
}
// The smallest padded premaster is 11 bytes of overhead. Small keys are
// publicly invalid.
- if (decrypt_len < 11 + premaster_secret_len) {
+ if (decrypt_len < 11 + premaster_secret.size()) {
OPENSSL_PUT_ERROR(SSL, SSL_R_DECRYPTION_FAILED);
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECRYPT_ERROR);
- goto err;
+ return ssl_hs_error;
}
// Check the padding. See RFC 3447, section 7.2.2.
- size_t padding_len = decrypt_len - premaster_secret_len;
+ size_t padding_len = decrypt_len - premaster_secret.size();
uint8_t good = constant_time_eq_int_8(decrypt_buf[0], 0) &
constant_time_eq_int_8(decrypt_buf[1], 2);
for (size_t i = 2; i < padding_len - 1; i++) {
@@ -1177,13 +1164,10 @@
// Select, in constant time, either the decrypted premaster or the random
// premaster based on |good|.
- for (size_t i = 0; i < premaster_secret_len; i++) {
+ for (size_t i = 0; i < premaster_secret.size(); i++) {
premaster_secret[i] = constant_time_select_8(
good, decrypt_buf[padding_len + i], premaster_secret[i]);
}
-
- OPENSSL_free(decrypt_buf);
- decrypt_buf = NULL;
} else if (alg_k & SSL_kECDHE) {
// Parse the ClientKeyExchange.
CBS peer_key;
@@ -1191,15 +1175,16 @@
CBS_len(&client_key_exchange) != 0) {
OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
- goto err;
+ return ssl_hs_error;
}
// Compute the premaster.
uint8_t alert = SSL_AD_DECODE_ERROR;
- if (!hs->key_share->Finish(&premaster_secret, &premaster_secret_len, &alert,
- CBS_data(&peer_key), CBS_len(&peer_key))) {
+ if (!hs->key_share->Finish(
+ &premaster_secret, &alert,
+ MakeConstSpan(CBS_data(&peer_key), CBS_len(&peer_key)))) {
ssl3_send_alert(ssl, SSL3_AL_FATAL, alert);
- goto err;
+ return ssl_hs_error;
}
// The key exchange state may now be discarded.
@@ -1207,7 +1192,7 @@
} else if (!(alg_k & SSL_kPSK)) {
OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);
- goto err;
+ return ssl_hs_error;
}
// For a PSK cipher suite, the actual pre-master secret is combined with the
@@ -1216,7 +1201,7 @@
if (ssl->psk_server_callback == NULL) {
OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
- goto err;
+ return ssl_hs_error;
}
// Look up the key for the identity.
@@ -1226,24 +1211,21 @@
if (psk_len > PSK_MAX_PSK_LEN) {
OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
- goto err;
+ return ssl_hs_error;
} else if (psk_len == 0) {
// PSK related to the given identity not found.
OPENSSL_PUT_ERROR(SSL, SSL_R_PSK_IDENTITY_NOT_FOUND);
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNKNOWN_PSK_IDENTITY);
- goto err;
+ return ssl_hs_error;
}
if (alg_k & SSL_kPSK) {
// In plain PSK, other_secret is a block of 0s with the same length as the
// pre-shared key.
- premaster_secret_len = psk_len;
- premaster_secret = (uint8_t *)OPENSSL_malloc(premaster_secret_len);
- if (premaster_secret == NULL) {
- OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
- goto err;
+ if (!premaster_secret.Init(psk_len)) {
+ return ssl_hs_error;
}
- OPENSSL_memset(premaster_secret, 0, premaster_secret_len);
+ OPENSSL_memset(premaster_secret.data(), 0, premaster_secret.size());
}
ScopedCBB new_premaster;
@@ -1251,47 +1233,36 @@
uint8_t *new_data;
size_t new_len;
if (!CBB_init(new_premaster.get(),
- 2 + psk_len + 2 + premaster_secret_len) ||
+ 2 + psk_len + 2 + premaster_secret.size()) ||
!CBB_add_u16_length_prefixed(new_premaster.get(), &child) ||
- !CBB_add_bytes(&child, premaster_secret, premaster_secret_len) ||
+ !CBB_add_bytes(&child, premaster_secret.data(),
+ premaster_secret.size()) ||
!CBB_add_u16_length_prefixed(new_premaster.get(), &child) ||
!CBB_add_bytes(&child, psk, psk_len) ||
!CBB_finish(new_premaster.get(), &new_data, &new_len)) {
OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
- goto err;
+ return ssl_hs_error;
}
- OPENSSL_cleanse(premaster_secret, premaster_secret_len);
- OPENSSL_free(premaster_secret);
- premaster_secret = new_data;
- premaster_secret_len = new_len;
+ premaster_secret.Reset(new_data, new_len);
}
if (!ssl_hash_message(hs, msg)) {
- goto err;
+ return ssl_hs_error;
}
// Compute the master secret.
hs->new_session->master_key_length = tls1_generate_master_secret(
- hs, hs->new_session->master_key, premaster_secret, premaster_secret_len);
+ hs, hs->new_session->master_key, premaster_secret.data(),
+ premaster_secret.size());
if (hs->new_session->master_key_length == 0) {
- goto err;
+ return ssl_hs_error;
}
hs->new_session->extended_master_secret = hs->extended_master_secret;
ssl->method->next_message(ssl);
hs->state = state_read_client_certificate_verify;
- ret = ssl_hs_ok;
-
-err:
- if (premaster_secret != NULL) {
- OPENSSL_cleanse(premaster_secret, premaster_secret_len);
- OPENSSL_free(premaster_secret);
- }
- OPENSSL_free(decrypt_buf);
-
- return ret;
-
+ return ssl_hs_ok;
}
static enum ssl_hs_wait_t do_read_client_certificate_verify(SSL_HANDSHAKE *hs) {
diff --git a/ssl/internal.h b/ssl/internal.h
index 35ba513..9b397f2 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -146,6 +146,7 @@
#include <stdlib.h>
+#include <limits>
#include <new>
#include <type_traits>
#include <utility>
@@ -154,6 +155,7 @@
#include <openssl/err.h>
#include <openssl/mem.h>
#include <openssl/ssl.h>
+#include <openssl/span.h>
#include <openssl/stack.h>
@@ -233,6 +235,90 @@
#define PURE_VIRTUAL { abort(); }
#endif
+// Array<T> is an owning array of elements of |T|.
+template <typename T>
+class Array {
+ public:
+ // Array's default constructor creates an empty array.
+ Array() {}
+ Array(const Array &) = delete;
+ Array(Array &&other) { *this = std::move(other); }
+
+ ~Array() { Reset(); }
+
+ Array &operator=(const Array &) = delete;
+ Array &operator=(Array &&other) {
+ Reset();
+ other.Release(&data_, &size_);
+ return *this;
+ }
+
+ const T *data() const { return data_; }
+ T *data() { return data_; }
+ size_t size() const { return size_; }
+
+ const T &operator[](size_t i) const { return data_[i]; }
+ T &operator[](size_t i) { return data_[i]; }
+
+ T *begin() { return data_; }
+ const T *cbegin() const { return data_; }
+ T *end() { return data_ + size_; }
+ const T *cend() const { return data_ + size_; }
+
+ void Reset() { Reset(nullptr, 0); }
+
+ // Reset releases the current contents of the array and takes ownership of the
+ // raw pointer supplied by the caller.
+ void Reset(T *new_data, size_t new_size) {
+ for (size_t i = 0; i < size_; i++) {
+ data_[i].~T();
+ }
+ OPENSSL_free(data_);
+ data_ = new_data;
+ size_ = new_size;
+ }
+
+ // Release releases ownership of the array to a raw pointer supplied by the
+ // caller.
+ void Release(T **out, size_t *out_size) {
+ *out = data_;
+ *out_size = size_;
+ data_ = nullptr;
+ size_ = 0;
+ }
+
+ // Init replaces the array with a newly-allocated array of |new_size|
+ // default-constructed copies of |T|. It returns true on success and false on
+ // error.
+ //
+ // Note that if |T| is a primitive type like |uint8_t|, it is uninitialized.
+ bool Init(size_t new_size) {
+ Reset();
+ if (new_size == 0) {
+ return true;
+ }
+
+ if (new_size > std::numeric_limits<size_t>::max() / sizeof(T)) {
+ OPENSSL_PUT_ERROR(SSL, ERR_R_OVERFLOW);
+ return false;
+ }
+ data_ = reinterpret_cast<T*>(OPENSSL_malloc(new_size * sizeof(T)));
+ if (data_ == nullptr) {
+ OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
+ return false;
+ }
+ size_ = new_size;
+ for (size_t i = 0; i < size_; i++) {
+ new (&data_[i]) T;
+ }
+ return true;
+ }
+
+ private:
+ T *data_ = nullptr;
+ size_t size_ = 0;
+};
+
// Protocol versions.
//
@@ -839,30 +925,20 @@
// Accept performs a key exchange against the |peer_key| generated by |offer|.
// On success, it returns true, writes the public value to |out_public_key|,
- // and sets |*out_secret| and |*out_secret_len| to a newly-allocated buffer
- // containing the shared secret. The caller must release this buffer with
- // |OPENSSL_free|. On failure, it returns false and sets |*out_alert| to an
- // alert to send to the peer.
+ // and sets |*out_secret| the shared secret. On failure, it returns false and
+ // sets |*out_alert| to an alert to send to the peer.
//
// The default implementation calls |Offer| and then |Finish|, assuming a key
// exchange protocol where the peers are symmetric.
- //
- // TODO(davidben): out_secret should be a smart pointer.
- virtual bool Accept(CBB *out_public_key, uint8_t **out_secret,
- size_t *out_secret_len, uint8_t *out_alert,
- const uint8_t *peer_key, size_t peer_key_len);
+ virtual bool Accept(CBB *out_public_key, Array<uint8_t> *out_secret,
+ uint8_t *out_alert, Span<const uint8_t> peer_key);
// Finish performs a key exchange against the |peer_key| generated by
- // |Accept|. On success, it returns true and sets |*out_secret| and
- // |*out_secret_len| to a newly-allocated buffer containing the shared
- // secret. The caller must release this buffer with |OPENSSL_free|. On
- // failure, it returns zero and sets |*out_alert| to an alert to send to the
- // peer.
- //
- // TODO(davidben): out_secret should be a smart pointer.
- virtual bool Finish(uint8_t **out_secret, size_t *out_secret_len,
- uint8_t *out_alert, const uint8_t *peer_key,
- size_t peer_key_len) PURE_VIRTUAL;
+ // |Accept|. On success, it returns true and sets |*out_secret| to the shared
+ // secret. On failure, it returns zero and sets |*out_alert| to an alert to
+ // send to the peer.
+ virtual bool Finish(Array<uint8_t> *out_secret, uint8_t *out_alert,
+ Span<const uint8_t> peer_key) PURE_VIRTUAL;
};
// ssl_nid_to_group_id looks up the group corresponding to |nid|. On success, it
@@ -1235,8 +1311,7 @@
size_t peer_supported_group_list_len = 0;
// peer_key is the peer's ECDH key for a TLS 1.2 client.
- uint8_t *peer_key = nullptr;
- size_t peer_key_len = 0;
+ Array<uint8_t> peer_key;
// server_params, in a TLS 1.2 server, stores the ServerKeyExchange
// parameters. It has client and server randoms prepended for signing
@@ -1414,12 +1489,11 @@
int tls13_add_finished(SSL_HANDSHAKE *hs);
int tls13_process_new_session_ticket(SSL *ssl, const SSLMessage &msg);
-int ssl_ext_key_share_parse_serverhello(SSL_HANDSHAKE *hs, uint8_t **out_secret,
- size_t *out_secret_len,
+int ssl_ext_key_share_parse_serverhello(SSL_HANDSHAKE *hs,
+ Array<uint8_t> *out_secret,
uint8_t *out_alert, CBS *contents);
int ssl_ext_key_share_parse_clienthello(SSL_HANDSHAKE *hs, bool *out_found,
- uint8_t **out_secret,
- size_t *out_secret_len,
+ Array<uint8_t> *out_secret,
uint8_t *out_alert, CBS *contents);
int ssl_ext_key_share_add_serverhello(SSL_HANDSHAKE *hs, CBB *out);
diff --git a/ssl/ssl_key_share.cc b/ssl/ssl_key_share.cc
index 207f11e..3ceb180 100644
--- a/ssl/ssl_key_share.cc
+++ b/ssl/ssl_key_share.cc
@@ -17,6 +17,8 @@
#include <assert.h>
#include <string.h>
+#include <utility>
+
#include <openssl/bn.h>
#include <openssl/bytestring.h>
#include <openssl/curve25519.h>
@@ -71,8 +73,8 @@
return true;
}
- bool Finish(uint8_t **out_secret, size_t *out_secret_len, uint8_t *out_alert,
- const uint8_t *peer_key, size_t peer_key_len) override {
+ bool Finish(Array<uint8_t> *out_secret, uint8_t *out_alert,
+ Span<const uint8_t> peer_key) override {
assert(private_key_);
*out_alert = SSL_AD_INTERNAL_ERROR;
@@ -95,8 +97,8 @@
return false;
}
- if (!EC_POINT_oct2point(group.get(), peer_point.get(), peer_key,
- peer_key_len, bn_ctx.get())) {
+ if (!EC_POINT_oct2point(group.get(), peer_point.get(), peer_key.data(),
+ peer_key.size(), bn_ctx.get())) {
*out_alert = SSL_AD_DECODE_ERROR;
return false;
}
@@ -110,14 +112,13 @@
}
// Encode the x-coordinate left-padded with zeros.
- size_t secret_len = (EC_GROUP_get_degree(group.get()) + 7) / 8;
- UniquePtr<uint8_t> secret((uint8_t *)OPENSSL_malloc(secret_len));
- if (!secret || !BN_bn2bin_padded(secret.get(), secret_len, x)) {
+ Array<uint8_t> secret;
+ if (!secret.Init((EC_GROUP_get_degree(group.get()) + 7) / 8) ||
+ !BN_bn2bin_padded(secret.data(), secret.size(), x)) {
return false;
}
- *out_secret = secret.release();
- *out_secret_len = secret_len;
+ *out_secret = std::move(secret);
return true;
}
@@ -142,24 +143,24 @@
return !!CBB_add_bytes(out, public_key, sizeof(public_key));
}
- bool Finish(uint8_t **out_secret, size_t *out_secret_len, uint8_t *out_alert,
- const uint8_t *peer_key, size_t peer_key_len) override {
+ bool Finish(Array<uint8_t> *out_secret, uint8_t *out_alert,
+ Span<const uint8_t> peer_key) override {
*out_alert = SSL_AD_INTERNAL_ERROR;
- UniquePtr<uint8_t> secret((uint8_t *)OPENSSL_malloc(32));
- if (!secret) {
+ Array<uint8_t> secret;
+ if (!secret.Init(32)) {
OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
return false;
}
- if (peer_key_len != 32 || !X25519(secret.get(), private_key_, peer_key)) {
+ if (peer_key.size() != 32 ||
+ !X25519(secret.data(), private_key_, peer_key.data())) {
*out_alert = SSL_AD_DECODE_ERROR;
OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_ECPOINT);
return false;
}
- *out_secret = secret.release();
- *out_secret_len = 32;
+ *out_secret = std::move(secret);
return true;
}
@@ -202,12 +203,11 @@
}
}
-bool SSLKeyShare::Accept(CBB *out_public_key, uint8_t **out_secret,
- size_t *out_secret_len, uint8_t *out_alert,
- const uint8_t *peer_key, size_t peer_key_len) {
+bool SSLKeyShare::Accept(CBB *out_public_key, Array<uint8_t> *out_secret,
+ uint8_t *out_alert, Span<const uint8_t> peer_key) {
*out_alert = SSL_AD_INTERNAL_ERROR;
return Offer(out_public_key) &&
- Finish(out_secret, out_secret_len, out_alert, peer_key, peer_key_len);
+ Finish(out_secret, out_alert, peer_key);
}
int ssl_nid_to_group_id(uint16_t *out_group_id, int nid) {
diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc
index ec70d27..e32ef47 100644
--- a/ssl/t1_lib.cc
+++ b/ssl/t1_lib.cc
@@ -113,6 +113,8 @@
#include <stdlib.h>
#include <string.h>
+#include <utility>
+
#include <openssl/bytestring.h>
#include <openssl/digest.h>
#include <openssl/err.h>
@@ -2167,8 +2169,8 @@
return CBB_flush(out);
}
-int ssl_ext_key_share_parse_serverhello(SSL_HANDSHAKE *hs, uint8_t **out_secret,
- size_t *out_secret_len,
+int ssl_ext_key_share_parse_serverhello(SSL_HANDSHAKE *hs,
+ Array<uint8_t> *out_secret,
uint8_t *out_alert, CBS *contents) {
CBS peer_key;
uint16_t group_id;
@@ -2185,8 +2187,9 @@
return 0;
}
- if (!hs->key_share->Finish(out_secret, out_secret_len, out_alert,
- CBS_data(&peer_key), CBS_len(&peer_key))) {
+ if (!hs->key_share->Finish(
+ out_secret, out_alert,
+ MakeConstSpan(CBS_data(&peer_key), CBS_len(&peer_key)))) {
*out_alert = SSL_AD_INTERNAL_ERROR;
return 0;
}
@@ -2197,8 +2200,7 @@
}
int ssl_ext_key_share_parse_clienthello(SSL_HANDSHAKE *hs, bool *out_found,
- uint8_t **out_secret,
- size_t *out_secret_len,
+ Array<uint8_t> *out_secret,
uint8_t *out_alert, CBS *contents) {
uint16_t group_id;
CBS key_shares;
@@ -2241,29 +2243,25 @@
if (!found) {
*out_found = false;
- *out_secret = NULL;
- *out_secret_len = 0;
+ out_secret->Reset();
return 1;
}
// Compute the DH secret.
- uint8_t *secret = NULL;
- size_t secret_len;
+ Array<uint8_t> secret;
ScopedCBB public_key;
UniquePtr<SSLKeyShare> key_share = SSLKeyShare::Create(group_id);
- if (!key_share ||
- !CBB_init(public_key.get(), 32) ||
- !key_share->Accept(public_key.get(), &secret, &secret_len, out_alert,
- CBS_data(&peer_key), CBS_len(&peer_key)) ||
+ if (!key_share || !CBB_init(public_key.get(), 32) ||
+ !key_share->Accept(
+ public_key.get(), &secret, out_alert,
+ MakeConstSpan(CBS_data(&peer_key), CBS_len(&peer_key))) ||
!CBB_finish(public_key.get(), &hs->ecdh_public_key,
&hs->ecdh_public_key_len)) {
- OPENSSL_free(secret);
*out_alert = SSL_AD_ILLEGAL_PARAMETER;
return 0;
}
- *out_secret = secret;
- *out_secret_len = secret_len;
+ *out_secret = std::move(secret);
*out_found = true;
return 1;
}
diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc
index f50b077..38df531 100644
--- a/ssl/tls13_client.cc
+++ b/ssl/tls13_client.cc
@@ -336,22 +336,16 @@
}
// Resolve ECDHE and incorporate it into the secret.
- uint8_t *dhe_secret;
- size_t dhe_secret_len;
+ Array<uint8_t> dhe_secret;
alert = SSL_AD_DECODE_ERROR;
- if (!ssl_ext_key_share_parse_serverhello(hs, &dhe_secret, &dhe_secret_len,
- &alert, &key_share)) {
+ if (!ssl_ext_key_share_parse_serverhello(hs, &dhe_secret, &alert,
+ &key_share)) {
ssl3_send_alert(ssl, SSL3_AL_FATAL, alert);
return ssl_hs_error;
}
- if (!tls13_advance_key_schedule(hs, dhe_secret, dhe_secret_len)) {
- OPENSSL_free(dhe_secret);
- return ssl_hs_error;
- }
- OPENSSL_free(dhe_secret);
-
- if (!ssl_hash_message(hs, msg) ||
+ if (!tls13_advance_key_schedule(hs, dhe_secret.data(), dhe_secret.size()) ||
+ !ssl_hash_message(hs, msg) ||
!tls13_derive_handshake_secrets(hs)) {
return ssl_hs_error;
}
diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc
index 550f3b5..ea1beae 100644
--- a/ssl/tls13_server.cc
+++ b/ssl/tls13_server.cc
@@ -74,12 +74,10 @@
}
bool found_key_share;
- uint8_t *dhe_secret;
- size_t dhe_secret_len;
+ Array<uint8_t> dhe_secret;
uint8_t alert = SSL_AD_DECODE_ERROR;
if (!ssl_ext_key_share_parse_clienthello(hs, &found_key_share, &dhe_secret,
- &dhe_secret_len, &alert,
- &key_share)) {
+ &alert, &key_share)) {
ssl3_send_alert(ssl, SSL3_AL_FATAL, alert);
return 0;
}
@@ -89,9 +87,7 @@
return 0;
}
- int ok = tls13_advance_key_schedule(hs, dhe_secret, dhe_secret_len);
- OPENSSL_free(dhe_secret);
- return ok;
+ return tls13_advance_key_schedule(hs, dhe_secret.data(), dhe_secret.size());
}
static int ssl_ext_supported_versions_add_serverhello(SSL_HANDSHAKE *hs,
