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: correct pbkdf2 salt length recommendation #17524

Closed
wants to merge 1 commit into from
Closed

doc: correct pbkdf2 salt length recommendation #17524

wants to merge 1 commit into from

Conversation

willclarktech
Copy link
Contributor

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

Checklist
Affected core subsystem(s)

doc

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 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
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Technically correct.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

tniessen commented Dec 7, 2017

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
Copy link
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
Copy link
Member

Trott commented Dec 7, 2017

@BridgeAR
Copy link
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
Copy link
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
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@tniessen
Copy link
Member

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

@tniessen tniessen closed this Dec 12, 2017
@willclarktech 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
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
MylesBorins pushed a commit that referenced this pull request Jan 22, 2018
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 22, 2018
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2018
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
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. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants