Skip to content

Commit f3af453

Browse files
davidbenMylesBorins
davidben
authored and
MylesBorins
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 <ohtsu@ohtsu.org> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
1 parent d29d846 commit f3af453

10 files changed

+110
-99
lines changed

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

+1-1
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

+19-21
Original file line numberDiff line numberDiff line change
@@ -842,28 +842,31 @@ of two ways:
842842
- Using the [`sign.update()`][] and [`sign.sign()`][] methods to produce the
843843
signature.
844844

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

848849
Example: Using `Sign` objects as streams:
849850

850851
```js
851852
const crypto = require('crypto');
852-
const sign = crypto.createSign('RSA-SHA256');
853+
const sign = crypto.createSign('SHA256');
853854

854855
sign.write('some data to sign');
855856
sign.end();
856857

857858
const privateKey = getPrivateKeySomehow();
858859
console.log(sign.sign(privateKey, 'hex'));
859-
// Prints: the calculated signature
860+
// Prints: the calculated signature using the specified private key and
861+
// SHA-256. For RSA keys, the algorithm is RSASSA-PKCS1-v1_5 (see padding
862+
// parameter below for RSASSA-PSS). For EC keys, the algorithm is ECDSA.
860863
```
861864

862865
Example: Using the [`sign.update()`][] and [`sign.sign()`][] methods:
863866

864867
```js
865868
const crypto = require('crypto');
866-
const sign = crypto.createSign('RSA-SHA256');
869+
const sign = crypto.createSign('SHA256');
867870

868871
sign.update('some data to sign');
869872

@@ -872,27 +875,22 @@ console.log(sign.sign(privateKey, 'hex'));
872875
// Prints: the calculated signature
873876
```
874877

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

880-
Example: signing using ECDSA with SHA256
883+
Example: signing using legacy signature algorithm name
881884

882885
```js
883886
const crypto = require('crypto');
884-
const sign = crypto.createSign('sha256');
887+
const sign = crypto.createSign('RSA-SHA256');
885888

886889
sign.update('some data to sign');
887890

888-
const privateKey =
889-
`-----BEGIN EC PRIVATE KEY-----
890-
MHcCAQEEIF+jnWY1D5kbVYDNvxxo/Y+ku2uJPDwS0r/VuPZQrjjVoAoGCCqGSM49
891-
AwEHoUQDQgAEurOxfSxmqIRYzJVagdZfMMSjRNNhB8i3mXyIMq704m2m52FdfKZ2
892-
pQhByd5eyj3lgZ7m7jbchtdgyOF8Io/1ng==
893-
-----END EC PRIVATE KEY-----`;
894-
895-
console.log(sign.sign(privateKey).toString('hex'));
891+
const privateKey = getPrivateKeySomehow();
892+
console.log(sign.sign(privateKey, 'hex'));
893+
// Prints: the calculated signature
896894
```
897895

898896
### sign.sign(private_key[, output_format])
@@ -965,7 +963,7 @@ Example: Using `Verify` objects as streams:
965963

966964
```js
967965
const crypto = require('crypto');
968-
const verify = crypto.createVerify('RSA-SHA256');
966+
const verify = crypto.createVerify('SHA256');
969967

970968
verify.write('some data to sign');
971969
verify.end();
@@ -980,7 +978,7 @@ Example: Using the [`verify.update()`][] and [`verify.verify()`][] methods:
980978

981979
```js
982980
const crypto = require('crypto');
983-
const verify = crypto.createVerify('RSA-SHA256');
981+
const verify = crypto.createVerify('SHA256');
984982

985983
verify.update('some data to sign');
986984

‎src/node_crypto.cc

