Replacing PHPass with password_compat #10219

th3fallen · 2014-08-06T16:08:07Z

ghost · 2014-08-06T16:12:05Z

Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/contribute/agreement/ Alternatively you can add a comment here stating that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

DeepDiver1975 · 2014-08-06T16:20:34Z

Phpass is used in some apps as well. Please grep for it. Thx

th3fallen · 2014-08-06T16:22:13Z

@DeepDiver1975 i was grepping for the filename i see the usages now i'll update them in one moment after reverting the composer update

DeepDiver1975 · 2014-08-06T17:57:48Z

@owncloud-bot this is okay to test

DeepDiver1975 · 2014-08-06T17:58:21Z

@karlitschek @craigpg looks like @th3fallen is not yet member of the owncloud orga here on github - can you please take care? THX

th3fallen · 2014-08-06T17:59:21Z

@DeepDiver1975 they are probably waiting until I start on Monday.

karlitschek · 2014-08-06T18:00:55Z

@th3fallen Check your email :-)

th3fallen · 2014-08-06T18:01:39Z

@karlitschek you work fast lol thanks.

DeepDiver1975 · 2014-08-07T09:27:29Z

@owncloud-bot retest this please

ghost · 2014-08-07T10:06:42Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/6573/

bantu · 2014-08-10T22:18:56Z

tests/lib/user.php

