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

Increase security of existing MD5 passwords #6

Closed
lucaspiller opened this issue Mar 1, 2016 · 5 comments
Closed

Increase security of existing MD5 passwords #6

lucaspiller opened this issue Mar 1, 2016 · 5 comments

Comments

@lucaspiller
Copy link

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 storing bcrypt(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 to password_verify. At that point, once you have authenticated the user, you can generate a plain bcrypt hash so just bcrypt(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

@swalkinshaw
Copy link
Member

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?

@swalkinshaw
Copy link
Member

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.

@Foxaii
Copy link

Foxaii commented Mar 1, 2016

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.

@swalkinshaw
Copy link
Member

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.

@aaemnnosttv
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants