crypt-blowfish salt is too short

[photodude]

Pull Request for Issue #12427.

Summary of Changes

• Fixes the crypt-blowfish salt creation format for when salt is not supplied. See getSalt for crypt-blowfish is too short for php7+ and hhvm #12427 for details
• Increases the allowable length of salt, when salt is supplied
• Adjusts the unit test to account for the new expected CryptedPassword length with crypt-blowfish and no salt is supplied (i.e. one is auto generated)
• BC-related issues for old passwords are shown not to be an issue by new regression tests

Testing Instructions

merge be code review

Review Travis - all tests pass and HHVM no longer has a failure with JUserHelperTest::testGetCryptedPassword

Documentation Changes Required

none

[photodude]

I'm assuming the DroneIO failure here is unrelated to this PR, TravisCI passes as expected.

cc/ @yvesh

[Hackwar]

Considering that the method is being removed in 4.0 and I'm also not fully sure that this works securely, I would not change this and instead extend the documentation that this method is just here for backwards compatibility and should NOT be used.

[photodude]

@Hackwar I wouldn't skip making this change when 4.0 is still a ways off and there will be some level of support for 3.7 for some time since it will be released soon.
I believe as of 3.2.1 hashPassword() and verifyPassword() are our recommended, where getCryptedPassword() and getSalt() are primarily needed for BC to our old encryption.

If that is correct, I do agree that the method descriptions should also be updated for getCryptedPassword() and getSalt() to reflect that they are the old encryption system.

As for "secure", "password_hash() is compatible with crypt() therefore, password hashes created by crypt() can be used with password_hash()." I did find some statements from 2007 that indicated an issue with low iterations of crypt-blowfish, increasing the cost factor to greater than or equal to 10 should be sufficient.

[mbabker]

Any change made here needs to come with sufficient regression tests. A password generated pre-patch should still validate correctly post-patch. If we can't guarantee that I would suggest not merging the patch because it'd be too risky.

[photodude]

@mbabker do you have any suggestions on how to setup sufficient regression tests for pre-patch passwords?

[mbabker]

Well here's where it gets tricky because I don't know if we had a standardized API for validating passwords generated with getCryptedPassword(). If they are $2y prefixed hashes though presumably calling JUserHelper::verifyPassword() with the plain text password and a hash generated pre-patch would work correctly.

[Hackwar]

afaik that was one of the problems, that the passwords generated by getCryptedPassword() were in fact neither secure nor properly verifyable. Again: Mark the methods as dangerous and only present for backwards compatibility and look forward to 4.0, where this has been removed.

[photodude]

The new hashes from this patch are $2y$10 prefixed when using crypt-blowfish, I believe that is the same as those hashed with the newer hashPassword(). I based on things I've read, as long as the encryption for getCryptedPassword() is crypt-blowfish there shouldn't be an issue as "password_hash() is compatible with crypt() therefore, password hashes created by crypt() can be used with password_hash()." If one of the other encryption options for getCryptedPassword() is used that's where things get questionable or dangerous (plaintext anyone) ...

[photodude]

@mbabker I believe to effectively test sufficient regression against pre-patch passwords the passwords would need to be generated by a version of Joomla prior to 3.2.1 when hashPassword() and verifyPassword() were implemented. Any version from 3.2.1 on would be using the new encryption methods, and this patch would only affect upgrades from versions<=3.2.0 (and possibly any third party extensions still using the old methods).

[photodude]

@mbabker Looking at the code I'm questioning the need for sufficient regression tests for pre-patch passwords here.

For an existing password with an existing crypt-blowfish salt and password the code should follow the following path

• verifyPassword L328
  • existing hash is used with prefix $2$
  • elseif ($hash[0] == '$') L343
    • password_verify L347
    • password_needs_rehash L350

In other words these changes should only affect the unit test or a case where a 3rd party developer generates a new password using the old system with crypt-blowfish and doesn't provide a salt (thus using the fixed default salt for generating a hash with prefix $2y$10).

[mbabker]

