search

silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
722 stars 821 forks source link

RFC: Introduce a "Needs rehash" API to MemberPassword #8526

Open ScopeyNZ opened 5 years ago

ScopeyNZ commented 5 years ago

Affected Version

4.x

Description

In order to easily facilitate upgrading password hashes as they evolve (and computers get better at generating rainbow tables) we should add an API to check if password hashes need to be rehashed and use this new interface.

This will closely replicate the API provided by PHP for password hashing: http://php.net/manual/en/function.password-needs-rehash.php . Once this is done we should implement a PasswordEncryptor that uses the password_hash API from PHP and trust PHP core team to stay up to date - which they seems to be capable of doing: https://wiki.php.net/rfc/argon2_password_hash, https://wiki.php.net/rfc/argon2_password_hash_enhancements

Proposal

Firesphere commented 5 years ago

What's the point of the need for a rehash exactly? You mean if they happen to be in MD5, then automatically rehash to bcrypt?

ScopeyNZ commented 5 years ago

Yes. Or even if they're in bcrypt and you decide to upgrade to argon2i then it would happen on the next user login (because you need to rehash their plain-text password). It's transparent to the user but you're upgrading the password hash.

kinglozzer commented 5 years ago

+1 from me, I hit that shortcoming of our APIs in https://github.com/silverstripe/silverstripe-framework/pull/8061

ScopeyNZ commented 5 years ago

Ok I've been looking at this, also taking into account @madmatt's comments on #8525 about renaming these classes too. I'm trying to come up with an plan to essentially rewrite this hashing API to be password/member inspecific, and then have some nice drop in code so the existing API still works. Of course we'd still deprecate the whole existing API slated for removal in 5.0

Goals:

Write a new API

Here's my proposal for a new API:

CryptographicHashService

By default on construct of this service it will use a configured hash method. More on this later. It's pretty commonplace to store the salt on the result of the hash these days, that's what I recommend here. Validating a hash using this API will use a timing safe comparison function (not currently used).

needsRehash is available for use and will be used internally when verifying to update the hash.

I don't expect the setHashMethod to be commonly used.

PasswordService

This will wrap a CryptographicHashService. Should be self explanatory. Requiring a Member for validation means that you skip things like SELECT WHERE password_hash = ? which is not timing safe.

When verifying we can have a flow like verify --TRUE-> needsRehash --TRUE-> hash, $member->Hash = $newHash; $member->write().

HashMethodInterface & HashMethod... (DefaultPhp, PBKDF2, etc...)

Hash methods will have to have some sort of identifier. This is so we can identify hash methods from their hash powering the needsRehash API. Currently PHPs password_hash does this by prefixing some distinguishable string onto the front of the hash. If we're implementing our own hashers we'll have to do something similar. This could be derived from the FQN of the hash method?

verify is included because PHP provides an API in the form of password_verify for hashes generated with password_hash. In most cases this will just be a case of return hash_equals($hash, $this->hash($plaintext)) so providing an abstract here would be helpful.

needsRehash exists here again because individual hash methods may opt to indicate a rehash is required if the cost params change (or for password_hash the algo changes). Some HashMethods may just return false here.

Existing usage of PasswordEncryptor

I've had a look at the existing usage within a cwp-recipe-kitchen-sink installation:

Member::encryptWithUserSettings and it's usages:

Then we can deprecate all existing PasswordEncryptor APIs too. It'd be nice to keep a Security::(hash / verifyHash) API too.

I know this is a bit of a leap from "Introduce a needs rehash API". I guess I should rename the issue to "Refactor (password) hashing API"?.

kinglozzer commented 5 years ago

Nice write up @ScopeyNZ! I think the new API you’ve described looks great, I do have one minor concern (that came up here: https://github.com/silverstripe/silverstripe-framework/pull/8061#issuecomment-387675870). Do you think we could deprecate Member::encryptWithUserSettings(), but leave it using the old “encryption” (sic) APIs to ensure BC?

ScopeyNZ commented 5 years ago

Nice write up @ScopeyNZ! I think the new API you’ve described looks great, I do have one minor concern (that came up here: #8061 (comment)). Do you think we could deprecate Member::encryptWithUserSettings(), but leave it using the old “encryption” (sic) APIs to ensure BC?

I think it'll be possible to create a "HashMethod" that just wraps the legacy code. I'm going to try and work a little on a POC this Friday which will help identify if this is a problem. I am only a little worried about validating Member::encryptWithUserSettings() after a users hash method has been updated but the code is comparing an old hash. I think it could be possible though.

ScopeyNZ commented 5 years ago

I made some really good progress on Friday writing up a POC for this. There's some funky stuff going on that I'm going to have to change that I hope won't break BC. For example I don't know why we do a $member->Password = 'plaintext_password' and then run a validator on that. Seems sort of dangerous to ever assign the plaintext password on a member object.

robbieaverill commented 5 years ago

I don't know why we do a $member->Password = 'plaintext_password' and then run a validator on that. Seems sort of dangerous to ever assign the plaintext password on a member object.

This is done so that you can load a form's worth of data directly into a Member object, or assign a password directly then write it. It gets hashed on write. What might an alternative look like?

ScopeyNZ commented 5 years ago

I've raised a WIP PR for POC work here: #8806 . There's still a bunch of work to do but I'm happy to receive any comments if anyone can motivate themselves to have a glance.

This is done so that you can load a form's worth of data directly into a Member object, or assign a password directly then write it. It gets hashed on write. What might an alternative look like?

I have played with "an alternative" in the WIP PR. Personally I think it's pretty dangerous to have passwords temporarily stored in plain text on a Member object. Additionally the existing implementation doesn't really make it possible to store a plain text password on purpose - it's not very deterministic.