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

Modern password hashing #3530

Closed
dvz opened this issue Dec 28, 2018 · 4 comments

Comments

@dvz
Copy link
Contributor

commented Dec 28, 2018

Use \Illuminate\Hashing and extensions for modern password hashing.

Codebase:

  • modify create_password(), verify_user_password() wrapper functions to use MyBB's extension of \Illuminate\Hashing\HashManager,
  • implement bcrypt password hashing (Illuminate\Hashing\BcryptHasher) as default,
  • create MybbHasher hashing driver to handle old hashes,
  • modify verify_user_password() to allow plugins overwrite parameters (e.g. to decrypt values before checking),
  • modify LoginDataHandler::complete_login() to upgrade algorithm and parameters to default on the fly.

Database:

  • extend mybb_users.password column length,
  • add mybb_users.password_algorithm column for algorithm identification.

Defer to 1.9-compatible version of DVZ Hash for algorithm wrapping and encryption with intent to implement in subsequent branches; set argon2id as default algorithm once PHP 7.3 is required.

@dvz dvz added this to the 1.9.0 milestone Dec 28, 2018

@dvz dvz self-assigned this Dec 28, 2018

@dvz dvz added this to To do in MyBB 1.9.0 via automation Dec 28, 2018

@dvz dvz moved this from To do to In progress in MyBB 1.9.0 Dec 28, 2018

@dvz dvz pinned this issue Dec 28, 2018

@euantorano

This comment has been minimized.

Copy link
Member

commented Dec 28, 2018

Note that a password_algorithm column may not be needed. If using PHP's standard password_* functions, there is password_get_info() to resolve the algorithm and cost used for a given password. Assuming a core MyBB install of 1.8, the existing password will never start with $ (as the password is a base64 representation of an MD5 hash) so if it doesn't start with that, then it's an old MD5 hash.

@dvz

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2018

That's worth considering too, but we'll be dealing with a mix of functions that support password_get_info() and those that do not (mainly the old mybb and the wrapped mybb_bcrypt), which is why I went with an explicit column for now so that everything can be previewed and manipulated more easily (extensibility for custom algorithms aside).
With this scheme we can clear the additional field partially or entirely when the algorithm metadata is stored in the main string.

@effone

This comment has been minimized.

Copy link
Member

commented Dec 28, 2018

Why this is in 1.9? This can be designed in backward compatible way to update existing hashes to be modern. This should be part of 1.8.

@euantorano the check can be more bullet proof. Salt field will be blank. I can modify the function a way that if the native password_verify() fails it will check further with old MD5 method alongwith presence of salt in DB. If it matches it will update the new hash cleaning salt and log the user in. This can be so smooth that user will notice nothing. The update will be once per user.

There is no problem having this in 1.8. Then in 1.9 we can use Illuminate.

@euantorano

This comment has been minimized.

Copy link
Member

commented Dec 28, 2018

@dvz fair enough, makes sense to include a column then.

@effone the main reason for waiting until 1.9 is that PHP's password_hash() and associated functions are only available in PHP >= 5.5 and the password_compat library needs PHP >= 5.3.7. MyBB 1.8 needs to support all the way back to PHP 5.2.

I suppose we could select the best available algorithm based upon available functions, but then we could be maintaining 3 or 4 different password hashing schemes and all the associated support requests that would cause.

1.9 should't be too far away, and there's already @dvz's plugin for 1.8 to update the hashing, so I think leaving it until 1.9 makes sense.

dvz referenced this issue Jan 7, 2019

@dvz dvz added s:review-needed and removed s:in-progress labels Feb 5, 2019

@dvz dvz moved this from In progress to Needs review in MyBB 1.9.0 Feb 5, 2019

euantorano added a commit that referenced this issue Feb 10, 2019
Resolve #3530 Modern password hashing (#3571)
* Add Illuminate\Config\Repository, load old config values

* Add internal password algorithm recognition using Illuminate\Hashing

* add PHPDoc for dynamically accessed objects

* Create basic Config service provider

@dvz dvz added s:resolved and removed s:review-needed labels Feb 10, 2019

@dvz dvz closed this Feb 10, 2019

MyBB 1.9.0 automation moved this from Needs review to Done Feb 10, 2019

euantorano added a commit to euantorano/mybb that referenced this issue Mar 23, 2019
Resolve mybb#3530 Modern password hashing (mybb#3571)
* Add Illuminate\Config\Repository, load old config values

* Add internal password algorithm recognition using Illuminate\Hashing

* add PHPDoc for dynamically accessed objects

* Create basic Config service provider
euantorano added a commit to euantorano/mybb that referenced this issue Mar 23, 2019
Resolve mybb#3530 Modern password hashing (mybb#3571)
* Add Illuminate\Config\Repository, load old config values

* Add internal password algorithm recognition using Illuminate\Hashing

* add PHPDoc for dynamically accessed objects

* Create basic Config service provider

@dvz dvz unpinned this issue May 28, 2019

euantorano added a commit that referenced this issue Jun 2, 2019
Resolve #3530 Modern password hashing (#3571)
* Add Illuminate\Config\Repository, load old config values

* Add internal password algorithm recognition using Illuminate\Hashing

* add PHPDoc for dynamically accessed objects

* Create basic Config service provider
euantorano added a commit that referenced this issue Jun 2, 2019
Resolve #3530 Modern password hashing (#3571)
* Add Illuminate\Config\Repository, load old config values

* Add internal password algorithm recognition using Illuminate\Hashing

* add PHPDoc for dynamically accessed objects

* Create basic Config service provider
JordanMussi added a commit to JordanMussi/MyBB that referenced this issue Jun 10, 2019
dvz added a commit that referenced this issue Jun 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants
You can’t perform that action at this time.