crypto: simplify using pre-existing keys with ECDH

These changes simplify using ECDH with private keys that are not
dynamically generated with ECDH.generateKeys.

Support for computing the public key corresponding to the given private
key was added. Validity checks to reduce the possibility of computing
a weak or invalid shared secret were also added.

Finally, ECDH.setPublicKey was softly deprecated.

PR-URL: nodejs#3511
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

doc/api/crypto.markdown

@@ -260,28 +260,45 @@ then a buffer is returned.
 
 Sets the EC Diffie-Hellman private key. Key encoding can be `'binary'`,
 `'hex'` or `'base64'`. If no encoding is provided, then a buffer is
-expected.
+expected. If `private_key` is not valid for the curve specified when
+the ECDH object was created, then an error is thrown. Upon setting
+the private key, the associated public point (key) is also generated
+and set in the ECDH object.
+
+### ECDH.setPublicKey(public_key[, encoding])
+
+    Stability: 0 - Deprecated
+
+Sets the EC Diffie-Hellman public key. Key encoding can be `'binary'`,
+`'hex'` or `'base64'`. If no encoding is provided, then a buffer is
+expected. Note that there is not normally a reason to call this
+method. This is because ECDH only needs your private key and the
+other party's public key to compute the shared secret. Thus, usually
+either `generateKeys` or `setPrivateKey` will be called.
+Note that `setPrivateKey` attempts to generate the public point/key
+associated with the private key being set.
 
 Example (obtaining a shared secret):
 
     var crypto = require('crypto');
     var alice = crypto.createECDH('secp256k1');
     var bob = crypto.createECDH('secp256k1');
 
-    alice.generateKeys();
+    // Note: This is a shortcut way to specify one of Alice's previous private
+    // keys. It would be unwise to use such a predictable private key in a real
+    // application.
+    alice.setPrivateKey(
+      crypto.createHash('sha256').update('alice', 'utf8').digest()
+    );
+
+    // Bob uses a newly generated cryptographically strong pseudorandom key pair
     bob.generateKeys();
 
     var alice_secret = alice.computeSecret(bob.getPublicKey(), null, 'hex');
     var bob_secret = bob.computeSecret(alice.getPublicKey(), null, 'hex');
 
-    /* alice_secret and bob_secret should be the same */
-    console.log(alice_secret == bob_secret);
-
-### ECDH.setPublicKey(public_key[, encoding])
-
-Sets the EC Diffie-Hellman public key. Key encoding can be `'binary'`,
-`'hex'` or `'base64'`. If no encoding is provided, then a buffer is
-expected.
+    // alice_secret and bob_secret should be the same shared secret value
+    console.log(alice_secret === bob_secret);
 
 ## Class: Hash
 
@@ -771,6 +788,17 @@ default, set the `crypto.DEFAULT_ENCODING` field to 'binary'.  Note
 that new programs will probably expect buffers, so only use this as a
 temporary measure.
 
+Usage of `ECDH` with non-dynamically generated key pairs has been simplified.
+Now, `setPrivateKey` can be called with a preselected private key and the
+associated public point (key) will be computed and stored in the object.
+This allows you to only store and provide the private part of the EC key pair.
+`setPrivateKey` now also validates that the private key is valid for the curve.
+`ECDH.setPublicKey` is now deprecated as its inclusion in the API is not
+useful. Either a previously stored private key should be set, which
+automatically generates the associated public key, or `generateKeys` should be
+called. The main drawback of `ECDH.setPublicKey` is that it can be used to put
+the ECDH key pair into an inconsistent state.
+
 ## Caveats
 
 The crypto module still supports some algorithms which are already

src/node_crypto.cc

@@ -124,6 +124,13 @@ struct ClearErrorOnReturn {
   ~ClearErrorOnReturn() { ERR_clear_error(); }
 };
 
