Skip to content

Commit 1f57491

Browse files
davidbendavidben
committed
crypto: fix Node_SignFinal
PR nodejs#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 nodejs#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).
1 parent 52fe762 commit 1f57491

10 files changed

Lines changed: 110 additions & 99 deletions

‎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
@@ -916,28 +916,31 @@ of two ways:
916916
- Using the [`sign.update()`][] and [`sign.sign()`][] methods to produce the
917917
signature.
918918

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

922923
Example: Using `Sign` objects as streams:
923924

924925
```js
925926
const crypto = require('crypto');
926-
const sign = crypto.createSign('RSA-SHA256');
927+
const sign = crypto.createSign('SHA256');
927928

928929
sign.write('some data to sign');
929930
sign.end();
930931

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

936939
Example: Using the [`sign.update()`][] and [`sign.sign()`][] methods:
937940

938941
```js
939942
const crypto = require('crypto');
940-
const sign = crypto.createSign('RSA-SHA256');
943+
const sign = crypto.createSign('SHA256');
941944

942945
sign.update('some data to sign');
943946

@@ -946,27 +949,22 @@ console.log(sign.sign(privateKey, 'hex'));
946949
// Prints: the calculated signature
947950
```
948951

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

954-
Example: signing using ECDSA with SHA256
957+
Example: signing using legacy signature algorithm name
955958

956959
```js
957960
const crypto = require('crypto');
958-
const sign = crypto.createSign('sha256');
961+
const sign = crypto.createSign('RSA-SHA256');
959962

960963
sign.update('some data to sign');
961964

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

972970
### sign.sign(privateKey[, outputFormat])
@@ -1049,7 +1047,7 @@ Example: Using `Verify` objects as streams:
10491047

10501048
```js
10511049
const crypto = require('crypto');
1052-
const verify = crypto.createVerify('RSA-SHA256');
1050+
const verify = crypto.createVerify('SHA256');
10531051

10541052
verify.write('some data to sign');
10551053
verify.end();
@@ -1064,7 +1062,7 @@ Example: Using the [`verify.update()`][] and [`verify.verify()`][] methods:
10641062

10651063
```js
10661064
const crypto = require('crypto');
1067-
const verify = crypto.createVerify('RSA-SHA256');
1065+
const verify = crypto.createVerify('SHA256');
10681066

10691067
verify.update('some data to sign');
10701068

‎src/node_crypto.cc‎

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

39773977
static bool ApplyRSAOptions(EVP_PKEY* pkey, EVP_PKEY_CTX* pkctx, int padding,
39783978
int salt_len) {
3979-
if (pkey->type == EVP_PKEY_RSA || pkey->type == EVP_PKEY_RSA2) {
3979+
if (EVP_PKEY_id(pkey) == EVP_PKEY_RSA ||
3980+
EVP_PKEY_id(pkey) == EVP_PKEY_RSA2) {
39803981
if (EVP_PKEY_CTX_set_rsa_padding(pkctx, padding) <= 0)
39813982
return false;
39823983
if (padding == RSA_PKCS1_PSS_PADDING) {
@@ -4085,33 +4086,23 @@ static int Node_SignFinal(EVP_MD_CTX* mdctx, unsigned char* md,
40854086
if (!EVP_DigestFinal_ex(mdctx, m, &m_len))
40864087
return rv;
40874088

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

41174108
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);
@@ -589,8 +589,8 @@ const d = crypto.createDiffieHellman(p, 'hex');
589589
assert.strictEqual(d.verifyError, DH_NOT_SUITABLE_GENERATOR);
590590

591591
// Test RSA key signing/verification
592-
const rsaSign = crypto.createSign('RSA-SHA1');
593-
const rsaVerify = crypto.createVerify('RSA-SHA1');
592+
const rsaSign = crypto.createSign('SHA1');
593+
const rsaVerify = crypto.createVerify('SHA1');
594594
assert.ok(rsaSign instanceof crypto.Sign);
595595
assert.ok(rsaVerify instanceof crypto.Verify);
596596

@@ -625,13 +625,13 @@ assert.strictEqual(rsaVerify.verify(rsaPubPem, rsaSignature, 'hex'), true);
625625
'8195e0268da7eda23d9825ac43c724e86ceeee0d0d4465678652ccaf6501' +
626626
'0ddfb299bedeb1ad';
627627

628-
const sign = crypto.createSign('RSA-SHA256');
628+
const sign = crypto.createSign('SHA256');
629629
sign.update(input);
630630

631631
const output = sign.sign(privateKey, 'hex');
632632
assert.strictEqual(output, signature);
633633

634-
const verify = crypto.createVerify('RSA-SHA256');
634+
const verify = crypto.createVerify('SHA256');
635635
verify.update(input);
636636

637637
assert.strictEqual(verify.verify(publicKey, signature, 'hex'), true);
@@ -649,11 +649,11 @@ assert.strictEqual(rsaVerify.verify(rsaPubPem, rsaSignature, 'hex'), true);
649649

650650
// DSA signatures vary across runs so there is no static string to verify
651651
// against
652-
const sign = crypto.createSign('DSS1');
652+
const sign = crypto.createSign('SHA1');
653653
sign.update(input);
654654
const signature = sign.sign(privateKey, 'hex');
655655

656-
const verify = crypto.createVerify('DSS1');
656+
const verify = crypto.createVerify('SHA1');
657657
verify.update(input);
658658

659659
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)