Skip to content

Commit

Permalink
src: fix error handling for CryptoJob::ToResult
Browse files Browse the repository at this point in the history
PR-URL: #37076
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
  • Loading branch information
tniessen authored and MylesBorins committed Apr 5, 2021
1 parent ad7e344 commit 6ad0b6f
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 10 deletions.
11 changes: 7 additions & 4 deletions src/crypto/crypto_keygen.h
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down
14 changes: 10 additions & 4 deletions src/crypto/crypto_keys.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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(
Expand All @@ -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>
Expand Down
23 changes: 21 additions & 2 deletions src/crypto/crypto_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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)));
}
Expand Down
31 changes: 31 additions & 0 deletions test/parallel/test-crypto-keygen.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const {
createVerify,
generateKeyPair,
generateKeyPairSync,
getCurves,
publicEncrypt,
privateDecrypt,
sign,
Expand Down Expand Up @@ -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');
}));
}
}
}

0 comments on commit 6ad0b6f

Please sign in to comment.