Skip to content
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

switch to libxcrypt implementation when all algo available #7580

Closed
wants to merge 3 commits into from

Conversation

remicollet
Copy link
Member

@remicollet remicollet commented Oct 15, 2021

For discussion:

@nikic can you please have a look, some changes coming from you in 7d05bc8

Using libxcrypt could also allow to add more algo, especially yescrypt which mays have interest for password hashing
(also used by pam in some modern linux distro)

ext/standard/config.m4 Outdated Show resolved Hide resolved
@nikic
Copy link
Member

nikic commented Oct 15, 2021

Would it make sense to use our own implementation for algorithms we support and fall back to system implementation if we don't? This will avoid behavior differences.

@remicollet
Copy link
Member Author

remicollet commented Oct 15, 2021

Would it make sense to use our own implementation for algorithms we support and fall back to system implementation if we don't? This will avoid behavior differences.

Not for "security guys", who prefer using a well known / audited / reviewed implementation ;)

@remicollet
Copy link
Member Author

3rd commit fix the PHP_CRYPT_R_STYLE function in build/php.m4 which was broken and breaks ZTS builds

@remicollet
Copy link
Member Author

seems ext/standard/tests/strings/bug50052.phpt is failing on some libcrypt.so variant... at least the one used in CI... :(

@nikic
Copy link
Member

nikic commented Oct 15, 2021

FreeBSD CI has even more failures:

Test DES with invalid fallback [ext/standard/tests/crypt/des_fallback_invalid_salt.phpt]
Official blowfish tests (http://cvsweb.openwall.com/cgi/cvsweb.cgi/Owl/packages/glibc/crypt_blowfish/wrapper.c) [ext/standard/tests/strings/crypt_blowfish.phpt]
crypt() SHA-256 [ext/standard/tests/strings/crypt_sha256.phpt]
crypt() SHA-512 [ext/standard/tests/strings/crypt_sha512.phpt]

This is exactly what I'm concerned about. There are behavior differences between crypt implementations.

@remicollet
Copy link
Member Author

Will work back later on this, perhaps with a new build flag --enable-system-libcryp-yes-I-know-this-may-have-bad-side-effects as at least in work on some distribution (probably the ones using libxcrypt instead of old libcrypt)

At least this allow to find an issue, fixed separately in pr #7582

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants