FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

crypto: PBKDF2 works with `int` not `ssize_t` by indutny · Pull Request #5397 · nodejs/node · GitHub

/ node Public

crypto: PBKDF2 works with int not ssize_t - #5397

Closed
indutny wants to merge 4 commits into
nodejs:masterfrom
indutny:fix/gh-5396
ConversationCommits (4)ChecksFiles changed
Closed

crypto: PBKDF2 works with int not ssize_t#5397
indutny wants to merge 4 commits into
nodejs:masterfrom
indutny:fix/gh-5396

Conversation

indutny commented Feb 23, 2016

Copy link
Copy Markdown
Member

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    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 int as they are int in evp.h.

Check that raw_keylen fits into int before passing it to OpenSSL.

Fix: #5396

crypto: PBKDF2 works with int not ssize_t
d924513 
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
indutny mentioned this pull request Feb 23, 2016
Closed

indutny commented Feb 23, 2016

Copy link
Copy Markdown
Member Author

cc @nodejs/crypto

R = @bnoordhuis or @shigeki

mscdex added crypto Issues and PRs related to the crypto subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Feb 23, 2016

dcousens commented Feb 24, 2016

Copy link
Copy Markdown

ACK.

shigeki commented Feb 24, 2016

Copy link
Copy Markdown
Contributor

LGTM if CI is fine.

indutny commented Feb 24, 2016

Copy link
Copy Markdown
Member Author

@shigeki I'm afraid that CI is down 😢

(please pardon my terrible sense of humor).

shigeki commented Feb 24, 2016

Copy link
Copy Markdown
Contributor

@indutny lol :-) Hoping it's back soon from #5194.

bnoordhuis reviewed Feb 24, 2016
View reviewed changes
Comment thread 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';
});

bnoordhuis Feb 24, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

You can condense the check callback to just /Bad key length/.

indutny Feb 25, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ack.

bnoordhuis commented Feb 24, 2016

Copy link
Copy Markdown
Member

LGTM with a suggestion.

fanatid mentioned this pull request Feb 24, 2016
Closed

indutny commented Feb 25, 2016

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/1755/

indutny commented Feb 26, 2016

Copy link
Copy Markdown
Member Author

@bnoordhuis I took some courage and changed the rest of "Bad key length" occurrences. Hopefully this is OK to you.

indutny commented Feb 26, 2016

Copy link
Copy Markdown
Member Author

CI is unhappy with the change. Looks like I will need to study this a bit more than I thought.

bnoordhuis reviewed Feb 26, 2016
View reviewed changes
Comment thread 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)) {

bnoordhuis Feb 26, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

You should probably write this as:

if (!std::isfinite(raw_keylen) || raw_keylen < 0 || raw_keylen > INT_MAX) {
  // ...
}

indutny Mar 1, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ack.

indutny commented Mar 1, 2016

Copy link
Copy Markdown
Member Author

New CI: https://ci.nodejs.org/job/node-test-pull-request/1786/

indutny commented Mar 1, 2016

Copy link
Copy Markdown
Member Author

CI is green, the commit goes in. Landed in da3f425, thank you everyone!

indutny closed this Mar 1, 2016
indutny added a commit that referenced this pull request Mar 1, 2016
crypto: PBKDF2 works with int not ssize_t
da3f425 
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>
indutny deleted the fix/gh-5396 branch March 1, 2016 06:08
Fishrock123 mentioned this pull request Mar 1, 2016
Merged
5 tasks
Fishrock123 pushed a commit that referenced this pull request Mar 2, 2016
crypto: PBKDF2 works with int not ssize_t
88f3935 
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
fanatid mentioned this pull request Mar 8, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Mar 17, 2016
crypto: PBKDF2 works with int not ssize_t
2539ade 
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>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
crypto: PBKDF2 works with int not ssize_t
97e95d0 
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>
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

crypto.pbkdf2 with an keylen too long crashes in a segfalt

7 participants

Back | FazBrowse Home | New Git URL