Always use PHP's own crypt_r implementation, don't support system crypt

[alexdowad]

Looking forward to hearing your comments.

[alexdowad]

Failure on Appveyor looks to be spurious.

[nikic]

Feel free to land everything but the last commit already.

[alexdowad]

Feel free to land everything but the last commit already.

I did think the last commit would be more controversial than the others. 😄

[nikic]

For the last commit, I have some concerns:

• man 5 crypt lists at least two algorithms we don't support: $y$ (yescript) and $7$ (scrypt).
• PHP has some bcrypt related weirdness, because it supports $2x$ and $2y$ prefixes. There may be some behavior discrepancy with glibc here.

[nikic]

So, now I'm left wondering what actually happens if we use the glibc implementation, wrt to $2y$.

My current state is that we probably never use the glibc implementation, because $php_crypt_r gets set to 0, because the function is checked in the wrong library (and later in the correct library).

[nikic]

I've applied 7d05bc8 to fix the crypt_r detection, and instead added a || true to the code picking the implementation, so it's a bit more honest about what's going on :)

[nikic]

I've also fixed one behavior difference in 4c4af2b (landed this one in 7.4).

There's a diff in ext/standard/tests/strings/crypt_sha256.phpt:

Iteration 8 failed.
Expected: <$5$rounds=1000$roundstoolow$yfvwcWrQ8l/K0DAWyuPMDNHpIVlTQebY9l/gL972bIC>
Got       <*0>
Passes.

It looks like we aren't rejecting overly low rounds.

[nikic]

I've addressed that discrepancy with 8a8c8d4.

[nikic]

Regarding the last commit, I think we should be supporting system crypt as the fallback implementation for all the hashes we don't know. That's not how it works right now, but I think that's how it should work.

[alexdowad]

For the last commit, I have some concerns:

• man 5 crypt lists at least two algorithms we don't support: $y$ (yescript) and $7$ (scrypt).
• PHP has some bcrypt related weirdness, because it supports $2x$ and $2y$ prefixes. There may be some behavior discrepancy with glibc here.

...

Regarding the last commit, I think we should be supporting system crypt as the fallback implementation for all the hashes we don't know. That's not how it works right now, but I think that's how it should work.

I have a different view.

Users don't care about libc or what hashes it supports. (Many PHP users probably don't even know what libc is.) Users definitely do not want their programs to fail when moved from a dev machine to a server, or from one server to another, because a different version of glibc is installed there.

It is better not to create unnecessary dependencies on the underlying system libraries. We need to use system APIs to access the hardware or modify the system state. But where just pure computation is involved, it almost never makes sense to rely on them.

If glibc supports some hashes which PHP doesn't know about, I would be happy to implement them.

[alexdowad]

By the way, @nikic, thanks for finding those other problems with PHP's crypt_r. 👍

[nikic]

Regarding the last commit, I think we should be supporting system crypt as the fallback implementation for all the hashes we don't know. That's not how it works right now, but I think that's how it should work.

I have a different view.

Users don't care about libc or what hashes it supports. (Many PHP users probably don't even know what libc is.) Users definitely do not want their programs to fail when moved from a dev machine to a server, or from one server to another, because a different version of glibc is installed there.

It is better not to create unnecessary dependencies on the underlying system libraries. We need to use system APIs to access the hardware or modify the system state. But where just pure computation is involved, it almost never makes sense to rely on them.

If glibc supports some hashes which PHP doesn't know about, I would be happy to implement them.

We already have a high-level portable API for password hashing, which is the password_hash_* family of functions. You shouldn't be messing with tricky crypt() API if a portable password hash is all you want.

Using crypt() nowadays only ever makes sense if you specifically need to interoperate with crypt() based systems, e.g. if you want to process passwd files from PHP. For such use-cases, the only thing you care about is that crypt() in PHP behaves consistently with other crypt() based APIs on the same system.

[alexdowad]

Regarding the last commit, I think we should be supporting system crypt as the fallback implementation for all the hashes we don't know. That's not how it works right now, but I think that's how it should work.

I have a different view.
Users don't care about libc or what hashes it supports. (Many PHP users probably don't even know what libc is.) Users definitely do not want their programs to fail when moved from a dev machine to a server, or from one server to another, because a different version of glibc is installed there.
It is better not to create unnecessary dependencies on the underlying system libraries. We need to use system APIs to access the hardware or modify the system state. But where just pure computation is involved, it almost never makes sense to rely on them.
If glibc supports some hashes which PHP doesn't know about, I would be happy to implement them.

We already have a high-level portable API for password hashing, which is the password_hash_* family of functions. You shouldn't be messing with tricky crypt() API if a portable password hash is all you want.

Using crypt() nowadays only ever makes sense if you specifically need to interoperate with crypt() based systems, e.g. if you want to process passwd files from PHP. For such use-cases, the only thing you care about is that crypt() in PHP behaves consistently with other crypt() based APIs on the same system.

OK. Fair enough.

If that is so, it does call into question why we have the in-tree implementation of crypt_r and its various hashes. It sounds like it is basically just for people who are misusing the crypt() function.

[alexdowad]

@nikic... So I guess I can close this PR?

Do you think the docs for crypt should be updated to reflect its intended use?

[nikic]

Just my 2c ;) Maybe, given that we have not been using system crypt for so long due to a configure bug, it makes more sense to drop support for it as proposed.

I wonder if @remicollet or @oerdnj patch PHP to use system crypt or not. (Distros commonly disable any custom implementations we have :)

[oerdnj]

Here's the commit from packaging repo:

commit ff02f63e0687d2325b89c51da883c5af005d1068
Author: Ondřej Surý <[email protected]>
Date:   Fri Nov 16 15:17:55 2012 +0100

    Return to PHP versions of crypt() functions, it's to big burden to carry on the patch

[alexdowad]

Here's the commit from packaging repo:

commit ff02f63e0687d2325b89c51da883c5af005d1068
Author: Ondřej Surý <[email protected]>
Date:   Fri Nov 16 15:17:55 2012 +0100

    Return to PHP versions of crypt() functions, it's to big burden to carry on the patch

Thank you! Which repo is this?

[alexdowad]

Closing due to inactivity.