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: refactor argument validation for pbkdf2 #15746

Closed
wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Oct 3, 2017

Move input argument validation to js, using internal/errors.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 3, 2017
@jasnell jasnell added errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version. labels Oct 3, 2017
@jasnell jasnell requested a review from a team October 3, 2017 04:12

if (keylen < 0 ||
!Number.isFinite(keylen) ||
isNaN(keylen) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Number.isFinite already checks if it is a NaN

Copy link
Member Author

Choose a reason for hiding this comment

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

ah right, good point :-)

}
);

crypto.DEFAULT_ENCODING = 'utf8';
Copy link
Contributor

Choose a reason for hiding this comment

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

DEFAULT_ENCODING would not affect the digest, right? Do we really need the following tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does. DEFAULT_ENCODING changes the encoding of the derived key returned.

Copy link
Member

Choose a reason for hiding this comment

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

It concerns me a bit that the way it is put in documentation right now suggest use of DEFAULT_ENCODING. Can we discourage it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's a good point. Document it's possible but strongly discourage it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It affects the derived key, but these tests are validating only the digest. So these tests look superfluous to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

ha! actually I think I was intending these to be something else but got distracted and didn't finish editing the test! If you notice, the items below are identical to the items above ;-) I'll take another pass at that lol

crypto.pbkdf2('password', 'salt', 1, 4073741824, 'sha256',
common.mustNotCall());
}, /^TypeError: Bad key length$/);
[Infinity, -Infinity, NaN, -1, 4073741824].forEach((i) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a dumb question: Till today my understanding was that INT_MAX would be the size of the word of the given processor architecture. But today, even on my 64-bit machine, the value is defined as 2147483647. I am a little confused here. C++ Spec is not very clear in this regard. Do you know where I can get a clear answer?

Copy link
Member

Choose a reason for hiding this comment

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

@thefourtheye INT_MAX is the maximal value of the signed int type. int can be anything larger than or equal to 16 bits, although most compilers implement it as 32 bits. The number you are seeing is 2^31-1.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tniessen That is exactly the problem. There is no guarantee that it is going to be always 2^31-1, right? Otherwise this test will fail.

Copy link
Member

Choose a reason for hiding this comment

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

@thefourtheye I believe all compilers we officially support use 32 bit ints on all supported platforms, though I might be mistaken. If I remember correctly, there is also INT32_MAX, but I am not sure it is more appropriate to use. INT_MAX seems fine as long as we use int internally. If INT_MAX is greater on some platforms, e.g. 2^63-1, it will likely be bigger than MAX_SAFE_INTEGER (which I believe is around 2^54).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, "all compilers we officially support"- that is the key here. Let me start a CI to confirm this.

Copy link
Member

Choose a reason for hiding this comment

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

If just to be safe, we can use process.binding('constants').crypto.INT_MAX + 1 here

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

LGTM

@thefourtheye
Copy link
Contributor

@jasnell jasnell force-pushed the pbkdf2_internal_errors branch 2 times, most recently from 05c3eb3 to 812d575 Compare October 16, 2017 16:49
@jasnell
Copy link
Member Author

jasnell commented Oct 16, 2017

<a id="ERR_CRYPTO_INVALID_DIGEST"></a>
### ERR_CRYPTO_INVALID_DIGEST

Used when an invalid crypto digest algorithm is specified.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a link to crypto.getHashes() here?

Move input argument validation to js, using internal/errors.

Also update docs

* `password` and `salt` may be Buffers or any TypedArrays
* `crypto.DEFAULT_ENCODING` changes the returned derivedKey type
@jasnell
Copy link
Member Author

jasnell commented Oct 23, 2017

jasnell added a commit that referenced this pull request Oct 23, 2017
Move input argument validation to js, using internal/errors.

Also update docs

* `password` and `salt` may be Buffers or any TypedArrays
* `crypto.DEFAULT_ENCODING` changes the returned derivedKey type

PR-URL: #15746
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Oct 23, 2017

Landed in 7124b46

@jasnell jasnell closed this Oct 23, 2017
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
Move input argument validation to js, using internal/errors.

Also update docs

* `password` and `salt` may be Buffers or any TypedArrays
* `crypto.DEFAULT_ENCODING` changes the returned derivedKey type

PR-URL: nodejs/node#15746
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
Move input argument validation to js, using internal/errors.

Also update docs

* `password` and `salt` may be Buffers or any TypedArrays
* `crypto.DEFAULT_ENCODING` changes the returned derivedKey type

PR-URL: nodejs/node#15746
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
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++. errors Issues and PRs related to JavaScript errors originated in Node.js core. lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants