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

Bad default settings #83

Open
Sc00bz opened this issue Jun 28, 2018 · 4 comments
Open

Bad default settings #83

Sc00bz opened this issue Jun 28, 2018 · 4 comments

Comments

@Sc00bz
Copy link

Sc00bz commented Jun 28, 2018

Default settings of PBKDF2-SHA1 with output length of 66 bytes is bad. The way PBKDF2 works the defender is doing 4 times more work than an attacker. There should be a limit of 20 bytes to prevent this.

Also consider changing it to PBKDF2-SHA512 with a max length of 64 bytes. SHA512 is about 2 times better than SHA1 or SHA256 because SHA512 uses 64 bit integers. On a 64 bit processor you gain a 2x advantage over an attacker (it's really that you take back a 2x disadvantage). Although this might change once SHA instructions are common/available. Since this might make SHA1 and SHA256 maybe around 3x faster and thus about 1.5x better than SHA512.

TL;DR Generally speaking, use a 128 bit salt and a 256 bit hash. Larger is pointless and even half those are fine, but don't go lower than half.

There is no point to go more than 256 bits in a password hash as a password has much less entropy. Also 128 bits is fine, but I wouldn't go lower than that. For salt, 128 bits is awesome and 64 bit is "fine". For salt, there is no point to going above 256 bits or even near. Although my inner cryptographer is yelling "use a 256 bit salt"... OK fine for encryption and easier proofs use a 256 bit salt. Now my inner password cracker wants to talk real world. Let's talk global scale (2^33 people) and crazy scale 7 new salts per person per day for 50 years (2^17 salts/person). So 2^50 random 64 bit salts that means you have 99.996948% unique salts or a 0.003052% speed up in cracking. Note there are about 2^35 collisions with 2 or more, but about 32 groups of 3 collisions. So yeah for those 96 salts that collided into groups of 3 it sucks if those are the only hashes targeted, then it's 3x faster. BUT 96/2^50 is negligible. Anyway a 64 bit salt is "fine", but even though I know this, I'd just use 128 bits and you'll "never" collide.

@tjconcept
Copy link
Collaborator

After some quick searching it seems there is at least nothing bad in what you're suggesting, and it does give a ~2x speedup on hashing, which I fear an attacker could get for free anyway.

I implemented your suggestions in this version of the library: https://github.com/srcagency/credentials. A review would be much appreciated :)

@ericelliott
Copy link
Owner

There should be a limit of 20 bytes to prevent this.

I'm open to a PR to fix these issues.

@Sc00bz
Copy link
Author

Sc00bz commented Oct 11, 2018

@tjconcept I meant to reply to this last week. Your "~2x speedup on hashing" comment made me think there was something wrong with Node's PBKDF2 implementation. Anyway I found a bug in Node and I guess I had telemetry on or it's a massive coincidence, but it got fixed within 24 hours. Which was before I decided to reported it.

Anyway your settings are no longer bad but are wasteful because of space. A 512 bit salt and 512 bit hash are "stupid long". 256 bits for each is "crazy long but I can see someone arguing it" (see above). 128 bits for each is great. Consider changing default length from 64 to 16 (or 18 so that it's dividable by 6 because of base 64 and it's at least 16)... 24 is fine as that's a nice-ish base 2 number that's also dividable by 6. So from an OCD perspective 24 is ideal and well beyond long enough for authentication. Also 18 is 144 bits which is 😎 a gross number of bits.

@tjconcept
Copy link
Collaborator

tjconcept commented Oct 11, 2018

Thanks.

So, what you are suggesting is:

  • Use SHA-256
  • Use a 256 bits salt
  • Use a key length of 24

Currently the key length is set to be "same as salt", and I read somewhere that some of these parameters should be the same - I don't remember which - but can you share your view on that (which parameters are related/should be equal)?

it got fixed within 24 hours

Do you have a link for the commit/issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants