Skip to content

Doesn't verify password before upgrading the password hash#4

Merged
Ayesh merged 1 commit intoAyesh:masterfrom
Sc00bz:patch-1
Mar 4, 2018
Merged

Doesn't verify password before upgrading the password hash#4
Ayesh merged 1 commit intoAyesh:masterfrom
Sc00bz:patch-1

Conversation

@Sc00bz
Copy link
Copy Markdown
Contributor

@Sc00bz Sc00bz commented Mar 4, 2018

When the password hash is being upgraded, anyone can login:

  • Try password of "password". Access denied. Password hash upgraded with password of "password".
  • Try password of "password". Access granted.

@Ayesh
Copy link
Copy Markdown
Owner

Ayesh commented Mar 4, 2018

Thank you for the PR. I can also see the issue, except that the steps you mentioned to reproduce don't look right.

Right now, PASSWORD_DEFAULT is the only hashing algorithm we use in the plugin. When upgrading from regular WordPress passwords, password_get_info doesn't work, so there is no password hash update.

If the database contains a password hash that is not default (bcrypt), that is when the hash is updated. In other words, only when we upgrade from a PHP password_hash-compatible hashing algorithm. Luckily, there is no release with Argon support from passwords yet, so this only happens when the hash is set by a third party. Could you confirm it?

Also, I would like your opinion if this should warrant a security update. I think it does and I can email WordPress security team if you too agree.

@Ayesh Ayesh merged commit 3e2035b into Ayesh:master Mar 4, 2018
@Sc00bz
Copy link
Copy Markdown
Contributor Author

Sc00bz commented Mar 5, 2018

PASSWORD_DEFAULT was going to be changed to PASSWORD_ARGON2I in version 7.4 but that was delayed. When PASSWORD_DEFAULT changes this bug will show its face. Also password_needs_rehash should be given the current settings for the hash so that they can be upgraded on change. Now that this is fixed I'll submit a pull request for that. That is one thing I'm wondering about https://github.com/Ayesh/wordpress-password-hash/blob/3e2035bc7941a120e6ad019b3427bf41fedd4dae/wp-php-password-hash.php#L51 vs https://github.com/Ayesh/wordpress-password-hash/blob/3e2035bc7941a120e6ad019b3427bf41fedd4dae/wp-php-password-hash.php#L72. You also test if $user_id is set in one but not the other. Do you need to check if $user_id is set because the DB query should just fail? Oh I guess it's faster to test if $user_id is set then waiting for a query to fail.

@Sc00bz Sc00bz deleted the patch-1 branch March 5, 2018 16:24
@Sc00bz
Copy link
Copy Markdown
Contributor Author

Sc00bz commented Mar 5, 2018

Oh right I definitely not clear originally. I meant when it is upgraded from bcrypt. Which might happen in 6+ months if this is not fixed.

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

Successfully merging this pull request may close these issues.

2 participants