Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Security] Do not mix password_*() API with libsodium one#29863
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
javiereguiluz left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for the insightful explanation and the fix!
…(chalasr)This PR was merged into the 3.4 branch.Discussion----------[Security] Do not mix password_*() API with libsodium one| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | n/a| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aArgon2IPasswordEncoder uses native `password_hash()` and `password_verify()` functions if the current PHP installation embeds Argon2 support (>=7.2, compiled `--with-password-argon2`).Otherwise, it fallbacks to the libsodium extension.This was fine at time the encoder was introduced, but meanwhile libsodium changed the algorithm used by `sodium_crypto_pwhash_str()` which is now argon2id, that goes outside of the scope of the encoder which was designed to deal with `argon2i` only.Nothing we can do as databases may already contain passwords hashed with argon2id, the encoder must keep validating those.However, the PHP installation may change as time goes by, and could suddenly embed the Argon2 core integration. In this case, the encoder would use the `password_verify()` function which would fail in case the password was not hashed using argon2i.This PR prevents it by detecting that argon2id was used, avoiding usage of `password_verify()`.Seehttps://github.com/jedisct1/libsodium-php/issues/194 and#28093 for references.Patch cannot be tested as it is platform dependent.Side note: I'm currently working on a new implementation for 4.3 that will properly supports argon2id (which has been added to the PHP core sodium integration in 7.3) and argon2i, distinctively.Commits-------d6cfde9 [Security] Do not mix usage of password_*() functions and sodium_*() ones
PhilETaylor commentedMar 12, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hey there..@chalasr Today I was investigating my own move from bcrypt to argon2id... My app is up and running in production (myJoomla.com) and is currently using bcrypt. If I change to All my passwords arebcrypt in the db at the moment. HOWEVER I CAN STILL LOGIN WITH ANY USERS CORRECT PASSWORD WHEN THEIR HASHED PASSWORD IS IN BCRYPT AND WHEN SYMFONY IS CONFIGURED TO USE ARGONI I have debugged this all the way down to this if statement in vendor/symfony/symfony/src/Symfony/Component/Security/Core/Encoder/Argon2iPasswordEncoder.php: If $encoded is a bcrypt password (Example below) then this will return TRUE and the user will be authenticated with their bcrypted password, even though Symfony is configured to use argon2i encoder!
I like that, as it makes my move to argon2i easier, but I dont believe that it should ever do this, this is the Argon Encoder, it should not be allowed to return TRUE for isPasswordValid when the encoded password is bcrypt. It should return FALSE. Thoughts? Kindest regards |
PhilETaylor commentedMar 12, 2019
others dont think this is a bug?#29827 oh well.... |
Argon2IPasswordEncoder uses native
password_hash()andpassword_verify()functions if the current PHP installation embeds Argon2 support (>=7.2, compiled--with-password-argon2).Otherwise, it fallbacks to the libsodium extension.
This was fine at time the encoder was introduced, but meanwhile libsodium changed the algorithm used by
sodium_crypto_pwhash_str()which is now argon2id, that goes outside of the scope of the encoder which was designed to deal withargon2ionly.Nothing we can do as databases may already contain passwords hashed with argon2id, the encoder must keep validating those.
However, the PHP installation may change as time goes by, and could suddenly embed the Argon2 core integration. In this case, the encoder would use the
password_verify()function which would fail in case the password was not hashed using argon2i.This PR prevents it by detecting that argon2id was used, avoiding usage of
password_verify().Seehttps://github.com/jedisct1/libsodium-php/issues/194 and#28093 for references.
Patch cannot be tested as it is platform dependent.
Side note: I'm currently working on a new implementation for 4.3 that will properly supports argon2id (which has been added to the PHP core sodium integration in 7.3) and argon2i, distinctively.