Skip to content

Commit

Permalink
crypto: simplify using pre-existing keys with ECDH
Browse files Browse the repository at this point in the history 
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: #3511
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
  • Loading branch information
@rvagg
Michael Ruddy authored and rvagg committed Dec 8, 2015
1 parent 0c2a0dc commit da5ac55
Show file tree
Hide file tree
Showing 4 changed files with 197 additions and 38 deletions.
48 changes: 38 additions & 10 deletions doc/api/crypto.markdown
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down
76 changes: 66 additions & 10 deletions src/node_crypto.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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[] = {
Expand Down Expand Up @@ -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;
}


Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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);
}


Expand All @@ -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_);
}


Expand Down
5 changes: 3 additions & 2 deletions src/node_crypto.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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_;
};
Expand Down
106 changes: 90 additions & 16 deletions test/parallel/test-crypto-dh.js
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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);
});

0 comments on commit da5ac55

Please sign in to comment.