[Security] Replace Argon2*PasswordEncoder by SodiumPasswordEncoder #31019

chalasr · 2019-04-08T18:02:22Z

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.

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

src/Symfony/Component/Security/Core/Encoder/SodiumPasswordEncoder.php

nicolas-grekas · 2019-04-08T20:13:12Z

src/Symfony/Component/Security/Core/Encoder/SodiumPasswordEncoder.php

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


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

indeed, all removed

This reverts commit dc95a6f.

fabpot · 2019-04-09T06:05:33Z

Thank you @chalasr.

…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

teohhanhui · 2019-04-09T12:31:01Z

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

teohhanhui · 2019-04-09T12:32:10Z

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

chalasr · 2019-04-09T18:35:39Z

@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 https://github.com/jedisct1/libsodium-php/issues/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 · 2019-04-10T12:05:43Z

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 · 2019-04-10T12:12:46Z

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 · 2019-04-11T15:50:53Z

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

stof · 2019-04-17T09:22:27Z

src/Symfony/Component/Security/CHANGELOG.md

- * Added `Argon2idPasswordEncoder`
- * Deprecated using `Argon2iPasswordEncoder` while only the `argon2id` algorithm
-   is supported, use `Argon2idPasswordEncoder` instead
+ * deprecated `Argon2iPasswordEncoder`, use `SodiumPasswordEncoder` instead


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.

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

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

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.

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

danielchodusov · 2019-05-10T17:59:58Z

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 · 2019-05-10T19:16:35Z

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 · 2019-05-11T10:30:29Z

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

chalasr added Security SecurityBundle labels Apr 8, 2019

chalasr added this to the next milestone Apr 8, 2019

carsonbot added Status: Needs Review Feature Deprecation labels Apr 8, 2019

chalasr mentioned this pull request Apr 8, 2019

Replace Argon2idPasswordEncoder by SodiumPasswordEncoder #31016

Closed

chalasr force-pushed the sodium-encoder branch 2 times, most recently from 3581d68 to 2d73bca Compare April 8, 2019 18:11