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

@jasnell jasnell force-pushed the jasnell:pbkdf2_internal_errors branch Oct 3, 2017
@jasnell jasnell requested a review from nodejs/tsc Oct 3, 2017
@lpinca
lpinca approved these changes Oct 3, 2017
lib/internal/crypto/pbkdf2.js Outdated

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

This comment has been minimized.

@thefourtheye

thefourtheye Oct 3, 2017
Contributor

Number.isFinite already checks if it is a NaN

This comment has been minimized.

@jasnell

jasnell Oct 3, 2017
Author Member

ah right, good point :-)

test/parallel/test-crypto-pbkdf2.js Outdated
}
);

crypto.DEFAULT_ENCODING = 'utf8';

This comment has been minimized.

@thefourtheye

thefourtheye Oct 3, 2017
Contributor

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

This comment has been minimized.

@jasnell

jasnell Oct 3, 2017
Author Member

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

This comment has been minimized.

@indutny

indutny Oct 3, 2017
Member

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

This comment has been minimized.

@jasnell

jasnell Oct 3, 2017
Author Member

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

This comment has been minimized.

@thefourtheye

thefourtheye Oct 4, 2017
Contributor

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

This comment has been minimized.

@jasnell

jasnell Oct 4, 2017
Author Member

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

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

This comment has been minimized.

@thefourtheye

thefourtheye Oct 3, 2017
Contributor

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?

This comment has been minimized.

@tniessen

tniessen Oct 4, 2017
Member

@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.

This comment has been minimized.

@thefourtheye

thefourtheye Oct 4, 2017
Contributor

@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.

This comment has been minimized.

@tniessen

tniessen Oct 4, 2017
Member

@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).

This comment has been minimized.

@thefourtheye

thefourtheye Oct 5, 2017
Contributor

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

This comment has been minimized.

@joyeecheung

joyeecheung Oct 17, 2017
Member

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

@indutny
indutny approved these changes Oct 3, 2017
Copy link
Member

@indutny indutny left a comment

LGTM

@jasnell jasnell force-pushed the jasnell:pbkdf2_internal_errors branch 2 times, most recently Oct 16, 2017
doc/api/errors.md Outdated
<a id="ERR_CRYPTO_INVALID_DIGEST"></a>
### ERR_CRYPTO_INVALID_DIGEST

Used when an invalid crypto digest algorithm is specified.

This comment has been minimized.

@joyeecheung

joyeecheung Oct 17, 2017
Member

Maybe a link to crypto.getHashes() here?

jasnell added 2 commits Oct 3, 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
@jasnell jasnell force-pushed the jasnell:pbkdf2_internal_errors branch to 572b090 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 jasnell commented Oct 23, 2017

Landed in 7124b46

@jasnell jasnell closed this Oct 23, 2017
addaleax added 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 added 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.