Skip to content

Commit

Permalink
crypto: clear err stack after ECDH::BufferToPoint
Browse files Browse the repository at this point in the history
Functions that call `ECDH::BufferToPoint` were not clearing the
error stack on failure, so an invalid key could leave leftover
error state and cause subsequent (unrelated) signing operations
to fail.

PR-URL: #13275
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
rfk authored and jasnell committed Jun 5, 2017
1 parent c7ebf6e commit 50b5f8b
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 0 deletions.
4 changes: 4 additions & 0 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5136,6 +5136,8 @@ void ECDH::ComputeSecret(const FunctionCallbackInfo<Value>& args) {
ECDH* ecdh;
ASSIGN_OR_RETURN_UNWRAP(&ecdh, args.Holder());

MarkPopErrorOnReturn mark_pop_error_on_return;

if (!ecdh->IsKeyPairValid())
return env->ThrowError("Invalid key pair");

Expand Down Expand Up @@ -5282,6 +5284,8 @@ void ECDH::SetPublicKey(const FunctionCallbackInfo<Value>& args) {

THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "Public key");

MarkPopErrorOnReturn mark_pop_error_on_return;

EC_POINT* pub = ecdh->BufferToPoint(Buffer::Data(args[0].As<Object>()),
Buffer::Length(args[0].As<Object>()));
if (pub == nullptr)
Expand Down
23 changes: 23 additions & 0 deletions test/parallel/test-crypto-dh.js
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ assert.strictEqual(bad_dh.verifyError, DH_NOT_SUITABLE_GENERATOR);


const availableCurves = new Set(crypto.getCurves());
const availableHashes = new Set(crypto.getHashes());

// Oakley curves do not clean up ERR stack, it was causing unexpected failure
// when accessing other OpenSSL APIs afterwards.
Expand Down Expand Up @@ -307,6 +308,28 @@ if (availableCurves.has('prime256v1') && availableCurves.has('secp256k1')) {
});
}

// Use of invalid keys was not cleaning up ERR stack, and was causing
// unexpected failure in subsequent signing operations.
if (availableCurves.has('prime256v1') && availableHashes.has('sha256')) {
const curve = crypto.createECDH('prime256v1');
const invalidKey = Buffer.alloc(65);
invalidKey.fill('\0');
curve.generateKeys();
assert.throws(() => {
curve.computeSecret(invalidKey);
}, /^Error: Failed to translate Buffer to a EC_POINT$/);
// Check that signing operations are not impacted by the above error.
const ecPrivateKey =
'-----BEGIN EC PRIVATE KEY-----\n' +
'MHcCAQEEIF+jnWY1D5kbVYDNvxxo/Y+ku2uJPDwS0r/VuPZQrjjVoAoGCCqGSM49\n' +
'AwEHoUQDQgAEurOxfSxmqIRYzJVagdZfMMSjRNNhB8i3mXyIMq704m2m52FdfKZ2\n' +
'pQhByd5eyj3lgZ7m7jbchtdgyOF8Io/1ng==\n' +
'-----END EC PRIVATE KEY-----';
assert.doesNotThrow(() => {
crypto.createSign('SHA256').sign(ecPrivateKey);
});
}

// invalid test: curve argument is undefined
assert.throws(() => {
crypto.createECDH();
Expand Down

0 comments on commit 50b5f8b

Please sign in to comment.