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

src: simplify calls to BN_bin2bn in prime gen #37169

Closed
wants to merge 1 commit into from

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Feb 1, 2021

This is more or less equivalent to the previous implementation from #36997, with two subtle differences.

  1. The add and rem options are not stored in secure memory (if it is enabled). However, these options are mostly used for DH, in which case these parameters aren't secret. Either way, they are passed from JS through insecure memory (either as BigInt or ArrayBuffer), so it does not help much to protect them here. (I suggested storing the prime in secure memory in crypto: add generatePrime/checkPrime #36997, but not add or rem.)
  2. Memory allocation failure now always throws ERR_CRYPTO_OPERATION_FAILED. Previously, it would either throw ERR_CRYPTO_OPERATION_FAILED if memory allocation failed during BN_secure_new, or ERR_INVALID_ARG_VALUE if memory allocation failed during BN_bin2bn. The latter doesn't make much sense because the failure of BN_bin2bn is most likely unrelated to the value of the options.

@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. labels Feb 1, 2021
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@tniessen tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 1, 2021
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@tniessen
Copy link
Member Author

tniessen commented Feb 3, 2021

Landed in 8b14046. Thanks for reviewing @jasnell and @Trott!

@tniessen tniessen closed this Feb 3, 2021
tniessen added a commit that referenced this pull request Feb 3, 2021
PR-URL: #37169
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@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: #37169
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
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
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants