Skip to content

Commit

Permalink
crypto: fix Node_SignFinal
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@davidben @MylesBorins
davidben authored and MylesBorins committed Oct 25, 2017
1 parent 9cebe82 commit 10a7035
Show file tree
Hide file tree
Showing 10 changed files with 110 additions and 99 deletions.
2 changes: 1 addition & 1 deletion benchmark/crypto/rsa-sign-verify-throughput.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ keylen_list.forEach(function(key) {

var bench = common.createBenchmark(main, {
writes: [500],
algo: ['RSA-SHA1', 'RSA-SHA224', 'RSA-SHA256', 'RSA-SHA384', 'RSA-SHA512'],
algo: ['SHA1', 'SHA224', 'SHA256', 'SHA384', 'SHA512'],
keylen: keylen_list,
len: [1024, 102400, 2 * 102400, 3 * 102400, 1024 * 1024]
});
Expand Down
40 changes: 19 additions & 21 deletions doc/api/crypto.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -842,28 +842,31 @@ of two ways:
- Using the [`sign.update()`][] and [`sign.sign()`][] methods to produce the
signature.

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

Example: Using `Sign` objects as streams:

```js
const crypto = require('crypto');
const sign = crypto.createSign('RSA-SHA256');
const sign = crypto.createSign('SHA256');

sign.write('some data to sign');
sign.end();

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

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

```js
const crypto = require('crypto');
const sign = crypto.createSign('RSA-SHA256');
const sign = crypto.createSign('SHA256');

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

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

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

Example: signing using ECDSA with SHA256
Example: signing using legacy signature algorithm name

```js
const crypto = require('crypto');
const sign = crypto.createSign('sha256');
const sign = crypto.createSign('RSA-SHA256');

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

const privateKey =
`-----BEGIN EC PRIVATE KEY-----
MHcCAQEEIF+jnWY1D5kbVYDNvxxo/Y+ku2uJPDwS0r/VuPZQrjjVoAoGCCqGSM49
AwEHoUQDQgAEurOxfSxmqIRYzJVagdZfMMSjRNNhB8i3mXyIMq704m2m52FdfKZ2
pQhByd5eyj3lgZ7m7jbchtdgyOF8Io/1ng==
-----END EC PRIVATE KEY-----`;

console.log(sign.sign(privateKey).toString('hex'));
const privateKey = getPrivateKeySomehow();
console.log(sign.sign(privateKey, 'hex'));
// Prints: the calculated signature
```

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

```js
const crypto = require('crypto');
const verify = crypto.createVerify('RSA-SHA256');
const verify = crypto.createVerify('SHA256');

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

```js
const crypto = require('crypto');
const verify = crypto.createVerify('RSA-SHA256');
const verify = crypto.createVerify('SHA256');

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

Expand Down
47 changes: 19 additions & 28 deletions src/node_crypto.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3993,7 +3993,8 @@ void SignBase::CheckThrow(SignBase::Error error) {

static bool ApplyRSAOptions(EVP_PKEY* pkey, EVP_PKEY_CTX* pkctx, int padding,
int salt_len) {
if (pkey->type == EVP_PKEY_RSA || pkey->type == EVP_PKEY_RSA2) {
if (EVP_PKEY_id(pkey) == EVP_PKEY_RSA ||
EVP_PKEY_id(pkey) == EVP_PKEY_RSA2) {
if (EVP_PKEY_CTX_set_rsa_padding(pkctx, padding) <= 0)
return false;
if (padding == RSA_PKCS1_PSS_PADDING) {
Expand Down Expand Up @@ -4102,33 +4103,23 @@ static int Node_SignFinal(EVP_MD_CTX* mdctx, unsigned char* md,
if (!EVP_DigestFinal_ex(mdctx, m, &m_len))
return rv;

if (mdctx->digest->flags & EVP_MD_FLAG_PKEY_METHOD_SIGNATURE) {
size_t sltmp = static_cast<size_t>(EVP_PKEY_size(pkey));
pkctx = EVP_PKEY_CTX_new(pkey, nullptr);
if (pkctx == nullptr)
goto err;
if (EVP_PKEY_sign_init(pkctx) <= 0)
goto err;
if (!ApplyRSAOptions(pkey, pkctx, padding, pss_salt_len))
goto err;
if (EVP_PKEY_CTX_set_signature_md(pkctx, mdctx->digest) <= 0)
goto err;
if (EVP_PKEY_sign(pkctx, md, &sltmp, m, m_len) <= 0)
goto err;
*sig_len = sltmp;
rv = 1;
err:
EVP_PKEY_CTX_free(pkctx);
return rv;
}

if (mdctx->digest->sign == nullptr) {
EVPerr(EVP_F_EVP_SIGNFINAL, EVP_R_NO_SIGN_FUNCTION_CONFIGURED);
return 0;
}

return mdctx->digest->sign(mdctx->digest->type, m, m_len, md, sig_len,
pkey->pkey.ptr);
size_t sltmp = static_cast<size_t>(EVP_PKEY_size(pkey));
pkctx = EVP_PKEY_CTX_new(pkey, nullptr);
if (pkctx == nullptr)
goto err;
if (EVP_PKEY_sign_init(pkctx) <= 0)
goto err;
if (!ApplyRSAOptions(pkey, pkctx, padding, pss_salt_len))
goto err;
if (EVP_PKEY_CTX_set_signature_md(pkctx, EVP_MD_CTX_md(mdctx)) <= 0)
goto err;
if (EVP_PKEY_sign(pkctx, md, &sltmp, m, m_len) <= 0)
goto err;
*sig_len = sltmp;
rv = 1;
err:
EVP_PKEY_CTX_free(pkctx);
return rv;
}

SignBase::Error Sign::SignFinal(const char* key_pem,
Expand Down
4 changes: 2 additions & 2 deletions test/fixtures/0-dns/create-cert.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const BN = asn1.bignum;
const id_at_commonName = [ 2, 5, 4, 3 ];
const rsaEncryption = [1, 2, 840, 113549, 1, 1, 1];
const sha256WithRSAEncryption = [1, 2, 840, 113549, 1, 1, 11];
const sigalg = 'RSA-SHA256';
const digest = 'SHA256';

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

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

const sign = crypto.createSign(sigalg);
const sign = crypto.createSign(digest);
sign.update(tbs_der);
const signature = sign.sign(private_key);

Expand Down
24 changes: 12 additions & 12 deletions test/parallel/test-crypto-binary-default.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -404,28 +404,28 @@ assert.throws(function() {
}, /^Error: Digest method not supported$/);

// Test signing and verifying
const s1 = crypto.createSign('RSA-SHA1')
const s1 = crypto.createSign('SHA1')
.update('Test123')
.sign(keyPem, 'base64');
const s1Verified = crypto.createVerify('RSA-SHA1')
const s1Verified = crypto.createVerify('SHA1')
.update('Test')
.update('123')
.verify(certPem, s1, 'base64');
assert.strictEqual(s1Verified, true, 'sign and verify (base 64)');

const s2 = crypto.createSign('RSA-SHA256')
const s2 = crypto.createSign('SHA256')
.update('Test123')
.sign(keyPem); // binary
const s2Verified = crypto.createVerify('RSA-SHA256')
const s2Verified = crypto.createVerify('SHA256')
.update('Test')
.update('123')
.verify(certPem, s2); // binary
assert.strictEqual(s2Verified, true, 'sign and verify (binary)');

const s3 = crypto.createSign('RSA-SHA1')
const s3 = crypto.createSign('SHA1')
.update('Test123')
.sign(keyPem, 'buffer');
const s3Verified = crypto.createVerify('RSA-SHA1')
const s3Verified = crypto.createVerify('SHA1')
.update('Test')
.update('123')
.verify(certPem, s3);
Expand Down Expand Up @@ -569,8 +569,8 @@ const d = crypto.createDiffieHellman(p, 'hex');
assert.strictEqual(d.verifyError, DH_NOT_SUITABLE_GENERATOR);

// Test RSA key signing/verification
const rsaSign = crypto.createSign('RSA-SHA1');
const rsaVerify = crypto.createVerify('RSA-SHA1');
const rsaSign = crypto.createSign('SHA1');
const rsaVerify = crypto.createVerify('SHA1');
assert.ok(rsaSign instanceof crypto.Sign);
assert.ok(rsaVerify instanceof crypto.Verify);

Expand Down Expand Up @@ -606,13 +606,13 @@ assert.strictEqual(rsaVerify.verify(rsaPubPem, rsaSignature, 'hex'), true);
'8195e0268da7eda23d9825ac43c724e86ceeee0d0d4465678652ccaf6501' +
'0ddfb299bedeb1ad';

const sign = crypto.createSign('RSA-SHA256');
const sign = crypto.createSign('SHA256');
sign.update(input);

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

const verify = crypto.createVerify('RSA-SHA256');
const verify = crypto.createVerify('SHA256');
verify.update(input);

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

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

const verify = crypto.createVerify('DSS1');
const verify = crypto.createVerify('SHA1');
verify.update(input);

assert.strictEqual(verify.verify(publicKey, signature, 'hex'), true);
Expand Down
46 changes: 34 additions & 12 deletions test/parallel/test-crypto-rsa-dsa.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,8 @@ test_rsa('RSA_PKCS1_PADDING');
test_rsa('RSA_PKCS1_OAEP_PADDING');

// Test RSA key signing/verification
let rsaSign = crypto.createSign('RSA-SHA1');
let rsaVerify = crypto.createVerify('RSA-SHA1');
let rsaSign = crypto.createSign('SHA1');
let rsaVerify = crypto.createVerify('SHA1');
assert.ok(rsaSign);
assert.ok(rsaVerify);

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

// Test RSA key signing/verification with encrypted key
rsaSign = crypto.createSign('RSA-SHA1');
rsaSign = crypto.createSign('SHA1');
rsaSign.update(rsaPubPem);
assert.doesNotThrow(() => {
const signOptions = { key: rsaKeyPemEncrypted, passphrase: 'password' };
rsaSignature = rsaSign.sign(signOptions, 'hex');
});
assert.strictEqual(rsaSignature, expectedSignature);

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

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

const sign = crypto.createSign('RSA-SHA256');
const sign = crypto.createSign('SHA256');
sign.update(input);

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

const verify = crypto.createVerify('RSA-SHA256');
const verify = crypto.createVerify('SHA256');
verify.update(input);

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

// Test the legacy signature algorithm name.
const sign2 = crypto.createSign('RSA-SHA256');
sign2.update(input);

const output2 = sign2.sign(privateKey, 'hex');
assert.strictEqual(signature, output2);

const verify2 = crypto.createVerify('SHA256');
verify2.update(input);

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


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

// DSA signatures vary across runs so there is no static string to verify
// against
const sign = crypto.createSign('DSS1');
const sign = crypto.createSign('SHA1');
sign.update(input);
const signature = sign.sign(dsaKeyPem, 'hex');

const verify = crypto.createVerify('DSS1');
const verify = crypto.createVerify('SHA1');
verify.update(input);

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

// Test the legacy 'DSS1' name.
const sign2 = crypto.createSign('DSS1');
sign2.update(input);
const signature2 = sign2.sign(dsaKeyPem, 'hex');

const verify2 = crypto.createVerify('DSS1');
verify2.update(input);

assert.strictEqual(verify2.verify(dsaPubPem, signature2, 'hex'), true);
}


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

{
const sign = crypto.createSign('DSS1');
const sign = crypto.createSign('SHA1');
sign.update(input);
assert.throws(() => {
sign.sign({ key: dsaKeyPemEncrypted, passphrase: 'wrong' }, 'hex');
Expand All @@ -236,7 +258,7 @@ const input = 'I AM THE WALRUS';
{
// DSA signatures vary across runs so there is no static string to verify
// against
const sign = crypto.createSign('DSS1');
const sign = crypto.createSign('SHA1');
sign.update(input);

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

const verify = crypto.createVerify('DSS1');
const verify = crypto.createVerify('SHA1');
verify.update(input);

assert.strictEqual(verify.verify(dsaPubPem, signature, 'hex'), true);
Expand Down
Loading

0 comments on commit 10a7035

Please sign in to comment.