PHP version requirement

[tpetry]

You've stated that at least PHP 5.3.8 is needed because in PHP 5.3.7 has been a bug in crypt (#55439). But this bug has been introduced in PHP 5.3.7 and is fixed in PHP 5.3.8. The correct PHP version requirement would be to allow all PHP 5.3 versions except 5.3.7.

Setting this more correct version requirement makes it possible to use your PHP implementation of the PHP 5.5 password library with more PHP installations. Debian for example has PHP 5.3.3 and any installation with php of debian repo wouldn't be able to use the library.

[sandermarechal]

I too would like the requirement dropped.

The situation with Debian is a little different than @tpetry mentions. Debian has backported the fixes to PHP 5.3.3-7+squeeze4 (the current version in Debian is +squeeze-14). That said, PHP 5.3.3 works with this library. Please drop the requirement from your composer.json.

[philjohn]

Indeed. Server-oriented distros will tend to backport fixes, which makes simple version checks like this useless.

[adduc]

@tpetry, I believe that 5.3.7 is required not due to the bug in crypt, but rather due to the information provided at http://php.net/security/crypt_blowfish.php.

[sandermarechal]

@adduc Debian has backported all of that to it's PHP 5.3.3 version, including the new $2y$ hash from 5.3.7. This library works perfectly on Debian (and Ubuntu and probably a bunch more distro's that backported the fix). That is why we like the version check to be removed.

[gergoerdosi]

In that case I support the removal of the version check too. If we want to test something, then we should test the existence of $2y$, something like:

if (crypt('password', '$2y$01$salt$') == '*0') {
    trigger_error()

But maybe we should just let the application implement this check. @ircmaxell What's your opinion on this issue?

[ircmaxell]

Ok, so I've been thinking about this for a while, and here's my stance:

Running crypt every time the library is included is way too much overhead (since a composer install will load it on every page load). So that's right out from the start.

Requiring 5.3 won't work, because $2y$ wasn't introduced until 5.3.7... So lifting the version requirement is also out.

That leaves us with a bit of an issue. An issue that's directly attributable to the screwed up release processes of Debian. And yes, I call them screwed up because the second that they back ported a fix, the version ceased to be 5.3.3, and became their own branch. So calling it 5.3.3 is a huge misnomer. But that's besides this point.

I'm at a bit of a loss on to how to fix this. On one hand, removing the requirement would open significant issues for people which wouldn't be obvious (they install it on a non-debian 5.3.3). On the other, leaving it as is disables the library for debian users... So there's no really good solution that I can implement from my end.

Thoughts?

[tpetry]

My solution would be staying as compatible with old php versions as possible. I would only throw an exception while hashing when the crypt bug has been detected. This would make the api incompatible but would prevent horrible security problems.

[dossy]

How about ignoring the PHP version requirement entirely, and checking the result of crypt in password_hash and triggering an error if the return value indicates incompatibility?

[dossy]

Bonus points: make the triggered error message clear that it is due to an incompatible implementation of crypt.

[dossy]

IOW, duck-typing FTW.

[philjohn]

It's not just Debian, other long term support distros also backport (I would imagine CentOS/RHEL has the same issue, being officially pegged to 5.3.3 in release 6.3).

One option might be having users set a constant to signal that they don't wish to run the version check. Something like PASSWORD_COMPAT_NO_CHECK, and maybe offer a small test script to verify if their install of PHP has the vulnerable version of crypt_bcrypt?

[ircmaxell]

Here's what I'm thinking:

Remove the check at runtime completely. Leave the check in composer though...

I could theoretically create an installer for composer which would throw an exception if it's not supported on install... But that's not great either (there would be more code in the installer than the library itself)...

thoughts?

[Crell]

@ircmaxell For argument's sake... what's the value in putting a hard PHP requirement into Composer rather than just documenting "dude, this don't work in PHP version X.Y.Z"? If someone installs this library on 5.3.5, say, what exactly is the problem? Is it just "avoiding 5.3.7" that is the goal?

[ircmaxell]

@Crell: the issue is that it won't work in regular 5.3 at all. The $2y crypt format was added in 5.3.7. So to prevent people installing the library and winding up with bool: false in their database (because they didn't check the output), I put 5.3.7 as the required version...

[adduc]

Is there a specific call to crypt that would be different between < 5.3.7 vs > 5.3.7 to identify whether the $2y would be supported?

EDIT: Nevermind, read @gergoerdosi's comment earlier and tested it in 5.3.3 and 5.3.18 (only versions available to me at the moment) with two separate outputs consistent to what he was mentioning.

[ircmaxell]

@adduc Actually, that's a good point. I just tried this on 3v4l, and it looks like it might work... http://3v4l.org/8ugqB#tabs

On good versions, it throws the *0 error which says that it recognizes the config, but the salt is incorrect (there was an error), and no hash is actually executed (not precisely true, but close enough for our purposes here).

On bad versions, it treats it as a DES based hash, and runs a fast hash.

[dossy]

I still don't understand why you can't remove the version requirement and instead, just do:

    $ret = crypt($password, $hash);

    if (!is_string($ret) || strlen($ret) <= 13) {
        return false;
    }

    if (strncmp("$2y$", $ret, 4)) {
        trigger_error("password_hash(): crypt() function not compatible, see http://php.net/manual/en/password_compat.requirements.php", E_USER_ERROR);
    }

This eliminates the "the version number is a lie" problem, and eliminates the "don't waste CPU cycles checking for compatibility when the library is sourced" issue, and this blows up as it should on incompatible systems so that users won't silently think everything's working when it's not.

[adduc]