+// Pop errors from OpenSSL's error stack that were added
+// between when this was constructed and destructed.
+struct MarkPopErrorOnReturn {
+  MarkPopErrorOnReturn() { ERR_set_mark(); }
+  ~MarkPopErrorOnReturn() { ERR_pop_to_mark(); }
+};
+
 static uv_mutex_t* locks;
 
 const char* const root_certs[] = {
@@ -4656,8 +4663,6 @@ void ECDH::GenerateKeys(const FunctionCallbackInfo<Value>& args) {
 
   if (!EC_KEY_generate_key(ecdh->key_))
     return env->ThrowError("Failed to generate EC_KEY");
-
-  ecdh->generated_ = true;
 }
 
 
@@ -4697,6 +4702,9 @@ void ECDH::ComputeSecret(const FunctionCallbackInfo<Value>& args) {
 
   ECDH* ecdh = Unwrap<ECDH>(args.Holder());
 
+  if (!ecdh->IsKeyPairValid())
+    return env->ThrowError("Invalid key pair");
+
   EC_POINT* pub = ecdh->BufferToPoint(Buffer::Data(args[0]),
                                       Buffer::Length(args[0]));
   if (pub == nullptr)
@@ -4728,9 +4736,6 @@ void ECDH::GetPublicKey(const FunctionCallbackInfo<Value>& args) {
 
   ECDH* ecdh = Unwrap<ECDH>(args.Holder());
 
-  if (!ecdh->generated_)
-    return env->ThrowError("You should generate ECDH keys first");
-
   const EC_POINT* pub = EC_KEY_get0_public_key(ecdh->key_);
   if (pub == nullptr)
     return env->ThrowError("Failed to get ECDH public key");
@@ -4763,9 +4768,6 @@ void ECDH::GetPrivateKey(const FunctionCallbackInfo<Value>& args) {
 
   ECDH* ecdh = Unwrap<ECDH>(args.Holder());
 
-  if (!ecdh->generated_)
-    return env->ThrowError("You should generate ECDH keys first");
-
   const BIGNUM* b = EC_KEY_get0_private_key(ecdh->key_);
   if (b == nullptr)
     return env->ThrowError("Failed to get ECDH private key");
@@ -4799,12 +4801,42 @@ void ECDH::SetPrivateKey(const FunctionCallbackInfo<Value>& args) {
   if (priv == nullptr)
     return env->ThrowError("Failed to convert Buffer to BN");
 
+  if (!ecdh->IsKeyValidForCurve(priv)) {
+    BN_free(priv);
+    return env->ThrowError("Private key is not valid for specified curve.");
+  }
+
   int result = EC_KEY_set_private_key(ecdh->key_, priv);
   BN_free(priv);
 
   if (!result) {
     return env->ThrowError("Failed to convert BN to a private key");
   }
+
+  // To avoid inconsistency, clear the current public key in-case computing
+  // the new one fails for some reason.
+  EC_KEY_set_public_key(ecdh->key_, nullptr);
+
+  MarkPopErrorOnReturn mark_pop_error_on_return;
+  (void) &mark_pop_error_on_return;  // Silence compiler warning.
+
+  const BIGNUM* priv_key = EC_KEY_get0_private_key(ecdh->key_);
+  CHECK_NE(priv_key, nullptr);
+
+  EC_POINT* pub = EC_POINT_new(ecdh->group_);
+  CHECK_NE(pub, nullptr);
+
+  if (!EC_POINT_mul(ecdh->group_, pub, priv_key, nullptr, nullptr, nullptr)) {
+    EC_POINT_free(pub);
+    return env->ThrowError("Failed to generate ECDH public key");
+  }
+
+  if (!EC_KEY_set_public_key(ecdh->key_, pub)) {
+    EC_POINT_free(pub);
+    return env->ThrowError("Failed to set generated public key");
+  }
+
+  EC_POINT_free(pub);
 }
 
 
@@ -4818,12 +4850,36 @@ void ECDH::SetPublicKey(const FunctionCallbackInfo<Value>& args) {
   EC_POINT* pub = ecdh->BufferToPoint(Buffer::Data(args[0].As<Object>()),
                                       Buffer::Length(args[0].As<Object>()));
   if (pub == nullptr)
-    return;
+    return env->ThrowError("Failed to convert Buffer to EC_POINT");
 
   int r = EC_KEY_set_public_key(ecdh->key_, pub);
   EC_POINT_free(pub);
   if (!r)
-    return env->ThrowError("Failed to convert BN to a private key");
+    return env->ThrowError("Failed to set EC_POINT as the public key");
+}
+
+
+bool ECDH::IsKeyValidForCurve(const BIGNUM* private_key) {
+  ASSERT_NE(group_, nullptr);
+  CHECK_NE(private_key, nullptr);
+  // Private keys must be in the range [1, n-1].
+  // Ref: Section 3.2.1 - http://www.secg.org/sec1-v2.pdf
+  if (BN_cmp(private_key, BN_value_one()) < 0) {
+    return false;
+  }
+  BIGNUM* order = BN_new();
+  CHECK_NE(order, nullptr);
+  bool result = EC_GROUP_get_order(group_, order, nullptr) &&
+                BN_cmp(private_key, order) < 0;
+  BN_free(order);
+  return result;
+}
+
+
+bool ECDH::IsKeyPairValid() {
+  MarkPopErrorOnReturn mark_pop_error_on_return;
+  (void) &mark_pop_error_on_return;  // Silence compiler warning.
+  return 1 == EC_KEY_check_key(key_);
 }
 
 

src/node_crypto.h

@@ -693,7 +693,6 @@ class ECDH : public BaseObject {
  protected:
   ECDH(Environment* env, v8::Local<v8::Object> wrap, EC_KEY* key)
       : BaseObject(env, wrap),
-        generated_(false),
         key_(key),
         group_(EC_KEY_get0_group(key_)) {
     MakeWeak<ECDH>(this);
@@ -710,7 +709,9 @@ class ECDH : public BaseObject {
 
   EC_POINT* BufferToPoint(char* data, size_t len);
 
-  bool generated_;
+  bool IsKeyPairValid();
+  bool IsKeyValidForCurve(const BIGNUM* private_key);
+
   EC_KEY* key_;
   const EC_GROUP* group_;
 };

test/parallel/test-crypto-dh.js

@@ -1,13 +1,13 @@
 'use strict';
-var common = require('../common');
-var assert = require('assert');
-var constants = require('constants');
+const common = require('../common');
+const assert = require('assert');
+const constants = require('constants');
 
 if (!common.hasCrypto) {
   console.log('1..0 # Skipped: missing crypto');
   return;
 }
-var crypto = require('crypto');
+const crypto = require('crypto');
 
 // Test Diffie-Hellman with two parties sharing a secret,
 // using various encodings as we go along
@@ -150,36 +150,110 @@ assert.equal(bad_dh.verifyError, constants.DH_NOT_SUITABLE_GENERATOR);
 
 
 // Test ECDH
-var ecdh1 = crypto.createECDH('prime256v1');
-var ecdh2 = crypto.createECDH('prime256v1');
-var key1 = ecdh1.generateKeys();
-var key2 = ecdh2.generateKeys('hex');
-var secret1 = ecdh1.computeSecret(key2, 'hex', 'base64');
-var secret2 = ecdh2.computeSecret(key1, 'binary', 'buffer');
+const ecdh1 = crypto.createECDH('prime256v1');
+const ecdh2 = crypto.createECDH('prime256v1');
+key1 = ecdh1.generateKeys();
+key2 = ecdh2.generateKeys('hex');
+secret1 = ecdh1.computeSecret(key2, 'hex', 'base64');
+secret2 = ecdh2.computeSecret(key1, 'binary', 'buffer');
 
 assert.equal(secret1, secret2.toString('base64'));
 
 // Point formats
 assert.equal(ecdh1.getPublicKey('buffer', 'uncompressed')[0], 4);
-var firstByte = ecdh1.getPublicKey('buffer', 'compressed')[0];
+let firstByte = ecdh1.getPublicKey('buffer', 'compressed')[0];
 assert(firstByte === 2 || firstByte === 3);
-var firstByte = ecdh1.getPublicKey('buffer', 'hybrid')[0];
+firstByte = ecdh1.getPublicKey('buffer', 'hybrid')[0];
 assert(firstByte === 6 || firstByte === 7);
 
 // ECDH should check that point is on curve
-var ecdh3 = crypto.createECDH('secp256k1');
-var key3 = ecdh3.generateKeys();
+const ecdh3 = crypto.createECDH('secp256k1');
+const key3 = ecdh3.generateKeys();
 
 assert.throws(function() {
-  var secret3 = ecdh2.computeSecret(key3, 'binary', 'buffer');
+  ecdh2.computeSecret(key3, 'binary', 'buffer');
 });
 
 // ECDH should allow .setPrivateKey()/.setPublicKey()
-var ecdh4 = crypto.createECDH('prime256v1');
+const ecdh4 = crypto.createECDH('prime256v1');
 
 ecdh4.setPrivateKey(ecdh1.getPrivateKey());
 ecdh4.setPublicKey(ecdh1.getPublicKey());
 
 assert.throws(function() {
   ecdh4.setPublicKey(ecdh3.getPublicKey());
+}, /Failed to convert Buffer to EC_POINT/);
+
+// Verify that we can use ECDH without having to use newly generated keys.
+const ecdh5 = crypto.createECDH('secp256k1');
+
+// Verify errors are thrown when retrieving keys from an uninitialized object.
+assert.throws(function() {
+  ecdh5.getPublicKey();
+}, /Failed to get ECDH public key/);
+assert.throws(function() {
+  ecdh5.getPrivateKey();
+}, /Failed to get ECDH private key/);
+
+// A valid private key for the secp256k1 curve.
+const cafebabeKey = 'cafebabe'.repeat(8);
+// Associated compressed and uncompressed public keys (points).
+const cafebabePubPtComp =
+'03672a31bfc59d3f04548ec9b7daeeba2f61814e8ccc40448045007f5479f693a3';
+const cafebabePubPtUnComp =
+'04672a31bfc59d3f04548ec9b7daeeba2f61814e8ccc40448045007f5479f693a3' +
+'2e02c7f93d13dc2732b760ca377a5897b9dd41a1c1b29dc0442fdce6d0a04d1d';
+ecdh5.setPrivateKey(cafebabeKey, 'hex');
+assert.equal(ecdh5.getPrivateKey('hex'), cafebabeKey);
+// Show that the public point (key) is generated while setting the private key.
+assert.equal(ecdh5.getPublicKey('hex'), cafebabePubPtUnComp);
+
+// Compressed and uncompressed public points/keys for other party's private key
+// 0xDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEF
+const peerPubPtComp =
+'02c6b754b20826eb925e052ee2c25285b162b51fdca732bcf67e39d647fb6830ae';
+const peerPubPtUnComp =
+'04c6b754b20826eb925e052ee2c25285b162b51fdca732bcf67e39d647fb6830ae' +
+'b651944a574a362082a77e3f2b5d9223eb54d7f2f76846522bf75f3bedb8178e';
+
+const sharedSecret =
+'1da220b5329bbe8bfd19ceef5a5898593f411a6f12ea40f2a8eead9a5cf59970';
+
+assert.equal(ecdh5.computeSecret(peerPubPtComp, 'hex', 'hex'), sharedSecret);
+assert.equal(ecdh5.computeSecret(peerPubPtUnComp, 'hex', 'hex'), sharedSecret);
+
+// Verify that we still have the same key pair as before the computation.
+assert.equal(ecdh5.getPrivateKey('hex'), cafebabeKey);
+assert.equal(ecdh5.getPublicKey('hex'), cafebabePubPtUnComp);
+
+// Verify setting and getting compressed and non-compressed serializations.
+ecdh5.setPublicKey(cafebabePubPtComp, 'hex');
+assert.equal(ecdh5.getPublicKey('hex'), cafebabePubPtUnComp);
+assert.equal(ecdh5.getPublicKey('hex', 'compressed'), cafebabePubPtComp);
+ecdh5.setPublicKey(cafebabePubPtUnComp, 'hex');
+assert.equal(ecdh5.getPublicKey('hex'), cafebabePubPtUnComp);
+assert.equal(ecdh5.getPublicKey('hex', 'compressed'), cafebabePubPtComp);
+
+// Show why allowing the public key to be set on this type does not make sense.
+ecdh5.setPublicKey(peerPubPtComp, 'hex');
+assert.equal(ecdh5.getPublicKey('hex'), peerPubPtUnComp);
+assert.throws(function() {
+  // Error because the public key does not match the private key anymore.
+  ecdh5.computeSecret(peerPubPtComp, 'hex', 'hex');
+}, /Invalid key pair/);
+
+// Set to a valid key to show that later attempts to set an invalid key are
+// rejected.
+ecdh5.setPrivateKey(cafebabeKey, 'hex');
+
+[ // Some invalid private keys for the secp256k1 curve.
+  '0000000000000000000000000000000000000000000000000000000000000000',
+  'FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141',
+  'FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF',
+].forEach(function(element, index, object) {
+  assert.throws(function() {
+    ecdh5.setPrivateKey(element, 'hex');
+  }, /Private key is not valid for specified curve/);
+  // Verify object state did not change.
+  assert.equal(ecdh5.getPrivateKey('hex'), cafebabeKey);
 });