Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit da5ac55

Browse filesBrowse files
Michael Ruddyrvagg
Michael Ruddy
authored and
rvagg
committed
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: #3511 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
1 parent 0c2a0dc commit da5ac55
Copy full SHA for da5ac55

File tree

Expand file treeCollapse file tree

4 files changed

+197
-38
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

4 files changed

+197
-38
lines changed
Open diff view settings
Collapse file

‎doc/api/crypto.markdown‎

Copy file name to clipboardExpand all lines: doc/api/crypto.markdown
+38-10Lines changed: 38 additions & 10 deletions
  • Display the source diff
  • Display the rich diff
Original file line numberDiff line numberDiff line change
@@ -260,28 +260,45 @@ then a buffer is returned.
260260

261261
Sets the EC Diffie-Hellman private key. Key encoding can be `'binary'`,
262262
`'hex'` or `'base64'`. If no encoding is provided, then a buffer is
263-
expected.
263+
expected. If `private_key` is not valid for the curve specified when
264+
the ECDH object was created, then an error is thrown. Upon setting
265+
the private key, the associated public point (key) is also generated
266+
and set in the ECDH object.
267+
268+
### ECDH.setPublicKey(public_key[, encoding])
269+
270+
Stability: 0 - Deprecated
271+
272+
Sets the EC Diffie-Hellman public key. Key encoding can be `'binary'`,
273+
`'hex'` or `'base64'`. If no encoding is provided, then a buffer is
274+
expected. Note that there is not normally a reason to call this
275+
method. This is because ECDH only needs your private key and the
276+
other party's public key to compute the shared secret. Thus, usually
277+
either `generateKeys` or `setPrivateKey` will be called.
278+
Note that `setPrivateKey` attempts to generate the public point/key
279+
associated with the private key being set.
264280

265281
Example (obtaining a shared secret):
266282

267283
var crypto = require('crypto');
268284
var alice = crypto.createECDH('secp256k1');
269285
var bob = crypto.createECDH('secp256k1');
270286

271-
alice.generateKeys();
287+
// Note: This is a shortcut way to specify one of Alice's previous private
288+
// keys. It would be unwise to use such a predictable private key in a real
289+
// application.
290+
alice.setPrivateKey(
291+
crypto.createHash('sha256').update('alice', 'utf8').digest()
292+
);
293+
294+
// Bob uses a newly generated cryptographically strong pseudorandom key pair
272295
bob.generateKeys();
273296

274297
var alice_secret = alice.computeSecret(bob.getPublicKey(), null, 'hex');
275298
var bob_secret = bob.computeSecret(alice.getPublicKey(), null, 'hex');
276299

277-
/* alice_secret and bob_secret should be the same */
278-
console.log(alice_secret == bob_secret);
279-
280-
### ECDH.setPublicKey(public_key[, encoding])
281-
282-
Sets the EC Diffie-Hellman public key. Key encoding can be `'binary'`,
283-
`'hex'` or `'base64'`. If no encoding is provided, then a buffer is
284-
expected.
300+
// alice_secret and bob_secret should be the same shared secret value
301+
console.log(alice_secret === bob_secret);
285302

286303
## Class: Hash
287304

@@ -761,6 +778,17 @@ default, set the `crypto.DEFAULT_ENCODING` field to 'binary'. Note
761778
that new programs will probably expect buffers, so only use this as a
762779
temporary measure.
763780

781+
Usage of `ECDH` with non-dynamically generated key pairs has been simplified.
782+
Now, `setPrivateKey` can be called with a preselected private key and the
783+
associated public point (key) will be computed and stored in the object.
784+
This allows you to only store and provide the private part of the EC key pair.
785+
`setPrivateKey` now also validates that the private key is valid for the curve.
786+
`ECDH.setPublicKey` is now deprecated as its inclusion in the API is not
787+
useful. Either a previously stored private key should be set, which
788+
automatically generates the associated public key, or `generateKeys` should be
789+
called. The main drawback of `ECDH.setPublicKey` is that it can be used to put
790+
the ECDH key pair into an inconsistent state.
791+
764792
## Caveats
765793

