From 9110ab3ce67dc6fbfe87b8e3ddc998d2c7d50923 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 25 Jan 2019 17:27:00 -0600 Subject: [PATCH 1/2] 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. --- src/node_crypto.cc | 72 +++++++++++++++++++--------------------------- 1 file changed, 30 insertions(+), 42 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 01593914a1f501..d8de2a85abff99 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1628,6 +1628,24 @@ static void AddFingerprintDigest(const unsigned char* md, } } +static MaybeLocal ECPointToBuffer(Environment* env, + const EC_GROUP* group, + const EC_POINT* point, + point_conversion_form_t form) { + size_t len = EC_POINT_point2oct(group, point, form, nullptr, 0, nullptr); + if (len == 0) { + env->ThrowError("Failed to get public key length"); + return MaybeLocal(); + } + MallocedBuffer buf(len); + len = EC_POINT_point2oct(group, point, form, buf.data, buf.size, nullptr); + if (len == 0) { + env->ThrowError("Failed to get public key"); + return MaybeLocal(); + } + return Buffer::New(env, buf.release(), len); +} + static Local X509ToObject(Environment* env, X509* cert) { EscapableHandleScope scope(env->isolate()); Local context = env->context(); @@ -1744,16 +1762,12 @@ static Local X509ToObject(Environment* env, X509* cert) { } } - unsigned char* pub = nullptr; - size_t publen = EC_KEY_key2buf(ec.get(), EC_KEY_get_conv_form(ec.get()), - &pub, nullptr); - if (publen > 0) { - Local buf = Buffer::New(env, pub, publen).ToLocalChecked(); - // Ownership of pub pointer accepted by Buffer. - pub = nullptr; + const EC_POINT* pubkey = EC_KEY_get0_public_key(ec.get()); + if (pubkey != nullptr) { + Local buf = + ECPointToBuffer(env, group, pubkey, EC_KEY_get_conv_form(ec.get())) + .ToLocalChecked(); info->Set(context, env->pubkey_string(), buf).FromJust(); - } else { - CHECK_NULL(pub); } const int nid = EC_GROUP_get_curve_name(group); @@ -5256,26 +5270,14 @@ void ECDH::GetPublicKey(const FunctionCallbackInfo& args) { if (pub == nullptr) return env->ThrowError("Failed to get ECDH public key"); - int size; CHECK(args[0]->IsUint32()); uint32_t val = args[0].As()->Value(); point_conversion_form_t form = static_cast(val); - size = EC_POINT_point2oct(ecdh->group_, pub, form, nullptr, 0, nullptr); - if (size == 0) - return env->ThrowError("Failed to get public key length"); - - unsigned char* out = node::Malloc(size); - - int r = EC_POINT_point2oct(ecdh->group_, pub, form, out, size, nullptr); - if (r != size) { - free(out); - return env->ThrowError("Failed to get public key"); - } - - Local buf = - Buffer::New(env, reinterpret_cast(out), size).ToLocalChecked(); - args.GetReturnValue().Set(buf); + MaybeLocal buf = + ECPointToBuffer(env, EC_KEY_get0_group(ecdh->key_.get()), pub, form); + if (buf.IsEmpty()) return; + args.GetReturnValue().Set(buf.ToLocalChecked()); } @@ -6165,23 +6167,9 @@ void ConvertKey(const FunctionCallbackInfo& args) { uint32_t val = args[2].As()->Value(); point_conversion_form_t form = static_cast(val); - int size = EC_POINT_point2oct( - group.get(), pub.get(), form, nullptr, 0, nullptr); - - if (size == 0) - return env->ThrowError("Failed to get public key length"); - - unsigned char* out = node::Malloc(size); - - int r = EC_POINT_point2oct(group.get(), pub.get(), form, out, size, nullptr); - if (r != size) { - free(out); - return env->ThrowError("Failed to get public key"); - } - - Local buf = - Buffer::New(env, reinterpret_cast(out), size).ToLocalChecked(); - args.GetReturnValue().Set(buf); + MaybeLocal buf = ECPointToBuffer(env, group.get(), pub.get(), form); + if (buf.IsEmpty()) return; + args.GetReturnValue().Set(buf.ToLocalChecked()); } From eb7748b46c6590126a8b5c0bef63f6800359bb30 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 29 Jan 2019 05:51:09 +0000 Subject: [PATCH 2/2] Use MaybeLocal::ToLocal and don't crash X509ToObject on error. --- src/node_crypto.cc | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index d8de2a85abff99..803667018f805e 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1628,24 +1628,27 @@ static void AddFingerprintDigest(const unsigned char* md, } } + static MaybeLocal ECPointToBuffer(Environment* env, const EC_GROUP* group, const EC_POINT* point, - point_conversion_form_t form) { + point_conversion_form_t form, + const char** error) { size_t len = EC_POINT_point2oct(group, point, form, nullptr, 0, nullptr); if (len == 0) { - env->ThrowError("Failed to get public key length"); + if (error != nullptr) *error = "Failed to get public key length"; return MaybeLocal(); } MallocedBuffer buf(len); len = EC_POINT_point2oct(group, point, form, buf.data, buf.size, nullptr); if (len == 0) { - env->ThrowError("Failed to get public key"); + if (error != nullptr) *error = "Failed to get public key"; return MaybeLocal(); } return Buffer::New(env, buf.release(), len); } + static Local X509ToObject(Environment* env, X509* cert) { EscapableHandleScope scope(env->isolate()); Local context = env->context(); @@ -1763,10 +1766,11 @@ static Local X509ToObject(Environment* env, X509* cert) { } const EC_POINT* pubkey = EC_KEY_get0_public_key(ec.get()); - if (pubkey != nullptr) { - Local buf = - ECPointToBuffer(env, group, pubkey, EC_KEY_get_conv_form(ec.get())) - .ToLocalChecked(); + Local buf; + if (pubkey != nullptr && + ECPointToBuffer( + env, group, pubkey, EC_KEY_get_conv_form(ec.get()), nullptr) + .ToLocal(&buf)) { info->Set(context, env->pubkey_string(), buf).FromJust(); } @@ -5266,6 +5270,7 @@ void ECDH::GetPublicKey(const FunctionCallbackInfo& args) { ECDH* ecdh; ASSIGN_OR_RETURN_UNWRAP(&ecdh, args.Holder()); + const EC_GROUP* group = EC_KEY_get0_group(ecdh->key_.get()); const EC_POINT* pub = EC_KEY_get0_public_key(ecdh->key_.get()); if (pub == nullptr) return env->ThrowError("Failed to get ECDH public key"); @@ -5274,10 +5279,11 @@ void ECDH::GetPublicKey(const FunctionCallbackInfo& args) { uint32_t val = args[0].As()->Value(); point_conversion_form_t form = static_cast(val); - MaybeLocal buf = - ECPointToBuffer(env, EC_KEY_get0_group(ecdh->key_.get()), pub, form); - if (buf.IsEmpty()) return; - args.GetReturnValue().Set(buf.ToLocalChecked()); + const char* error; + Local buf; + if (!ECPointToBuffer(env, group, pub, form, &error).ToLocal(&buf)) + return env->ThrowError(error); + args.GetReturnValue().Set(buf); } @@ -6167,9 +6173,11 @@ void ConvertKey(const FunctionCallbackInfo& args) { uint32_t val = args[2].As()->Value(); point_conversion_form_t form = static_cast(val); - MaybeLocal buf = ECPointToBuffer(env, group.get(), pub.get(), form); - if (buf.IsEmpty()) return; - args.GetReturnValue().Set(buf.ToLocalChecked()); + const char* error; + Local buf; + if (!ECPointToBuffer(env, group.get(), pub.get(), form, &error).ToLocal(&buf)) + return env->ThrowError(error); + args.GetReturnValue().Set(buf); }