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

Projects
None yet
3 participants
@chalasr
Copy link
Member

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 jedisct1/libsodium-php#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.

@javiereguiluz
Copy link
Member

javiereguiluz left a comment

Thanks for the insightful explanation and the fix!

@chalasr chalasr merged commit d6cfde9 into symfony:3.4 Jan 18, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

chalasr added a commit that referenced this pull request Jan 18, 2019

bug #29863 [Security] Do not mix password_*() API with libsodium one …
…(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 jedisct1/libsodium-php#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 chalasr:fix-argon2i-verif branch Jan 19, 2019

This was referenced Jan 29, 2019

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