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,