Skip to content

Commit c26e93b

Browse files
davidbenjasnell
davidben
authored and
jasnell
committed
crypto: fix Node_SignFinal
PR #11705 switched Node away from using using OpenSSL's legacy EVP_Sign* and EVP_Verify* APIs. Instead, it computes a hash normally via EVP_Digest* and then uses EVP_PKEY_sign and EVP_PKEY_verify to verify the hash directly. This change corrects two problems: 1. The documentation still recommends the signature algorithm EVP_MD names of OpenSSL's legacy APIs. OpenSSL has since moved away from thosee, which is why ECDSA was strangely inconsistent. (This is why "ecdsa-with-SHA256" was missing.) 2. Node_SignFinal copied some code from EVP_SignFinal's internals. This is problematic for OpenSSL 1.1.0 and is missing a critical check that prevents pkey->pkey.ptr from being cast to the wrong type. To resolve this, remove the non-EVP_PKEY_sign codepath. This codepath is no longer necessary. PR #11705's verify half was already assuming all EVP_PKEYs supported EVP_PKEY_sign and EVP_PKEY_verify. Also, in the documentation, point users towards using hash function names which are more consisent. This avoids an ECDSA special-case and some strangeness around RSA-PSS ("RSA-SHA256" is the OpenSSL name of the sha256WithRSAEncryption OID which is not used for RSA-PSS). PR-URL: #15024 Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
1 parent b1a0bdc commit c26e93b

File tree

10 files changed

+110
-99
lines changed

10 files changed

+110
-99
lines changed

