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

[Security] Do not mix password_*() API with libsodium one #29863

Merged
merged 1 commit into from Jan 18, 2019

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Jan 12, 2019

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/a

Argon2IPasswordEncoder 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().

See https://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.

Copy link
Member

@javiereguiluz javiereguiluz left a 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 chalasr merged commit d6cfde9 into symfony:3.4 Jan 18, 2019
chalasr pushed a commit that referenced this pull request Jan 18, 2019
…(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/a

Argon2IPasswordEncoder 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()`.

See https://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
@chalasr chalasr deleted the fix-argon2i-verif branch January 19, 2019 23:26
This was referenced Jan 29, 2019
@PhilETaylor
Copy link
Contributor

PhilETaylor commented Mar 12, 2019

Hey there.. @chalasr
(Symfony 3.4.23, PHP 7.3.3, Alpine 3.9 based custom docker image)

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 algorithm: argon2i in my security.yml encoders I believe I have discovered a bug with your change.

All my passwords are bcrypt 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 (\PHP_VERSION_ID >= 70200 && \defined('PASSWORD_ARGON2I') && (false === strpos($encoded, '$argon2id$'))) {
            return !$this->isPasswordTooLong($raw) && password_verify($raw, $encoded);
        }

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!

$2y$11$RA2EHcK/VwKdPS.lKBl/POJ3MYfa2j3Dt92B3PelAlzRmbctxeDO.

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
Phil.

@PhilETaylor
Copy link
Contributor

others dont think this is a bug? #29827 oh well....

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

Successfully merging this pull request may close these issues.

None yet

4 participants