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

doc: Adding best practises for crypto.pbkdf2 #3290

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
9 participants
@tomgco
Copy link
Contributor

commented Oct 9, 2015

Added some information around usages of how to use iterations, how to
choose decent salts and updating the example to have a significant work
factor and to use sha512.

@tflanagan

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2015

The change regarding salts is a tad misleading. For each encrypted string, yes, you should create a new salt, but you need to preserve that salt for decryption.

Granted someone who doesn't know that shouldn't really be using this function.

@silverwind silverwind added the doc label Oct 9, 2015

@mscdex mscdex added the crypto label Oct 9, 2015

@brendanashworth

View changes

doc/api/crypto.markdown Outdated
higher the number, the more secure it will be, but will take a longer amount of
time to complete.

Chosen salts should also be unique and random (ideally using crypto.randomBytes)

This comment has been minimized.

Copy link
@brendanashworth

brendanashworth Oct 10, 2015

Member

It'd be great if crypto.randomBytes could be linked to the documentation for the function.

@jasnell

This comment has been minimized.

Copy link
Member

commented Oct 14, 2015

@indutny

This comment has been minimized.

Copy link
Member

commented Oct 14, 2015

LGTM

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 5, 2015

@tomgco ... can I ask you to rebase and update the PR? @shigeki ... I'd like your feedback on this too before landing

@tomgco tomgco force-pushed the tomgco:crypto-docs branch Nov 6, 2015

@tomgco

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2015

@tomgco ... can I ask you to rebase and update the PR? @shigeki ... I'd like your feedback on this too before landing

Hey @jasnell, rebased and updated, also with a link as @brendanashworth suggested.

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 6, 2015

@tomgco thank you. let's give @shigeki another day or two to review. I know he's likely fairly tied up with NodeFest in Japan right now so it could take a couple days.

@shigeki

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2015

@jasnell Very sorry for my late reply. I missed this.

@tomgco I have one question and one comment on the description in regarding salt.

the length of the salts should also match the length of the chosen digest function

IIRC, in PBKDF2 the salt is put into HMAC with the number of iteration counter so that its length is not necessary to match the length of the digest function. As in the example, the length of salt is 4. Of course, the longer is the better. Do you have any reason for matching with it?

Chosen salts should also be unique and random

Reading section 4 of RFC2818, https://tools.ietf.org/html/rfc2898#section-4 , and A.2.1 of NIST 800-132 , http://csrc.nist.gov/publications/nistpubs/800-132/nist-sp800-132.pdf , there is a use case to add a non-random octet to a salt according to its purpose. So the description above is not always the case. I think we need not to write a whole description as the same of that in the references above and it is enough to write a pointer to them with a short comment.

Description in regarding iteration is good. At first, I thought the iteration number of 100,000 in the example seems to be so large, but I found that it took only 1.6 sec on my host so it is no problem.

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 13, 2015

@tomgco ... just a quick ping on this. Did you see @shigeki's question?

@tomgco

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2015

@tomgco ... just a quick ping on this. Did you see @shigeki's question?

No I didn't, thanks for the ping.

@shigeki,

IIRC, in PBKDF2 the salt is put into HMAC with the number of iteration counter so that its length is not necessary to match the length of the digest function. As in the example, the length of salt is 4. Of course, the longer is the better. Do you have any reason for matching with it?

I use it as a general rule of thumb, based on the strength ( and the final computed result ) of the hash function (from SHA-256 onwards), A publication that NIST released in 2010 recommended the minimum salt length be 128bits, so to recommend the length of the digest function exceeds the recommended values and grows with the introduction of longer hash-functions.

Reading section 4 of RFC2818, https://tools.ietf.org/html/rfc2898#section-4 , and A.2.1 of NIST 800-132 , http://csrc.nist.gov/publications/nistpubs/800-132/nist-sp800-132.pdf , there is a use case to add a non-random octet to a salt according to its purpose. So the description above is not always the case. I think we need not to write a whole description as the same of that in the references above and it is enough to write a pointer to them with a short comment.

