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.

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 crypto.verify callback invocation with a private keyobject #37795

Closed
wants to merge 1 commit into from

Conversation

panva
Copy link
Member

@panva panva commented Mar 18, 2021

fixes #37794

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Mar 18, 2021
@panva
Copy link
Member Author

panva commented Mar 18, 2021

Before

const crypto = require('crypto');
const assert = require('assert');

const data = Buffer.from('hello');
const { privateKey } = crypto.generateKeyPairSync('ed25519');
const signature = crypto.sign(null, data, privateKey);

assert(crypto.verify(null, data, privateKey, signature)); // OK

crypto.verify(null, data, privateKey, signature, (err, verified) => { // 💥
  assert(!err);
  assert(verified);
});
// node[49326]: ../src/crypto/crypto_sig.cc:850:static bool node::crypto::SignTraits::DeriveBits(node::Environment *, const node::crypto::SignConfiguration &, node::crypto::ByteSource *): Assertion `(params.key->GetKeyType()) == (kKeyTypePublic)' failed.

After

const crypto = require('crypto');
const assert = require('assert');

const data = Buffer.from('hello');
const { privateKey } = crypto.generateKeyPairSync('ed25519');
const signature = crypto.sign(null, data, privateKey);

assert(crypto.verify(null, data, privateKey, signature)); // OK

crypto.verify(null, data, privateKey, signature, (err, verified) => {
  assert(!err);
  assert(verified); // OK
});

@nodejs-github-bot
Copy link
Collaborator

@panva panva requested a review from jasnell March 18, 2021 11:37
@panva
Copy link
Member Author

panva commented Mar 18, 2021

cc @nodejs/crypto

@panva panva requested a review from tniessen March 18, 2021 12:14
@panva
Copy link
Member Author

panva commented Mar 18, 2021

@jasnell should we fast-track and do v15.12.1?

@jasnell
Copy link
Member

jasnell commented Mar 18, 2021

Yeah i think so

@panva panva added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 18, 2021
@panva
Copy link
Member Author

panva commented Mar 18, 2021

👍 to fast-track

src/crypto/crypto_sig.cc Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@panva
Copy link
Member Author

panva commented Mar 18, 2021

Landed in 5e6386e

@panva panva closed this Mar 18, 2021
panva added a commit that referenced this pull request Mar 18, 2021
fixes #37794

PR-URL: #37795
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@panva
Copy link
Member Author

panva commented Mar 18, 2021

@nodejs/releasers This fixes a possible crash introduced in a new optional argument added to a stable API with yesterday's 15.12.0 release (#37500). We're proposing a quick 15.12.1 patch release.

@panva panva deleted the cb-verify-fix branch March 18, 2021 19:14
ruyadorno pushed a commit that referenced this pull request Mar 20, 2021
fixes #37794

PR-URL: #37795
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. fast-track PRs that do not need to wait for 48 hours to land. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crypto: verify with callback crashes when private key is used
6 participants