crypto: ecdh - generate public key when setting private key and more

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
 
@@ -754,6 +771,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[] = {
@@ -4651,8 +4658,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;
 }
 
 
@@ -4692,6 +4697,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)
@@ -4723,9 +4731,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");
@@ -4758,9 +4763,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");
@@ -4794,12 +4796,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);
 }
 
 
@@ -4813,12 +4845,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);
 });