Skip to content

Commit

Permalink
src: fix memory leaks and refactor ByteSource
Browse files Browse the repository at this point in the history 
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: #43202
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
@tniessen @danielleadams
tniessen authored and danielleadams committed Jun 13, 2022
1 parent 7e8a00a commit 931ecfa
Show file tree
Hide file tree
Showing 16 changed files with 255 additions and 335 deletions.
95 changes: 43 additions & 52 deletions src/crypto/crypto_aes.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<char*>(params.tag.get()))) {
if (!EVP_CIPHER_CTX_ctrl(ctx.get(),
EVP_CTRL_AEAD_SET_TAG,
params.tag.size(),
const_cast<char*>(params.tag.data<char>()))) {
return WebCryptoCipherStatus::FAILED;
}
break;
Expand Down Expand Up @@ -125,9 +124,7 @@ WebCryptoCipherStatus AES_Cipher(
return WebCryptoCipherStatus::FAILED;
}

char* data = MallocOpenSSL<char>(buf_len);
ByteSource buf = ByteSource::Allocated(data, buf_len);
unsigned char* ptr = reinterpret_cast<unsigned char*>(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
Expand All @@ -139,36 +136,36 @@ 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<unsigned char>(),
in.size())) {
} else if (!EVP_CipherUpdate(ctx.get(),
buf.data<unsigned char>(),
&out_len,
in.data<unsigned char>(),
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<unsigned char>() + total, &out_len)) {
return WebCryptoCipherStatus::FAILED;
}
total += out_len;

// 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<unsigned char>() + 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;
}
Expand Down Expand Up @@ -295,38 +292,34 @@ WebCryptoCipherStatus AES_CTR_Cipher(
return WebCryptoCipherStatus::FAILED;
}

// Output size is identical to the input size
char* data = MallocOpenSSL<char>(in.size());
ByteSource buf = ByteSource::Allocated(data, in.size());
unsigned char* ptr = reinterpret_cast<unsigned char*>(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<unsigned char>(),
ptr);
if (status == WebCryptoCipherStatus::OK)
*out = std::move(buf);
auto status = AES_CTR_Cipher2(key_data,
cipher_mode,
params,
in,
params.iv.data<unsigned char>(),
buf.data<unsigned char>());
if (status == WebCryptoCipherStatus::OK) *out = std::move(buf).release();
return status;
}

BN_ULONG blocks_part1 = BN_get_word(remaining_until_reset.get());
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<unsigned char>(),
ptr);
auto status =
AES_CTR_Cipher2(key_data,
cipher_mode,
params,
ByteSource::Foreign(in.data<char>(), input_size_part1),
params.iv.data<unsigned char>(),
buf.data<unsigned char>());

if (status != WebCryptoCipherStatus::OK)
return status;
Expand All @@ -335,18 +328,16 @@ WebCryptoCipherStatus AES_CTR_Cipher(
std::vector<unsigned char> 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<char>() + input_size_part1,
in.size() - input_size_part1),
new_counter_block.data(),
buf.data<unsigned char>() + input_size_part1);

if (status == WebCryptoCipherStatus::OK) *out = std::move(buf).release();

return status;
}
Expand Down
3 changes: 1 addition & 2 deletions src/crypto/crypto_common.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -525,8 +525,7 @@ MaybeLocal<Value> 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<unsigned char>());
}
}

Expand Down
13 changes: 4 additions & 9 deletions src/crypto/crypto_dh.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -606,18 +606,13 @@ ByteSource StatelessDiffieHellmanThreadsafe(
EVP_PKEY_derive(ctx.get(), nullptr, &out_size) <= 0)
return ByteSource();

char* buf = MallocOpenSSL<char>(out_size);
ByteSource out = ByteSource::Allocated(buf, out_size);

