From 845633a7c62ad9ffd33880a43e05a49382bb76bb Mon Sep 17 00:00:00 2001
From: "Jose M. Palacios Diaz" <jmpd1988@gmail.com>
Date: Mon, 6 Nov 2017 17:22:42 -0500
Subject: [PATCH] crypto: better docs for cases where peer's public key is
 invalid

changes in c++ are in the computeSecret function, but the thrown
exception that was moved to JS land was in BufferToPoint
function, here i let the allocation error be thrown so the only value
returned is the nullptr that i use later to catch the error in
computeSecret, to then construct the exception in JS land.

an ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY error was added to errors.js
and with that, subsequent changes to docs and tests were made.

PR-URL: https://github.com/nodejs/node/pull/16849
Refs: https://www.iacr.org/archive/pkc2003/25670211/25670211.pdf
Fixes: https://github.com/nodejs/node/issues/16625
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
---
 doc/api/crypto.md                    | 12 +++++++++++-
 doc/api/errors.md                    |  7 +++++++
 lib/internal/crypto/diffiehellman.js |  2 ++
 lib/internal/errors.js               |  2 ++
 src/node_crypto.cc                   |  7 +++++--
 test/parallel/test-crypto-dh.js      | 20 ++++++++++++++------
 6 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/doc/api/crypto.md b/doc/api/crypto.md
index 9d6e344667520e..a5619b92df36fa 100644
--- a/doc/api/crypto.md
+++ b/doc/api/crypto.md
@@ -651,7 +651,11 @@ added: v0.11.14
 changes:
   - version: v6.0.0
     pr-url: https://github.com/nodejs/node/pull/5522
-    description: The default `inputEncoding` changed from `binary` to `utf8`.
+    description: The default `inputEncoding` changed from `binary` to `utf8`
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/16849
+    description: Changed error format to better support invalid public key
+                 error
 -->
 - `otherPublicKey` {string | Buffer | TypedArray | DataView}
 - `inputEncoding` {string}
@@ -668,6 +672,12 @@ provided, `otherPublicKey` is expected to be a [`Buffer`][], `TypedArray`, or
 If `outputEncoding` is given a string will be returned; otherwise a
 [`Buffer`][] is returned.
 
+`ecdh.computeSecret` will throw an
+`ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY` error when `otherPublicKey`
+lies outside of the elliptic curve. Since `otherPublicKey` is
+usually supplied from a remote user over an insecure network,
+its recommended for developers to handle this exception accordingly.
+
 ### ecdh.generateKeys([encoding[, format]])
 <!-- YAML
 added: v0.11.14
diff --git a/doc/api/errors.md b/doc/api/errors.md
index df38bf32a94174..fab75e9bc45fe0 100644
--- a/doc/api/errors.md
+++ b/doc/api/errors.md
@@ -676,6 +676,13 @@ of OpenSSL being used.
 An invalid value for the `format` argument was passed to the `crypto.ECDH()`
 class `getPublicKey()` method.
 
+<a id="ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY"></a>
+### ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY
+
+An invalid value for the `key` argument has been passed to the
+`crypto.ECDH()` class `computeSecret()` method. It means that the public
+key lies outside of the elliptic curve.
+
 <a id="ERR_CRYPTO_ENGINE_UNKNOWN"></a>
 ### ERR_CRYPTO_ENGINE_UNKNOWN
 
diff --git a/lib/internal/crypto/diffiehellman.js b/lib/internal/crypto/diffiehellman.js
index 95b689df949251..687bae83fdecbb 100644
--- a/lib/internal/crypto/diffiehellman.js
+++ b/lib/internal/crypto/diffiehellman.js
@@ -96,6 +96,8 @@ function dhComputeSecret(key, inEnc, outEnc) {
   inEnc = inEnc || encoding;
   outEnc = outEnc || encoding;
   var ret = this._handle.computeSecret(toBuf(key, inEnc));
+  if (typeof ret === 'string')
+    throw new errors.Error('ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY');
   if (outEnc && outEnc !== 'buffer')
     ret = ret.toString(outEnc);
   return ret;
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
index 7fa567739e9880..dadba400b500bc 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -281,6 +281,8 @@ E('ERR_CPU_USAGE', 'Unable to obtain cpu usage %s');
 E('ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED',
   'Custom engines not supported by this OpenSSL');
 E('ERR_CRYPTO_ECDH_INVALID_FORMAT', 'Invalid ECDH format: %s');
+E('ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY',
+  'Public key is not valid for specified curve');
 E('ERR_CRYPTO_ENGINE_UNKNOWN', 'Engine "%s" was not found');
 E('ERR_CRYPTO_FIPS_FORCED',
   'Cannot set FIPS mode, it was forced with --force-fips at startup.');
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 88d10bee788e1a..b5b0e4e21cbb2e 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -5220,7 +5220,6 @@ EC_POINT* ECDH::BufferToPoint(char* data, size_t len) {
       len,
       nullptr);
   if (!r) {
-    env()->ThrowError("Failed to translate Buffer to a EC_POINT");
     goto fatal;
   }
 
@@ -5247,8 +5246,12 @@ void ECDH::ComputeSecret(const FunctionCallbackInfo<Value>& args) {
 
   EC_POINT* pub = ecdh->BufferToPoint(Buffer::Data(args[0]),
                                       Buffer::Length(args[0]));
-  if (pub == nullptr)
+  if (pub == nullptr) {
+    args.GetReturnValue().Set(
+        FIXED_ONE_BYTE_STRING(env->isolate(),
+        "ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY"));
     return;
+  }
 
   // NOTE: field_size is in bits
   int field_size = EC_GROUP_get_degree(ecdh->group_);
diff --git a/test/parallel/test-crypto-dh.js b/test/parallel/test-crypto-dh.js
index c162d660bd4b0b..d55540dd88035c 100644
--- a/test/parallel/test-crypto-dh.js
+++ b/test/parallel/test-crypto-dh.js
@@ -222,9 +222,13 @@ if (availableCurves.has('prime256v1') && availableCurves.has('secp256k1')) {
   const ecdh3 = crypto.createECDH('secp256k1');
   const key3 = ecdh3.generateKeys();
 
-  assert.throws(() => {
-    ecdh2.computeSecret(key3, 'latin1', 'buffer');
-  }, /^Error: Failed to translate Buffer to a EC_POINT$/);
+  common.expectsError(
+    () => ecdh2.computeSecret(key3, 'latin1', 'buffer'),
+    {
+      code: 'ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY',
+      type: Error,
+      message: 'Public key is not valid for specified curve'
+    });
 
   // ECDH should allow .setPrivateKey()/.setPublicKey()
   const ecdh4 = crypto.createECDH('prime256v1');
@@ -331,9 +335,13 @@ if (availableCurves.has('prime256v1') && availableHashes.has('sha256')) {
   const invalidKey = Buffer.alloc(65);
   invalidKey.fill('\0');
   curve.generateKeys();
-  assert.throws(() => {
-    curve.computeSecret(invalidKey);
-  }, /^Error: Failed to translate Buffer to a EC_POINT$/);
+  common.expectsError(
+    () => curve.computeSecret(invalidKey),
+    {
+      code: 'ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY',
+      type: Error,
+      message: 'Public key is not valid for specified curve'
+    });
   // Check that signing operations are not impacted by the above error.
   const ecPrivateKey =
     '-----BEGIN EC PRIVATE KEY-----\n' +