Skip to content

crypt() salt parameter is no longer optional #1533

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

Open
dregad opened this issue Apr 21, 2022 · 9 comments
Open

crypt() salt parameter is no longer optional #1533

dregad opened this issue Apr 21, 2022 · 9 comments
Labels
bug Documentation contains incorrect information good first issue Good for newcomers

Comments

@dregad
Copy link
Contributor

dregad commented Apr 21, 2022

From manual page: https://php.net/function.crypt


This manual page is somewhat out-of-date, regarding the $salt parameter.

The salt parameter is optional. However, crypt() creates a weak hash without the salt, and raises an E_NOTICE error without it. Make sure to specify a strong enough salt for better security.

PHP >= 8.0 throws PHP Warning: Uncaught ArgumentCountError: crypt() expects exactly 2 arguments, 1 given. The mention that the parameter is mandatory in PHP 8 is mentioned further down (in the changelog section), IMO it would be worth rephrasing along the lines of
Until PHP 8, the salt parameter was optional. However...

Additionally, the function returns *0 instead of a CRYPT_STD_DES hash as it did with PHP 7.x when given a null salt. PHP 8.1 throws Deprecated: crypt(): Passing null to parameter #2 ($salt) of type string is deprecated (8.0 does not even throw a warning) - this is a bit more than deprecation...

@cmb69
Copy link
Member

cmb69 commented Apr 21, 2022

Indeed, the info about that parameter being optional needs to be changed. We prefer the wording "Prior to PHP 8.0.0, …"

The deprecation of passing null is not specifc to this function, but a general change. The salt parameter never was nullable, so prior to PHP 8.1.0, passing null is the same as passing an empty string. I need to investigate closer what happens in that case.

@cmb69 cmb69 added the bug Documentation contains incorrect information label Apr 21, 2022
@dregad
Copy link
Contributor Author

dregad commented Apr 21, 2022

Thanks for the fast reply.

We prefer the wording "Prior to PHP 8.0.0, …"

Fine with me 👍
Note that the description of salt parameter should be updated as well
An optional salt string to base the hashing on. If not provided, the behaviour is defined by the algorithm implementation and can lead to unexpected results.

The deprecation of passing null is not specifc to this function, but a general change.

Duh. You're right of course... Silly me 😳

I need to investigate closer what happens in that case.

crypt() returns *0 in PHP 8.x, vs a CRYPT_MD5 (e.g. $1$bBNoVIxE$hE.91bqDEtouTpR4s/0ZB1) in earlier versions.

@cmb69
Copy link
Member

cmb69 commented Apr 21, 2022

I need to investigate closer what happens in that case.

An empty $salt causes failure. Actually, the $salt needs to be at least two characters for STD_DES, an underscore followed by at least eight characters for EXT_DES, or the marker (e.g. $1$) for any of the other types. Otherwise crypt() fails. So making the $salt parameter mandatory still doesn't shield users from calling crypt() without an effective salt. :(

@dregad
Copy link
Contributor Author

dregad commented Apr 21, 2022

My point is that this is a change in behavior in PHP 8 compared to 7.4 and older. Assuming this was intentional, it should be documented. Otherwise it's a bug that should be fixed...

echo crypt('password', '');

Output for 8.x *0
Output for 7.x and older $1$bBNoVIxE$hE.91bqDEtouTpR4s/0ZB1

Doc currently says
If no salt is provided, PHP will auto-generate either a standard two character (DES) salt, or a twelve character (MD5), depending on the availability of MD5 crypt().

@cmb69
Copy link
Member

cmb69 commented Apr 21, 2022

Oh, right, the behavior for an empty string passed as $salt has changed, and this might have been accidential. The relevant commit is 032f862133dbd2acc04cb75004428d6209f6046b. The line that triggered the calculation of a fallback salt was if (!*salt) {, not if (!salt) {. Maybe @nikic can clarify?

@dregad
Copy link
Contributor Author

dregad commented Apr 21, 2022

For the record, I know it's a weak encryption and that password_hash() should be used instead; I am actually in the process of updating legacy code to achieve that. I'm just trying to maintain backwards-compatibility if possible, but it looks like it may not be the case.

@cmb69
Copy link
Member

cmb69 commented Apr 21, 2022

I'm just trying to maintain backwards-compatibility if possible, but it looks like it may not be the case.

Like I said, I'm not sure whether that issue will be fixed. However, you can work around this in userland by providing an own crypt() function, which uses the old algorithm to calculate a salt, and then forwarding to PHP's crypt().

OTOH, you may be better off switching to the password_*() functions right away; these should be backward compatible to existing crypt hashes, to my knowledge.

@nikic
Copy link
Member

nikic commented Apr 21, 2022

@cmb69 The change is intentional. The user should generate an $1$ style salt themselves if they want to continue using an extremely weak hash function.

@dregad
Copy link
Contributor Author

dregad commented Apr 21, 2022

Thanks both for following up. I was pretty sure this was an intentional change (which makes perfect sense), the only problem is that it was not reflected in the documentation.

It may be a good idea to explain *0 under return values.

@Girgias Girgias added the good first issue Good for newcomers label Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Documentation contains incorrect information good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants