From 2864ee90ff0f17d07ad0682b0c656348e3d68762 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= <tniessen@tnie.de>
Date: Thu, 1 Apr 2021 17:31:45 +0200
Subject: [PATCH] src: fix error handling for CryptoJob::ToResult

PR-URL: https://github.com/nodejs/node/pull/37076
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
---
 src/crypto/crypto_keygen.h          | 11 ++++++----
 src/crypto/crypto_keys.cc           | 14 +++++++++----
 src/crypto/crypto_util.h            | 23 +++++++++++++++++++--
 test/parallel/test-crypto-keygen.js | 31 +++++++++++++++++++++++++++++
 4 files changed, 69 insertions(+), 10 deletions(-)

diff --git a/src/crypto/crypto_keygen.h b/src/crypto/crypto_keygen.h
index e96d850cee1b5a..59e7e2ce7cec4e 100644
--- a/src/crypto/crypto_keygen.h
+++ b/src/crypto/crypto_keygen.h
@@ -96,10 +96,13 @@ class KeyGenJob final : public CryptoJob<KeyGenTraits> {
     Environment* env = AsyncWrap::env();
     CryptoErrorStore* errors = CryptoJob<KeyGenTraits>::errors();
     AdditionalParams* params = CryptoJob<KeyGenTraits>::params();
-    if (status_ == KeyGenJobStatus::OK &&
-        LIKELY(!KeyGenTraits::EncodeKey(env, params, result).IsNothing())) {
-      *err = Undefined(env->isolate());
-      return v8::Just(true);
+
+    if (status_ == KeyGenJobStatus::OK) {
+      v8::Maybe<bool> ret = KeyGenTraits::EncodeKey(env, params, result);
+      if (ret.IsJust() && ret.FromJust()) {
+        *err = Undefined(env->isolate());
+      }
+      return ret;
     }
 
     if (errors->Empty())
diff --git a/src/crypto/crypto_keys.cc b/src/crypto/crypto_keys.cc
index f72ca8322db960..4e8408086e9bff 100644
--- a/src/crypto/crypto_keys.cc
+++ b/src/crypto/crypto_keys.cc
@@ -602,6 +602,11 @@ size_t ManagedEVPPKey::size_of_public_key() const {
       pkey_.get(), nullptr, &len) == 1) ? len : 0;
 }
 
+// This maps true to Just<bool>(true) and false to Nothing<bool>().
+static inline Maybe<bool> Tristate(bool b) {
+  return b ? Just(true) : Nothing<bool>();
+}
+
 Maybe<bool> ManagedEVPPKey::ToEncodedPublicKey(
     Environment* env,
     ManagedEVPPKey key,
@@ -613,9 +618,10 @@ Maybe<bool> ManagedEVPPKey::ToEncodedPublicKey(
     // private key.
     std::shared_ptr<KeyObjectData> data =
           KeyObjectData::CreateAsymmetric(kKeyTypePublic, std::move(key));
-    return Just(KeyObjectHandle::Create(env, data).ToLocal(out));
+    return Tristate(KeyObjectHandle::Create(env, data).ToLocal(out));
   }
-  return Just(WritePublicKey(env, key.get(), config).ToLocal(out));
+
+  return Tristate(WritePublicKey(env, key.get(), config).ToLocal(out));
 }
 
 Maybe<bool> ManagedEVPPKey::ToEncodedPrivateKey(
@@ -627,10 +633,10 @@ Maybe<bool> ManagedEVPPKey::ToEncodedPrivateKey(
   if (config.output_key_object_) {
     std::shared_ptr<KeyObjectData> data =
         KeyObjectData::CreateAsymmetric(kKeyTypePrivate, std::move(key));
-    return Just(KeyObjectHandle::Create(env, data).ToLocal(out));
+    return Tristate(KeyObjectHandle::Create(env, data).ToLocal(out));
   }
 
-  return Just(WritePrivateKey(env, key.get(), config).ToLocal(out));
+  return Tristate(WritePrivateKey(env, key.get(), config).ToLocal(out));
 }
 
 NonCopyableMaybe<PrivateKeyEncodingConfig>
diff --git a/src/crypto/crypto_util.h b/src/crypto/crypto_util.h
index 928c0cd38969be..27bb6310b884d1 100644
--- a/src/crypto/crypto_util.h
+++ b/src/crypto/crypto_util.h
@@ -349,9 +349,27 @@ class CryptoJob : public AsyncWrap, public ThreadPoolWork {
     if (status == UV_ECANCELED) return;
     v8::HandleScope handle_scope(env->isolate());
     v8::Context::Scope context_scope(env->context());
+
+    // TODO(tniessen): Remove the exception handling logic here as soon as we
+    // can verify that no code path in ToResult will ever throw an exception.
+    v8::Local<v8::Value> exception;
     v8::Local<v8::Value> args[2];
-    if (ptr->ToResult(&args[0], &args[1]).FromJust())
+    {
+      node::errors::TryCatchScope try_catch(env);
+      v8::Maybe<bool> ret = ptr->ToResult(&args[0], &args[1]);
+      if (!ret.IsJust()) {
+        CHECK(try_catch.HasCaught());
+        exception = try_catch.Exception();
+      } else if (!ret.FromJust()) {
+        return;
+      }
+    }
+
+    if (exception.IsEmpty()) {
       ptr->MakeCallback(env->ondone_string(), arraysize(args), args);
+    } else {
+      ptr->MakeCallback(env->ondone_string(), 1, &exception);
+    }
   }
 
   virtual v8::Maybe<bool> ToResult(
@@ -384,7 +402,8 @@ class CryptoJob : public AsyncWrap, public ThreadPoolWork {
     v8::Local<v8::Value> ret[2];
     env->PrintSyncTrace();
     job->DoThreadPoolWork();
-    if (job->ToResult(&ret[0], &ret[1]).FromJust()) {
+    v8::Maybe<bool> result = job->ToResult(&ret[0], &ret[1]);
+    if (result.IsJust() && result.FromJust()) {
       args.GetReturnValue().Set(
           v8::Array::New(env->isolate(), ret, arraysize(ret)));
     }
diff --git a/test/parallel/test-crypto-keygen.js b/test/parallel/test-crypto-keygen.js
index e5c8a107ba58a1..8f3727771306ef 100644
--- a/test/parallel/test-crypto-keygen.js
+++ b/test/parallel/test-crypto-keygen.js
@@ -12,6 +12,7 @@ const {
   createVerify,
   generateKeyPair,
   generateKeyPairSync,
+  getCurves,
   publicEncrypt,
   privateDecrypt,
   sign,
@@ -1314,3 +1315,33 @@ if (!common.hasOpenSSL3) {
     );
   }
 }
+
+{
+  // This test creates EC key pairs on curves without associated OIDs.
+  // Specifying a key encoding should not crash.
+
+  if (process.versions.openssl >= '1.1.1i') {
+    for (const namedCurve of ['Oakley-EC2N-3', 'Oakley-EC2N-4']) {
+      if (!getCurves().includes(namedCurve))
+        continue;
+
+      const params = {
+        namedCurve,
+        publicKeyEncoding: {
+          format: 'der',
+          type: 'spki'
+        }
+      };
+
+      assert.throws(() => {
+        generateKeyPairSync('ec', params);
+      }, {
+        code: 'ERR_OSSL_EC_MISSING_OID'
+      });
+
+      generateKeyPair('ec', params, common.mustCall((err) => {
+        assert.strictEqual(err.code, 'ERR_OSSL_EC_MISSING_OID');
+      }));
+    }
+  }
+}