From 50b5f8bac08d7976f9735e54f74bce45e36d5abc Mon Sep 17 00:00:00 2001
From: Ryan Kelly <ryan@rfk.id.au>
Date: Mon, 29 May 2017 14:20:04 +1000
Subject: [PATCH] crypto: clear err stack after ECDH::BufferToPoint

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: https://github.com/nodejs/node/pull/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>
---
 src/node_crypto.cc              |  4 ++++
 test/parallel/test-crypto-dh.js | 23 +++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 85d05a25061a69..8c2121687600dd 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -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");
 
@@ -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)
diff --git a/test/parallel/test-crypto-dh.js b/test/parallel/test-crypto-dh.js
index fed392cd9f0ba3..285f35b830a2f6 100644
--- a/test/parallel/test-crypto-dh.js
+++ b/test/parallel/test-crypto-dh.js
@@ -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.
@@ -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();