Either way it needs to be validated that aside from the changes introduced by this patch, nothing else breaks. Considering we're talking about password generation here, that's important. Yes, I get core is not using these methods, but anyone relying on them in the configuration affected by this change needs to be assured they aren't going to see issues upgrading.

[photodude]

If someone can provide a file with some mocked pre-patch passwords from Joomla prior to 3.2.1 when hashPassword() and verifyPassword() were implemented and a few from post 3.2.2-3.7 I can see about creating a unit test that will implement tests for the passwords to validate the passwords. (a unit test, that as pointed out, we should already have)

[Hackwar]

I would still strongly suggest to not modify this code anymore, except for marking ti as highly dangerous and to not use it. It is not the right code to use and we've just kept this because of pseudo-B/C. Considering the fuck-up that the implementation of this code was, we should have simply removed it and declard 3.2.0 an alpha version or something like that... Don't push up the value of broken code.

[photodude]

@Hackwar I am not trying to push up the value of old code not used on core and removed in the next version with this change. This should be only seen as a fix to the tests not as an encuragement to use these changed methods. I'm Only trying to get the HHVM tests to pass so we can legitimately encourage people to live test sites on HHVM with 3.7 alpha (and we are 2 PR's away from that happening).

[mbabker]

Deprecated or not, dangerous or not, it's in the API so it should be functional and valid to the extent that the API was designed. Imagine if we rejected bug fixes to JError, JObject, or JRequest simply because they have deprecated tags.

[photodude]

@mbabker I've reviewed the prepatch user password creation in 2.5.28

user passwords in j2.5 are created with $array['password'] = JUserHelper::hashPassword($array['password']);

I believe means j2.5 was using phpass (until the change in 3.2.1)

We can add a new item to the tests for verifying this prepatch mock password but it's not generated with the section modified here which means it's not validating this change.
I'm also finding it hard to find a normal condition in the Joomla core for a password to be generated with the changes made here prepatch or postpatch.

[photodude]

@mbabker following the existing tests I have added a single test line for a pre-patch crypt-blowfish without salt Password to show that an existing hash that was generated via crypt-blowfish without salt pre-patch is not affected.

It's a good thing that actual user passwords prior to J3.2.1 were using phpass since verify password would fail to verify a pre-patch crypt-blowfish without salt Password

[photodude]

@mbabker it looks like any pre-patch use of hashes generated via crypt-blowfish without salt and verified using crypt() or our getCryptedPassword() are going to fail on HHVM. It appears to be failing on hhvm for the same reasons that crypt-blowfish returns *0 rather than hash on HHVM with a $2$ prefix and no rounds specified resulting in fallback to DES

[photodude]

@mbabker after doing some digging, I found how to replicate the password hash creation used before 3.2.1 and #2683 implemented phpass and native PHP password_hash() Does this satisfy the regression test requirement?

[photodude]

@rdeutz @wilsonge, bump for review and merge consideration

[photodude]

Can we tag this with 3.7.0?

[brianteeman]

We usually only tag stuff when they are RTC On 7 Dec 2016 10:24 p.m., "Walt Sorensen" <notifications@github.com> wrote: Can we tag this with 3.7.0? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#12428 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABPH8WrNIksvcdJ9tCIV93mC-sp68JIVks5rFzI2gaJpZM4KX4Vw> .

[jeckodevelopment]

@photodude it needs at least two tests

[photodude]

@brianteeman That doesn't add up with the history of the version tags I've seen or the current list of 41 PR's with the Joomla-3.7 tag. Of the 41 open PR's with that tag 18 are not RTC. Generally, I've always seen these milestone tags applied to PR's to help ensure they are considered for a specific release.

[Bakual]

I've always seen these milestone tags applied to PR's to help ensure they are considered for a specific release.

If it is deemed important enough we sometimes add the tag prior to RTC.
Honestly, this PR isn't such a case. It's a nice to have but certainly not a must have.

[wilsonge]

As this is no longer used and we have a unit test I'm happy