src: fix error handling for CryptoJob::ToResult

nodejs · Jan 26, 2021 · 313002a · 313002a
1 parent 13ac5fb
commit 313002a
Show file tree

Hide file tree

Showing 5 changed files with 75 additions and 20 deletions.
diff --git a/src/crypto/crypto_keygen.h b/src/crypto/crypto_keygen.h
@@ -95,11 +95,16 @@ class KeyGenJob final : public CryptoJob<KeyGenTraits> {
       v8::Local<v8::Value>* result) override {
     Environment* env = AsyncWrap::env();
     CryptoErrorVector* 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.IsNothing())
+        return v8::Nothing<bool>();
+      if (ret.FromJust()) {
+        *err = Undefined(env->isolate());
+        return v8::Just(true);
+      }
     }
 
     if (errors->empty())
@@ -179,16 +184,16 @@ struct KeyPairGenTraits final {
       AdditionalParameters* params,
       v8::Local<v8::Value>* result) {
     v8::Local<v8::Value> keys[2];
-    if (ManagedEVPPKey::ToEncodedPublicKey(
+    if (!ManagedEVPPKey::ToEncodedPublicKey(
             env,
             std::move(params->key),
             params->public_key_encoding,
-            &keys[0]).IsNothing() ||
-        ManagedEVPPKey::ToEncodedPrivateKey(
+            &keys[0]) ||
+        !ManagedEVPPKey::ToEncodedPrivateKey(
             env,
             std::move(params->key),
             params->private_key_encoding,
-            &keys[1]).IsNothing()) {
+            &keys[1])) {
       return v8::Nothing<bool>();
     }
     *result = v8::Array::New(env->isolate(), keys, arraysize(keys));

diff --git a/src/crypto/crypto_keys.cc b/src/crypto/crypto_keys.cc
@@ -597,35 +597,35 @@ size_t ManagedEVPPKey::size_of_public_key() const {
       pkey_.get(), nullptr, &len) == 1) ? len : 0;
 }
 
-Maybe<bool> ManagedEVPPKey::ToEncodedPublicKey(
+bool ManagedEVPPKey::ToEncodedPublicKey(
     Environment* env,
     ManagedEVPPKey key,
     const PublicKeyEncodingConfig& config,
     Local<Value>* out) {
-  if (!key) return Nothing<bool>();
+  CHECK(key);
   if (config.output_key_object_) {
     // Note that this has the downside of containing sensitive data of the
     // private key.
     std::shared_ptr<KeyObjectData> data =
           KeyObjectData::CreateAsymmetric(kKeyTypePublic, std::move(key));
-    return Just(KeyObjectHandle::Create(env, data).ToLocal(out));
+    return KeyObjectHandle::Create(env, data).ToLocal(out);
   }
-  return Just(WritePublicKey(env, key.get(), config).ToLocal(out));
+  return WritePublicKey(env, key.get(), config).ToLocal(out);
 }
 
-Maybe<bool> ManagedEVPPKey::ToEncodedPrivateKey(
+bool ManagedEVPPKey::ToEncodedPrivateKey(
     Environment* env,
     ManagedEVPPKey key,
     const PrivateKeyEncodingConfig& config,
     Local<Value>* out) {
-  if (!key) return Nothing<bool>();
+  CHECK(key);
   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 KeyObjectHandle::Create(env, data).ToLocal(out);
   }
 
-  return Just(WritePrivateKey(env, key.get(), config).ToLocal(out));
+  return WritePrivateKey(env, key.get(), config).ToLocal(out);
 }
 
 NonCopyableMaybe<PrivateKeyEncodingConfig>

diff --git a/src/crypto/crypto_keys.h b/src/crypto/crypto_keys.h
@@ -110,13 +110,13 @@ class ManagedEVPPKey : public MemoryRetainer {
       unsigned int* offset,
       bool allow_key_object);
 
-  static v8::Maybe<bool> ToEncodedPublicKey(
+  static bool ToEncodedPublicKey(
       Environment* env,
       ManagedEVPPKey key,
       const PublicKeyEncodingConfig& config,
       v8::Local<v8::Value>* out);
 
-  static v8::Maybe<bool> ToEncodedPrivateKey(
+  static bool ToEncodedPrivateKey(
       Environment* env,
       ManagedEVPPKey key,
       const PrivateKeyEncodingConfig& config,

diff --git a/src/crypto/crypto_util.h b/src/crypto/crypto_util.h
@@ -303,9 +303,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(
@@ -338,7 +356,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
@@ -12,6 +12,7 @@ const {
   createVerify,
   generateKeyPair,
   generateKeyPairSync,
+  getCurves,
   publicEncrypt,
   privateDecrypt,
   sign,
@@ -1283,3 +1284,33 @@ const sec1EncExp = (cipher) => getRegExpForPEM('EC PRIVATE KEY', cipher);
     }));
   }
 }
+
+{
+  // 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');
+      }));
+    }
+  }
+}