+			'91dvsoahmc' => '$2a$08$8hll38YeDb4osgZ6klqL3uUrBnnl4HOf1B1lJnjZuecaqvE.Nw3BS',
+		);
+
+		foreach ($passes as $key => $val) {


You probably should use dataProvider instead of a loop.

DeepDiver1975 · 2014-08-11T11:06:13Z

@th3fallen before this can be merged into master we need to submit the prs to the other repos - because of it touches enterprise code we shall merge this AFTER OC7EE release - THX

bantu · 2014-08-11T18:09:02Z

lib/private/user/database.php

+			if (password_verify($password, $storedHash)) {
+				// Check and see if our password needs to be rehashed
+				if (password_needs_rehash($storedHash, PASSWORD_BCRYPT)) {
+					password_hash($password, PASSWORD_BCRYPT);


This looks incomplete. You will have to actually put the updated password hash into the database. Even if the password is the same, the hash changes and needs to be updated.

$this->setPassword(); ?

th3fallen · 2014-08-21T14:38:14Z

this will also resolve #5150

Please do not merge until after OC7EE release

LukasReschke · 2014-08-21T14:39:36Z

apps/files_sharing/lib/helper.php

@@ -99,10 +98,7 @@ public static function authenticate($linkItem, $password) {
 		if ($password !== null) {
 			if ($linkItem['share_type'] == \OCP\Share::SHARE_TYPE_LINK) {
 				// Check Password
-				$forcePortable = (CRYPT_BLOWFISH != 1);
-				$hasher = new PasswordHash(8, $forcePortable);
-				if (!($hasher->CheckPassword($password.OC_Config::getValue('passwordsalt', ''),


This seems also to break old passwords.

You are correct, my generated passes in the test where incorrect i will correct this.

th3fallen · 2014-08-21T16:07:08Z

Just a note for myself as this is not the focus currently

fix legacy detection and acceptance
fix unit test to reflect
make hashing cost user configurable
push fixes

DeepDiver1975 · 2014-11-03T16:11:36Z

looks like there is a modernized version of phpass - https://github.com/hautelook/phpass - which adds composer support and is compliant with php 5.5 and php 5.6

@th3fallen @LukasReschke are there still needs to replace phpass with something else?

The effort to update phpass is for sure less then introducing ppassword-compat - THX

LukasReschke · 2014-11-03T16:21:10Z

Well, password_compat is nearly considered a standard library with thousands of users nowadays but the updated phpass version has not even 40 stars.

I'd still stick to a change to password_compat.

th3fallen · 2014-11-03T16:22:39Z

@DeepDiver1975 That version is still the same just namespaced, and restyled. It is still exposing the hash used for passwords and falls back to md5 generation for some configurations. I still think that password_compat is going to be more secure in the long run and more future proof.

DeepDiver1975 · 2014-11-03T16:28:35Z

@LukasReschke @th3fallen understood your points - but is there really a hard security issue with phpass? As stated above the password hash migration adds quite some risk.

LukasReschke · 2014-11-03T16:44:46Z

@LukasReschke @th3fallen understood your points - but is there really a hard security issue with phpass? As stated above the password hash migration adds quite some risk.

Well, in terms of security using phpass is completely fine. And even if it would have a vulnerability it would be very hard to abuse. (and if somebody has already access to the DB he can do worse things than cracking your password)

This is more about getting rid of unsupported and not really anylonger maintained software (it even has some hacks included to work with phpBB3 hashes....) and replacing it with a proper alternative that is even directly bundled with PHP. The password_compat lib is only required as fallback when an older PHP version is used.

I'd welcome a change to that because that is certainly a more future-proof approach and offers quite some advantages. Absolutely, migration requires proper testing and is not that easy but better be safe than sorry.

DeepDiver1975 · 2014-11-03T17:00:12Z

Thanks @LukasReschke - @th3fallen we will revive this pr these days - tech-dept attached

meanwhile I'll push phpass to composer to get rid of all require_once out there in the wild .....

THX

DeepDiver1975 · 2014-11-04T10:19:41Z

meanwhile I'll push phpass to composer to get rid of all require_once out there in the wild .....

We are reusing this class in many places - this is shit

@LukasReschke wasn't there an public api for hashing passwords in core? THX

LukasReschke · 2014-11-04T10:33:28Z

wasn't there an public api for hashing passwords in core? THX

No. - Also I'm not sure if it makes sense adding one if we anyways plan to switch to https://github.com/ircmaxell/password_compat

DeepDiver1975 · 2014-11-04T10:35:13Z

No. - Also I'm not sure if it makes sense adding one if we anyways plan to switch

well - the question is if we ever have to switch again or password_compat is changing their interface/deprecating a function in favor of another/... we have to change a dozen of locations again.

LukasReschke · 2014-11-04T10:38:52Z

well - the question is if we ever have to switch again or password_compat is changing their interface/deprecating a function in favor of another/... we have to change a dozen of locations again.

password_compat is reproducing the native PHP API. Which is simply password_hash($password, PASSWORD_DEFAULT) and password_verify($password, $hash). I don't think that moving this in their own function would make things easier at all. - We would still have to do the same migration work.

DeepDiver1975 · 2014-11-04T10:40:55Z

I see - but we should at least offer a migration function to handle old hashes vs new hashes - let me have a look at the code we have in here ...

DeepDiver1975 · 2014-11-04T11:01:21Z

apps/files_sharing/lib/connector/publicauth.php

-				} else {
-					return true;
-				}
+				return password_verify($password, $linkItem['share_with']);


How will this ever work? All hashes need to be migrated - as a result we need to ship password_compat AND phpass with the next major version and offer a proper migration pass for all apps as well.

DeepDiver1975 · 2014-11-04T11:09:48Z

After chatting with @LukasReschke we agreed on defining a public api in core which will encapsulate hashing functionalities. We will basically have two functions:

hash($password)
verify($hash, $password, &$newHash)

verify will return the newHash in case the existing hash know to be outdated (long time ago sha hashes or phpass hashes)

password salt and any other options will be taken care of within the implementation of verify()

I'm closing this PR now as we have to walk a different path

Public interface for hashing which also works with legacy ownCloud hashes and supports updating the legacy hash via a passed reference. Follow-up of #10219 (comment) Requires owncloud-archive/3rdparty#136

bantu reviewed Aug 10, 2014
View reviewed changes

bantu reviewed Aug 11, 2014
View reviewed changes

th3fallen added Enhancement and removed 5 - To review labels Aug 21, 2014

th3fallen added this to the ownCloud 8 milestone Aug 21, 2014

LukasReschke reviewed Aug 21, 2014
View reviewed changes

DeepDiver1975 added the technical debt label Nov 3, 2014

DeepDiver1975 reviewed Nov 4, 2014
View reviewed changes

DeepDiver1975 closed this Nov 4, 2014

LukasReschke mentioned this pull request Nov 4, 2014

Add OCP\Security\IHasher #11947

Merged

DeepDiver1975 modified the milestones: ownCloud 8, 8.0-current Jan 9, 2015

lock bot locked as resolved and limited conversation to collaborators Aug 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replacing PHPass with password_compat #10219

Replacing PHPass with password_compat #10219

th3fallen commented Aug 6, 2014

ghost commented Aug 6, 2014

DeepDiver1975 commented Aug 6, 2014

th3fallen commented Aug 6, 2014

DeepDiver1975 commented Aug 6, 2014

DeepDiver1975 commented Aug 6, 2014

th3fallen commented Aug 6, 2014

karlitschek commented Aug 6, 2014

th3fallen commented Aug 6, 2014

DeepDiver1975 commented Aug 7, 2014

ghost commented Aug 7, 2014

bantu Aug 10, 2014

DeepDiver1975 commented Aug 11, 2014

bantu Aug 11, 2014

th3fallen commented Aug 21, 2014

LukasReschke Aug 21, 2014

th3fallen Aug 21, 2014

th3fallen commented Aug 21, 2014

DeepDiver1975 commented Nov 3, 2014

LukasReschke commented Nov 3, 2014

th3fallen commented Nov 3, 2014

DeepDiver1975 commented Nov 3, 2014

LukasReschke commented Nov 3, 2014

DeepDiver1975 commented Nov 3, 2014

DeepDiver1975 commented Nov 4, 2014

LukasReschke commented Nov 4, 2014

DeepDiver1975 commented Nov 4, 2014

LukasReschke commented Nov 4, 2014

DeepDiver1975 commented Nov 4, 2014

DeepDiver1975 Nov 4, 2014

DeepDiver1975 commented Nov 4, 2014

Replacing PHPass with password_compat #10219

Replacing PHPass with password_compat #10219

Conversation

th3fallen commented Aug 6, 2014

ghost commented Aug 6, 2014

DeepDiver1975 commented Aug 6, 2014

th3fallen commented Aug 6, 2014

DeepDiver1975 commented Aug 6, 2014

DeepDiver1975 commented Aug 6, 2014

th3fallen commented Aug 6, 2014

karlitschek commented Aug 6, 2014

th3fallen commented Aug 6, 2014

DeepDiver1975 commented Aug 7, 2014

ghost commented Aug 7, 2014

bantu Aug 10, 2014

Choose a reason for hiding this comment

DeepDiver1975 commented Aug 11, 2014

bantu Aug 11, 2014

Choose a reason for hiding this comment

th3fallen commented Aug 21, 2014

LukasReschke Aug 21, 2014

Choose a reason for hiding this comment

th3fallen Aug 21, 2014

Choose a reason for hiding this comment

th3fallen commented Aug 21, 2014

DeepDiver1975 commented Nov 3, 2014

LukasReschke commented Nov 3, 2014

th3fallen commented Nov 3, 2014

DeepDiver1975 commented Nov 3, 2014

LukasReschke commented Nov 3, 2014

DeepDiver1975 commented Nov 3, 2014

DeepDiver1975 commented Nov 4, 2014

LukasReschke commented Nov 4, 2014

DeepDiver1975 commented Nov 4, 2014

LukasReschke commented Nov 4, 2014

DeepDiver1975 commented Nov 4, 2014

DeepDiver1975 Nov 4, 2014

Choose a reason for hiding this comment

DeepDiver1975 commented Nov 4, 2014