php / doc-en

English PHP documentation
479 stars 714 forks source link

crypt(): --with-external-libcrypt undocumented #1566

Closed martenlehmann closed 2 years ago

martenlehmann commented 2 years ago

Description

The following code:

<?php

printf(
    "%s\n%s\n",
    crypt('whatever', '$6$+=./CIKSWMxwyohL$'),
    crypt('whatever', '$6$+=.äöü😀bCI$')
);

Resulted in this output:

$6$+=./CIKSWMxwyohL$sb5N9Nb5Bt2qDbLbOQ9RtoHxhC1K8w7ijWOzmio17b1CMLcjBGG3m8amcWHCoXJB0G25aZ4uutgnxjKbliUbQ/
$6$+=.äöü😀bCI$Xe9mv1hkb03sKiDi6JRdRbd02i0tz/46XpLPHoLB1pq0qSZV7To.NWmQieb7mA4eZ.kZBmfuCRJYlpDgi59Hu0

But I expected an exception or *0 instead of this output, because both salts contain invalid characters.

Background:

In our setup, PHP is used to create a hash, that will be distributed to and consumed by different other services (OpenLDAP, Apache HTTP Server, Python Backends etc.). Therefore we cannot use the latest hashing functions or algorithms, but the highest common denominator, which is the OS's crypt() function with SHA512.

Up until (including) Ubuntu 18.04, the OS's crypt() supported salts including +=./. Starting Ubuntu 20.04, the special characters have been limited to ./. However, PHP doesn't seem to care about that.

What's so bad about this?

There is no way to tell - using PHP - that the salts and generated hashes are invalid and will cause verification problems in other systems. Even PHP's hash_equals() and password_verify() functions have zero issue when verifying these hashes against the original passwords. Unfortunately, all services consuming these hashes and relying on the system's crypt() for verification do.

In Python, the following code (using the first password and salt as above) will return *0:

import crypt
crypt.crypt('whatever', '$6$+=./CIKSWMxwyohL$')

So does Perl:

printf(
    "%s\n",
    crypt('whatever', '$6$+=./CIKSWMxwyohL$')
);

The passlib library for Python will even raise a more explicit ValueError exception: invalid characters in sha512_crypt salt

from passlib.hash import sha512_crypt
sha512_crypt.verify('whatever', '$6$+=./CIKSWMxwyohL$sb5N9Nb5Bt2qDbLbOQ9RtoHxhC1K8w7ijWOzmio17b1CMLcjBGG3m8amcWHCoXJB0G25aZ4uutgnxjKbliUbQ/')

passlib documents the allowed characters for the salt as follows: [./0-9A-Za-z] Perldoc on crypt() does it as well: [./0-9A-Za-z]

So I see two issues:

  1. As PHP doesn't provide a function to create a valid salt (like crypt.mksalt(crypt.METHOD_SHA512) in Python), this step relies on the creativity of the developer and since the allowed characters aren't well documented, there is a big chance that invalid characters will be part of the salt and thus the hash.
  2. As PHP doesn't seem to use the OS's crypt() and it's own implementation doesn't seem to validate the input as strict, there is a big chance this happens without noticing and unexpected errors in other systems will occur. A naive upgrade from Ubuntu 18.04 to 20.04 would have created tons of failed logins in our setup.

PHP Version

PHP 7.4.3

Operating System

Ubuntu 20.04

cmb69 commented 2 years ago

Well, PHP's implementation follows the informal specification; actually, the PHP implementation is closely based on the given implementation (it might be identical). And that specification does not apply base64-like encoding to the salt when producing the result string.

I think not base64-like encoding the salt is somewhat strange, but I don't think we can change that for BC reasons. Not sure what to do; maybe documenting the behavior explicitly is the best solution for now. That might require users to pass in a properly encoded salt, what obviously would reduce the entropy, but that may not be a problem in practice.

martenlehmann commented 2 years ago

There is surely a benefit of PHP's cross-platform implementation of crypt(), because otherwise this function wouldn't be available on non-UNIX platforms. In Python you would use the passlib password hashing library, which is not part of the standard library and the standard crypt module simply wouldn't work on eg. Windows.

What I'm missing in PHP, however, is a way to explicitly make use of and test against the OS's crypt() function when it is available.

cmb69 commented 2 years ago

What I'm missing in PHP, however, is a way to explicitly make use of and test against the OS's crypt() function when it is available.

As it is now, PHP uses system crypt if all algorithms and crypt_r() are supported.

martenlehmann commented 2 years ago

From a user perspective, if I install Ubuntu's php7.4 package, there's no way to tell which version of crypt() is being used or has been linked against at built-time.

Since Perl's and Python's crypt() functions independently yet both return *0 on invalid salts and PHP's crypt() in Ubuntu 20.04 doesn't, I'd assume it does not use the system crypt. Besides, the *0 return to mark an error doesn't seem well documented anywhere and has been pretty unexpected to me.

cmb69 commented 2 years ago

From a user perspective, if I install Ubuntu's php7.4 package, there's no way to tell which version of crypt() is being used or has been linked against at built-time.

It might be reasonable to expose this information somehow; maybe a constant (PHP_CRYPT_IMPL) or some entry in phpinfo().

Besides, the *0 return to mark an error doesn't seem well documented anywhere and has been pretty unexpected to me.

The documentation states:

Returns the hashed string or a string that is shorter than 13 characters and is guaranteed to differ from the salt on failure.

Since strlen("*0") < 13, this is covered by the documentation.

nikic commented 2 years ago

@cmb69 It's not obvious on older PHP versions, but PHP currently never uses system crypt. Historically this didn't happen due to a bug in the config.m4 detection, and now it's explicitly disabled until a decision to do otherwise is reached.

nikic commented 2 years ago

Actually ... we do support a --with-external-libcrypt option since https://github.com/php/php-src/pull/7584. Totally forgot about that, even though I reviewed it... It's not enabled automatically though.

cmb69 commented 2 years ago

Thanks, @nikic! So, as of PHP 8.1.0, there is --with-external-libcrypt, and previously, our own implementation would always be used. Since the config option can be seen in phpinfo()'s output, there is no need for another way to detect which crypt implementation is used. So, besides the actual issue of this ticket, we need to document the new --with-external-libcrypt.

nikic commented 2 years ago

As to the original issue, I'm not sure there's much we can do here -- the validation here is clearly a moving target for the system implementation, so if we adopted the stricter validation seen on Ubuntu 20.04 we would now fail to process hashes from Ubuntu 18.04. I think the only viable option here is to recommend users who care about interoperability with system crypt to build with --with-external-libcrypt, in which case we'd want to reclassify this as a documentation issue.

cmb69 commented 2 years ago

Reclassifying as documentation issue as per Nikita's comment above.