+19-28
Original file line numberDiff line numberDiff line change
@@ -3993,7 +3993,8 @@ void SignBase::CheckThrow(SignBase::Error error) {
39933993

39943994
static bool ApplyRSAOptions(EVP_PKEY* pkey, EVP_PKEY_CTX* pkctx, int padding,
39953995
int salt_len) {
3996-
if (pkey->type == EVP_PKEY_RSA || pkey->type == EVP_PKEY_RSA2) {
3996+
if (EVP_PKEY_id(pkey) == EVP_PKEY_RSA ||
3997+
EVP_PKEY_id(pkey) == EVP_PKEY_RSA2) {
39973998
if (EVP_PKEY_CTX_set_rsa_padding(pkctx, padding) <= 0)
39983999
return false;
39994000
if (padding == RSA_PKCS1_PSS_PADDING) {
@@ -4102,33 +4103,23 @@ static int Node_SignFinal(EVP_MD_CTX* mdctx, unsigned char* md,
41024103
if (!EVP_DigestFinal_ex(mdctx, m, &m_len))
41034104
return rv;
41044105

4105-
if (mdctx->digest->flags & EVP_MD_FLAG_PKEY_METHOD_SIGNATURE) {
4106-
size_t sltmp = static_cast<size_t>(EVP_PKEY_size(pkey));
4107-
pkctx = EVP_PKEY_CTX_new(pkey, nullptr);
4108-
if (pkctx == nullptr)
4109-
goto err;
4110-
if (EVP_PKEY_sign_init(pkctx) <= 0)
4111-
goto err;
4112-
if (!ApplyRSAOptions(pkey, pkctx, padding, pss_salt_len))
4113-
goto err;
4114-
if (EVP_PKEY_CTX_set_signature_md(pkctx, mdctx->digest) <= 0)
4115-
goto err;
4116-
if (EVP_PKEY_sign(pkctx, md, &sltmp, m, m_len) <= 0)
4117-
goto err;
4118-
*sig_len = sltmp;
4119-
rv = 1;
4120-
err:
4121-
EVP_PKEY_CTX_free(pkctx);
4122-
return rv;
4123-
}
4124-
4125-
if (mdctx->digest->sign == nullptr) {
4126-
EVPerr(EVP_F_EVP_SIGNFINAL, EVP_R_NO_SIGN_FUNCTION_CONFIGURED);
4127-
return 0;
4128-
}
4129-
4130-
return mdctx->digest->sign(mdctx->digest->type, m, m_len, md, sig_len,
4131-
pkey->pkey.ptr);
4106+
size_t sltmp = static_cast<size_t>(EVP_PKEY_size(pkey));
4107+
pkctx = EVP_PKEY_CTX_new(pkey, nullptr);
4108+
if (pkctx == nullptr)
4109+
goto err;
4110+
if (EVP_PKEY_sign_init(pkctx) <= 0)
4111+
goto err;
4112+
if (!ApplyRSAOptions(pkey, pkctx, padding, pss_salt_len))
4113+
goto err;
4114+
if (EVP_PKEY_CTX_set_signature_md(pkctx, EVP_MD_CTX_md(mdctx)) <= 0)
4115+
goto err;
4116+
if (EVP_PKEY_sign(pkctx, md, &sltmp, m, m_len) <= 0)
4117+
goto err;
4118+
*sig_len = sltmp;
4119+
rv = 1;
4120+
err:
4121+
EVP_PKEY_CTX_free(pkctx);
4122+
return rv;
41324123
}
41334124

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

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

+2-2
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

+12-12
Original file line numberDiff line numberDiff line change
@@ -404,28 +404,28 @@ assert.throws(function() {
404404
}, /^Error: Digest method not supported$/);
405405

406406
// Test signing and verifying
407-
const s1 = crypto.createSign('RSA-SHA1')
407+
const s1 = crypto.createSign('SHA1')
408408
.update('Test123')
409409
.sign(keyPem, 'base64');
410-
const s1Verified = crypto.createVerify('RSA-SHA1')
410+
const s1Verified = crypto.createVerify('SHA1')
411411
.update('Test')
412412
.update('123')
413413
.verify(certPem, s1, 'base64');
414414
assert.strictEqual(s1Verified, true, 'sign and verify (base 64)');
415415

416-
const s2 = crypto.createSign('RSA-SHA256')
416+
const s2 = crypto.createSign('SHA256')
417417
.update('Test123')
418418
.sign(keyPem); // binary
419-
const s2Verified = crypto.createVerify('RSA-SHA256')
419+
const s2Verified = crypto.createVerify('SHA256')
420420
.update('Test')
421421
.update('123')
422422
.verify(certPem, s2); // binary
423423
assert.strictEqual(s2Verified, true, 'sign and verify (binary)');
424424

425-
const s3 = crypto.createSign('RSA-SHA1')
425+
const s3 = crypto.createSign('SHA1')
426426
.update('Test123')
427427
.sign(keyPem, 'buffer');
428-
const s3Verified = crypto.createVerify('RSA-SHA1')
428+
const s3Verified = crypto.createVerify('SHA1')
429429
.update('Test')
430430
.update('123')
431431
.verify(certPem, s3);
@@ -569,8 +569,8 @@ const d = crypto.createDiffieHellman(p, 'hex');
569569
assert.strictEqual(d.verifyError, DH_NOT_SUITABLE_GENERATOR);
570570

571571
// Test RSA key signing/verification
572-
const rsaSign = crypto.createSign('RSA-SHA1');
573-
const rsaVerify = crypto.createVerify('RSA-SHA1');
572+
const rsaSign = crypto.createSign('SHA1');
573+
const rsaVerify = crypto.createVerify('SHA1');
574574
assert.ok(rsaSign instanceof crypto.Sign);
575575
assert.ok(rsaVerify instanceof crypto.Verify);
576576

@@ -606,13 +606,13 @@ assert.strictEqual(rsaVerify.verify(rsaPubPem, rsaSignature, 'hex'), true);
606606
'8195e0268da7eda23d9825ac43c724e86ceeee0d0d4465678652ccaf6501' +
607607
'0ddfb299bedeb1ad';
608608

609-
const sign = crypto.createSign('RSA-SHA256');
609+
const sign = crypto.createSign('SHA256');
610610
sign.update(input);
611611

612612
const output = sign.sign(privateKey, 'hex');
613613
assert.strictEqual(output, signature);
614614

615-
const verify = crypto.createVerify('RSA-SHA256');
615+
const verify = crypto.createVerify('SHA256');
616616
verify.update(input);
617617

618618
assert.strictEqual(verify.verify(publicKey, signature, 'hex'), true);
@@ -631,11 +631,11 @@ assert.strictEqual(rsaVerify.verify(rsaPubPem, rsaSignature, 'hex'), true);
631631

632632
// DSA signatures vary across runs so there is no static string to verify
633633
// against
634-
const sign = crypto.createSign('DSS1');
634+
const sign = crypto.createSign('SHA1');
635635
sign.update(input);
636636
const signature = sign.sign(privateKey, 'hex');
637637

638-
const verify = crypto.createVerify('DSS1');
638+
const verify = crypto.createVerify('SHA1');
639639
verify.update(input);
640640

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

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

+34-12
Original file line numberDiff line numberDiff line change
@@ -132,8 +132,8 @@ test_rsa('RSA_PKCS1_PADDING');
132132
test_rsa('RSA_PKCS1_OAEP_PADDING');
133133

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

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

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

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

167-
rsaSign = crypto.createSign('RSA-SHA1');
167+
rsaSign = crypto.createSign('SHA1');
168168
rsaSign.update(rsaPubPem);
169169
assert.throws(() => {
170170
const signOptions = { key: rsaKeyPemEncrypted, passphrase: 'wrong' };
@@ -188,16 +188,28 @@ assert.throws(() => {
188188
'8195e0268da7eda23d9825ac43c724e86ceeee0d0d4465678652ccaf6501' +
189189
'0ddfb299bedeb1ad';
190190

191-
const sign = crypto.createSign('RSA-SHA256');
191+
const sign = crypto.createSign('SHA256');
192192
sign.update(input);
193193

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

197-
const verify = crypto.createVerify('RSA-SHA256');
197+
const verify = crypto.createVerify('SHA256');
198198
verify.update(input);
199199

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

203215

@@ -209,14 +221,24 @@ assert.throws(() => {
209221

210222
// DSA signatures vary across runs so there is no static string to verify
211223
// against
212-
const sign = crypto.createSign('DSS1');
224+
const sign = crypto.createSign('SHA1');
213225
sign.update(input);
214226
const signature = sign.sign(dsaKeyPem, 'hex');
215227

216-
const verify = crypto.createVerify('DSS1');
228+
const verify = crypto.createVerify('SHA1');
217229
verify.update(input);
218230

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

222244

@@ -226,7 +248,7 @@ assert.throws(() => {
226248
const input = 'I AM THE WALRUS';
227249

228250
{
229-
const sign = crypto.createSign('DSS1');
251+
const sign = crypto.createSign('SHA1');
230252
sign.update(input);
231253
assert.throws(() => {
232254
sign.sign({ key: dsaKeyPemEncrypted, passphrase: 'wrong' }, 'hex');
@@ -236,7 +258,7 @@ const input = 'I AM THE WALRUS';
236258
{
237259
// DSA signatures vary across runs so there is no static string to verify
238260
// against
239-
const sign = crypto.createSign('DSS1');
261+
const sign = crypto.createSign('SHA1');
240262
sign.update(input);
241263

242264
let signature;
@@ -245,7 +267,7 @@ const input = 'I AM THE WALRUS';
245267
signature = sign.sign(signOptions, 'hex');
246268
});
247269

248-
const verify = crypto.createVerify('DSS1');
270+
const verify = crypto.createVerify('SHA1');
249271
verify.update(input);
250272

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

0 commit comments

Comments
 (0)