Fork me on GitHub
Subscribe 4

Ticket #214 (fixed enhancement)

Switch to a better password hashing system

  • Created: 2010-12-14 01:55:26
  • Reported by: Smartys
  • Assigned to: Reines
  • Milestone: 2.0-alpha1
  • Component: authentication
  • Priority: high

http://codahale.com/how-to-safely-store-a-password/

Take-aways:
A. We should be using per-user salts.
B. We should be using a function like bcrypt (in PHP, that's crypt using CRYPT_BLOWFISH).

Switching to this solution would cause a slight performance hit on pages where password hashing is necessary (although the exact cost is configurable) but would also make it infeasible to brute force passwords on a large scale for FluxBB (for more exact numbers, check out the page I linked to at the top).

History

Smartys 2010-12-14 01:55:38

  • Description changed. (Diff)

Franz 2010-12-14 02:27:14

Agreed. We did not want to add this to the 1.4 branch for backwards compatibility reasons, if I remember correctly.

So when should we do this? 2.0 is still somewhat distant wink

Reines 2010-12-14 12:35:19

  • Type changed from bug to enhancement.

Well to use bcrypt properly, we need PHP 5.3.0 as far as I can see, unless we were to go with a pure-PHP implementation?

Either way I would think we should put this off for 2.0.

Franz 2010-12-14 12:37:33

Just so I understand: In theory, this only causes a problem if somebody can obtain a hash from the database or the cookie, right?

I'm not underestimating this, I'm aware of how important this is.

Reines 2010-12-14 12:41:32

Yeah - at the moment we simply use SHA1 with no salt - so if the database was compromised, cracking peoples passwords would be trivial. Using a salt means the cracker has to brute force them rather than simply looking up existing tables, but due to the speed they can be tested it is do-able, especially if people use weak passwords. What Smartys is suggesting is using a purposely slow hashing algorithm, as a user logging in being delayed by 0.3 seconds (for example) isn't really a problem - but when cracking passwords the difference between 0.3 seconds per attempt and 1 nanosecond per attempt basically makes brute forcing them impossible.

Since the work factor is tunable, it is also an option we could allow the board admin to set (in the config file), so they can choose a tradeoff between speed and security.

Reines 2010-12-14 15:47:41

For anyone curious there is quite a good discussion here.

Franz 2010-12-14 17:53:29

Quick question: When making the work factor changeable, what happens when you change it after already having some users in the database?

Reines 2010-12-14 17:55:22

It should make the existing passwords unusable, which is why I was suggesting it be in the config file rather than admin panel.

Smartys 2010-12-14 18:32:42

I don't think that's true. At least in the PHP implementation (via crypt), the work factor appears to be part of the salt. In that case, the system would support in-place upgrades: as user logged in, if the work value didn't match, a new salt would be generated.

Reines 2010-12-14 22:40:26

Ah yeah you're right.

My initial thought was this is a good idea as the extra hit when logging in and registering wont happen often - but then I remembered currently we also use the id and password in a cookie to authenticate every single request; Adding 0.3 seconds (for example) to every single request is a lot.

I guess we could use a lower work factor to speed it up, but it's also going to be hard to decide what to set the default to - since FluxBB will be ran on very differing hardware.

Maybe the solution to that would be to move to a session based system?

Reines 2010-12-15 12:58:14

The other question would be, how to best handle this, compatibility wise.

PHPs native implementation requires PHP 5.3.0. Is that a suitable requirement for FluxBB 2.0, or should we be aiming to support 5.2.x?

We could use the native implementation where available, and fall back otherwise.

If we don't have PHPs native implementation, we can use either the one in phpass (which is used in phpBB3 actually), though it uses multiple iterations of MD5. The other option would be our own custom implementation, using multiple runs of some better hashing algorithm (I threw together this which supports changing both the hashing algorithm and number of runs).

Thoughts?

Franz 2010-12-15 13:05:07

I think we should aim at PHP 5.2 first. Depending on when we're getting closer to finish, we can take another look at the adoption of PHP 5.3.

5.3 has some nice features, but I don't think we really need any of them.
Correction: except for the hashing stuff, it seems.

Is your implementation just a proof-of-concept, or would this already be enough?

Reines 2010-12-15 14:07:54

Yeah I would rather support 5.2, and I think this would be the only thing requiring 5.3?

The stuff I threw together makes use of repeated hashing rather than blowfish, which according to phpass isn't as secure (it's what they refer to as "portable passwords") - though should be better than normal salt+hashing.