if (EVP_PKEY_derive(
ctx.get(),
reinterpret_cast<unsigned char*>(buf),
&out_size) <= 0) {
ByteSource::Builder out(out_size);
if (EVP_PKEY_derive(ctx.get(), out.data<unsigned char>(), &out_size) <= 0) {
return ByteSource();
}

ZeroPadDiffieHellmanSecret(out_size, buf, out.size());
return out;
ZeroPadDiffieHellmanSecret(out_size, out.data<char>(), out.size());
return std::move(out).release();
}
} // namespace

Expand Down
82 changes: 31 additions & 51 deletions src/crypto/crypto_ec.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -486,12 +486,9 @@ Maybe<bool> 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();
Expand All @@ -513,15 +510,14 @@ bool ECDHBitsTraits::DeriveBits(
return false;
}

data = MallocOpenSSL<char>(len);
ByteSource::Builder buf(len);

if (EVP_PKEY_derive(
ctx.get(),
reinterpret_cast<unsigned char*>(data),
&len) <= 0) {
if (EVP_PKEY_derive(ctx.get(), buf.data<unsigned char>(), &len) <= 0) {
return false;
}

*out = std::move(buf).release(len);

break;
}
default: {
Expand All @@ -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<char>(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<char>(), 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;
}

Expand Down Expand Up @@ -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) {
Expand All @@ -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<unsigned char>(len);
if (fn(m_pkey.get(), data, &len) == 0)
ByteSource::Builder data(len);
if (fn(m_pkey.get(), data.data<unsigned char>(), &len) == 0)
return WebCryptoKeyExportStatus::INVALID_KEY_TYPE;
*out = std::move(data).release(len);
} else {
if (key_data->GetKeyType() != kKeyTypePublic)
return WebCryptoKeyExportStatus::INVALID_KEY_TYPE;
Expand All @@ -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<unsigned char>(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<unsigned char>(), len, nullptr);
if (check_len == 0)
return WebCryptoKeyExportStatus::FAILED;

CHECK_EQ(len, check_len);
*out = std::move(data).release();
}

*out = ByteSource::Allocated(reinterpret_cast<char*>(data), len);

return WebCryptoKeyExportStatus::OK;
}
} // namespace
Expand Down Expand Up @@ -853,38 +844,27 @@ Maybe<bool> ExportJWKEdKey(
if (!EVP_PKEY_get_raw_public_key(pkey.get(), nullptr, &len))
return Nothing<bool>();

unsigned char* data = MallocOpenSSL<unsigned char>(len);
ByteSource out = ByteSource::Allocated(reinterpret_cast<char*>(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<unsigned char>(), &len) ||
!StringBytes::Encode(
env->isolate(),
reinterpret_cast<const char*>(data),
len,
BASE64URL,
&error).ToLocal(&encoded) ||
!target->Set(
env->context(),
env->jwk_d_string(),
encoded).IsJust()) {
env->isolate(), out.data<const char>(), len, BASE64URL, &error)
.ToLocal(&encoded) ||
!target->Set(env->context(), env->jwk_d_string(), encoded).IsJust()) {
if (!error.IsEmpty())
env->isolate()->ThrowException(error);
return Nothing<bool>();
}
}

if (!EVP_PKEY_get_raw_public_key(pkey.get(), data, &len) ||
if (!EVP_PKEY_get_raw_public_key(
pkey.get(), out.data<unsigned char>(), &len) ||
!StringBytes::Encode(
env->isolate(),
reinterpret_cast<const char*>(data),
len,
BASE64URL,
&error).ToLocal(&encoded) ||
!target->Set(
env->context(),
env->jwk_x_string(),
encoded).IsJust()) {
env->isolate(), out.data<const char>(), len, BASE64URL, &error)
.ToLocal(&encoded) ||
!target->Set(env->context(), env->jwk_x_string(), encoded).IsJust()) {
if (!error.IsEmpty())
env->isolate()->ThrowException(error);
return Nothing<bool>();
Expand Down
Loading

0 comments on commit 931ecfa

Please sign in to comment.