Although that is true, the complexity that would be needed to be put into the documentation would possible make it more difficult and scarier for a developer who is less security minded, but wants to use sensible defaults. And I could argue that if someone needed to implement an octect to signify the use case then this would fall outside of what should be in the documentation; for me, if we was to introduce that, it would be the same as describing the algorithm behind PBKDF2.

However you make a good point that maybe we could add the RFC as a link, and encourage the users of node to read through it to understand each parameters in more detail?

Description in regarding iteration is good. At first, I thought the iteration number of 100,000 in the example seems to be so large, but I found that it took only 1.6 sec on my host so it is no problem.

Maybe we should make a note also around how the longer the function takes the more desirable it is, as it becomes much harder for an attacker.

Thanks for the feedback,

Let me know if you would like me to incorporate anything that I have mentioned in this message in the PR, Thanks 👯

@shigeki

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2015

@tomgco I agree the doc should not be too complex. I also think its correctness is important because it is not only for novices but also for experts.
The length of salt affects how much we can protect from dictionary attack and it is not determined by hash function. Here is my proposal to change the description clearly noted that the salt is required to be unique and the length of its randomness is just a recommendation.

Chosen salts should also be unique. It is recommended that the salts are random and their length is greater than 16 bytes. See NIST 800-132 for details.

For simplicity, it is not necessary to add the description of a use case of a purpose prefix in the salt. Let it to the reference.

doc: Adding best practises for crypto.pbkdf2
Added some information around usages of how to use iterations, how to
choose decent salts and updating the example to have a significant work
factor and to use sha512.

@tomgco tomgco force-pushed the tomgco:crypto-docs branch to e1b39b0 Nov 23, 2015

@tomgco

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2015

@tomgco I agree the doc should not be too complex. I also think its correctness is important because it is not only for novices but also for experts.
The length of salt affects how much we can protect from dictionary attack and it is not determined by hash function. Here is my proposal to change the description clearly noted that the salt is required to be unique and the length of its randomness is just a recommendation.

Chosen salts should also be unique. It is recommended that the salts are random and their length is greater than 16 bytes. See NIST 800-132 for details.
For simplicity, it is not necessary to add the description of a use case of a purpose prefix in the salt. Let it to the reference.

Brilliant, I agree, thanks.

I have updated the PR accordingly, and believe we are all in agreement with the changes?

shigeki pushed a commit that referenced this pull request Nov 23, 2015

doc: Adding best practises for crypto.pbkdf2
Added some information around usages of how to use iterations, how to
choose decent salts and updating the example to have a significant
work factor and to use sha512.

PR-URL: #3290
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
@shigeki

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2015

Yes, thanks for revising. Landed in d16def5 with adding an external link to the NIST reference. Thanks for your collaboration again.

@shigeki shigeki closed this Nov 23, 2015

MylesBorins added a commit that referenced this pull request Nov 30, 2015

doc: Adding best practises for crypto.pbkdf2
Added some information around usages of how to use iterations, how to
choose decent salts and updating the example to have a significant
work factor and to use sha512.

PR-URL: #3290
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

rvagg added a commit that referenced this pull request Dec 4, 2015

doc: Adding best practises for crypto.pbkdf2
Added some information around usages of how to use iterations, how to
choose decent salts and updating the example to have a significant
work factor and to use sha512.

PR-URL: #3290
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

rvagg added a commit that referenced this pull request Dec 5, 2015

doc: Adding best practises for crypto.pbkdf2
Added some information around usages of how to use iterations, how to
choose decent salts and updating the example to have a significant
work factor and to use sha512.

PR-URL: #3290
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

@jasnell jasnell referenced this pull request Dec 17, 2015

Closed

Release 4.2.4 Planning #4321

jasnell added a commit that referenced this pull request Dec 17, 2015

doc: Adding best practises for crypto.pbkdf2
Added some information around usages of how to use iterations, how to
choose decent salts and updating the example to have a significant
work factor and to use sha512.

PR-URL: #3290
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

jasnell added a commit that referenced this pull request Dec 23, 2015

doc: Adding best practises for crypto.pbkdf2
Added some information around usages of how to use iterations, how to
choose decent salts and updating the example to have a significant
work factor and to use sha512.

PR-URL: #3290
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.