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] Replace Argon2*PasswordEncoder by SodiumPasswordEncoder #31019

Merged
merged 1 commit into from Apr 9, 2019

Conversation

Projects
None yet
7 participants
@chalasr
Copy link
Member

commented Apr 8, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #31016
License MIT
Doc PR todo symfony/symfony-docs#11368

See fixed ticket, much simpler/secure/maintainable.

@chalasr chalasr added this to the next milestone Apr 8, 2019

@chalasr chalasr force-pushed the chalasr:sodium-encoder branch 2 times, most recently from 3581d68 to 2d73bca Apr 8, 2019

@chalasr chalasr force-pushed the chalasr:sodium-encoder branch from 424f311 to 9a771b3 Apr 8, 2019

@chalasr chalasr force-pushed the chalasr:sodium-encoder branch 4 times, most recently from a8a6f27 to 686fe0e Apr 8, 2019

if (\function_exists('sodium_crypto_pwhash_str_verify')) {
$valid = \sodium_crypto_pwhash_str_verify($encoded, $raw);
\sodium_memzero($raw);

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 8, 2019

Member

copy-on-write makes this useless to me - it should be done on the original value, outside of the method

This comment has been minimized.

Copy link
@chalasr

chalasr Apr 8, 2019

Author Member

indeed, all removed

@chalasr chalasr force-pushed the chalasr:sodium-encoder branch from 686fe0e to 529211d Apr 8, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

Thank you @chalasr.

@fabpot fabpot merged commit 529211d into symfony:master Apr 9, 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

fabpot added a commit that referenced this pull request Apr 9, 2019

feature #31019 [Security] Replace Argon2*PasswordEncoder by SodiumPas…
…swordEncoder (chalasr)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Security] Replace Argon2*PasswordEncoder by SodiumPasswordEncoder

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      |  no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #31016
| License       | MIT
| Doc PR        | todo symfony/symfony-docs#11368

See fixed ticket, much simpler/secure/maintainable.

Commits
-------

529211d [Security] Replace Argon2*PasswordEncoder by SodiumPasswordEncoder

@chalasr chalasr deleted the chalasr:sodium-encoder branch Apr 9, 2019

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

I think this is really bad. The user should have control over which algorithm is used.

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

Implementation wise, it's possible to use sodium_crypto_pwhash instead, as I've discussed with @chalasr extensively about this.

@chalasr

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

@teohhanhui Allowing to specify the algorithm involves to rely on implementation details and to make strong assumptions that may be wrong at some point or don’t fit for all platforms. The result felt a bit hackish tbh.
Considering jedisct1/libsodium-php#194

sodium_crypto_pwhash_str() uses the best algorithm available in the currently installed version of libsodium for the current platform.
It can be Argon2i, Argon2id, or something else depending on the platform (Argon2 is currently not a good fit for constrained environments and WebAssembly). You shouldn’t assume that it is a specific algorithm.

Our role as a framework is to always provide the most secure alternative, that is what sodium_crypto_pwhash_str() does.
Also @paragonie does the same. Given the strong knowledge they have on this topic, I think we'd better not try to reinvent the wheel.

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

My plea to the Symfony team: please hold your horses on the deprecation of using specific password hashing algorithms. This deserves more input / feedback from the community, as it entails taking away control from the application developer.

Our role as a framework is to always provide the most secure alternative, that is what sodium_crypto_pwhash_str() does.

Of course, it's good to have sodium_default / php_default password encoders available. But the developer should still be able to choose a specific algorithm, depending on their needs.

I will try working on implementations that use sodium_crypto_pwhash.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Honestly, I think we should adopt the recommended practice and stop allowing ppl to choose the algo. Sticking to one algo will be broken in the future, because that's the life of hash algos. I'm all for deprecating the existing encoders that are bound to one algo.

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

If we're taking that route, then it should be done similar to this? So that passwords using old algorithms could be transparently re-hashed (upon password validation and detecting that a rehash is needed). It'll be invaluable for so many projects. (But IMO, actually the developer should still be able to configure which algorithms should be used.)

https://docs.spring.io/spring-security/site/docs/5.1.5.RELEASE/reference/html5/#pe-dpe
https://docs.spring.io/spring-security/site/docs/5.1.5.RELEASE/api/org/springframework/security/crypto/password/DelegatingPasswordEncoder.html
https://docs.spring.io/spring-security/site/docs/5.1.5.RELEASE/api/org/springframework/security/crypto/factory/PasswordEncoderFactories.html#createDelegatingPasswordEncoder--

I see there's #30914

@teohhanhui teohhanhui referenced this pull request Apr 11, 2019

Open

[RFC] Symfony Security rework tracking issue #30914

0 of 34 tasks complete
* Added `Argon2idPasswordEncoder`
* Deprecated using `Argon2iPasswordEncoder` while only the `argon2id` algorithm
is supported, use `Argon2idPasswordEncoder` instead
* deprecated `Argon2iPasswordEncoder`, use `SodiumPasswordEncoder` instead

This comment has been minimized.

Copy link
@stof

stof Apr 17, 2019

Member

should we actually deprecate it ? It can use the native API of PHP 7.2+ (and Argon2idPasswordEncoder could as well, giving control over the variant). SodiumPasswordEncoder is only based on the sodium extension.

IMO, we should only deprecated the sodium-based fallback in the Argon encoders, not deprecate the encoders themselves.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 17, 2019

Member

Having encoders bound to one algo is a bad practice. The documented best practice is to have a progressive encoder that knows how to verify old passwords while using the best available algo to encode new ones. So yes: this should be deprecated with this reasoning.
Of course, there is one missing step: rehashing. See #31139

This comment has been minimized.

Copy link
@teohhanhui

teohhanhui Apr 17, 2019

Contributor

The documented best practice is to have a progressive encoder that knows how to verify old passwords while using the best available algo to encode new ones.

The chain / delegating encoder should be composed of encoders using specific algorithms. At least, that's the case in Spring Security (see links above).

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 17, 2019

Member

The chain / delegating encoder should be composed of encoders using specific algorithms

PHP is a glue language while Java implements everything itself. The situation in PHP is that password_hash()/Sodium\crypto_pwhash_str already embed the chain and there is zero need to expose the individual algos as encoders.

I agree a chain encoder would be nice to provide a migration path from Pbkdf2/MessageDigest/Plaintext to Native/Sodium thought. But we need #31139 first.

This comment has been minimized.

Copy link
@teohhanhui

teohhanhui Apr 17, 2019

Contributor

The situation in PHP is that password_hash()/Sodium\crypto_pwhash_str already embed the chain and there is zero need to expose the individual algos as encoders.

Having a default (like with password_hash) / fallback (like with sodium_crypto_pwhash_str) mechanism does not create a "chain" (to be understood as for the purpose of rehashing). Anyway, I don't understand that argument of PHP vs Java, when it's been pointed out that it's indeed possible to provide encoders for specific algorithms (with extra effort in the case of sodium_crypto_pwhash).

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

@danielchodusov

This comment has been minimized.

Copy link

commented May 10, 2019

Hi,

I think this might cause a problem when I hash passwords on the machine with different Argon implementation and validate them on another one. I run app in php 7.2 on Debian but sometimes I need to run CLI command which add user from the machine which runs on 7.3 on MacOS with the different sodium library.

Means that I $passwordEncoder->isPasswordValid(...) will not validate the password when I generated on different machine?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 10, 2019

I agree this might be a problem. Yet the outcome is ok: you won't be able to log in. I can't imagine how we could do better while still providing state of the art security practices (always using the best possible hash algo)

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented May 11, 2019

But it's at least forward-compatible, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.