Accept DES hashes

[weirdan]

Native password_verify() does accept old insecure DES hashes (https://3v4l.org/hKl4X). This pull request re-enables verifying (but not creating) them.

[GrahamCampbell]

👎

[weirdan]

Reasons?

[GrahamCampbell]

There is no good reason to add it.

[ircmaxell]

While this may technically be a regression, I'm hesitant to pull this in for 2 reasons:

1. All versions of PHP that require this library are EOL, meaning this library is considered maintenance mode only. I really want to avoid functionality changes, and only really want to include security fixes.
2. This functionality is undocumented. While you can use password_verify() to verify crypt() hashes, that is undocumented behavior. The documented behavior is to verify hashes created from password_hash(). Meaning that this isn't a significant regression since it doesn't affect documented behavior.

So while I'm not outright saying no, I do not think this is significant enough to justify the change. I'm 100% open to hearing arguments though.

[weirdan]

All versions of PHP that require this library are EOL

They may be EOL to PHP core developers, but they are still prevalent in the wild according to http://w3techs.com/technologies/details/pl-php/5/all (linked from http://php.net/usage.php). So developers still have to support them.

This functionality is undocumented.

I guess it depends on what you consider to be documentation. RFC that you authored clearly says: "bool password_verify($password, $hash) - The function which verifies an existing hash. This hash can be created via password_hash(), or a normal crypt() hash".

Those counterpoints aside, for a password checking function it's equally important what it accepts and what it rejects. This library falls back automatically to native functions. Suppose someone relies on DES hashes being rejected and later upgrades to a PHP version having native password_verify(). Suddenly accounts with DES hashes that were effectively locked out before the upgrade are allowed to log in, without any warning.

I'd imagine one may say that merging this pull request may affect people using the library much the same way I described above. But in this case it would be developers who upgrade the library as opposed to hosting companies forcing the upgrade onto their users eventually. Developers are more likely to read the changelog for the library they upgrade and take appropriate actions.

[smxi]

I have to also second part of what weirdan says: These versions of php are very widespread. We run a VPS that runs a LTR of Ubuntu, there are also many such Redhat based machines out there. I have no idea when our system will get upgraded, it could be years. PHP version 5.3.29. So it's not really very relevant what PHP version is EOL or not in my opinion, for example, in our case, your library will let me do decent password encryption, on what is a fairly new VPS instance, relatively speaking. Thanks a lot for maintaining this library by the way.

This isn't addressing the technical issues raised here, just the question of realworld PHP versions. Ubuntu 12.04 precise in our case, that's only 3 years old.

php --version
PHP 5.3.29 (cli) (built: Sep 1 2014 02:02:35)
Copyright (c) 1997-2014 The PHP Group
Zend Engine v2.3.0, Copyright (c) 1998-2014 Zend Technologies

As you can see, this was updated only a little over 1 year ago, so it can hardly be considered out of date or obsolete.

I maintain a PHP library, since 2003 or so (browser_detection.inc), it requires PHP 4.2 or greater, heh. Breaking anything in any version of PHP that it supports would be a serious bug, which would require immediate correction since it's something that should always 'just work' no matter what system you drop it into. I don't even consider PHP version EOL status when I make decisions on updates to that library beyond making sure I'm not using something not supported in older php versions. I assume people are running it on any supported PHP version, period. I'll probably boost the suggested PHP version to 5.0 at some point in the future just because I honestly can't even remember what the differences are anymore between 4 and 5.

[waerloga]

This library's point is to enhance security. If this patch reduces security then it should be disallowed regardless of how much "backwards compatibility" it breaks.

[paragonie-scott]

I'd say 👎 on this, and also suggest to write an RFC PHP 7.1 to block non-bcrypt and non-Argon2i hashes to address the undefined behavior.

Secure cryptography is, by definition, necessarily incompatible with insecure cryptography. Otherwise, you invite downgrade attacks and it ceases to be secure.

If you really want, RFC a password_legacy_verify() that allows DES, SHA256Crypt, etc. hashes. But let's keep moving forward.

So developers still have to support them.

No, we don't!

Many choose to, but there is no obligation.

For example, I'm releasing a project soon that is pinned to a minimum PHP version of 7.0.0 and requires a PECL extension (libsodium). If your system prevents you from using it, then your options are limited, but mine are not.

I have no idea when our system will get upgraded, it could be years.

If it is years, your server's security is comparable to lacy swiss. According to Verizon's Data Breach Investigation Reports, the most common cause for data breaches is outdated software.

[ieatkillerbees]

tbh, sounds more like a candidate for removal from native password_verify()

Secure crypto systems should not have non-secure options.

[smxi]

The system gets security patches, it's a standard LTR/LTS type thing, same as anything else out there, redhat, Debian Stable etc. There's a lot of such instances out there. Those have an EOL when they stop getting security updates, they are also known as 'stable systems'. Maybe you aren't familiar with this concept, can't say, but it's quite normal. The time of security updates from next stable release varies widely too, centos/redhat, I think it's quite a long time, Debian is about 1 year past next stable. This is the support for Ubuntu LTS, for example. It's similar to Redhat/CentOS.
https://wiki.ubuntu.com/LTS
Ubuntu 12.04 gets support til the end of 2017, ie, as I said, it could be years. Probably will be.

Again, no comment about the technical issues of this, just the reality of EOL. Obviously, if something you make requires new features, you peg it to support the version that has those new features, however, that's not relevant to this particular library because it's purpose is specifically to emulate something later PHP versions have. Thanks again to the author/maintainer for doing that.

[paragonie-scott]

And shame on those Linux distros for not upgrading people to newer versions of PHP after a certain period of time, too!

If you want to put forth the effort to support unsupported versions of PHP, you are of course free to do so. But forcing your deficiencies on everyone who is able to keep their systems up to date isn't fair, and is needlessly regressive.

I can't in good conscience support this.

[smxi]

The definition of a stable LTR/LTS distro is that it doesn't change, this is how it's always been. There's no shame or non shame involved, for example. Just stable servers. I'm not sure you're addressing this to me, but it's useful to know how reality works when considering such issues.

It's hard to know who paragonie-scott is speaking too however. There's nothing deficient involved in the process of stable servers, for example, when 12-04 was released, the new version of PHP was still too new to consider for a stable server, so they used the last 5.3 version, exactly as they should. That version has been getting consistent updates, as it should. And that server has a quite standard life of about 5 years support. This is all totally industry standard, nothing unusual or weird about it, or deficient in any way. Usually depends on the freeze date of the package pool the server is made from, which is about 6 months give or take before the actual LTR/LTS release date. That's how it is, always has been, and I assume always will be. That gets slightly annoying if your server version was released right before a next major version update of some software or other, but it's not a big deal in any larger sense, for example, the only downside for me was having to find this library to implement this one bit of missing functionality. This is by the way also one of the best run hosting companies in the world.

[devjack]

I did a round up of php versions in the wild: Based on these figures:

• 5.3.x (> 5.3.7) is roughly 25.44% of installs in the wild:
• 5.4.x (any version) is roughly 30.80%

So this has a potential reach of 56.24% of PHP installs. Sadly I'd consider it optimistic thinking to see this reduce dramatically in the near future.

• Debian 7 EOL's 5.4 in 2018
• CentOS 5.11 EOL's 5.1 in 2017
• CentOS 6.7 EOL's 5.3 in 2020
• CentOS 7 EOL's 5.4 in 2024
• Ubuntu 12.04 EOLs 5.3 in 2017

Some of these EOL's are speculative... I'm working through the data now for a follow up post

Note that OS support for the version is primarily on functionality and stability. Security fixes are (usually) backported - but not for userland code like this

The fact of the matter is people who can't or won't upgrade their OS can still benefit from improved security via this userland backport.

Given we're explicitly dealing with legacy systems here I sit in the mindset of 'how can we make this less bad'. A blanket 'just upgrade' approach whilst ideal isn't hugely practical in most environments.

if it's accepted, it should at a bare minimum notify the client code that the hash is insecure via an exception or error (deprecated perhaps?).

I imagine it could look like:

try {
    $result = password_verify($password, $hash);
} catch (\InsecureHashException $e) {
   // trigger application logic to rehash password
}

My preference would be for an exception with a minor version bump, but an error thats written to logs etc. could be equally beneficial.

[weirdan]

This library's point is to enhance security.

Actually it says 'This library is intended to provide forward compatibility with the password_* functions that ship with PHP 5.5.' right there in the library description. It doesn't say it aims to be more secure than native alternative. In fact, this library silently falls back to native functions if they are available. Is that a downgrade vulnerability then?

tbh, sounds more like a candidate for removal from native password_verify()

Maybe. But then discussion of such removal should probably continue at bugs.php.net .

If you want to put forth the effort to support unsupported versions of PHP, you are of course free to do so. But forcing your deficiencies on everyone who is able to keep their systems up to date isn't fair, and is needlessly regressive.

People using this library, by definition, are still using old PHP versions. If you use PHP >= 5.5 you're using native password_* functions, so this pull request does not affect you at all.

I wonder if people opposing the change actually noticed that it only changes password_verify() function. password_hash() won't suddenly start generating DES hashes. So the only situation you would be affected by this change is if you already have DES hashes. Do you? And if you do, what are your upgrade plans?

[paragonie-scott]

I wonder if people opposing the change actually noticed that it only changes password_verify() function. password_hash() won't suddenly start generating DES hashes. So the only situation you would be affected by this change is if you already have DES hashes. Do you? And if you do, what are your upgrade plans?

If I had DES hashes, I'd just do what NeoThermic described here.

A DES hash is morally equivalent to an MD5 hash. If I suddenly switch to password_compat, then it's my responsibility to upgrade the legacy hashes to use bcrypt.

[paragonie-scott]

Footnote: There might be some value in documenting password migration strategies. I've had to do this for two different clients and had to negotiate two completely different outcomes.

[weirdan]

@paragonie-scott I mean php upgrade, not the stored hashes upgrade.

If I suddenly switch to password_compat, then it's my responsibility to upgrade the legacy hashes to use bcrypt.

Then you end up with no DES hashes and, thus, are not affected by this change. Anyway, if you somehow switched to password_compat, your security, by your own account, is comparable to lacy swiss because you're using outdated unsupported version of PHP (and, probably, other software as well).

[paragonie-scott]

Anyway, if you somehow switched to password_compat, your security, by your own account, is comparable to lacy swiss because you're using outdated unsupported version of PHP (and, probably, other software as well).

Luckily, I run 5.6 and 7.0, so that's not an issue.

---

We're getting off topic. Since password_verify() will accept a DES hash in 5.5.0, a true compatibility library must mirror the behavior, so this patch isn't harmful.

However, in the larger scheme of things, they should be rejected in a future version of PHP. That's a discussion for another day, but if you want to merge it @ircmaxell I'll withdraw my 👎.

[weirdan]

@devjack

My preference would be for an exception with a minor version bump, but an error thats written to logs etc. could be equally beneficial.

I'm totally ok with an error message, even if native function does not have it, since password_compat's errors are already different to native's. Throwing an exception, on the other hand, would effectively change public interface, which IMO defeats the point of a compat library. I'll add the message if @ircmaxell is ok with this.

[Toflar]

I can see valid arguments for both sides. I think it's perfectly valid to say that this is a forward BC library and should behave the same as the native functions because that's essentially the point of a forward BC library, isn't it? So to be really compatible, you'd have to merge this PR.
However, it's also perfectly valid to be hesitant here because we're talking about a security thing and why would one not have an uneasy feeling about merging something that actually feels wrong in the first place?

I just wanted to chip in another option that hasn't been mentioned here yet: Can we maybe make this configurable to this library only? Something like define('PASSWORD_COMPAT_LIB_VERIFY_DES', true) which would support @weirdan's use case and when updating to a newer PHP version and dropping the password_compat dependency will not affect your project and will just leave an orphan nonsense constant in the wild unless the developer is smart enough to remove it. I have to admit I really, really don't fancy this solution much because it's a global constant (which it has to be because otherwise you'd have a dependency on the library and could not simply remove it when updating your PHP version) and overall just feels a bit clunky. But as I stated before, I can see valid arguments for both sides and just the same as in politics what you usually do in such a case is come up with a compromise that both sides can work/live with :-) So we now have three options: merge, reject and make configurable somehow.
Again: Not sure if this is the way to go, I just wanted to spotlight that making it configurable would be an option too.

[weirdan]

The problem with configuration switch, as I see it, is that it's not in php core, so when you upgrade it gets ignored. Though this is something Anthony, being the author of native password_* functions, can change. And if he does, it could as well be a php.ini switch. This could also serve as a depreciation mechanism should there be a need to stop verifying other hashing methods ($1$, $2a$ comes to mind). Like 'warn - swich off - remove' process over the course of several php releases.

[carnage]

If using this lib properly it should accept all hashes that crypt can generate and return true for password needs rehash so that legacy passwords can be migrated.

As far as I am aware the sha 256 and sha 512 hashes from crypt are still considered secure so these should still be accepted as well

[weirdan]

@carnage we're talking about DES hashes here. Other crypt()-generated hashes like those you mentioned are not affected by this change.

[narfbg]

IMO, if it's a compatibility library, it should be aiming for 100% compatibility. Plus, I believe that allowing it to validate (but not produce) older crypt() hashes makes migration to the new API easier.

But these arguments about OS LTS releases are just silly ... Sure, we can't ignore Ubuntu and RedHat because of their sheer size, but they shouldn't be able to dictate how everything else works. If the PHP group says PHP 5.3 is EOL, it's EOL. Period.

[weirdan]

But these arguments about OS LTS releases are just silly

When I was talking about older releases my point was that currently 100% of people using this library are on EOL'd releases. So either you support them, or you have essentially zero auditory, in which case the library itself should probably be EOL'd.

Discussing merits of LTS vs current software belongs somewhere else, that much is true.

[zburnham]

Arguments over the morality and/or practicality of supporting EOL versions of php in this forum is a waste of time. Ask any developer, more than likely they will want to do any work required to be compatible with the current version.

Unfortunately it is rarely our decision. I mean, were hired because we understand the topic and are qualified to make those decisions, but for some reason, the people who make the decision can barely sign into Facebook. Marketing, sales, executive management, ultimately they control what is running on the server which runs their business. These guys are probably the least qualified people to make this decision, but there it is. I am willing to bet that more than one of you out there have added "upgrade php" to the Agile board.. and have seen that ticket get super-glued to the bottom of the list in favor of changing the shade of red on the homepage. That is certainly what happened at my last job.

The fix for this? 1) Let the technical people make technical decisions. 2) Stop hiring know-nothing MBAs that shouldn't be let anywhere near a computer.

[weirdan]

It's been a month since last comment, so it seems everyone who cared has already voiced their opinions. @ircmaxell, isn't it a good time to make the decision?