Adds support for something better than md5 hashes.#177
Conversation
|
Thanks for the PR! This will take a little longer to review, I'll try to get to it soon. |
gregstoll
left a comment
gregstoll
left a comment
There was a problem hiding this comment.
Thanks for the contribution! I've left some comments that need looking at...
|
|
||
| /* | ||
| * Password hashing method. This defines the algorithm used for hashing new passwords. | ||
| * Valid options are: |
There was a problem hiding this comment.
(thanks for the documentation here!)
This is...a lot of options to offer, and I doubt most people (myself included!) are going to have a good idea of the tradeoffs between the choices. Obviously we need to keep "salted md5" for compatibility, but could we just pick one or two of the most secure other options?
There was a problem hiding this comment.
Well, we could always require users to only use what we allow them to use. but I do not know their systems, and what they need to use, why does gallery2 support phpass? why shouldn't bcrypt be supported, most people believe this is secure, why use or not use argon2*, it is believed to be more secure than bcrypt, but bcrypt and argon2 are not fips compliant. I have no idea what other software people would use with this, and what password hashing method they would use. I would not want to limit their options when it's trivial to implement.
|
|
||
| /* Match crypt methods, which start with $x$ */ | ||
| if (isset($valid[0]) && $valid[0] === '$') { | ||
| if (strlen($valid) == 34 && (strpos($valid, '$P$') === 0 || strpos($valid, '$H$') === 0)) { |
There was a problem hiding this comment.
It looks like the PasswordHash only supports "$P$" prefixes - am I missing something?
There was a problem hiding this comment.
Ah, did not realised gallery2 had such an old version it didn't support the
| */ | ||
| function changePassword($newPassword) { | ||
| $this->setHashedPassword(GalleryUtilities::md5Salt($newPassword)); | ||
| $this->setHashedPassword(GalleryUtilities::hashPassword($newPassword)); |
There was a problem hiding this comment.
There are a handful of uses of md5Salt() in modules/core/test/phpunit - do those need to be updated too?
| } else if ($method == 'crypt-sha512') { | ||
| $salt = '$6$rounds=5000$' . substr(md5(mt_rand()), 0, 16) . '$'; | ||
| return crypt($password, $salt); | ||
| } else if ($method == 'argon2i' && defined('PASSWORD_ARGON2I')) { |
There was a problem hiding this comment.
If the user specifies "argon2i" but PASSWORD_ARGON2I isn't defined, this has us fall back to an insecure option. Can we return an error somehow instead? (same for "argon2id" below)
| return password_hash($password, PASSWORD_ARGON2ID); | ||
| } | ||
|
|
||
| return GalleryUtilities::md5Salt($password); |
There was a problem hiding this comment.
Similar to above, would be nice to check that $method is actually "salted md5" here and the user didn't just make a typo.
There was a problem hiding this comment.
it would make more sense to add this somewhere where the config file is loaded and validated then. it can check for a typo and error, and validate if argon2 is selected that they do exist. Otherwise the admin would never know about it until a password was changed.
2 participants
Adds bcrypt, argon2i, argon2id, and crypt based hashs (namely sha256$5$ and sha512 $6$ ).
added the isConfig, so existing configs will work without warnings, and for the unittests also.