‎benchmark/crypto/rsa-sign-verify-throughput.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ keylen_list.forEach(function(key) {
1818

1919
var bench = common.createBenchmark(main, {
2020
writes: [500],
21-
algo: ['RSA-SHA1', 'RSA-SHA224', 'RSA-SHA256', 'RSA-SHA384', 'RSA-SHA512'],
21+
algo: ['SHA1', 'SHA224', 'SHA256', 'SHA384', 'SHA512'],
2222
keylen: keylen_list,
2323
len: [1024, 102400, 2 * 102400, 3 * 102400, 1024 * 1024]
2424
});

‎doc/api/crypto.md

Lines changed: 19 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -918,28 +918,31 @@ of two ways:
918918
- Using the [`sign.update()`][] and [`sign.sign()`][] methods to produce the
919919
signature.
920920

921-
The [`crypto.createSign()`][] method is used to create `Sign` instances. `Sign`
922-
objects are not to be created directly using the `new` keyword.
921+
The [`crypto.createSign()`][] method is used to create `Sign` instances. The
922+
argument is the string name of the hash function to use. `Sign` objects are not
923+
to be created directly using the `new` keyword.
923924

924925
Example: Using `Sign` objects as streams:
925926

926927
```js
927928
const crypto = require('crypto');
928-
const sign = crypto.createSign('RSA-SHA256');
929+
const sign = crypto.createSign('SHA256');
929930

930931
sign.write('some data to sign');
931932
sign.end();
932933

933934
const privateKey = getPrivateKeySomehow();
934935
console.log(sign.sign(privateKey, 'hex'));
935-
// Prints: the calculated signature
936+
// Prints: the calculated signature using the specified private key and
937+
// SHA-256. For RSA keys, the algorithm is RSASSA-PKCS1-v1_5 (see padding
938+
// parameter below for RSASSA-PSS). For EC keys, the algorithm is ECDSA.
936939
```
937940

938941
Example: Using the [`sign.update()`][] and [`sign.sign()`][] methods:
939942

940943
```js
941944
const crypto = require('crypto');
942-
const sign = crypto.createSign('RSA-SHA256');
945+
const sign = crypto.createSign('SHA256');
943946

944947
sign.update('some data to sign');
945948

@@ -948,27 +951,22 @@ console.log(sign.sign(privateKey, 'hex'));
948951
// Prints: the calculated signature
949952
```
950953

951-
A `Sign` instance can also be created by just passing in the digest
952-
algorithm name, in which case OpenSSL will infer the full signature algorithm
953-
from the type of the PEM-formatted private key, including algorithms that
954-
do not have directly exposed name constants, e.g. 'ecdsa-with-SHA256'.
954+
In some cases, a `Sign` instance can also be created by passing in a signature
955+
algorithm name, such as 'RSA-SHA256'. This will use the corresponding digest
956+
algorithm. This does not work for all signature algorithms, such as
957+
'ecdsa-with-SHA256'. Use digest names instead.
955958

956-
Example: signing using ECDSA with SHA256
959+
Example: signing using legacy signature algorithm name
957960

958961
```js
959962
const crypto = require('crypto');
960-
const sign = crypto.createSign('sha256');
963+
const sign = crypto.createSign('RSA-SHA256');
961964

962965
sign.update('some data to sign');
963966

964-
const privateKey =
965-
`-----BEGIN EC PRIVATE KEY-----
966-
MHcCAQEEIF+jnWY1D5kbVYDNvxxo/Y+ku2uJPDwS0r/VuPZQrjjVoAoGCCqGSM49
967-
AwEHoUQDQgAEurOxfSxmqIRYzJVagdZfMMSjRNNhB8i3mXyIMq704m2m52FdfKZ2
968-
pQhByd5eyj3lgZ7m7jbchtdgyOF8Io/1ng==
969-
-----END EC PRIVATE KEY-----`;
970-
971-
console.log(sign.sign(privateKey).toString('hex'));
967+
const privateKey = getPrivateKeySomehow();
968+
console.log(sign.sign(privateKey, 'hex'));
969+
// Prints: the calculated signature
972970
```
973971

974972
### sign.sign(privateKey[, outputFormat])
@@ -1051,7 +1049,7 @@ Example: Using `Verify` objects as streams:
10511049

10521050
```js
10531051
const crypto = require('crypto');
1054-
const verify = crypto.createVerify('RSA-SHA256');
1052+
const verify = crypto.createVerify('SHA256');
10551053

10561054
verify.write('some data to sign');
10571055
verify.end();
@@ -1066,7 +1064,7 @@ Example: Using the [`verify.update()`][] and [`verify.verify()`][] methods:
10661064

10671065
```js
10681066
const crypto = require('crypto');
1069-
const verify = crypto.createVerify('RSA-SHA256');
1067+
const verify = crypto.createVerify('SHA256');
10701068

10711069
verify.update('some data to sign');
10721070

‎src/node_crypto.cc

Lines changed: 19 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -3974,7 +3974,8 @@ void SignBase::CheckThrow(SignBase::Error error) {
39743974

39753975
static bool ApplyRSAOptions(EVP_PKEY* pkey, EVP_PKEY_CTX* pkctx, int padding,
39763976
int salt_len) {
3977-
if (pkey->type == EVP_PKEY_RSA || pkey->type == EVP_PKEY_RSA2) {
3977+
if (EVP_PKEY_id(pkey) == EVP_PKEY_RSA ||
3978+
EVP_PKEY_id(pkey) == EVP_PKEY_RSA2) {
39783979
if (EVP_PKEY_CTX_set_rsa_padding(pkctx, padding) <= 0)
39793980
return false;
39803981
if (padding == RSA_PKCS1_PSS_PADDING) {
@@ -4083,33 +4084,23 @@ static int Node_SignFinal(EVP_MD_CTX* mdctx, unsigned char* md,
40834084
if (!EVP_DigestFinal_ex(mdctx, m, &m_len))
40844085
return rv;
40854086

4086-
if (mdctx->digest->flags & EVP_MD_FLAG_PKEY_METHOD_SIGNATURE) {
4087-
size_t sltmp = static_cast<size_t>(EVP_PKEY_size(pkey));
4088-
pkctx = EVP_PKEY_CTX_new(pkey, nullptr);
4089-
if (pkctx == nullptr)
4090-
goto err;
4091-
if (EVP_PKEY_sign_init(pkctx) <= 0)
4092-
goto err;
4093-
if (!ApplyRSAOptions(pkey, pkctx, padding, pss_salt_len))
4094-
goto err;
4095-
if (EVP_PKEY_CTX_set_signature_md(pkctx, mdctx->digest) <= 0)
4096-
goto err;
4097-
if (EVP_PKEY_sign(pkctx, md, &sltmp, m, m_len) <= 0)
4098-
goto err;
4099-
*sig_len = sltmp;
4100-
rv = 1;
4101-
err:
4102-
EVP_PKEY_CTX_free(pkctx);
4103-
return rv;
4104-
}
4105-
4106-
if (mdctx->digest->sign == nullptr) {
4107-
EVPerr(EVP_F_EVP_SIGNFINAL, EVP_R_NO_SIGN_FUNCTION_CONFIGURED);
4108-
return 0;
4109-
}
4110-
4111-
return mdctx->digest->sign(mdctx->digest->type, m, m_len, md, sig_len,
4112-
pkey->pkey.ptr);
4087+
size_t sltmp = static_cast<size_t>(EVP_PKEY_size(pkey));
4088+
pkctx = EVP_PKEY_CTX_new(pkey, nullptr);
4089+
if (pkctx == nullptr)
4090+
goto err;
4091+
if (EVP_PKEY_sign_init(pkctx) <= 0)
4092+
goto err;
4093+
if (!ApplyRSAOptions(pkey, pkctx, padding, pss_salt_len))
4094+
goto err;
4095+
if (EVP_PKEY_CTX_set_signature_md(pkctx, EVP_MD_CTX_md(mdctx)) <= 0)
4096+
goto err;
4097+
if (EVP_PKEY_sign(pkctx, md, &sltmp, m, m_len) <= 0)
4098+
goto err;
4099+
*sig_len = sltmp;
4100+
rv = 1;
4101+
err:
4102+
EVP_PKEY_CTX_free(pkctx);
4103+
return rv;
41134104
}
41144105

41154106
SignBase::Error Sign::SignFinal(const char* key_pem,

‎test/fixtures/0-dns/create-cert.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ const BN = asn1.bignum;
88
const id_at_commonName = [ 2, 5, 4, 3 ];
99
const rsaEncryption = [1, 2, 840, 113549, 1, 1, 1];
1010
const sha256WithRSAEncryption = [1, 2, 840, 113549, 1, 1, 11];
11-
const sigalg = 'RSA-SHA256';
11+
const digest = 'SHA256';
1212

1313
const private_key = fs.readFileSync('./0-dns-key.pem');
1414
// public key file can be generated from the private key with
@@ -59,7 +59,7 @@ const tbs = {
5959

6060
const tbs_der = rfc5280.TBSCertificate.encode(tbs, 'der');
6161

62-
const sign = crypto.createSign(sigalg);
62+
const sign = crypto.createSign(digest);
6363
sign.update(tbs_der);
6464
const signature = sign.sign(private_key);
6565

‎test/parallel/test-crypto-binary-default.js

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -424,28 +424,28 @@ assert.throws(function() {
424424
}, /^Error: Digest method not supported$/);
425425

426426
// Test signing and verifying
427-
const s1 = crypto.createSign('RSA-SHA1')
427+
const s1 = crypto.createSign('SHA1')
428428
.update('Test123')
429429
.sign(keyPem, 'base64');
430-
const s1Verified = crypto.createVerify('RSA-SHA1')
430+
const s1Verified = crypto.createVerify('SHA1')
431431
.update('Test')
432432
.update('123')
433433
.verify(certPem, s1, 'base64');
434434
assert.strictEqual(s1Verified, true, 'sign and verify (base 64)');
435435

436-
const s2 = crypto.createSign('RSA-SHA256')
436+
const s2 = crypto.createSign('SHA256')
437437
.update('Test123')
438438
.sign(keyPem); // binary
439-
const s2Verified = crypto.createVerify('RSA-SHA256')
439+
const s2Verified = crypto.createVerify('SHA256')
440440
.update('Test')
441441
.update('123')
442442
.verify(certPem, s2); // binary
443443
assert.strictEqual(s2Verified, true, 'sign and verify (binary)');
444444

445-
const s3 = crypto.createSign('RSA-SHA1')
445+
const s3 = crypto.createSign('SHA1')
446446
.update('Test123')
447447
.sign(keyPem, 'buffer');
448-
const s3Verified = crypto.createVerify('RSA-SHA1')
448+
const s3Verified = crypto.createVerify('SHA1')
449449
.update('Test')
450450
.update('123')
451451
.verify(certPem, s3);
@@ -610,8 +610,8 @@ const d = crypto.createDiffieHellman(p, 'hex');
610610
assert.strictEqual(d.verifyError, DH_NOT_SUITABLE_GENERATOR);
611611

612612
// Test RSA key signing/verification
613-
const rsaSign = crypto.createSign('RSA-SHA1');
614-
const rsaVerify = crypto.createVerify('RSA-SHA1');
613+
const rsaSign = crypto.createSign('SHA1');
614+
const rsaVerify = crypto.createVerify('SHA1');
615615
assert.ok(rsaSign instanceof crypto.Sign);
616616
assert.ok(rsaVerify instanceof crypto.Verify);
617617

@@ -646,13 +646,13 @@ assert.strictEqual(rsaVerify.verify(rsaPubPem, rsaSignature, 'hex'), true);
646646
'8195e0268da7eda23d9825ac43c724e86ceeee0d0d4465678652ccaf6501' +
647647
'0ddfb299bedeb1ad';
648648

649-
const sign = crypto.createSign('RSA-SHA256');
649+
const sign = crypto.createSign('SHA256');
650650
sign.update(input);
651651

652652
const output = sign.sign(privateKey, 'hex');
653653
assert.strictEqual(output, signature);
654654

655-
const verify = crypto.createVerify('RSA-SHA256');
655+
const verify = crypto.createVerify('SHA256');
656656
verify.update(input);
657657

658658
assert.strictEqual(verify.verify(publicKey, signature, 'hex'), true);
@@ -670,11 +670,11 @@ assert.strictEqual(rsaVerify.verify(rsaPubPem, rsaSignature, 'hex'), true);
670670

671671
// DSA signatures vary across runs so there is no static string to verify
672672
// against
673-
const sign = crypto.createSign('DSS1');
673+
const sign = crypto.createSign('SHA1');
674674
sign.update(input);
675675
const signature = sign.sign(privateKey, 'hex');
676676

677-
const verify = crypto.createVerify('DSS1');
677+
const verify = crypto.createVerify('SHA1');
678678
verify.update(input);
679679

680680
assert.strictEqual(verify.verify(publicKey, signature, 'hex'), true);

‎test/parallel/test-crypto-rsa-dsa.js

Lines changed: 34 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -131,8 +131,8 @@ test_rsa('RSA_PKCS1_PADDING');
131131
test_rsa('RSA_PKCS1_OAEP_PADDING');
132132

133133
// Test RSA key signing/verification
134-
let rsaSign = crypto.createSign('RSA-SHA1');
135-
let rsaVerify = crypto.createVerify('RSA-SHA1');
134+
let rsaSign = crypto.createSign('SHA1');
135+
let rsaVerify = crypto.createVerify('SHA1');
136136
assert.ok(rsaSign);
137137
assert.ok(rsaVerify);
138138

@@ -151,19 +151,19 @@ rsaVerify.update(rsaPubPem);
151151
assert.strictEqual(rsaVerify.verify(rsaPubPem, rsaSignature, 'hex'), true);
152152

153153
// Test RSA key signing/verification with encrypted key
154-
rsaSign = crypto.createSign('RSA-SHA1');
154+
rsaSign = crypto.createSign('SHA1');
155155
rsaSign.update(rsaPubPem);
156156
assert.doesNotThrow(() => {
157157
const signOptions = { key: rsaKeyPemEncrypted, passphrase: 'password' };
158158
rsaSignature = rsaSign.sign(signOptions, 'hex');
159159
});
160160
assert.strictEqual(rsaSignature, expectedSignature);
161161

162-
rsaVerify = crypto.createVerify('RSA-SHA1');
162+
rsaVerify = crypto.createVerify('SHA1');
163163
rsaVerify.update(rsaPubPem);
164164
assert.strictEqual(rsaVerify.verify(rsaPubPem, rsaSignature, 'hex'), true);
165165

166-
rsaSign = crypto.createSign('RSA-SHA1');
166+
rsaSign = crypto.createSign('SHA1');
167167
rsaSign.update(rsaPubPem);
168168
assert.throws(() => {
169169
const signOptions = { key: rsaKeyPemEncrypted, passphrase: 'wrong' };
@@ -186,16 +186,28 @@ assert.throws(() => {
186186
'8195e0268da7eda23d9825ac43c724e86ceeee0d0d4465678652ccaf6501' +
187187
'0ddfb299bedeb1ad';
188188

189-
const sign = crypto.createSign('RSA-SHA256');
189+
const sign = crypto.createSign('SHA256');
190190
sign.update(input);
191191

192192
const output = sign.sign(privateKey, 'hex');
193193
assert.strictEqual(signature, output);
194194

195-
const verify = crypto.createVerify('RSA-SHA256');
195+
const verify = crypto.createVerify('SHA256');
196196
verify.update(input);
197197

198198
assert.strictEqual(verify.verify(publicKey, signature, 'hex'), true);
199+
200+
// Test the legacy signature algorithm name.
201+
const sign2 = crypto.createSign('RSA-SHA256');
202+
sign2.update(input);
203+
204+
const output2 = sign2.sign(privateKey, 'hex');
205+
assert.strictEqual(signature, output2);
206+
207+
const verify2 = crypto.createVerify('SHA256');
208+
verify2.update(input);
209+
210+
assert.strictEqual(verify2.verify(publicKey, signature, 'hex'), true);
199211
}
200212

201213

@@ -207,14 +219,24 @@ assert.throws(() => {
207219

208220
// DSA signatures vary across runs so there is no static string to verify
209221
// against
210-
const sign = crypto.createSign('DSS1');
222+
const sign = crypto.createSign('SHA1');
211223
sign.update(input);
212224
const signature = sign.sign(dsaKeyPem, 'hex');
213225

214-
const verify = crypto.createVerify('DSS1');
226+
const verify = crypto.createVerify('SHA1');
215227
verify.update(input);
216228

217229
assert.strictEqual(verify.verify(dsaPubPem, signature, 'hex'), true);
230+
231+
// Test the legacy 'DSS1' name.
232+
const sign2 = crypto.createSign('DSS1');
233+
sign2.update(input);
234+
const signature2 = sign2.sign(dsaKeyPem, 'hex');
235+
236+
const verify2 = crypto.createVerify('DSS1');
237+
verify2.update(input);
238+
239+
assert.strictEqual(verify2.verify(dsaPubPem, signature2, 'hex'), true);
218240
}
219241

220242

@@ -224,7 +246,7 @@ assert.throws(() => {
224246
const input = 'I AM THE WALRUS';
225247

226248
{
227-
const sign = crypto.createSign('DSS1');
249+
const sign = crypto.createSign('SHA1');
228250
sign.update(input);
229251
assert.throws(() => {
230252
sign.sign({ key: dsaKeyPemEncrypted, passphrase: 'wrong' }, 'hex');
@@ -234,7 +256,7 @@ const input = 'I AM THE WALRUS';
234256
{
235257
// DSA signatures vary across runs so there is no static string to verify
236258
// against
237-
const sign = crypto.createSign('DSS1');
259+
const sign = crypto.createSign('SHA1');
238260
sign.update(input);
239261

240262
let signature;
@@ -243,7 +265,7 @@ const input = 'I AM THE WALRUS';
243265
signature = sign.sign(signOptions, 'hex');
244266
});
245267

246-
const verify = crypto.createVerify('DSS1');
268+
const verify = crypto.createVerify('SHA1');
247269
verify.update(input);
248270

249271
assert.strictEqual(verify.verify(dsaPubPem, signature, 'hex'), true);

0 commit comments

Comments
 (0)