766794
The crypto module still supports some algorithms which are already
Collapse file

‎src/node_crypto.cc‎

Copy file name to clipboardExpand all lines: src/node_crypto.cc
+66-10Lines changed: 66 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,13 @@ struct ClearErrorOnReturn {
124124
~ClearErrorOnReturn() { ERR_clear_error(); }
125125
};
126126

127+
// Pop errors from OpenSSL's error stack that were added
128+
// between when this was constructed and destructed.
129+
struct MarkPopErrorOnReturn {
130+
MarkPopErrorOnReturn() { ERR_set_mark(); }
131+
~MarkPopErrorOnReturn() { ERR_pop_to_mark(); }
132+
};
133+
127134
static uv_mutex_t* locks;
128135

129136
const char* const root_certs[] = {
@@ -4656,8 +4663,6 @@ void ECDH::GenerateKeys(const FunctionCallbackInfo<Value>& args) {
46564663

46574664
if (!EC_KEY_generate_key(ecdh->key_))
46584665
return env->ThrowError("Failed to generate EC_KEY");
4659-
4660-
ecdh->generated_ = true;
46614666
}
46624667

46634668

@@ -4697,6 +4702,9 @@ void ECDH::ComputeSecret(const FunctionCallbackInfo<Value>& args) {
46974702

46984703
ECDH* ecdh = Unwrap<ECDH>(args.Holder());
46994704

4705+
if (!ecdh->IsKeyPairValid())
4706+
return env->ThrowError("Invalid key pair");
4707+
47004708
EC_POINT* pub = ecdh->BufferToPoint(Buffer::Data(args[0]),
47014709
Buffer::Length(args[0]));
47024710
if (pub == nullptr)
@@ -4728,9 +4736,6 @@ void ECDH::GetPublicKey(const FunctionCallbackInfo<Value>& args) {
47284736

47294737
ECDH* ecdh = Unwrap<ECDH>(args.Holder());
47304738

4731-
if (!ecdh->generated_)
4732-
return env->ThrowError("You should generate ECDH keys first");
4733-
47344739
const EC_POINT* pub = EC_KEY_get0_public_key(ecdh->key_);
47354740
if (pub == nullptr)
47364741
return env->ThrowError("Failed to get ECDH public key");
@@ -4763,9 +4768,6 @@ void ECDH::GetPrivateKey(const FunctionCallbackInfo<Value>& args) {
47634768

47644769
ECDH* ecdh = Unwrap<ECDH>(args.Holder());
47654770

4766-
if (!ecdh->generated_)
4767-
return env->ThrowError("You should generate ECDH keys first");
4768-
47694771
const BIGNUM* b = EC_KEY_get0_private_key(ecdh->key_);
47704772
if (b == nullptr)
47714773
return env->ThrowError("Failed to get ECDH private key");
@@ -4799,12 +4801,42 @@ void ECDH::SetPrivateKey(const FunctionCallbackInfo<Value>& args) {
47994801
if (priv == nullptr)
48004802
return env->ThrowError("Failed to convert Buffer to BN");
48014803

4804+
if (!ecdh->IsKeyValidForCurve(priv)) {
4805+
BN_free(priv);
4806+
return env->ThrowError("Private key is not valid for specified curve.");
4807+
}
4808+
48024809
int result = EC_KEY_set_private_key(ecdh->key_, priv);
48034810
BN_free(priv);
48044811

48054812
if (!result) {
48064813
return env->ThrowError("Failed to convert BN to a private key");
48074814
}
4815+
4816+
// To avoid inconsistency, clear the current public key in-case computing
4817+
// the new one fails for some reason.
4818+
EC_KEY_set_public_key(ecdh->key_, nullptr);
4819+
4820+
MarkPopErrorOnReturn mark_pop_error_on_return;
4821+
(void) &mark_pop_error_on_return; // Silence compiler warning.
4822+
4823+
const BIGNUM* priv_key = EC_KEY_get0_private_key(ecdh->key_);
4824+
CHECK_NE(priv_key, nullptr);
4825+
4826+
EC_POINT* pub = EC_POINT_new(ecdh->group_);
4827+
CHECK_NE(pub, nullptr);
4828+
4829+
if (!EC_POINT_mul(ecdh->group_, pub, priv_key, nullptr, nullptr, nullptr)) {
4830+
EC_POINT_free(pub);
4831+
return env->ThrowError("Failed to generate ECDH public key");
4832+
}
4833+
4834+
if (!EC_KEY_set_public_key(ecdh->key_, pub)) {
4835+
EC_POINT_free(pub);
4836+
return env->ThrowError("Failed to set generated public key");
4837+
}
4838+
4839+
EC_POINT_free(pub);
48084840
}
48094841

48104842

@@ -4818,12 +4850,36 @@ void ECDH::SetPublicKey(const FunctionCallbackInfo<Value>& args) {
48184850
EC_POINT* pub = ecdh->BufferToPoint(Buffer::Data(args[0].As<Object>()),
48194851
Buffer::Length(args[0].As<Object>()));
48204852
if (pub == nullptr)
4821-
return;
4853+
return env->ThrowError("Failed to convert Buffer to EC_POINT");
48224854

48234855
int r = EC_KEY_set_public_key(ecdh->key_, pub);
48244856
EC_POINT_free(pub);
48254857
if (!r)
4826-
return env->ThrowError("Failed to convert BN to a private key");
4858+
return env->ThrowError("Failed to set EC_POINT as the public key");
4859+
}
4860+
4861+
4862+
bool ECDH::IsKeyValidForCurve(const BIGNUM* private_key) {
4863+
ASSERT_NE(group_, nullptr);
4864+
CHECK_NE(private_key, nullptr);
4865+
// Private keys must be in the range [1, n-1].
4866+
// Ref: Section 3.2.1 - http://www.secg.org/sec1-v2.pdf
4867+
if (BN_cmp(private_key, BN_value_one()) < 0) {
4868+
return false;
4869+
}
4870+
BIGNUM* order = BN_new();
4871+
CHECK_NE(order, nullptr);
4872+
bool result = EC_GROUP_get_order(group_, order, nullptr) &&
4873+
BN_cmp(private_key, order) < 0;
4874+
BN_free(order);
4875+
return result;
4876+
}
4877+
4878+
4879+
bool ECDH::IsKeyPairValid() {
4880+
MarkPopErrorOnReturn mark_pop_error_on_return;
4881+
(void) &mark_pop_error_on_return; // Silence compiler warning.
4882+
return 1 == EC_KEY_check_key(key_);
48274883
}
48284884

48294885

Collapse file

‎src/node_crypto.h‎

Copy file name to clipboardExpand all lines: src/node_crypto.h
+3-2Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -693,7 +693,6 @@ class ECDH : public BaseObject {
693693
protected:
694694
ECDH(Environment* env, v8::Local<v8::Object> wrap, EC_KEY* key)
695695
: BaseObject(env, wrap),
696-
generated_(false),
697696
key_(key),
698697
group_(EC_KEY_get0_group(key_)) {
699698
MakeWeak<ECDH>(this);
@@ -710,7 +709,9 @@ class ECDH : public BaseObject {
710709

711710
EC_POINT* BufferToPoint(char* data, size_t len);
712711

713-
bool generated_;
712+
bool IsKeyPairValid();
713+
bool IsKeyValidForCurve(const BIGNUM* private_key);
714+
714715
EC_KEY* key_;
715716
const EC_GROUP* group_;
716717
};
Collapse file

‎test/parallel/test-crypto-dh.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-crypto-dh.js
+90-16Lines changed: 90 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
'use strict';
2-
var common = require('../common');
3-
var assert = require('assert');
4-
var constants = require('constants');
2+
const common = require('../common');
3+
const assert = require('assert');
4+
const constants = require('constants');
55

66
if (!common.hasCrypto) {
77
console.log('1..0 # Skipped: missing crypto');
88
return;
99
}
10-
var crypto = require('crypto');
10+
const crypto = require('crypto');
1111

1212
// Test Diffie-Hellman with two parties sharing a secret,
1313
// using various encodings as we go along
@@ -150,36 +150,110 @@ assert.equal(bad_dh.verifyError, constants.DH_NOT_SUITABLE_GENERATOR);
150150

151151

152152
// Test ECDH
153-
var ecdh1 = crypto.createECDH('prime256v1');
154-
var ecdh2 = crypto.createECDH('prime256v1');
155-
var key1 = ecdh1.generateKeys();
156-
var key2 = ecdh2.generateKeys('hex');
157-
var secret1 = ecdh1.computeSecret(key2, 'hex', 'base64');
158-
var secret2 = ecdh2.computeSecret(key1, 'binary', 'buffer');
153+
const ecdh1 = crypto.createECDH('prime256v1');
154+
const ecdh2 = crypto.createECDH('prime256v1');
155+
key1 = ecdh1.generateKeys();
156+
key2 = ecdh2.generateKeys('hex');
157+
secret1 = ecdh1.computeSecret(key2, 'hex', 'base64');
158+
secret2 = ecdh2.computeSecret(key1, 'binary', 'buffer');
159159

160160
assert.equal(secret1, secret2.toString('base64'));
161161

162162
// Point formats
163163
assert.equal(ecdh1.getPublicKey('buffer', 'uncompressed')[0], 4);
164-
var firstByte = ecdh1.getPublicKey('buffer', 'compressed')[0];
164+
let firstByte = ecdh1.getPublicKey('buffer', 'compressed')[0];
165165
assert(firstByte === 2 || firstByte === 3);
166-
var firstByte = ecdh1.getPublicKey('buffer', 'hybrid')[0];
166+
firstByte = ecdh1.getPublicKey('buffer', 'hybrid')[0];
167167
assert(firstByte === 6 || firstByte === 7);
168168

169169
// ECDH should check that point is on curve
170-
var ecdh3 = crypto.createECDH('secp256k1');
171-
var key3 = ecdh3.generateKeys();
170+
const ecdh3 = crypto.createECDH('secp256k1');
171+
const key3 = ecdh3.generateKeys();
172172

173173
assert.throws(function() {
174-
var secret3 = ecdh2.computeSecret(key3, 'binary', 'buffer');
174+
ecdh2.computeSecret(key3, 'binary', 'buffer');
175175
});
176176

177177
// ECDH should allow .setPrivateKey()/.setPublicKey()
178-
var ecdh4 = crypto.createECDH('prime256v1');
178+
const ecdh4 = crypto.createECDH('prime256v1');
179179

180180
ecdh4.setPrivateKey(ecdh1.getPrivateKey());
181181
ecdh4.setPublicKey(ecdh1.getPublicKey());
182182

183183
assert.throws(function() {
184184
ecdh4.setPublicKey(ecdh3.getPublicKey());
185+
}, /Failed to convert Buffer to EC_POINT/);
186+
187+
// Verify that we can use ECDH without having to use newly generated keys.
188+
const ecdh5 = crypto.createECDH('secp256k1');
189+
190+
// Verify errors are thrown when retrieving keys from an uninitialized object.
191+
assert.throws(function() {
192+
ecdh5.getPublicKey();
193+
}, /Failed to get ECDH public key/);
194+
assert.throws(function() {
195+
ecdh5.getPrivateKey();
196+
}, /Failed to get ECDH private key/);
197+
198+
// A valid private key for the secp256k1 curve.
199+
const cafebabeKey = 'cafebabe'.repeat(8);
200+
// Associated compressed and uncompressed public keys (points).
201+
const cafebabePubPtComp =
202+
'03672a31bfc59d3f04548ec9b7daeeba2f61814e8ccc40448045007f5479f693a3';
203+
const cafebabePubPtUnComp =
204+
'04672a31bfc59d3f04548ec9b7daeeba2f61814e8ccc40448045007f5479f693a3' +
205+
'2e02c7f93d13dc2732b760ca377a5897b9dd41a1c1b29dc0442fdce6d0a04d1d';
206+
ecdh5.setPrivateKey(cafebabeKey, 'hex');
207+
assert.equal(ecdh5.getPrivateKey('hex'), cafebabeKey);
208+
// Show that the public point (key) is generated while setting the private key.
209+
assert.equal(ecdh5.getPublicKey('hex'), cafebabePubPtUnComp);
210+
211+
// Compressed and uncompressed public points/keys for other party's private key
212+
// 0xDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEF
213+
const peerPubPtComp =
214+
'02c6b754b20826eb925e052ee2c25285b162b51fdca732bcf67e39d647fb6830ae';
215+
const peerPubPtUnComp =
216+
'04c6b754b20826eb925e052ee2c25285b162b51fdca732bcf67e39d647fb6830ae' +
217+
'b651944a574a362082a77e3f2b5d9223eb54d7f2f76846522bf75f3bedb8178e';
218+
219+
const sharedSecret =
220+
'1da220b5329bbe8bfd19ceef5a5898593f411a6f12ea40f2a8eead9a5cf59970';
221+
222+
assert.equal(ecdh5.computeSecret(peerPubPtComp, 'hex', 'hex'), sharedSecret);
223+
assert.equal(ecdh5.computeSecret(peerPubPtUnComp, 'hex', 'hex'), sharedSecret);
224+
225+
// Verify that we still have the same key pair as before the computation.
226+
assert.equal(ecdh5.getPrivateKey('hex'), cafebabeKey);
227+
assert.equal(ecdh5.getPublicKey('hex'), cafebabePubPtUnComp);
228+
229+
// Verify setting and getting compressed and non-compressed serializations.
230+
ecdh5.setPublicKey(cafebabePubPtComp, 'hex');
231+
assert.equal(ecdh5.getPublicKey('hex'), cafebabePubPtUnComp);
232+
assert.equal(ecdh5.getPublicKey('hex', 'compressed'), cafebabePubPtComp);
233+
ecdh5.setPublicKey(cafebabePubPtUnComp, 'hex');
234+
assert.equal(ecdh5.getPublicKey('hex'), cafebabePubPtUnComp);
235+
assert.equal(ecdh5.getPublicKey('hex', 'compressed'), cafebabePubPtComp);
236+
237+
// Show why allowing the public key to be set on this type does not make sense.
238+
ecdh5.setPublicKey(peerPubPtComp, 'hex');
239+
assert.equal(ecdh5.getPublicKey('hex'), peerPubPtUnComp);
240+
assert.throws(function() {
241+
// Error because the public key does not match the private key anymore.
242+
ecdh5.computeSecret(peerPubPtComp, 'hex', 'hex');
243+
}, /Invalid key pair/);
244+
245+
// Set to a valid key to show that later attempts to set an invalid key are
246+
// rejected.
247+
ecdh5.setPrivateKey(cafebabeKey, 'hex');
248+
249+
[ // Some invalid private keys for the secp256k1 curve.
250+
'0000000000000000000000000000000000000000000000000000000000000000',
251+
'FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141',
252+
'FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF',
253+
].forEach(function(element, index, object) {
254+
assert.throws(function() {
255+
ecdh5.setPrivateKey(element, 'hex');
256+
}, /Private key is not valid for specified curve/);
257+
// Verify object state did not change.
258+
assert.equal(ecdh5.getPrivateKey('hex'), cafebabeKey);
185259
});

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.