Skip to content

Commit 3e010af

Browse files
davidbenaddaleax
authored andcommitted
crypto: fix malloc mixing in X509ToObject
EC_KEY_key2buf returns an OPENSSL_malloc'd pointer so it shouldn't be passed into Buffer::New, which expect a libc malloc'd pointer. Instead, factor out the ECDH::GetPublicKey code which uses EC_POINT_point2oct. This preserves the existing behavior where encoding failures are silently ignored, but it is probably safe to CHECK fail them instead. PR-URL: #25717 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
1 parent ee3165d commit 3e010af

File tree

1 file changed

+30
-42
lines changed

1 file changed

+30
-42
lines changed

src/node_crypto.cc

+30-42
Original file line numberDiff line numberDiff line change
@@ -1613,6 +1613,24 @@ static void AddFingerprintDigest(const unsigned char* md,
16131613
}
16141614
}
16151615

1616+
static MaybeLocal<Object> ECPointToBuffer(Environment* env,
1617+
const EC_GROUP* group,
1618+
const EC_POINT* point,
1619+
point_conversion_form_t form) {
1620+
size_t len = EC_POINT_point2oct(group, point, form, nullptr, 0, nullptr);
1621+
if (len == 0) {
1622+
env->ThrowError("Failed to get public key length");
1623+
return MaybeLocal<Object>();
1624+
}
1625+
MallocedBuffer<unsigned char> buf(len);
1626+
len = EC_POINT_point2oct(group, point, form, buf.data, buf.size, nullptr);
1627+
if (len == 0) {
1628+
env->ThrowError("Failed to get public key");
1629+
return MaybeLocal<Object>();
1630+
}
1631+
return Buffer::New(env, buf.release(), len);
1632+
}
1633+
16161634
static Local<Object> X509ToObject(Environment* env, X509* cert) {
16171635
EscapableHandleScope scope(env->isolate());
16181636
Local<Context> context = env->context();
@@ -1729,16 +1747,12 @@ static Local<Object> X509ToObject(Environment* env, X509* cert) {
17291747
}
17301748
}
17311749

1732-
unsigned char* pub = nullptr;
1733-
size_t publen = EC_KEY_key2buf(ec.get(), EC_KEY_get_conv_form(ec.get()),
1734-
&pub, nullptr);
1735-
if (publen > 0) {
1736-
Local<Object> buf = Buffer::New(env, pub, publen).ToLocalChecked();
1737-
// Ownership of pub pointer accepted by Buffer.
1738-
pub = nullptr;
1750+
const EC_POINT* pubkey = EC_KEY_get0_public_key(ec.get());
1751+
if (pubkey != nullptr) {
1752+
Local<Object> buf =
1753+
ECPointToBuffer(env, group, pubkey, EC_KEY_get_conv_form(ec.get()))
1754+
.ToLocalChecked();
17391755
info->Set(context, env->pubkey_string(), buf).FromJust();
1740-
} else {
1741-
CHECK_NULL(pub);
17421756
}
17431757

17441758
const int nid = EC_GROUP_get_curve_name(group);
@@ -5238,26 +5252,14 @@ void ECDH::GetPublicKey(const FunctionCallbackInfo<Value>& args) {
52385252
if (pub == nullptr)
52395253
return env->ThrowError("Failed to get ECDH public key");
52405254

5241-
int size;
52425255
CHECK(args[0]->IsUint32());
52435256
uint32_t val = args[0].As<Uint32>()->Value();
52445257
point_conversion_form_t form = static_cast<point_conversion_form_t>(val);
52455258

5246-
size = EC_POINT_point2oct(ecdh->group_, pub, form, nullptr, 0, nullptr);
5247-
if (size == 0)
5248-
return env->ThrowError("Failed to get public key length");
5249-
5250-
unsigned char* out = node::Malloc<unsigned char>(size);
5251-
5252-
int r = EC_POINT_point2oct(ecdh->group_, pub, form, out, size, nullptr);
5253-
if (r != size) {
5254-
free(out);
5255-
return env->ThrowError("Failed to get public key");
5256-
}
5257-
5258-
Local<Object> buf =
5259-
Buffer::New(env, reinterpret_cast<char*>(out), size).ToLocalChecked();
5260-
args.GetReturnValue().Set(buf);
5259+
MaybeLocal<Object> buf =
5260+
ECPointToBuffer(env, EC_KEY_get0_group(ecdh->key_.get()), pub, form);
5261+
if (buf.IsEmpty()) return;
5262+
args.GetReturnValue().Set(buf.ToLocalChecked());
52615263
}
52625264

52635265

@@ -6145,23 +6147,9 @@ void ConvertKey(const FunctionCallbackInfo<Value>& args) {
61456147
uint32_t val = args[2].As<Uint32>()->Value();
61466148
point_conversion_form_t form = static_cast<point_conversion_form_t>(val);
61476149

6148-
int size = EC_POINT_point2oct(
6149-
group.get(), pub.get(), form, nullptr, 0, nullptr);
6150-
6151-
if (size == 0)
6152-
return env->ThrowError("Failed to get public key length");
6153-
6154-
unsigned char* out = node::Malloc<unsigned char>(size);
6155-
6156-
int r = EC_POINT_point2oct(group.get(), pub.get(), form, out, size, nullptr);
6157-
if (r != size) {
6158-
free(out);
6159-
return env->ThrowError("Failed to get public key");
6160-
}
6161-
6162-
Local<Object> buf =
6163-
Buffer::New(env, reinterpret_cast<char*>(out), size).ToLocalChecked();
6164-
args.GetReturnValue().Set(buf);
6150+
MaybeLocal<Object> buf = ECPointToBuffer(env, group.get(), pub.get(), form);
6151+
if (buf.IsEmpty()) return;
6152+
args.GetReturnValue().Set(buf.ToLocalChecked());
61656153
}
61666154

61676155

0 commit comments

Comments
 (0)