crypto: PBKDF2 works with int not ssize_t#5397
Closed
indutny wants to merge 4 commits intonodejs:masterfrom
Closed
crypto: PBKDF2 works with int not ssize_t#5397indutny wants to merge 4 commits intonodejs:masterfrom
int not ssize_t#5397indutny wants to merge 4 commits intonodejs:masterfrom
Conversation
Change types of all PBKDF2 params to `int` as they are `int` in `evp.h`. Check that `raw_keylen` fits into `int` before passing it to OpenSSL. Fix: nodejs#5396
Member
Author
|
cc @nodejs/crypto R = @bnoordhuis or @shigeki |
mscdex
added
crypto
|
ACK. |
Contributor
|
LGTM if CI is fine. |
Member
Author
|
@shigeki I'm afraid that CI is down 😢 (please pardon my terrible sense of humor). |
Contributor
test/parallel/test-crypto-pbkdf2.js
Outdated
| crypto.pbkdf2('password', 'salt', 1, 4073741824, 'sha256', common.fail); | ||
| }, function(err) { | ||
| return err instanceof Error && err.message === 'Bad key length'; | ||
| }); |
Member
There was a problem hiding this comment.
You can condense the check callback to just /Bad key length/.
Member
|
LGTM with a suggestion. |
Member
Author
Member
Author
|
@bnoordhuis I took some courage and changed the rest of "Bad key length" occurrences. Hopefully this is OK to you. |
Member
Author
|
CI is unhappy with the change. Looks like I will need to study this a bit more than I thought. |
src/node_crypto.cc
Outdated
| keylen = args[3]->NumberValue(); | ||
| if (keylen < 0 || isnan(keylen) || isinf(keylen)) { | ||
| raw_keylen = args[3]->NumberValue(); | ||
| if (raw_keylen < 0.0 || isnan(raw_keylen) || isinf(raw_keylen)) { |
Member
There was a problem hiding this comment.
You should probably write this as:
if (!std::isfinite(raw_keylen) || raw_keylen < 0 || raw_keylen > INT_MAX) {
// ...
}
Member
Author
Member
Author
|
CI is green, the commit goes in. Landed in da3f425, thank you everyone! |
Fishrock123
pushed a commit
that referenced
this pull request
Mar 2, 2016
Change types of all PBKDF2 params to `int` as they are `int` in `evp.h`. Check that `raw_keylen` fits into `int` before passing it to OpenSSL. Fix: #5396 PR-URL: #5397 Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp> Reviewed-By: Ben Noorhduis <info@bnoordhuis.nl> Conflicts: test/parallel/test-crypto-pbkdf2.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request check-list
Please make sure to review and check all of these items:
make -j8 test(UNIX) orvcbuild test nosign(Windows) pass withthis change (including linting)?
test (or a benchmark) included?
existing APIs, or introduces new ones)?
NOTE: these things are not required to open a PR and can be done afterwards /
while the PR is open.
Affected core subsystem(s)
Please provide affected core subsystem(s) (like buffer, cluster, crypto, etc)
Description of change
Change types of all PBKDF2 params to
intas they areintinevp.h.Check that
raw_keylenfits intointbefore passing it to OpenSSL.Fix: #5396