Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.Sign up
SEC-1932 Add Pbkdf2PasswordEncoder #51
I have signed and agree to the terms of the SpringSource Individual Contributor License Agreement.
This shows: You should never do two tasks at once (at least that's true for me ...).
Somehow, I mixed up StandardPE and NoOpPE - silly me. Sorry for the unfounded comment.
Regarding BCryptPasswordEncoder: If we have a new AbstractPasswordEncoder, I think that the expectation is that all concrete PEs extend that; in my last comment, I did not mean that BCryptPE does extend AbstractPE, but that it would be nice to somehow unify all PEs to use the same AbstractPE.
Be that as it may: Great that PBKDF2 is now available in spring-security.
I will remove my previous comment as it is clearly in the wrong.
Thanks for the Pull Request @rworsnop and sorry for the delays in merging this! I'm trying to get caught up on PRs now and have made it a personal goal to do much better in the future.
I'm looking over the pull request and wanted to get some discussion going around some of the defaults we should use.
If anyone has any thoughts or references to credible resources on what the default values should be please provide them.
Thanks again @rworsnop for your efforts and sorry for the delays!
Thanks for picking this up. It's been a while since I did this, so my memory of where these numbers came from is not completely clear!
In terms of iterations, I have also seen 8ms as a recommendation, but I do believe this is one of those things that is a case by case basis making a default a difficult choice. Also I need to correct myself on LastPass. They seem to use 100K iterations see http://blog.lastpass.com/2011/05/lastpass-security-notification.html Does 100K seem excessive?
I'm still a bit unsure about the dkLen defaults at this point.
Also worth mentioning is that it would be ideal if we could use PBKDF2 SHA-256, but Java doesn't support it.
I'm going to let this sit for a few days in hopes that a few people I have pinged will have some sort of additional insights.
PS: I think eventually we probably need a mechanism that will allow better flexibility in supporting multiple algorithms / rounds / etc and automatic migrations. But this is certainly out of the scope for this specific ticket.
Ah, I was confusing LastPass's client-side encryption (which does default to 5K rounds) with what they do server-side for authentication.
Based on some tests I just did, 100K does seem excessive for a default.
By way of comparison, I ran the same test with a default BCryptPasswordEncoder and it averaged 84 ms. I then tried PBKDF2PasswordEncoder with 10K rounds (dkLen=256) and it averaged 83 ms. Spooky.
I don't really know what the most sensible default for dkLen is; but keep in mind that WPA2 uses PBKDF2 to convert a text password to a key for use with AES-256; so that's why it uses dkLen=256.
Thanks for your additional efforts @rworsnop! I have a colleague that is well versed in such things and has agreed to give feedback later today. At that point I think we can collect all the information and get some defaults, document some of the parameters a bit better (i.e. why the defaults were chosen) and merge the request. Thanks again for your efforts!
@rworsnop I just wanted to let you know that finding good defaults has been a bit challenging. With the help of @abedra we were able to recruit jOHN Steven to help figure out reasonable defaults. He is a bit busy this week, but should be able to provide feedback next week. This will be plenty early for the first milestone release.
Thanks for the patience, ‘life’s been wild recently. Find my notes from a conversation with Rob below. Discussion considers 4.0.0.M1 from a perspective of “refactor” or “update” rather than “bug fix”. The objective is to secure password storage starting from the existing code mindful that it has to support a wide range of adopters that don’t want to devote time to thinking about it ;-)
[Choice of Password Encoder]
*ACTION 01: Use an adaptive-one way function for the *DEFAULT PW storage scheme;
It appears as though 4.0.0 uses a fork of the mind rot implementation of Bcrypt: a fine choice. It’s default work factor (c=10) is also a good choice—it places password verify time around or above 0.5 seconds real time.
Consider adding configuration for use of PBKDF2 as an alternative encoder IF adopters require usage of an implementation from the default JCE crypto provider—an issue I’ve encountered a few times. Note, however, that PBKDF2 allows the attacker to leverage GPU-based optimizations for brute force attacks that Bcrypt does not. I’ll provide more information if desired.
Elsewhere, I suggest alternative approaches to adaptive one-way functions for good security and performance reasons. Here, again, I suggest the aforementioned approach.
**ACTION 02: REMOVE or provide salted digest encodings on a DEPRECATED basis only.
This advice should be applied to the salted SHA2 iterated 1024 times as well. Effectively, this scheme and its ilk provide no appreciable protection. More subtlety, iteration of a hash, such as SHAx, allows for attacks that iterating HMACs, due to their construction, does not. This is one reason why PBKDF2 uses HMACSHA rather than SHA.
Historically, the crypt function used for Unix passwords was tuned to take 1 second on hardware of the time. Security consultants have posted 0.08, 0.10, 0.2, and 0.5 seconds for balance between responsiveness and security.
My recommendation is 0.5 seconds for those where that response time is deemed acceptable.
**ACTION 03: Use a cryptographically-strong (pseudo-) randomly generated salt for each credential being stored.
Yes, using a 128b salt is a good choice. Adopters sometimes complain that long salts affect performance or storage unacceptably, though I’ve not seen evidence of this. An minimum size of 64b prevents lookup-based attacks.
[Planning for Change]
**ACTION 04: Append metadata to credentials’ stored form indicative of the digesting algorithm, any work factor information, and the salt.
Bcrypt’s output format already provides much of this information, taking the form:
Because existing stable versions of SpringSecurity, and deployed PW databases, use an insecure salted digest scheme, I recommend prepending a scheme identifier to the Bcrypt format. An adapter should apply this format to other password encoding scheme’s output.
Remember, the output of digests, and adaptive one-way functions are irreversible. Long inactive users may thus be one (or more) encoding schemes behind the deployment’s present scheme. Additionally, as has been suggested in forum discussions, deployers may want to use more onerous work factors for more privileged accounts.
**ACTION 05: Update verify implementation to read prepended scheme identifier and route the stored credential to the right verification routine, stripping unexpected metadata as appropriate for the verification algorithm.
In the comment on github you state:
Is there a way to guarantee a non-blocking source with Java? My understanding is that the SecureRandom implementations are vendor specific and may/may not be blocking.
Hi, this is a very nice discussion, and I learned a lot from it. But the problem is still unresolved and I had to deploy PBKDF2 into production. So until this feature is approved, let me kindly redirect people who need this, to my how to add pbkdf2 to spring security i wrote.