-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Always use PHP's own crypt_r implementation, don't support system crypt #5761
base: master
Are you sure you want to change the base?
Conversation
Failure on Appveyor looks to be spurious. |
Feel free to land everything but the last commit already. |
I did think the last commit would be more controversial than the others. 😄 |
For the last commit, I have some concerns:
|
So, now I'm left wondering what actually happens if we use the glibc implementation, wrt to My current state is that we probably never use the glibc implementation, because |
I've applied 7d05bc8 to fix the crypt_r detection, and instead added a |
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:
It looks like we aren't rejecting overly low rounds. |
I've addressed that discrepancy with 8a8c8d4. |
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. |
PHP contains a (presumably) correct, portable, and efficient implementation of `crypt_r` hashing, including all the possible hash algorithms which libc's `crypt_r` may possibly implement. Since this is so, why should we test for the platform `crypt_r` and only use our own implementation if it's not there? What benefit could there be from using libc's `crypt_r`? Maybe it might be faster? If so, PHP's implementation can be optimized. There is nothing "magic" which libc can do and which PHP can't do. If we didn't have a full implementation, it would make sense to take advantage of the one in libc, but since we do, this is just complicating things.
9732c4c
to
1b3c6b1
Compare
By the way, @nikic, thanks for finding those other problems with PHP's crypt_r. 👍 |
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 |
@nikic... So I guess I can close this PR? Do you think the docs for |
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 :) |
Here's the commit from packaging repo:
|
Thank you! Which repo is this? |
Looking forward to hearing your comments.