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

doc: correct pbkdf2 salt length recommendation by willclarktech · Pull Request #17524 · nodejs/node · GitHub

/ node Public

doc: correct pbkdf2 salt length recommendation - #17524

Closed
willclarktech wants to merge 1 commit into
nodejs:masterfrom
willclarktech:doc-pbkdf2-salt-length
ConversationCommits (1)ChecksFiles changed
Closed

doc: correct pbkdf2 salt length recommendation#17524
willclarktech wants to merge 1 commit into
nodejs:masterfrom
willclarktech:doc-pbkdf2-salt-length

Conversation

willclarktech commented Dec 7, 2017

Copy link
Copy Markdown
Contributor

Corrects the recommended salt length recommendation in the documentation for crypto.pbkdf2 and crypto.pbkdf2Sync.

Checklist
Affected core subsystem(s)

doc

doc: correct pbkdf2 salt length recommendation
d183381 
According to the linked document: "The length of the randomly-generated
portion of the salt shall be at least 128 bits." [NIST SP 800-132]
nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. labels Dec 7, 2017
tniessen approved these changes Dec 7, 2017
View reviewed changes

tniessen left a comment

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

Technically correct.

bnoordhuis approved these changes Dec 7, 2017
View reviewed changes

bnoordhuis left a comment

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

LGTM, although it raises some questions:

  1. Longer salts don't really affect PBKDF2's performance so why stop at at 16 bytes?
  2. Shorter salts don't affect PBKDF2's security properties so why stop at 16 bytes?

(The salt thwarts precomputed dictionary attacks; even a two-byte salt goes a long way towards mitigating that.)

tniessen commented Dec 7, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Regardless of my understanding of the implications below, NIST is a reasonable reference and it should not be up to us to make any security recommendations.

Longer salts don't really affect PBKDF2's performance so why stop at at 16 bytes?

This is a minimum recommendation so longer values are always permitted, but as salts need to be stored along with their hash values, it would be inefficient to store needlessly long salts.

Shorter salts don't affect PBKDF2's security properties so why stop at 16 bytes? [...] even a two-byte salt goes a long way towards mitigating that

Shorter salts are suited to prevent dictionary attacks, but PBKDF2 can also be used to reduce the chance of key reuse, which heavily relies on the salt length. For example, 100 operations using the same password and a two-byte salt would have a collision probability of 7%, and after 400 more operations, there is an 85% chance of having reused the same key.

bnoordhuis commented Dec 7, 2017

Copy link
Copy Markdown
Member

Regardless of my understanding of the implications below, NIST is a reasonable reference and it should not be up to us to make any security recommendations.

Oh, I'm not disagreeing but if you look at e.g. OWASP's recommendations, it says to use a 32 byte or even 64 byte salt. (And if you look elsewhere, you can find recommendations of as little as 4 or 8 bytes. So many options to chose from...)

Trott commented Dec 7, 2017

Copy link
Copy Markdown
Member

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

BridgeAR commented Dec 12, 2017

Copy link
Copy Markdown
Member

I guess the question here is what we personally feel like what is right to recommend. So do we want to have the current recommendation to be the same as NIST or recommend a higher value as currently?

tniessen commented Dec 12, 2017

Copy link
Copy Markdown
Member

As long as we reference the NIST SP, this pull request is correct, so I am going to go ahead and land this.

Apart from that, we should not make any quantitative security recommendations apart from those specified by appropriate institutions, unless they are specific to node. As @bnoordhuis pointed out, other recommendations suggest using different salt lengths, which is a fact we could mention at this point as part of the documentation.

tniessen pushed a commit to tniessen/node that referenced this pull request Dec 12, 2017
doc: correct pbkdf2 salt length recommendation
560c182 
According to the linked document: "The length of the randomly-generated
portion of the salt shall be at least 128 bits." [NIST SP 800-132]

PR-URL: nodejs#17524
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

tniessen commented Dec 12, 2017

Copy link
Copy Markdown
Member

Landed in 560c182, thank you for your first contribution! 🎉

tniessen closed this Dec 12, 2017
willclarktech deleted the doc-pbkdf2-salt-length branch December 13, 2017 08:59
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
doc: correct pbkdf2 salt length recommendation
ed4d013 
According to the linked document: "The length of the randomly-generated
portion of the salt shall be at least 128 bits." [NIST SP 800-132]

PR-URL: #17524
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins mentioned this pull request Jan 10, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Jan 22, 2018
doc: correct pbkdf2 salt length recommendation
ca71b00 
According to the linked document: "The length of the randomly-generated
portion of the salt shall be at least 128 bits." [NIST SP 800-132]

PR-URL: #17524
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 22, 2018
doc: correct pbkdf2 salt length recommendation
2d4d9aa 
According to the linked document: "The length of the randomly-generated
portion of the salt shall be at least 128 bits." [NIST SP 800-132]

PR-URL: #17524
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins mentioned this pull request Jan 24, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
doc: correct pbkdf2 salt length recommendation
12c2011 
According to the linked document: "The length of the randomly-generated
portion of the salt shall be at least 128 bits." [NIST SP 800-132]

PR-URL: #17524
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
doc: correct pbkdf2 salt length recommendation
417bf07 
According to the linked document: "The length of the randomly-generated
portion of the salt shall be at least 128 bits." [NIST SP 800-132]

PR-URL: #17524
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
doc: correct pbkdf2 salt length recommendation
83fe79c 
According to the linked document: "The length of the randomly-generated
portion of the salt shall be at least 128 bits." [NIST SP 800-132]

PR-URL: #17524
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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

bnoordhuis bnoordhuis approved these changes
lpinca lpinca approved these changes
cjihrig cjihrig approved these changes
tniessen tniessen approved these changes
+1 more reviewer
danbev danbev approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

Back | FazBrowse Home | New Git URL