quy 2010-12-15 20:52:12

Would flood protection on failed logins work where you have to wait 60 seconds before trying again?

Smartys 2010-12-15 20:53:25

Brute-force login prevention is an entirely separate issue. This is about how the passwords are stored so as to protect them in the event of a database compromise.

Reines 2010-12-15 20:58:34

Smartys what's your thoughts on crypt+CRYPT_BLOWFISH with PHP 5.3.0, vs phpass portable passwords, vs my take?

Smartys 2010-12-15 21:16:05

- I think something implemented within the language itself is the optimal choice, if possible.
- I think phpass's code hurt my head trying to understand what was going on there. I can't be sure that there isn't something wrong with their implementation (I don't have the time to sit down and reason it out).
- Your implementation is simple and easy to read. There is something troubling me though, and it could just be because I'm not well versed in how stretching typically is implemented.

On each iteration (excluding the first), you create a new hash by concatenating the hash and the plaintext password. Doesn't that defeat the purpose of the iterations, since a brute force attack (albeit a very resource intensive attack) can be generated to attack only the last iteration?

I checked out Wikipedia, and the article on key strengthening seems to agree with me: http://en.wikipedia.org/wiki/Key_strengthening. You want to use the salt, not the password, on each iteration. That way, the only way to get back to the original password is to go through the entire process.

Reines 2010-12-15 21:54:48

That occurred to me too, but it's what phpass does :s

From phpass:

            $hash = md5($salt . $password, TRUE);
            do {
                $hash = md5($hash . $password, TRUE);
            } while (--$count);

Seems unlikely, but could their code be wrong? I'll do some reading I guess and see...

Reines 2010-12-15 22:14:27

Maybe another option to look at would be PBKDF2?

Reines 2010-12-16 12:44:54

  • Milestone set to 2.0-beta1.

Reines 2011-02-25 00:46:43

  • Milestone changed from 2.0-beta1 to 2.0-alpha1.

Reines 2011-03-20 12:27:24

  • Component changed from code to authentication.

Reines 2011-04-06 10:14:57

  • Owner set to Reines.

I've now implemented a PasswordHash class which uses bcrypt if available, and falls back to PBKDF2 otherwise.

Any thoughts or issues with this?

Reines 2011-04-06 10:29:42

I think as far as actually changing FluxBB to make use of this class goes, it should be done in #306, since that will require changing the authentication system a lot anyway.

Reines 2011-04-09 19:20:12

I've commit some of this into the fluxbb-2.0-sessions branch.

The functions random_pass() and random_key() need tidied up still, and changed to make use of PasswordHash::random_bytes().

To handle converting from old installs I was thinking we should make a plugin which converts old style passwords upon login - so I've put the code for this in a commented block saying TEMP at the moment. I've also only supported converting from 1.4 style passwords at the moment - should said plugin try to handle 1.2 and 1.3 style too, or should we only support from 1.4? (The problem with only 1.4 is even on a 1.4 install some users may have 1.3 style passwords if it was converted from 1.3 and users haven't logged in since - even on fluxbb.org we have a bunch. Though if the user hasn't logged in for that long, expecting them to manually reset their password wouldn't be too much to ask).

Franz 2011-04-09 21:12:12

I'd say the code to add for 1.2 and 1.3 shouldn't be too hard, so I would actually vote to support those, too - especially in a plugin.

Reines 2011-04-11 23:52:22

  • Status changed from open to fixed.