From 931ecfa03319f52fb38e61f53fc7f0cbe807d77a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Sun, 12 Jun 2022 13:24:24 +0200 Subject: [PATCH] src: fix memory leaks and refactor `ByteSource` Add ByteSource::Builder to replace the common MallocOpenSSL() + ByteSource::Allocated() pattern. Remove ByteSource::reset() that is unused. Remove ByteSource::Resize() to make ByteSource truly immutable (until moved away). Instead, ByteSource::Builder::release() takes an optional size argument that truncates the resulting ByteSource. Fix occurrences of MallocOpenSSL() that do not always free the allocated memory by using the new ByteSource::Builder class instead. Remove ByteSource::get() and replace uses with ByteSource::data(). Remove ReallocOpenSSL() because it likely only saves us a few bytes whenever we use it. PR-URL: https://github.com/nodejs/node/pull/43202 Reviewed-By: Anna Henningsen Reviewed-By: Darshan Sen Reviewed-By: James M Snell --- src/crypto/crypto_aes.cc | 95 ++++++++++++++++-------------------- src/crypto/crypto_common.cc | 3 +- src/crypto/crypto_dh.cc | 13 ++--- src/crypto/crypto_ec.cc | 82 ++++++++++++------------------- src/crypto/crypto_hash.cc | 38 +++++---------- src/crypto/crypto_hkdf.cc | 29 +++++------ src/crypto/crypto_hmac.cc | 16 +++--- src/crypto/crypto_keys.cc | 6 +-- src/crypto/crypto_pbkdf2.cc | 23 ++++----- src/crypto/crypto_random.cc | 8 +-- src/crypto/crypto_rsa.cc | 21 +++----- src/crypto/crypto_scrypt.cc | 27 +++++------ src/crypto/crypto_sig.cc | 73 ++++++++++++---------------- src/crypto/crypto_spkac.cc | 2 +- src/crypto/crypto_util.cc | 57 ++++++---------------- src/crypto/crypto_util.h | 97 +++++++++++++++++++++++-------------- 16 files changed, 255 insertions(+), 335 deletions(-) diff --git a/src/crypto/crypto_aes.cc b/src/crypto/crypto_aes.cc index 76d3e3853451d4..5f84e21fc23763 100644 --- a/src/crypto/crypto_aes.cc +++ b/src/crypto/crypto_aes.cc @@ -89,11 +89,10 @@ WebCryptoCipherStatus AES_Cipher( case kWebCryptoCipherDecrypt: // If in decrypt mode, the auth tag must be set in the params.tag. CHECK(params.tag); - if (!EVP_CIPHER_CTX_ctrl( - ctx.get(), - EVP_CTRL_AEAD_SET_TAG, - params.tag.size(), - const_cast(params.tag.get()))) { + if (!EVP_CIPHER_CTX_ctrl(ctx.get(), + EVP_CTRL_AEAD_SET_TAG, + params.tag.size(), + const_cast(params.tag.data()))) { return WebCryptoCipherStatus::FAILED; } break; @@ -125,9 +124,7 @@ WebCryptoCipherStatus AES_Cipher( return WebCryptoCipherStatus::FAILED; } - char* data = MallocOpenSSL(buf_len); - ByteSource buf = ByteSource::Allocated(data, buf_len); - unsigned char* ptr = reinterpret_cast(data); + ByteSource::Builder buf(buf_len); // In some outdated version of OpenSSL (e.g. // ubi81_sharedlibs_openssl111fips_x64) may be used in sharedlib mode, the @@ -139,20 +136,19 @@ WebCryptoCipherStatus AES_Cipher( // Refs: https://github.com/nodejs/node/pull/38913#issuecomment-866505244 if (in.size() == 0) { out_len = 0; - } else if (!EVP_CipherUpdate( - ctx.get(), - ptr, - &out_len, - in.data(), - in.size())) { + } else if (!EVP_CipherUpdate(ctx.get(), + buf.data(), + &out_len, + in.data(), + in.size())) { return WebCryptoCipherStatus::FAILED; } total += out_len; CHECK_LE(out_len, buf_len); - ptr += out_len; out_len = EVP_CIPHER_CTX_block_size(ctx.get()); - if (!EVP_CipherFinal_ex(ctx.get(), ptr, &out_len)) { + if (!EVP_CipherFinal_ex( + ctx.get(), buf.data() + total, &out_len)) { return WebCryptoCipherStatus::FAILED; } total += out_len; @@ -160,15 +156,16 @@ WebCryptoCipherStatus AES_Cipher( // If using AES_GCM, grab the generated auth tag and append // it to the end of the ciphertext. if (cipher_mode == kWebCryptoCipherEncrypt && mode == EVP_CIPH_GCM_MODE) { - data += out_len; - if (!EVP_CIPHER_CTX_ctrl(ctx.get(), EVP_CTRL_AEAD_GET_TAG, tag_len, ptr)) + if (!EVP_CIPHER_CTX_ctrl(ctx.get(), + EVP_CTRL_AEAD_GET_TAG, + tag_len, + buf.data() + total)) return WebCryptoCipherStatus::FAILED; total += tag_len; } // It's possible that we haven't used the full allocated space. Size down. - buf.Resize(total); - *out = std::move(buf); + *out = std::move(buf).release(total); return WebCryptoCipherStatus::OK; } @@ -295,24 +292,20 @@ WebCryptoCipherStatus AES_CTR_Cipher( return WebCryptoCipherStatus::FAILED; } - // Output size is identical to the input size - char* data = MallocOpenSSL(in.size()); - ByteSource buf = ByteSource::Allocated(data, in.size()); - unsigned char* ptr = reinterpret_cast(data); + // Output size is identical to the input size. + ByteSource::Builder buf(in.size()); // Also just like in chromium's implementation, if we can process // the input without wrapping the counter, we'll do it as a single // call here. If we can't, we'll fallback to the a two-step approach if (BN_cmp(remaining_until_reset.get(), num_output.get()) >= 0) { - auto status = AES_CTR_Cipher2( - key_data, - cipher_mode, - params, - in, - params.iv.data(), - ptr); - if (status == WebCryptoCipherStatus::OK) - *out = std::move(buf); + auto status = AES_CTR_Cipher2(key_data, + cipher_mode, + params, + in, + params.iv.data(), + buf.data()); + if (status == WebCryptoCipherStatus::OK) *out = std::move(buf).release(); return status; } @@ -320,13 +313,13 @@ WebCryptoCipherStatus AES_CTR_Cipher( BN_ULONG input_size_part1 = blocks_part1 * kAesBlockSize; // Encrypt the first part... - auto status = AES_CTR_Cipher2( - key_data, - cipher_mode, - params, - ByteSource::Foreign(in.get(), input_size_part1), - params.iv.data(), - ptr); + auto status = + AES_CTR_Cipher2(key_data, + cipher_mode, + params, + ByteSource::Foreign(in.data(), input_size_part1), + params.iv.data(), + buf.data()); if (status != WebCryptoCipherStatus::OK) return status; @@ -335,18 +328,16 @@ WebCryptoCipherStatus AES_CTR_Cipher( std::vector new_counter_block = BlockWithZeroedCounter(params); // Encrypt the second part... - status = AES_CTR_Cipher2( - key_data, - cipher_mode, - params, - ByteSource::Foreign( - in.get() + input_size_part1, - in.size() - input_size_part1), - new_counter_block.data(), - ptr + input_size_part1); - - if (status == WebCryptoCipherStatus::OK) - *out = std::move(buf); + status = + AES_CTR_Cipher2(key_data, + cipher_mode, + params, + ByteSource::Foreign(in.data() + input_size_part1, + in.size() - input_size_part1), + new_counter_block.data(), + buf.data() + input_size_part1); + + if (status == WebCryptoCipherStatus::OK) *out = std::move(buf).release(); return status; } diff --git a/src/crypto/crypto_common.cc b/src/crypto/crypto_common.cc index 0b172e40ee766b..3341859e04c890 100644 --- a/src/crypto/crypto_common.cc +++ b/src/crypto/crypto_common.cc @@ -525,8 +525,7 @@ MaybeLocal GetSerialNumber(Environment* env, X509* cert) { if (bn) { char* data = BN_bn2hex(bn.get()); ByteSource buf = ByteSource::Allocated(data, strlen(data)); - if (buf) - return OneByteString(env->isolate(), buf.get()); + if (buf) return OneByteString(env->isolate(), buf.data()); } } diff --git a/src/crypto/crypto_dh.cc b/src/crypto/crypto_dh.cc index ab4fc0f6e246f7..faadb97fa4cc85 100644 --- a/src/crypto/crypto_dh.cc +++ b/src/crypto/crypto_dh.cc @@ -606,18 +606,13 @@ ByteSource StatelessDiffieHellmanThreadsafe( EVP_PKEY_derive(ctx.get(), nullptr, &out_size) <= 0) return ByteSource(); - char* buf = MallocOpenSSL(out_size); - ByteSource out = ByteSource::Allocated(buf, out_size); - - if (EVP_PKEY_derive( - ctx.get(), - reinterpret_cast(buf), - &out_size) <= 0) { + ByteSource::Builder out(out_size); + if (EVP_PKEY_derive(ctx.get(), out.data(), &out_size) <= 0) { return ByteSource(); } - ZeroPadDiffieHellmanSecret(out_size, buf, out.size()); - return out; + ZeroPadDiffieHellmanSecret(out_size, out.data(), out.size()); + return std::move(out).release(); } } // namespace diff --git a/src/crypto/crypto_ec.cc b/src/crypto/crypto_ec.cc index e9fe65ffd571ca..e6a90200a43483 100644 --- a/src/crypto/crypto_ec.cc +++ b/src/crypto/crypto_ec.cc @@ -486,12 +486,9 @@ Maybe ECDHBitsTraits::AdditionalConfig( return Just(true); } -bool ECDHBitsTraits::DeriveBits( - Environment* env, - const ECDHBitsConfig& params, - ByteSource* out) { - - char* data = nullptr; +bool ECDHBitsTraits::DeriveBits(Environment* env, + const ECDHBitsConfig& params, + ByteSource* out) { size_t len = 0; ManagedEVPPKey m_privkey = params.private_->GetAsymmetricKey(); ManagedEVPPKey m_pubkey = params.public_->GetAsymmetricKey(); @@ -513,15 +510,14 @@ bool ECDHBitsTraits::DeriveBits( return false; } - data = MallocOpenSSL(len); + ByteSource::Builder buf(len); - if (EVP_PKEY_derive( - ctx.get(), - reinterpret_cast(data), - &len) <= 0) { + if (EVP_PKEY_derive(ctx.get(), buf.data(), &len) <= 0) { return false; } + *out = std::move(buf).release(len); + break; } default: { @@ -543,22 +539,18 @@ bool ECDHBitsTraits::DeriveBits( const EC_POINT* pub = EC_KEY_get0_public_key(public_key); int field_size = EC_GROUP_get_degree(group); len = (field_size + 7) / 8; - data = MallocOpenSSL(len); - CHECK_NOT_NULL(data); + ByteSource::Builder buf(len); CHECK_NOT_NULL(pub); CHECK_NOT_NULL(private_key); - if (ECDH_compute_key( - data, - len, - pub, - private_key, - nullptr) <= 0) { + if (ECDH_compute_key(buf.data(), len, pub, private_key, nullptr) <= + 0) { return false; } + + *out = std::move(buf).release(); } } - ByteSource buf = ByteSource::Allocated(data, len); - *out = std::move(buf); + return true; } @@ -646,7 +638,6 @@ WebCryptoKeyExportStatus EC_Raw_Export( const EC_KEY* ec_key = EVP_PKEY_get0_EC_KEY(m_pkey.get()); - unsigned char* data; size_t len = 0; if (ec_key == nullptr) { @@ -666,9 +657,10 @@ WebCryptoKeyExportStatus EC_Raw_Export( // Get the size of the raw key data if (fn(m_pkey.get(), nullptr, &len) == 0) return WebCryptoKeyExportStatus::INVALID_KEY_TYPE; - data = MallocOpenSSL(len); - if (fn(m_pkey.get(), data, &len) == 0) + ByteSource::Builder data(len); + if (fn(m_pkey.get(), data.data(), &len) == 0) return WebCryptoKeyExportStatus::INVALID_KEY_TYPE; + *out = std::move(data).release(len); } else { if (key_data->GetKeyType() != kKeyTypePublic) return WebCryptoKeyExportStatus::INVALID_KEY_TYPE; @@ -680,17 +672,16 @@ WebCryptoKeyExportStatus EC_Raw_Export( len = EC_POINT_point2oct(group, point, form, nullptr, 0, nullptr); if (len == 0) return WebCryptoKeyExportStatus::FAILED; - data = MallocOpenSSL(len); - size_t check_len = - EC_POINT_point2oct(group, point, form, data, len, nullptr); + ByteSource::Builder data(len); + size_t check_len = EC_POINT_point2oct( + group, point, form, data.data(), len, nullptr); if (check_len == 0) return WebCryptoKeyExportStatus::FAILED; CHECK_EQ(len, check_len); + *out = std::move(data).release(); } - *out = ByteSource::Allocated(reinterpret_cast(data), len); - return WebCryptoKeyExportStatus::OK; } } // namespace @@ -853,38 +844,27 @@ Maybe ExportJWKEdKey( if (!EVP_PKEY_get_raw_public_key(pkey.get(), nullptr, &len)) return Nothing(); - unsigned char* data = MallocOpenSSL(len); - ByteSource out = ByteSource::Allocated(reinterpret_cast(data), len); + ByteSource::Builder out(len); if (key->GetKeyType() == kKeyTypePrivate) { - if (!EVP_PKEY_get_raw_private_key(pkey.get(), data, &len) || + if (!EVP_PKEY_get_raw_private_key( + pkey.get(), out.data(), &len) || !StringBytes::Encode( - env->isolate(), - reinterpret_cast(data), - len, - BASE64URL, - &error).ToLocal(&encoded) || - !target->Set( - env->context(), - env->jwk_d_string(), - encoded).IsJust()) { + env->isolate(), out.data(), len, BASE64URL, &error) + .ToLocal(&encoded) || + !target->Set(env->context(), env->jwk_d_string(), encoded).IsJust()) { if (!error.IsEmpty()) env->isolate()->ThrowException(error); return Nothing(); } } - if (!EVP_PKEY_get_raw_public_key(pkey.get(), data, &len) || + if (!EVP_PKEY_get_raw_public_key( + pkey.get(), out.data(), &len) || !StringBytes::Encode( - env->isolate(), - reinterpret_cast(data), - len, - BASE64URL, - &error).ToLocal(&encoded) || - !target->Set( - env->context(), - env->jwk_x_string(), - encoded).IsJust()) { + env->isolate(), out.data(), len, BASE64URL, &error) + .ToLocal(&encoded) || + !target->Set(env->context(), env->jwk_x_string(), encoded).IsJust()) { if (!error.IsEmpty()) env->isolate()->ThrowException(error); return Nothing(); diff --git a/src/crypto/crypto_hash.cc b/src/crypto/crypto_hash.cc index 8f7128569c7aa5..24dc436d24855e 100644 --- a/src/crypto/crypto_hash.cc +++ b/src/crypto/crypto_hash.cc @@ -171,38 +171,29 @@ void Hash::HashDigest(const FunctionCallbackInfo& args) { // so we need to cache it. // See https://github.com/nodejs/node/issues/28245. - char* md_value = MallocOpenSSL(len); - ByteSource digest = ByteSource::Allocated(md_value, len); + ByteSource::Builder digest(len); size_t default_len = EVP_MD_CTX_size(hash->mdctx_.get()); int ret; if (len == default_len) { ret = EVP_DigestFinal_ex( - hash->mdctx_.get(), - reinterpret_cast(md_value), - &len); + hash->mdctx_.get(), digest.data(), &len); // The output length should always equal hash->md_len_ CHECK_EQ(len, hash->md_len_); } else { ret = EVP_DigestFinalXOF( - hash->mdctx_.get(), - reinterpret_cast(md_value), - len); + hash->mdctx_.get(), digest.data(), len); } if (ret != 1) return ThrowCryptoError(env, ERR_get_error()); - hash->digest_ = std::move(digest); + hash->digest_ = std::move(digest).release(); } Local error; - MaybeLocal rc = - StringBytes::Encode(env->isolate(), - hash->digest_.get(), - len, - encoding, - &error); + MaybeLocal rc = StringBytes::Encode( + env->isolate(), hash->digest_.data(), len, encoding, &error); if (rc.IsEmpty()) { CHECK(!error.IsEmpty()); env->isolate()->ThrowException(error); @@ -291,28 +282,25 @@ bool HashTraits::DeriveBits( if (UNLIKELY(!ctx || EVP_DigestInit_ex(ctx.get(), params.digest, nullptr) <= 0 || EVP_DigestUpdate( - ctx.get(), - params.in.get(), - params.in.size()) <= 0)) { + ctx.get(), params.in.data(), params.in.size()) <= 0)) { return false; } if (LIKELY(params.length > 0)) { unsigned int length = params.length; - char* data = MallocOpenSSL(length); - ByteSource buf = ByteSource::Allocated(data, length); - unsigned char* ptr = reinterpret_cast(data); + ByteSource::Builder buf(length); size_t expected = EVP_MD_CTX_size(ctx.get()); - int ret = (length == expected) - ? EVP_DigestFinal_ex(ctx.get(), ptr, &length) - : EVP_DigestFinalXOF(ctx.get(), ptr, length); + int ret = + (length == expected) + ? EVP_DigestFinal_ex(ctx.get(), buf.data(), &length) + : EVP_DigestFinalXOF(ctx.get(), buf.data(), length); if (UNLIKELY(ret != 1)) return false; - *out = std::move(buf); + *out = std::move(buf).release(); } return true; diff --git a/src/crypto/crypto_hkdf.cc b/src/crypto/crypto_hkdf.cc index b1efcbe55fa898..79a84c12f55853 100644 --- a/src/crypto/crypto_hkdf.cc +++ b/src/crypto/crypto_hkdf.cc @@ -102,34 +102,27 @@ bool HKDFTraits::DeriveBits( ByteSource* out) { EVPKeyCtxPointer ctx = EVPKeyCtxPointer(EVP_PKEY_CTX_new_id(EVP_PKEY_HKDF, nullptr)); - if (!ctx || - !EVP_PKEY_derive_init(ctx.get()) || - !EVP_PKEY_CTX_hkdf_mode( - ctx.get(), EVP_PKEY_HKDEF_MODE_EXTRACT_AND_EXPAND) || + if (!ctx || !EVP_PKEY_derive_init(ctx.get()) || + !EVP_PKEY_CTX_hkdf_mode(ctx.get(), + EVP_PKEY_HKDEF_MODE_EXTRACT_AND_EXPAND) || !EVP_PKEY_CTX_set_hkdf_md(ctx.get(), params.digest) || !EVP_PKEY_CTX_set1_hkdf_salt( - ctx.get(), - reinterpret_cast(params.salt.get()), - params.salt.size()) || + ctx.get(), params.salt.data(), params.salt.size()) || !EVP_PKEY_CTX_set1_hkdf_key( - ctx.get(), - reinterpret_cast(params.key->GetSymmetricKey()), - params.key->GetSymmetricKeySize()) || + ctx.get(), + reinterpret_cast(params.key->GetSymmetricKey()), + params.key->GetSymmetricKeySize()) || !EVP_PKEY_CTX_add1_hkdf_info( - ctx.get(), - reinterpret_cast(params.info.get()), - params.info.size())) { + ctx.get(), params.info.data(), params.info.size())) { return false; } size_t length = params.length; - char* data = MallocOpenSSL(length); - ByteSource buf = ByteSource::Allocated(data, length); - unsigned char* ptr = reinterpret_cast(data); - if (EVP_PKEY_derive(ctx.get(), ptr, &length) <= 0) + ByteSource::Builder buf(length); + if (EVP_PKEY_derive(ctx.get(), buf.data(), &length) <= 0) return false; - *out = std::move(buf); + *out = std::move(buf).release(); return true; } diff --git a/src/crypto/crypto_hmac.cc b/src/crypto/crypto_hmac.cc index 296ae541a3e68f..afda92265e8dca 100644 --- a/src/crypto/crypto_hmac.cc +++ b/src/crypto/crypto_hmac.cc @@ -88,7 +88,7 @@ void Hmac::HmacInit(const FunctionCallbackInfo& args) { const node::Utf8Value hash_type(env->isolate(), args[0]); ByteSource key = ByteSource::FromSecretKeyBytes(env, args[1]); - hmac->HmacInit(*hash_type, key.get(), key.size()); + hmac->HmacInit(*hash_type, key.data(), key.size()); } bool Hmac::HmacUpdate(const char* data, size_t len) { @@ -241,17 +241,14 @@ bool HmacTraits::DeriveBits( return false; } - char* data = MallocOpenSSL(EVP_MAX_MD_SIZE); - ByteSource buf = ByteSource::Allocated(data, EVP_MAX_MD_SIZE); - unsigned char* ptr = reinterpret_cast(data); + ByteSource::Builder buf(EVP_MAX_MD_SIZE); unsigned int len; - if (!HMAC_Final(ctx.get(), ptr, &len)) { + if (!HMAC_Final(ctx.get(), buf.data(), &len)) { return false; } - buf.Resize(len); - *out = std::move(buf); + *out = std::move(buf).release(len); return true; } @@ -267,9 +264,8 @@ Maybe HmacTraits::EncodeOutput( break; case SignConfiguration::kVerify: *result = - out->size() > 0 && - out->size() == params.signature.size() && - memcmp(out->get(), params.signature.get(), out->size()) == 0 + out->size() > 0 && out->size() == params.signature.size() && + memcmp(out->data(), params.signature.data(), out->size()) == 0 ? v8::True(env->isolate()) : v8::False(env->isolate()); break; diff --git a/src/crypto/crypto_keys.cc b/src/crypto/crypto_keys.cc index 5dd04edfbfa1cf..ba37f24c48dad5 100644 --- a/src/crypto/crypto_keys.cc +++ b/src/crypto/crypto_keys.cc @@ -305,7 +305,7 @@ MaybeLocal WritePrivateKey( char* pass = nullptr; size_t pass_len = 0; if (!config.passphrase_.IsEmpty()) { - pass = const_cast(config.passphrase_->get()); + pass = const_cast(config.passphrase_->data()); pass_len = config.passphrase_->size(); if (pass == nullptr) { // OpenSSL will not actually dereference this pointer, so it can be any @@ -744,7 +744,7 @@ ManagedEVPPKey ManagedEVPPKey::GetPrivateKeyFromJs( EVPKeyPointer pkey; ParseKeyResult ret = - ParsePrivateKey(&pkey, config.Release(), key.get(), key.size()); + ParsePrivateKey(&pkey, config.Release(), key.data(), key.size()); return GetParsedKey(env, std::move(pkey), ret, "Failed to read private key"); } else { @@ -893,7 +893,7 @@ ManagedEVPPKey KeyObjectData::GetAsymmetricKey() const { const char* KeyObjectData::GetSymmetricKey() const { CHECK_EQ(key_type_, kKeyTypeSecret); - return symmetric_key_.get(); + return symmetric_key_.data(); } size_t KeyObjectData::GetSymmetricKeySize() const { diff --git a/src/crypto/crypto_pbkdf2.cc b/src/crypto/crypto_pbkdf2.cc index 345c7fe5ce6c15..963d0db6c62a45 100644 --- a/src/crypto/crypto_pbkdf2.cc +++ b/src/crypto/crypto_pbkdf2.cc @@ -115,26 +115,23 @@ bool PBKDF2Traits::DeriveBits( Environment* env, const PBKDF2Config& params, ByteSource* out) { - char* data = MallocOpenSSL(params.length); - ByteSource buf = ByteSource::Allocated(data, params.length); - unsigned char* ptr = reinterpret_cast(data); + ByteSource::Builder buf(params.length); // Both pass and salt may be zero length here. // The generated bytes are stored in buf, which is // assigned to out on success. - if (PKCS5_PBKDF2_HMAC( - params.pass.get(), - params.pass.size(), - params.salt.data(), - params.salt.size(), - params.iterations, - params.digest, - params.length, - ptr) <= 0) { + if (PKCS5_PBKDF2_HMAC(params.pass.data(), + params.pass.size(), + params.salt.data(), + params.salt.size(), + params.iterations, + params.digest, + params.length, + buf.data()) <= 0) { return false; } - *out = std::move(buf); + *out = std::move(buf).release(); return true; } diff --git a/src/crypto/crypto_random.cc b/src/crypto/crypto_random.cc index 648fda211c4305..d0736a9cf1277a 100644 --- a/src/crypto/crypto_random.cc +++ b/src/crypto/crypto_random.cc @@ -222,9 +222,9 @@ bool CheckPrimeTraits::DeriveBits( ctx.get(), nullptr); if (ret < 0) return false; - char* data = MallocOpenSSL(1); - data[0] = ret; - *out = ByteSource::Allocated(data, 1); + ByteSource::Builder buf(1); + buf.data()[0] = ret; + *out = std::move(buf).release(); return true; } @@ -233,7 +233,7 @@ Maybe CheckPrimeTraits::EncodeOutput( const CheckPrimeConfig& params, ByteSource* out, v8::Local* result) { - *result = out->get()[0] ? True(env->isolate()) : False(env->isolate()); + *result = out->data()[0] ? True(env->isolate()) : False(env->isolate()); return Just(true); } diff --git a/src/crypto/crypto_rsa.cc b/src/crypto/crypto_rsa.cc index bd732a70a8ffe6..57cec1a8fd2b51 100644 --- a/src/crypto/crypto_rsa.cc +++ b/src/crypto/crypto_rsa.cc @@ -223,7 +223,7 @@ WebCryptoCipherStatus RSA_Cipher( size_t label_len = params.label.size(); if (label_len > 0) { - void* label = OPENSSL_memdup(params.label.get(), label_len); + void* label = OPENSSL_memdup(params.label.data(), label_len); CHECK_NOT_NULL(label); if (EVP_PKEY_CTX_set0_rsa_oaep_label( ctx.get(), @@ -244,22 +244,17 @@ WebCryptoCipherStatus RSA_Cipher( return WebCryptoCipherStatus::FAILED; } - char* data = MallocOpenSSL(out_len); - ByteSource buf = ByteSource::Allocated(data, out_len); - unsigned char* ptr = reinterpret_cast(data); + ByteSource::Builder buf(out_len); - if (cipher( - ctx.get(), - ptr, - &out_len, - in.data(), - in.size()) <= 0) { + if (cipher(ctx.get(), + buf.data(), + &out_len, + in.data(), + in.size()) <= 0) { return WebCryptoCipherStatus::FAILED; } - buf.Resize(out_len); - - *out = std::move(buf); + *out = std::move(buf).release(out_len); return WebCryptoCipherStatus::OK; } } // namespace diff --git a/src/crypto/crypto_scrypt.cc b/src/crypto/crypto_scrypt.cc index 4ddf705703c723..88d355446c0984 100644 --- a/src/crypto/crypto_scrypt.cc +++ b/src/crypto/crypto_scrypt.cc @@ -121,26 +121,23 @@ bool ScryptTraits::DeriveBits( Environment* env, const ScryptConfig& params, ByteSource* out) { - char* data = MallocOpenSSL(params.length); - ByteSource buf = ByteSource::Allocated(data, params.length); - unsigned char* ptr = reinterpret_cast(data); + ByteSource::Builder buf(params.length); // Both the pass and salt may be zero-length at this point - if (!EVP_PBE_scrypt( - params.pass.get(), - params.pass.size(), - params.salt.data(), - params.salt.size(), - params.N, - params.r, - params.p, - params.maxmem, - ptr, - params.length)) { + if (!EVP_PBE_scrypt(params.pass.data(), + params.pass.size(), + params.salt.data(), + params.salt.size(), + params.N, + params.r, + params.p, + params.maxmem, + buf.data(), + params.length)) { return false; } - *out = std::move(buf); + *out = std::move(buf).release(); return true; } diff --git a/src/crypto/crypto_sig.cc b/src/crypto/crypto_sig.cc index c4483a71744e35..a72649ab8ec16f 100644 --- a/src/crypto/crypto_sig.cc +++ b/src/crypto/crypto_sig.cc @@ -174,18 +174,15 @@ ByteSource ConvertSignatureToP1363( if (n == kNoDsaSignature) return ByteSource(); - const unsigned char* sig_data = - reinterpret_cast(signature.get()); + const unsigned char* sig_data = signature.data(); - char* outdata = MallocOpenSSL(n * 2); - memset(outdata, 0, n * 2); - ByteSource out = ByteSource::Allocated(outdata, n * 2); - unsigned char* ptr = reinterpret_cast(outdata); + ByteSource::Builder out(n * 2); + memset(out.data(), 0, n * 2); - if (!ExtractP1363(sig_data, ptr, signature.size(), n)) + if (!ExtractP1363(sig_data, out.data(), signature.size(), n)) return ByteSource(); - return out; + return std::move(out).release(); } ByteSource ConvertSignatureToDER( @@ -195,8 +192,7 @@ ByteSource ConvertSignatureToDER( if (n == kNoDsaSignature) return std::move(out); - const unsigned char* sig_data = - reinterpret_cast(out.get()); + const unsigned char* sig_data = out.data(); if (out.size() != 2 * n) return ByteSource(); @@ -526,7 +522,7 @@ SignBase::Error Verify::VerifyFinal(const ManagedEVPPKey& pkey, ApplyRSAOptions(pkey, pkctx.get(), padding, saltlen) && EVP_PKEY_CTX_set_signature_md(pkctx.get(), EVP_MD_CTX_md(mdctx.get())) > 0) { - const unsigned char* s = reinterpret_cast(sig.get()); + const unsigned char* s = sig.data(); const int r = EVP_PKEY_verify(pkctx.get(), s, sig.size(), m, m_len); *verify_result = r == 1; } @@ -570,7 +566,7 @@ void Verify::VerifyFinal(const FunctionCallbackInfo& args) { ByteSource signature = hbuf.ToByteSource(); if (dsa_sig_enc == kSigEncP1363) { signature = ConvertSignatureToDER(pkey, hbuf.ToByteSource()); - if (signature.get() == nullptr) + if (signature.data() == nullptr) return crypto::CheckThrow(env, Error::kSignMalformedSignature); } @@ -746,9 +742,8 @@ bool SignTraits::DeriveBits( switch (params.mode) { case SignConfiguration::kSign: { - size_t len; - unsigned char* data = nullptr; if (IsOneShot(params.key)) { + size_t len; if (!EVP_DigestSign( context.get(), nullptr, @@ -758,20 +753,18 @@ bool SignTraits::DeriveBits( crypto::CheckThrow(env, SignBase::Error::kSignPrivateKey); return false; } - data = MallocOpenSSL(len); - if (!EVP_DigestSign( - context.get(), - data, - &len, - params.data.data(), - params.data.size())) { - crypto::CheckThrow(env, SignBase::Error::kSignPrivateKey); - return false; - } - ByteSource buf = - ByteSource::Allocated(reinterpret_cast(data), len); - *out = std::move(buf); + ByteSource::Builder buf(len); + if (!EVP_DigestSign(context.get(), + buf.data(), + &len, + params.data.data(), + params.data.size())) { + crypto::CheckThrow(env, SignBase::Error::kSignPrivateKey); + return false; + } + *out = std::move(buf).release(len); } else { + size_t len; if (!EVP_DigestSignUpdate( context.get(), params.data.data(), @@ -780,35 +773,34 @@ bool SignTraits::DeriveBits( crypto::CheckThrow(env, SignBase::Error::kSignPrivateKey); return false; } - data = MallocOpenSSL(len); - ByteSource buf = - ByteSource::Allocated(reinterpret_cast(data), len); - if (!EVP_DigestSignFinal(context.get(), data, &len)) { + ByteSource::Builder buf(len); + if (!EVP_DigestSignFinal( + context.get(), buf.data(), &len)) { crypto::CheckThrow(env, SignBase::Error::kSignPrivateKey); return false; } if (UseP1363Encoding(params.key, params.dsa_encoding)) { - *out = ConvertSignatureToP1363(env, params.key, buf); + *out = ConvertSignatureToP1363( + env, params.key, std::move(buf).release()); } else { - buf.Resize(len); - *out = std::move(buf); + *out = std::move(buf).release(len); } } break; } case SignConfiguration::kVerify: { - char* data = MallocOpenSSL(1); - data[0] = 0; - *out = ByteSource::Allocated(data, 1); + ByteSource::Builder buf(1); + buf.data()[0] = 0; if (EVP_DigestVerify( context.get(), params.signature.data(), params.signature.size(), params.data.data(), params.data.size()) == 1) { - data[0] = 1; + buf.data()[0] = 1; } + *out = std::move(buf).release(); } } @@ -825,9 +817,8 @@ Maybe SignTraits::EncodeOutput( *result = out->ToArrayBuffer(env); break; case SignConfiguration::kVerify: - *result = out->get()[0] == 1 - ? v8::True(env->isolate()) - : v8::False(env->isolate()); + *result = out->data()[0] == 1 ? v8::True(env->isolate()) + : v8::False(env->isolate()); break; default: UNREACHABLE(); diff --git a/src/crypto/crypto_spkac.cc b/src/crypto/crypto_spkac.cc index c29d94edc0f9f9..7cda346907e421 100644 --- a/src/crypto/crypto_spkac.cc +++ b/src/crypto/crypto_spkac.cc @@ -122,7 +122,7 @@ void ExportChallenge(const FunctionCallbackInfo& args) { return args.GetReturnValue().SetEmptyString(); Local outString = - Encode(env->isolate(), cert.get(), cert.size(), BUFFER); + Encode(env->isolate(), cert.data(), cert.size(), BUFFER); args.GetReturnValue().Set(outString); } diff --git a/src/crypto/crypto_util.cc b/src/crypto/crypto_util.cc index 58a5d88d7a10de..5d8f0bbe8e3cb4 100644 --- a/src/crypto/crypto_util.cc +++ b/src/crypto/crypto_util.cc @@ -89,7 +89,7 @@ int PasswordCallback(char* buf, int size, int rwflag, void* u) { size_t len = passphrase->size(); if (buflen < len) return -1; - memcpy(buf, passphrase->get(), len); + memcpy(buf, passphrase->data(), len); return len; } @@ -327,12 +327,6 @@ ByteSource::~ByteSource() { OPENSSL_clear_free(allocated_data_, size_); } -void ByteSource::reset() { - OPENSSL_clear_free(allocated_data_, size_); - data_ = nullptr; - size_ = 0; -} - ByteSource& ByteSource::operator=(ByteSource&& other) noexcept { if (&other != this) { OPENSSL_clear_free(allocated_data_, size_); @@ -371,45 +365,29 @@ MaybeLocal ByteSource::ToBuffer(Environment* env) { return Buffer::New(env, ab, 0, ab->ByteLength()); } -const char* ByteSource::get() const { - return data_; -} - -size_t ByteSource::size() const { - return size_; -} - ByteSource ByteSource::FromBIO(const BIOPointer& bio) { CHECK(bio); BUF_MEM* bptr; BIO_get_mem_ptr(bio.get(), &bptr); - char* data = MallocOpenSSL(bptr->length); - memcpy(data, bptr->data, bptr->length); - return Allocated(data, bptr->length); + ByteSource::Builder out(bptr->length); + memcpy(out.data(), bptr->data, bptr->length); + return std::move(out).release(); } ByteSource ByteSource::FromEncodedString(Environment* env, Local key, enum encoding enc) { size_t length = 0; - size_t actual = 0; - char* data = nullptr; + ByteSource out; if (StringBytes::Size(env->isolate(), key, enc).To(&length) && length > 0) { - data = MallocOpenSSL(length); - actual = StringBytes::Write(env->isolate(), data, length, key, enc); - - CHECK(actual <= length); - - if (actual == 0) { - OPENSSL_clear_free(data, length); - data = nullptr; - } else if (actual < length) { - data = reinterpret_cast(OPENSSL_realloc(data, actual)); - } + ByteSource::Builder buf(length); + size_t actual = + StringBytes::Write(env->isolate(), buf.data(), length, key, enc); + out = std::move(buf).release(actual); } - return Allocated(data, actual); + return out; } ByteSource ByteSource::FromStringOrBuffer(Environment* env, @@ -423,11 +401,11 @@ ByteSource ByteSource::FromString(Environment* env, Local str, CHECK(str->IsString()); size_t size = str->Utf8Length(env->isolate()); size_t alloc_size = ntc ? size + 1 : size; - char* data = MallocOpenSSL(alloc_size); + ByteSource::Builder out(alloc_size); int opts = String::NO_OPTIONS; if (!ntc) opts |= String::NO_NULL_TERMINATION; - str->WriteUtf8(env->isolate(), data, alloc_size, nullptr, opts); - return Allocated(data, size); + str->WriteUtf8(env->isolate(), out.data(), alloc_size, nullptr, opts); + return std::move(out).release(); } ByteSource ByteSource::FromBuffer(Local buffer, bool ntc) { @@ -460,16 +438,11 @@ ByteSource ByteSource::FromSymmetricKeyObjectHandle(Local handle) { key->Data()->GetSymmetricKeySize()); } -ByteSource::ByteSource(const char* data, char* allocated_data, size_t size) - : data_(data), - allocated_data_(allocated_data), - size_(size) {} - -ByteSource ByteSource::Allocated(char* data, size_t size) { +ByteSource ByteSource::Allocated(void* data, size_t size) { return ByteSource(data, data, size); } -ByteSource ByteSource::Foreign(const char* data, size_t size) { +ByteSource ByteSource::Foreign(const void* data, size_t size) { return ByteSource(data, nullptr, size); } diff --git a/src/crypto/crypto_util.h b/src/crypto/crypto_util.h index 7a795fe5e81f94..e5d9410039cad1 100644 --- a/src/crypto/crypto_util.h +++ b/src/crypto/crypto_util.h @@ -29,11 +29,12 @@ #endif // OPENSSL_FIPS #include +#include +#include #include +#include #include #include -#include -#include namespace node { namespace crypto { @@ -218,37 +219,74 @@ T* MallocOpenSSL(size_t count) { return static_cast(mem); } -template -T* ReallocOpenSSL(T* buf, size_t count) { - void* mem = OPENSSL_realloc(buf, MultiplyWithOverflowCheck(count, sizeof(T))); - CHECK_IMPLIES(mem == nullptr, count == 0); - return static_cast(mem); -} - // A helper class representing a read-only byte array. When deallocated, its // contents are zeroed. class ByteSource { public: + class Builder { + public: + // Allocates memory using OpenSSL's memory allocator. + explicit Builder(size_t size) + : data_(MallocOpenSSL(size)), size_(size) {} + + Builder(Builder&& other) = delete; + Builder& operator=(Builder&& other) = delete; + Builder(const Builder&) = delete; + Builder& operator=(const Builder&) = delete; + + ~Builder() { OPENSSL_clear_free(data_, size_); } + + // Returns the underlying non-const pointer. + template + T* data() { + return reinterpret_cast(data_); + } + + // Returns the (allocated) size in bytes. + size_t size() const { return size_; } + + // Finalizes the Builder and returns a read-only view that is optionally + // truncated. + ByteSource release(std::optional resize = std::nullopt) && { + if (resize) { + CHECK_LE(*resize, size_); + if (*resize == 0) { + OPENSSL_clear_free(data_, size_); + data_ = nullptr; + } + size_ = *resize; + } + ByteSource out = ByteSource::Allocated(data_, size_); + data_ = nullptr; + size_ = 0; + return out; + } + + private: + void* data_; + size_t size_; + }; + ByteSource() = default; ByteSource(ByteSource&& other) noexcept; ~ByteSource(); ByteSource& operator=(ByteSource&& other) noexcept; - const char* get() const; + ByteSource(const ByteSource&) = delete; + ByteSource& operator=(const ByteSource&) = delete; - template - const T* data() const { return reinterpret_cast(get()); } + template + const T* data() const { + return reinterpret_cast(data_); + } - size_t size() const; + size_t size() const { return size_; } operator bool() const { return data_ != nullptr; } BignumPointer ToBN() const { - return BignumPointer(BN_bin2bn( - reinterpret_cast(get()), - size(), - nullptr)); + return BignumPointer(BN_bin2bn(data(), size(), nullptr)); } // Creates a v8::BackingStore that takes over responsibility for @@ -260,19 +298,8 @@ class ByteSource { v8::MaybeLocal ToBuffer(Environment* env); - void reset(); - - // Allows an Allocated ByteSource to be truncated. - void Resize(size_t newsize) { - CHECK_LE(newsize, size_); - CHECK_NOT_NULL(allocated_data_); - char* new_data_ = ReallocOpenSSL(allocated_data_, newsize); - data_ = allocated_data_ = new_data_; - size_ = newsize; - } - - static ByteSource Allocated(char* data, size_t size); - static ByteSource Foreign(const char* data, size_t size); + static ByteSource Allocated(void* data, size_t size); + static ByteSource Foreign(const void* data, size_t size); static ByteSource FromEncodedString(Environment* env, v8::Local value, @@ -295,18 +322,16 @@ class ByteSource { static ByteSource FromSymmetricKeyObjectHandle(v8::Local handle); - ByteSource(const ByteSource&) = delete; - ByteSource& operator=(const ByteSource&) = delete; - static ByteSource FromSecretKeyBytes( Environment* env, v8::Local value); private: - const char* data_ = nullptr; - char* allocated_data_ = nullptr; + const void* data_ = nullptr; + void* allocated_data_ = nullptr; size_t size_ = 0; - ByteSource(const char* data, char* allocated_data, size_t size); + ByteSource(const void* data, void* allocated_data, size_t size) + : data_(data), allocated_data_(allocated_data), size_(size) {} }; enum CryptoJobMode {