diff --git a/doc/api/crypto.markdown b/doc/api/crypto.markdown index 62e83e62db2210..f90ec1c429a826 100644 --- a/doc/api/crypto.markdown +++ b/doc/api/crypto.markdown @@ -260,7 +260,23 @@ 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): @@ -268,20 +284,21 @@ Example (obtaining a shared secret): 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 @@ -761,6 +778,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 diff --git a/src/node_crypto.cc b/src/node_crypto.cc index bd7314c9db902c..e7df4b4ef86f32 100644 --- a/src/node_crypto.cc +++ b/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& 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& args) { ECDH* ecdh = Unwrap(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& args) { ECDH* ecdh = Unwrap(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& args) { ECDH* ecdh = Unwrap(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& 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& args) { EC_POINT* pub = ecdh->BufferToPoint(Buffer::Data(args[0].As()), Buffer::Length(args[0].As())); 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_); } diff --git a/src/node_crypto.h b/src/node_crypto.h index beeb2fb4458267..d3d66e32dd1313 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -693,7 +693,6 @@ class ECDH : public BaseObject { protected: ECDH(Environment* env, v8::Local wrap, EC_KEY* key) : BaseObject(env, wrap), - generated_(false), key_(key), group_(EC_KEY_get0_group(key_)) { MakeWeak(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_; }; diff --git a/test/parallel/test-crypto-dh.js b/test/parallel/test-crypto-dh.js index f3e54c46a8e16b..a1e8fb1d7da4ac 100644 --- a/test/parallel/test-crypto-dh.js +++ b/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); });