Skip to content

Commit 775173a

Browse files
panvaaduh95
authored andcommitted
crypto: optimize private EC JWK import
Avoid creating temporary EVP objects and repeating key validation while retaining the private scalar range and public/private consistency checks. Assisted-by: GitHub Copilot Signed-off-by: Filip Skokan <panva.ip@gmail.com> PR-URL: #65908 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
1 parent 28acafe commit 775173a

4 files changed

Lines changed: 103 additions & 2 deletions

File tree

deps/ncrypto/ncrypto.cc

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5348,6 +5348,24 @@ bool ECPointPointer::mul(const EC_GROUP* group, const BIGNUM* priv_key) {
53485348

53495349
// ============================================================================
53505350

5351+
bool ECKeyPointer::checkPrivateKey() const {
5352+
const auto group = getGroup();
5353+
const auto priv = getPrivateKey();
5354+
const auto pub = getPublicKey();
5355+
if (group == nullptr || priv == nullptr || pub == nullptr) return false;
5356+
5357+
auto order = BignumPointer::New();
5358+
if (!order || !EC_GROUP_get_order(group, order.get(), nullptr) ||
5359+
BN_is_zero(priv) || BN_is_negative(priv) ||
5360+
BN_cmp(priv, order.get()) >= 0) {
5361+
return false;
5362+
}
5363+
5364+
auto expected = ECPointPointer::New(group);
5365+
return expected && expected.mul(group, priv) &&
5366+
EC_POINT_cmp(group, expected.get(), pub, nullptr) == 0;
5367+
}
5368+
53515369
#if NCRYPTO_USE_LEGACY_KEY_TYPES
53525370
ECKeyPointer::ECKeyPointer() : key_(nullptr) {}
53535371

deps/ncrypto/ncrypto.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1764,6 +1764,7 @@ class ECKeyPointer final {
17641764
bool setPublicKeyRaw(const BignumPointer& x, const BignumPointer& y);
17651765
bool generate();
17661766
bool checkKey() const;
1767+
bool checkPrivateKey() const;
17671768
DataPointer computeSecret(const ECPointPointer& peer) const;
17681769

17691770
const EC_GROUP* getGroup() const;

src/crypto/crypto_ec.cc

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -756,15 +756,18 @@ KeyObjectData ImportJWKEcKey(Environment* env, Local<Object> jwk) {
756756
return {};
757757
}
758758
// Verify that the public point matches the private scalar (d*G == (x,y)).
759-
if (!ec.checkKey()) {
759+
if (!ec.checkPrivateKey()) {
760760
THROW_ERR_CRYPTO_INVALID_JWK(env, "Invalid JWK EC key");
761761
return {};
762762
}
763763
}
764764

765765
auto pkey = EVPKeyPointer::New();
766766
if (!pkey) return {};
767-
CHECK(pkey.set(ec));
767+
if (!pkey.set(ec)) {
768+
THROW_ERR_CRYPTO_INVALID_JWK(env, "Invalid JWK EC key");
769+
return {};
770+
}
768771

769772
return KeyObjectData::CreateAsymmetric(type, std::move(pkey));
770773
}
Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
7+
const assert = require('assert');
8+
const {
9+
createECDH,
10+
createPrivateKey,
11+
createPublicKey,
12+
getCurves,
13+
getFips,
14+
sign,
15+
verify,
16+
} = require('crypto');
17+
18+
const curves = [
19+
['prime256v1', 'P-256', 32,
20+
'ffffffff00000000ffffffffffffffffbce6faada7179e84f3b9cac2fc632551'],
21+
['secp384r1', 'P-384', 48,
22+
'ffffffffffffffffffffffffffffffffffffffffffffffffc7634d81f4372ddf' +
23+
'581a0db248b0a77aecec196accc52973'],
24+
['secp521r1', 'P-521', 66,
25+
'01ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff' +
26+
'fa51868783bf2f966b7fcc0148f709a5d03bb5c9b8899c47aebb6fb71e91386409'],
27+
];
28+
if (!getFips() && getCurves().includes('secp256k1')) {
29+
curves.push(['secp256k1', 'secp256k1', 32,
30+
'fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141']);
31+
}
32+
33+
for (const [namedCurve, crv, width, orderHex] of curves) {
34+
const order = BigInt(`0x${orderHex}`);
35+
const encode = (scalar) => Buffer.from(
36+
scalar.toString(16).padStart(width * 2, '0'), 'hex');
37+
const makeJwk = (scalar) => {
38+
const ecdh = createECDH(namedCurve);
39+
ecdh.setPrivateKey(encode(scalar));
40+
const point = ecdh.getPublicKey();
41+
return {
42+
kty: 'EC',
43+
crv,
44+
x: point.subarray(1, 1 + width).toString('base64url'),
45+
y: point.subarray(1 + width).toString('base64url'),
46+
d: encode(scalar).toString('base64url'),
47+
};
48+
};
49+
const generator = makeJwk(1n);
50+
const other = makeJwk(2n);
51+
const message = Buffer.from('EC JWK private key consistency');
52+
53+
for (const jwk of [generator, other, makeJwk(order - 1n)]) {
54+
const key = createPrivateKey({ format: 'jwk', key: jwk });
55+
assert.deepStrictEqual(key.export({ format: 'jwk' }), jwk);
56+
const publicJwk = { kty: jwk.kty, crv, x: jwk.x, y: jwk.y };
57+
const publicKey = createPublicKey({ format: 'jwk', key: publicJwk });
58+
assert(verify('sha256', message, publicKey, sign('sha256', message, key)));
59+
}
60+
61+
const invalid = [
62+
{ ...generator, d: other.d },
63+
{ ...generator, x: other.x, y: other.y },
64+
...[0n, order, order + 1n].map((scalar) => ({
65+
...generator, d: encode(scalar).toString('base64url'),
66+
})),
67+
{ ...generator, d: '' },
68+
{
69+
...generator,
70+
x: Buffer.alloc(width).toString('base64url'),
71+
y: Buffer.alloc(width).toString('base64url'),
72+
},
73+
];
74+
for (const jwk of invalid) {
75+
assert.throws(() => createPrivateKey({ format: 'jwk', key: jwk }), {
76+
code: 'ERR_CRYPTO_INVALID_JWK',
77+
});
78+
}
79+
}

0 commit comments

Comments
 (0)