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: check for valid iteration length in pbkdf2 #3173

Closed
wants to merge 1 commit into from

Conversation

johannhof
Copy link

An iteration length of NaN or Infinity is currently silently coerced to
0, which is more or less undefined behaviour. This commit makes NaN and
Infinity invalid iteration parameter values.

We're doing the same checks for other parameters already, so it would
make sense to be consistent here.

@thefourtheye thefourtheye added the crypto Issues and PRs related to the crypto subsystem. label Oct 4, 2015
@thefourtheye
Copy link
Contributor

cc @nodejs/crypto

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

@indutny @shigeki ... looks like this was never reviewed.
@nodejs/collaborators

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 22, 2016
iter = args[2]->Int32Value();
if (iter < 0) {
iter = args[2]->NumberValue();
if (iter < 0 || isnan(iter) || isinf(iter)) {
type_error = "Bad iterations";
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose the error could be more explanatory

Copy link
Contributor

Choose a reason for hiding this comment

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

would bump this to a semver-major. let's do that in either a difference commit or PR so it can be backported.

@johannhof
Copy link
Author

Oh hey I'd given up on this, thanks for reviving! I'll check your suggestions and get back :)

@jasnell
Copy link
Member

jasnell commented Jun 6, 2016

ping @johannhof :-)

An iteration length of NaN or Infinity is currently silently coerced to
0, which is more or less undefined behaviour. This commit makes NaN and
Infinity invalid iteration parameter values.

We're doing the same checks for other parameters already, so it would
make sense to be consistent here.
@johannhof
Copy link
Author

@nodejs/crypto sorry for the wait, ready for another review

@johannhof
Copy link
Author

huh, can I not mention teams? then ping @jasnell @indutny @thefourtheye @trevnorris :)

@jasnell
Copy link
Member

jasnell commented Jun 20, 2016

@johannhof ... Thanks! will take a look a bit later today!

@jasnell
Copy link
Member

jasnell commented Jun 20, 2016

I wondering if it would be better for this check to be done in the JS instead. @nodejs/crypto

@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:01
@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 28, 2017
@jasnell
Copy link
Member

jasnell commented Feb 28, 2017

@johannhof ... is this still something you'd like to pursue?

@jasnell
Copy link
Member

jasnell commented Mar 24, 2017

Closing due to lack of forward progress. Can reopen and revisit if necessary

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. semver-major PRs that contain breaking changes and should be released in the next major version. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants