NativeEncoder::needsRehash passes string as 2nd argument to password_needs_rehash; this has to be an int

[olia-bn]

Symfony version(s) affected: 4.4.7, others

Description
Symfony\Component\Security\Core\Encoder\NativePasswordEncoder::needsRehash is calling PHP native function password_needs_rehash with 2nd argument (algorithm) being string while it needs to be an int.

https://github.com/symfony/security-core/blob/7b409f4f186728081f37e9e788c1617dd2b6d702/Encoder/NativePasswordEncoder.php#L108

Same issue will affect NativePasswordEncoder::encodePassword function
https://github.com/symfony/security-core/blob/7b409f4f186728081f37e9e788c1617dd2b6d702/Encoder/NativePasswordEncoder.php#L71

How to reproduce
No special set up required; algorithm variable of the NativePasswordEncoder is a string and it's directly passed into password_needs_rehash

Possible Solution
Cast the variable to string before passing ?

[nicolas-grekas]

Hi @olia-bn, actually, the expected type changed in PHP 7.4: it's a string now IIRC.

[olia-bn]

https://www.php.net/manual/en/function.password-needs-rehash.php explicitly defines it as an int, and there's nothing about type change for this function in change in change log https://www.php.net/manual/en/doc.changelog.php

[nicolas-grekas]

I suppose that the doc is not up to date...
Here is the behavioral hint about this: https://3v4l.org/qnrM7

[wouterj]

@nicolas-grekas is correct here, see this RFC that is marked as "implemented (php 7.4)": https://wiki.php.net/rfc/password_registry

Seems like, unless the user overwrites the setting, Symfony is using the constants: https://github.com/symfony/security-core/blob/7b409f4f186728081f37e9e788c1617dd2b6d702/Encoder/NativePasswordEncoder.php#L51

When extracting this, the code seems to work without errors: https://3v4l.org/7JGBn

@olia-bn did you get a PHP error while using this feature or how did you find it/why want you to change it?

[curry684]

We've got a semi-old project that was originally started in the Symfony 3.x era. It explicitly defines a bcrypt encoder in the security config section, and we're running it on PHP 7.2. It died on a notice indicating 'suspicious string to number conversion' or something.

From what I can quickly see it's a rare combination of how bcrypt is translated internally to the NativePasswordEncoder, which barely anyone will run into as sodium/auto have been default for quite a while.

Seems like, unless the user overwrites the setting, Symfony is using the constants:

That line still casts to string, thus causing the issue on PHP<7.4.

[olia-bn]

@nicolas-grekas 👍
Time to fix PHP manual as well!

We are running it on 7.2.27

@wouterj I got a notice escalated to exception on dev environment:
Notice: A non well formed numeric value encountered

[wouterj]

Hmm, @nicolas-grekas are you maybe aware of the reason why there is an explicit (string) typecast for $this->algo? Seems like we can solve the notice by just accepting int/string, whatever is contained in the constants.

Time to fix PHP manual as well!

@olia-bn are you any good with XML? 😛 https://github.com/php/doc-en/blob/master/reference/password/functions/password-needs-rehash.xml

[stof]

The string cast looks wrong indeed, due to older versions having an integer.

[olia-bn]

I think @stof hits the nail on the head.

My suggestion to cart it to int before calling the native function was based on the (not up to date) documentation, but since the class itself is using PHP defined constants, simply removing the casting (and type hint in constructor) would solve the issue for all PHP versions

[wouterj]

@olia-bn do you want to create a PR doing that in 4.4? :)

[curry684]

Ok, I just started building the PR with @olia-bn and then we discovered it's not a real bug but a DX issue as I wrote a test to fail and it didn't at first.

The string cast is slightly suspicious for old PHP versions, but not a real issue as PHP will internally happily juggle a string with a "pure" integer back to an integer for BC reasons:

$ php7.2 -r "var_dump(password_hash('foo', (string) PASSWORD_BCRYPT));"
Command line code:1:
string(60) "$2y$10$7GuTSTVam05noCLFuVf1b.IT1Qw5BgRWJ84aE96.33amFwlvaIywG"

It WILL however give the seen notice when we explicitly feed it the PHP 7.4 constants:

$ php7.2 -r "var_dump(password_hash('foo', '2y'));"
PHP Notice:  A non well formed numeric value encountered in Command line code on line 1
PHP Stack trace:
PHP   1. {main}() Command line code:0
PHP   2. password_hash() Command line code:1
Command line code:1:
string(96) "$argon2i$v=19$m=65536,t=4,p=1$akIyZkNUS1RiN0ZNdUdjTA$/BcMwZshNRAkM5DMYsoefm4OKE8UVeLVVvO9D2QwxmA"

This got me puzzled as to why we would at some point be mixing those up, only to realize the code doesn't but the developer might accidentally do so (and did in this case).

Our development server has all PHP versions from 7.0 to 7.4 installed, and we select the right one via .htaccess statements. The CLI runs 7.4 by default. So when the developer updated the code, she ran the usual console commands and it rebuilt the container on 7.4, preparing the NativePasswordEncoder service with 7.4 constants.

And then when she logged in on the website, running 7.2, it crashed on instantiating the service to check for rehash.

So, the string cast is harmless as far as I'm concerned, and removing it won't fix this issue anyway (it would crash on 'unknown algorithm type 2y requested'). The question is now whether we should detect this situation as I do foresee it might crash some production websites in the world when a sysadmin forgets to clear/warmup caches while upgrading PHP on their server.

[curry684]

Come to think of it - @nicolas-grekas shouldn't DI somehow safeguard against using a container compiled on a different PHP version anyway?

Appending PHP_VERSION_ID to the serialize call in ContainerBuilder::hash would pretty much solve this and all future similar problems. Could create some challenges for releasing sites with pre-built containers though.

[enumag]

I just got a Notice: A non well formed numeric value encountered. It seems to be caused by me accidentally using PHP 7.4 in CLI but 7.3 in Nginx.

[stof]

Doing the cache warmup with a different PHP version that the PHP-FPM runtime using the cache is an unsupported use case.

[wouterj]

Should we maybe do what @curry684 suggested: Append php version to the container hash to make sure this no longer happens (or add an exception if the version is not the same as the one building the container?)

[curry684]

is an unsupported use case

But it's still happening a lot unintentionally, this specific change is just exposing it more painfully.

or add an exception if the version is not the same as the one building the container?

I don't think we want to let people accidentally kill production sites by running a console command.

I think embedding the major/minor version in the hash should prevent most issues, we'd just need to document it at https://symfony.com/doc/current/components/dependency_injection/workflow.html#working-with-a-cached-container

[nicolas-grekas]

embedding the major/minor version in the hash

That would be quite confusing I fear. We'd better make the logic related to this issue able to cope with both type of values.

Up for a PR?

[curry684]

I have no clue what you just said 😆

[nicolas-grekas]

Check #36824