-
-
Notifications
You must be signed in to change notification settings - Fork 68
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
Increase security of existing MD5 passwords #6
Comments
We're aware of this limitation and agree with you 👍. However, the goal of this plugin was to remain as simple as possible to tread lightly and err on the side of caution. I was even on the fence about including the migration that exists in the code now but it's pretty straight forward. Most of the tools we make at Roots are meant to be used on new sites. Sage and Bedrock are starters/boilerplates for example. This plugin works best when using it on a brand new site since you can skip any complications like you're talking about. That being said, we'd be open to adding functionality like this if it's well tested. Maybe even make it optional as a one-time process? |
Oh and we aren't trying to imply this magically solves all password issues on existing sites. I'll probably add another section/FAQ to the README so people know exactly what's going on in different cases. |
My position is this kind of functionality should be left to a separate plugin. Keeping the codebase lightweight means it's more easily understood and quicker to be vetted, which will hopefully lead to a faster adoption rate. The code to handle mass password migrations for membership/community sites, which could have tens of users, or millions, would easily outweigh the existing code and by some margin. When you get to that point you may as well just include this plugin as a composer dependency of the mass password migration plugin. |
I added a note about this in the FAQ. If anyone is interested in creating a solid plugin for password migrations, we could offer to have it live under the Roots organization. |
I'm not sure if this 100% addresses the issue here, but you could always regenerate the authentication salts which would invalidate any existing logged in cookies thus forcing all users to login again. With that said, the batch processing action would probably make more sense as a wp-cli command. Seems kind of silly to create a plugin for a one-time use like that. |
When installing this plugin the passwords of existing users are still stored as an MD5 hash. When the user logs in, there is a check to see if they have a legacy MD5 hash, and if so (upon successful authentication) the provided value will be used to 'upgrade' it to a bcrypt hash.
This on it's own doesn't do much to increase security, as until the user logs in their password will still be stored as an MD5 hash. Depending on how often users log in, it may be a very long time (think membership / community sites) until a secure bcrypt hash is stored for every user. If the database is compromised, you will still be leaking MD5 hashes which for simple passwords are trivial to reverse.
A more secure option would be to run all the existing MD5 hashes through
password_hash
when the plugin is installed, so in affect you are storingbcrypt(md5(password))
- no more MD5. A legacy flag can be added as a prefix in the hash field, and then when authenticating you can check that, to see if it's a legacy password that the should be MD5'ed before being passed topassword_verify
. At that point, once you have authenticated the user, you can generate a plain bcrypt hash so justbcrypt(password)
is stored.This is explained further, with example PHP code, in this Reddit post (not mine):
https://www.reddit.com/r/PHP/comments/3lwxlw/hash_and_verify_passwords_in_php_the_right_way/cva6y6p
The text was updated successfully, but these errors were encountered: