Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: fix Node_SignFinal #15024

Closed
wants to merge 1 commit into from
Closed

crypto: fix Node_SignFinal #15024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion benchmark/crypto/rsa-sign-verify-throughput.js
View file
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -916,28 +916,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:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is better to add a description about a signature algorithm in the comment of examples like

-// Prints: the calculated signature
+// Prints: the calculated signature with an algorithm of the type of
+// private key with SHA256. When an RSA private key is used, the
+// signature algorithm is RSA-SHA256. It is ECDSA with SHA256 in case
+// of an EC private key.

Copy link
Contributor Author

@davidben davidben Aug 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrote something to that effect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me.

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

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

Expand All @@ -946,27 +949,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(privateKey[, outputFormat])
Expand Down Expand Up @@ -1049,7 +1047,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 @@ -1064,7 +1062,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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3976,7 +3976,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 @@ -4085,33 +4086,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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same fix as in openssl-1.1.0 and EVP_MD_FLAG_PKEY_METHOD_SIGNATURE was removed in openssl/openssl@7f572e9. Is it sure we do not need this code path even in 1.0.2?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node is already assuming this codepath isn't needed in the verify half. This codepath is because, in 0.9.8, an EVP_PKEY didn't have EVP_PKEY_sign and EVP_PKEY_verify. Those were implemented in the signature EVP_MDs. In 1.0.0, an EVP_PKEY became less tied to EVP_MD and could sign and verify digests on their own.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your explanation about its history. I thought it might be safe for we do not have NoneWithRSA but I did not have the confidence to remove that code path. .

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
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,28 +424,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 @@ -589,8 +589,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 @@ -625,13 +625,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 @@ -649,11 +649,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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,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 @@ -151,19 +151,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 @@ -186,16 +186,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 @@ -207,14 +219,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 @@ -224,7 +246,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 @@ -234,7 +256,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 @@ -243,7 +265,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