I'm not exactly well-versed in GitHub (or Git in general), but I was able to make a few repos for testing an installer class for this library.

The forked version of this repo with a fixed composer.json is at https://github.com/Adduc/password_compat, a version of the installer at https://github.com/Adduc/password_compat_installer, and a sample repo testing out the functionality at https://github.com/Adduc/password_compat_use

[Crell]

My thinking is along the lines of what dossy is suggesting. Basically, rather than version number detection in Composer, do feature detection in PHP. You can download the library, but if you try to run it on a known-broken system (or on a not-known-good system) it fails hard with an Exception. Not quite as nice as not letting you download it in the first place and getting your hopes up, but more accurate about when it will work.

[sandermarechal]

I agree with Crell. That also solves a potential problem where people use Composer to download this library on a machine with a good PHP version, but rsync or push all their code to a machine with a bad PHP version. The runtime detection (as opposed to install time detection) would warn the user about the issue.

[ircmaxell]

I just pushed 1.0.0, and in it I removed the run-time version check and the composer dependency.

Run version-test.php to determine if your php version is compatible. If it returns false, it's not. Now it's on you to determine the version correctly...

[ircmaxell]

I'm re-opening this, as I just ran a test against 5.3.3-debian on Squeeze (6.0.7), and it failed. It appears that $2y$ is not available on Debian Squeeze with a stock install of php5-cli...

If anyone can elaborate as to how it possibly could have worked, I'm all ears. The version-test.php file definitely fails.

If not, I am going to re-instate the version check, as there isn't a way to actually run it under 5.3.3-debian...

[mvl22]

I see at:
http://stackoverflow.com/questions/12459896/password-compat-for-older-php-version/12461336#12461336
you mention:

Now, what can you do if you're stuck on a lower version? You could fork the library and adjust $2y$ to $2a$. That will at least get you to work. Passwords generated in this manner will be portable with future versions (the library is designed to be able to verify older crypt() passwords).

Please excuse my lack of expert knowledge in this area, but is there scope to use $2a$ if the library detects at runtime that $2y$ is not present, so that people can at least start to use the library until Debian et al patch to support $2y$?

[phindmarsh]

We run PHP 5.3.3-7+squeeze14 with Suhosin-Patch on Debian and have the same problem ($2y$ returning false). The research I did indicates that the bug has indeed been backported, however the new 2y prefix has not been. My understanding was it is safe to use 2a since the bug is no longer present (from memory the specific backported patch can be found here).

I made an API breaking change to the library to accomodate this, adding a new constant PASSWORD_COMPAT_2A that when true switches to the 2a algorithm (see here). I don't necessarily think this is a nice solution, however I wasn't able to come up with a better one.

[ircmaxell]

@phindmarsh and @mvl22 My main concern here is that providing support to 2a which has known vulnerabilities against it the majority of installations. I feel the better alternative is to just educate people to upgrade to a better, more stable, and more secure version. If you're on Debian, switch to a DotDeb repo and install PHP from there. If you're on CentOS, switch to REMI. The point is the alternatives exist...

And I don't think it should be the role of a security library to compromise security for pretty much any reason...

[phindmarsh]

@ircmaxell agreed, I knew the implications of my changes so was happy to do so. My circumstances dictate that I can't update my php version at the moment, however when that changes I'll be reverting back to the original library.

From my (limited) understanding the Debian philosophy is to not introduce any new functionality in a security patch, hence why I assume the 2y prefix was not backported with the fix. I agree this is a dangerous edge case to leverage and I don't think it's the purpose of this library to accommodate that. As you said, education is a better solution here.

[tchalvak]

Just to throw in a little... ...additional impetus to this issue, the Debian Version 6.0 with backports from dot-deb seems to have halted at:
php --version
PHP 5.3.6-6~dotdeb.0 with Suhosin-Patch (cli) (built: Apr 17 2011 12:27:20)
Copyright (c) 1997-2011 The PHP Group
Zend Engine v2.3.0, Copyright (c) 1998-2011 Zend Technologies

I believe this may render the library completely incompatible... ...would be nice if there were a way around that?

Admittedly, 6.0 is now just beyond it's support period as well, but I expect it's very prevalent on servers on the web.

[Adirelle]

Even though Debian didn't backported the $2y$ prefix to their 5.3.3 package, they did add $2x$, that supports the flawed version. So you can test if the $2a$ prefix is secure using this:

$always_flawed = crypt('password_with_utf8_éèçù', '$2x$04$0123456789012345678901$');
$maybe_flawed = crypt('password_with_utf8_éèçù', '$2a$04$0123456789012345678901$');
if($maybe_flawed === $always_flawed) {
  trigger_error('flawed $2a$');
}

However, this would only work for debian 5.3.3 packages, as $2x$ is probably not available before 5.3.7 in other distros.

[apmuthu]

# php -v
PHP 5.3.3-7+squeeze17 with Suhosin-Patch (cli) (built: Aug 26 2013 07:26:12)
Copyright (c) 1997-2009 The PHP Group
Zend Engine v2.3.0, Copyright (c) 1998-2010 Zend Technologies

[hkdobrev]

I think a require directive for "php": ">=5.3.7" is needed for the general public. The backporting and security patching of past PHP releases is branching off PHP.

IMHO having a security vulnerability before 5.3.7 is the common case and the other is the exception.

It is better to inform users of this library in any way possible they might be affected. If users know their distribution of PHP is fixed then they could install the library by just copying the file, adding a submodule or whatever.

It is not OK to leave this out only because a part of the users may not be affected. Constraints are always better when it comes to security.

[sarciszewski]

By the way, Debian moved to 5.4.x

https://packages.debian.org/wheezy/php5