crypto: fix malloc mixing in X509ToObject

davidben · addaleax · commit 3e010aff832d · 2019-02-06T15:50:36.000+01:00
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>
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -1613,6 +1613,24 @@ static void AddFingerprintDigest(const unsigned char* md,
   }
 }
 
+static MaybeLocal<Object> 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<Object>();
+  }
+  MallocedBuffer<unsigned char> 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<Object>();
+  }
+  return Buffer::New(env, buf.release(), len);
+}
+
 static Local<Object> X509ToObject(Environment* env, X509* cert) {
   EscapableHandleScope scope(env->isolate());
   Local<Context> context = env->context();
@@ -1729,16 +1747,12 @@ static Local<Object> 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<Object> 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<Object> 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);
@@ -5238,26 +5252,14 @@ void ECDH::GetPublicKey(const FunctionCallbackInfo<Value>& 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<Uint32>()->Value();
   point_conversion_form_t form = static_cast<point_conversion_form_t>(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<unsigned char>(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<Object> buf =
-      Buffer::New(env, reinterpret_cast<char*>(out), size).ToLocalChecked();
-  args.GetReturnValue().Set(buf);
+  MaybeLocal<Object> buf =
+      ECPointToBuffer(env, EC_KEY_get0_group(ecdh->key_.get()), pub, form);
+  if (buf.IsEmpty()) return;
+  args.GetReturnValue().Set(buf.ToLocalChecked());
 }
 
 
@@ -6145,23 +6147,9 @@ void ConvertKey(const FunctionCallbackInfo<Value>& args) {
   uint32_t val = args[2].As<Uint32>()->Value();
   point_conversion_form_t form = static_cast<point_conversion_form_t>(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<unsigned char>(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<Object> buf =
-      Buffer::New(env, reinterpret_cast<char*>(out), size).ToLocalChecked();
-  args.GetReturnValue().Set(buf);
+  MaybeLocal<Object> buf = ECPointToBuffer(env, group.get(), pub.get(), form);
+  if (buf.IsEmpty()) return;
+  args.GetReturnValue().Set(buf.ToLocalChecked());
 }
 
 
