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 and simplify prime option validation #37164

Merged
merged 1 commit into from
Feb 6, 2021

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Feb 1, 2021

This commit fixes the handling of negative BigInts in the generatePrime and generatePrimeSync functions and adds tests for these cases.

Additionally, it extracts the logic to set up the RandomPrimeJob to a new function to eliminate duplicate code.

@tniessen tniessen added the crypto Issues and PRs related to the crypto subsystem. label Feb 1, 2021
@tniessen tniessen requested a review from jasnell February 1, 2021 02:52
@nodejs-github-bot
Copy link
Collaborator

if (bigint < 0) {
throw new ERR_OUT_OF_RANGE(name, '>= 0', bigint);
}

const hex = bigint.toString(16);
Copy link
Member Author

@tniessen tniessen Feb 1, 2021

Choose a reason for hiding this comment

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

Explanation: The value hex begins with a minus sign if bigint < 0, causing Buffer.from(hex, 'hex') to return an empty buffer, which OpenSSL treats as a 0.

@tniessen tniessen added the review wanted PRs that need reviews. label Feb 5, 2021
@tniessen
Copy link
Member Author

tniessen commented Feb 5, 2021

Ping @nodejs/crypto

@tniessen tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 6, 2021
PR-URL: nodejs#37164
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott Trott force-pushed the crypto-prime-handle-negative-bigint branch from a6664c8 to 406984e Compare February 6, 2021 16:56
@Trott
Copy link
Member

Trott commented Feb 6, 2021

Landed in 406984e

@Trott Trott merged commit 406984e into nodejs:master Feb 6, 2021
@tniessen
Copy link
Member Author

tniessen commented Feb 6, 2021

Thanks @Trott, and thanks for reviewing @Trott and @jasnell!

@tniessen tniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 6, 2021
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
PR-URL: #37164
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
This was referenced Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants