From 129f6806f6c77622b295f08e03ebb3a4fbecc7d8 Mon Sep 17 00:00:00 2001 From: Alexandre Alapetite Date: Sun, 8 Apr 2018 12:28:04 +0200 Subject: [PATCH 1/6] Fix update password rehash Fixes https://github.com/friendica/friendica/issues/4743 The logic for updating password was wrong: https://github.com/friendica/friendica/commit/b0a764b14c2f2798f7eb223e58d47530f80609c1#diff-1466bb1a0a37fe9f7cf52eda8f3b431aR150 --- src/Model/User.php | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Model/User.php b/src/Model/User.php index 4ae43c3e11..abf4d1d3e4 100644 --- a/src/Model/User.php +++ b/src/Model/User.php @@ -127,18 +127,18 @@ class User { $user = self::getAuthenticationInfo($user_info); - if ($user['legacy_password']) { - if (password_verify(self::hashPasswordLegacy($password), $user['password'])) { - self::updatePassword($user['uid'], $password); - - return $user['uid']; - } - } elseif (password_verify($password, $user['password'])) { + if (password_verify($password, $user['password'])) { if (password_needs_rehash($user['password'], PASSWORD_DEFAULT)) { self::updatePassword($user['uid'], $password); } return $user['uid']; + } elseif (!empty($user['legacy_password']) || strpos($user['password'], '$') === false) { + if (self::hashPasswordLegacy($password) === $user['password']) { + self::updatePassword($user['uid'], $password); + + return $user['uid']; + } } throw new Exception(L10n::t('Login failed')); From cb26cd6d5d5d1a34baf0ce48afd9bbb971b9b4e6 Mon Sep 17 00:00:00 2001 From: Alexandre Alapetite Date: Sun, 8 Apr 2018 14:42:18 +0200 Subject: [PATCH 2/6] Remove legacy_password test --- src/Model/User.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Model/User.php b/src/Model/User.php index abf4d1d3e4..ef495a45c2 100644 --- a/src/Model/User.php +++ b/src/Model/User.php @@ -133,7 +133,7 @@ class User } return $user['uid']; - } elseif (!empty($user['legacy_password']) || strpos($user['password'], '$') === false) { + } elseif (strpos($user['password'], '$') === false) { if (self::hashPasswordLegacy($password) === $user['password']) { self::updatePassword($user['uid'], $password); From 82f1f2f00e4493c3d1d4ff1df9161cc0957defee Mon Sep 17 00:00:00 2001 From: Alexandre Alapetite Date: Sun, 8 Apr 2018 14:53:12 +0200 Subject: [PATCH 3/6] Remove SQL column legacy_password --- database.sql | 1 - src/Database/DBStructure.php | 1 - src/Model/User.php | 6 ++---- src/Util/ExAuth.php | 2 +- update.php | 7 ++----- 5 files changed, 5 insertions(+), 12 deletions(-) diff --git a/database.sql b/database.sql index aa87247db3..c4b93e2873 100644 --- a/database.sql +++ b/database.sql @@ -1019,7 +1019,6 @@ CREATE TABLE IF NOT EXISTS `user` ( `guid` varchar(64) NOT NULL DEFAULT '' COMMENT '', `username` varchar(255) NOT NULL DEFAULT '' COMMENT '', `password` varchar(255) NOT NULL DEFAULT '' COMMENT '', - `legacy_password` boolean NOT NULL DEFAULT '0' COMMENT 'Is the password hash double-hashed?', `nickname` varchar(255) NOT NULL DEFAULT '' COMMENT '', `email` varchar(255) NOT NULL DEFAULT '' COMMENT '', `openid` varchar(255) NOT NULL DEFAULT '' COMMENT '', diff --git a/src/Database/DBStructure.php b/src/Database/DBStructure.php index 67c8d7b8a6..275d9562bb 100644 --- a/src/Database/DBStructure.php +++ b/src/Database/DBStructure.php @@ -1726,7 +1726,6 @@ class DBStructure "guid" => ["type" => "varchar(64)", "not null" => "1", "default" => "", "comment" => ""], "username" => ["type" => "varchar(255)", "not null" => "1", "default" => "", "comment" => ""], "password" => ["type" => "varchar(255)", "not null" => "1", "default" => "", "comment" => ""], - "legacy_password" => ["type" => "boolean", "not null" => "1", "default" => "0", "comment" => "Is the password hash double-hashed?"], "nickname" => ["type" => "varchar(255)", "not null" => "1", "default" => "", "comment" => ""], "email" => ["type" => "varchar(255)", "not null" => "1", "default" => "", "comment" => ""], "openid" => ["type" => "varchar(255)", "not null" => "1", "default" => "", "comment" => ""], diff --git a/src/Model/User.php b/src/Model/User.php index ef495a45c2..6178906aa2 100644 --- a/src/Model/User.php +++ b/src/Model/User.php @@ -170,13 +170,12 @@ class User if (!isset($user['uid']) || !isset($user['password']) - || !isset($user['legacy_password']) ) { throw new Exception(L10n::t('Not enough information to authenticate')); } } elseif (is_int($user_info) || is_string($user_info)) { if (is_int($user_info)) { - $user = dba::selectFirst('user', ['uid', 'password', 'legacy_password'], + $user = dba::selectFirst('user', ['uid', 'password'], [ 'uid' => $user_info, 'blocked' => 0, @@ -186,7 +185,7 @@ class User ] ); } else { - $user = dba::fetch_first('SELECT `uid`, `password`, `legacy_password` + $user = dba::fetch_first('SELECT `uid`, `password` FROM `user` WHERE (`email` = ? OR `username` = ? OR `nickname` = ?) AND `blocked` = 0 @@ -277,7 +276,6 @@ class User 'password' => $pasword_hashed, 'pwdreset' => null, 'pwdreset_time' => null, - 'legacy_password' => false ]; return dba::update('user', $fields, ['uid' => $uid]); } diff --git a/src/Util/ExAuth.php b/src/Util/ExAuth.php index d4436e32af..cdf663b42c 100644 --- a/src/Util/ExAuth.php +++ b/src/Util/ExAuth.php @@ -226,7 +226,7 @@ class ExAuth if ($a->get_hostname() == $aCommand[2]) { $this->writeLog(LOG_INFO, 'internal auth for ' . $sUser . '@' . $aCommand[2]); - $aUser = dba::selectFirst('user', ['uid', 'password', 'legacy_password'], ['nickname' => $sUser]); + $aUser = dba::selectFirst('user', ['uid', 'password'], ['nickname' => $sUser]); if (DBM::is_result($aUser)) { $uid = $aUser['uid']; $success = User::authenticate($aUser, $aCommand[3]); diff --git a/update.php b/update.php index bc14b3a29f..0cbc0302fd 100644 --- a/update.php +++ b/update.php @@ -149,12 +149,9 @@ function update_1203() { } function update_1244() { - // Sets legacy_password for all legacy hashes - dba::update('user', ['legacy_password' => true], ['SUBSTR(password, 1, 4) != "$2y$"']); - // All legacy hashes are re-hashed using the new secure hashing function - $stmt = dba::select('user', ['uid', 'password'], ['legacy_password' => true]); - while($user = dba::fetch($stmt)) { + $stmt = dba::select('user', ['uid', 'password'], ['password NOT LIKE "$%"']); + while ($user = dba::fetch($stmt)) { dba::update('user', ['password' => User::hashPassword($user['password'])], ['uid' => $user['uid']]); } From e860cdf6a8769c8441ed3dba33f192898c78ab40 Mon Sep 17 00:00:00 2001 From: Alexandre Alapetite Date: Sun, 8 Apr 2018 16:02:25 +0200 Subject: [PATCH 4/6] Swap if / elseif https://github.com/friendica/friendica/pull/4782#discussion_r179947984 --- src/Model/User.php | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Model/User.php b/src/Model/User.php index 6178906aa2..27f7ff66f7 100644 --- a/src/Model/User.php +++ b/src/Model/User.php @@ -127,18 +127,18 @@ class User { $user = self::getAuthenticationInfo($user_info); - if (password_verify($password, $user['password'])) { - if (password_needs_rehash($user['password'], PASSWORD_DEFAULT)) { - self::updatePassword($user['uid'], $password); - } - - return $user['uid']; - } elseif (strpos($user['password'], '$') === false) { + if (strpos($user['password'], '$') === false) { if (self::hashPasswordLegacy($password) === $user['password']) { self::updatePassword($user['uid'], $password); return $user['uid']; } + } elseif (password_verify($password, $user['password'])) { + if (password_needs_rehash($user['password'], PASSWORD_DEFAULT)) { + self::updatePassword($user['uid'], $password); + } + + return $user['uid']; } throw new Exception(L10n::t('Login failed')); From 991a3d959e658f4335ffb182d417e6edd3d8fcf4 Mon Sep 17 00:00:00 2001 From: Alexandre Alapetite Date: Sun, 15 Apr 2018 10:51:22 +0200 Subject: [PATCH 5/6] Revert "Remove SQL column legacy_password" This reverts commit 82f1f2f00e4493c3d1d4ff1df9161cc0957defee. --- database.sql | 1 + src/Database/DBStructure.php | 1 + src/Model/User.php | 6 ++++-- src/Util/ExAuth.php | 2 +- update.php | 7 +++++-- 5 files changed, 12 insertions(+), 5 deletions(-) diff --git a/database.sql b/database.sql index c4b93e2873..aa87247db3 100644 --- a/database.sql +++ b/database.sql @@ -1019,6 +1019,7 @@ CREATE TABLE IF NOT EXISTS `user` ( `guid` varchar(64) NOT NULL DEFAULT '' COMMENT '', `username` varchar(255) NOT NULL DEFAULT '' COMMENT '', `password` varchar(255) NOT NULL DEFAULT '' COMMENT '', + `legacy_password` boolean NOT NULL DEFAULT '0' COMMENT 'Is the password hash double-hashed?', `nickname` varchar(255) NOT NULL DEFAULT '' COMMENT '', `email` varchar(255) NOT NULL DEFAULT '' COMMENT '', `openid` varchar(255) NOT NULL DEFAULT '' COMMENT '', diff --git a/src/Database/DBStructure.php b/src/Database/DBStructure.php index 275d9562bb..67c8d7b8a6 100644 --- a/src/Database/DBStructure.php +++ b/src/Database/DBStructure.php @@ -1726,6 +1726,7 @@ class DBStructure "guid" => ["type" => "varchar(64)", "not null" => "1", "default" => "", "comment" => ""], "username" => ["type" => "varchar(255)", "not null" => "1", "default" => "", "comment" => ""], "password" => ["type" => "varchar(255)", "not null" => "1", "default" => "", "comment" => ""], + "legacy_password" => ["type" => "boolean", "not null" => "1", "default" => "0", "comment" => "Is the password hash double-hashed?"], "nickname" => ["type" => "varchar(255)", "not null" => "1", "default" => "", "comment" => ""], "email" => ["type" => "varchar(255)", "not null" => "1", "default" => "", "comment" => ""], "openid" => ["type" => "varchar(255)", "not null" => "1", "default" => "", "comment" => ""], diff --git a/src/Model/User.php b/src/Model/User.php index 27f7ff66f7..d66c73d7eb 100644 --- a/src/Model/User.php +++ b/src/Model/User.php @@ -170,12 +170,13 @@ class User if (!isset($user['uid']) || !isset($user['password']) + || !isset($user['legacy_password']) ) { throw new Exception(L10n::t('Not enough information to authenticate')); } } elseif (is_int($user_info) || is_string($user_info)) { if (is_int($user_info)) { - $user = dba::selectFirst('user', ['uid', 'password'], + $user = dba::selectFirst('user', ['uid', 'password', 'legacy_password'], [ 'uid' => $user_info, 'blocked' => 0, @@ -185,7 +186,7 @@ class User ] ); } else { - $user = dba::fetch_first('SELECT `uid`, `password` + $user = dba::fetch_first('SELECT `uid`, `password`, `legacy_password` FROM `user` WHERE (`email` = ? OR `username` = ? OR `nickname` = ?) AND `blocked` = 0 @@ -276,6 +277,7 @@ class User 'password' => $pasword_hashed, 'pwdreset' => null, 'pwdreset_time' => null, + 'legacy_password' => false ]; return dba::update('user', $fields, ['uid' => $uid]); } diff --git a/src/Util/ExAuth.php b/src/Util/ExAuth.php index cdf663b42c..d4436e32af 100644 --- a/src/Util/ExAuth.php +++ b/src/Util/ExAuth.php @@ -226,7 +226,7 @@ class ExAuth if ($a->get_hostname() == $aCommand[2]) { $this->writeLog(LOG_INFO, 'internal auth for ' . $sUser . '@' . $aCommand[2]); - $aUser = dba::selectFirst('user', ['uid', 'password'], ['nickname' => $sUser]); + $aUser = dba::selectFirst('user', ['uid', 'password', 'legacy_password'], ['nickname' => $sUser]); if (DBM::is_result($aUser)) { $uid = $aUser['uid']; $success = User::authenticate($aUser, $aCommand[3]); diff --git a/update.php b/update.php index 0cbc0302fd..bc14b3a29f 100644 --- a/update.php +++ b/update.php @@ -149,9 +149,12 @@ function update_1203() { } function update_1244() { + // Sets legacy_password for all legacy hashes + dba::update('user', ['legacy_password' => true], ['SUBSTR(password, 1, 4) != "$2y$"']); + // All legacy hashes are re-hashed using the new secure hashing function - $stmt = dba::select('user', ['uid', 'password'], ['password NOT LIKE "$%"']); - while ($user = dba::fetch($stmt)) { + $stmt = dba::select('user', ['uid', 'password'], ['legacy_password' => true]); + while($user = dba::fetch($stmt)) { dba::update('user', ['password' => User::hashPassword($user['password'])], ['uid' => $user['uid']]); } From 360e2e6342499cc2fc071cc8a8c1729ca3cd3460 Mon Sep 17 00:00:00 2001 From: Alexandre Alapetite Date: Sun, 15 Apr 2018 11:12:32 +0200 Subject: [PATCH 6/6] Revert removal of legacy_password column https://github.com/friendica/friendica/pull/4782#issuecomment-380978218 --- src/Model/User.php | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/Model/User.php b/src/Model/User.php index d66c73d7eb..2621897f4e 100644 --- a/src/Model/User.php +++ b/src/Model/User.php @@ -128,12 +128,22 @@ class User $user = self::getAuthenticationInfo($user_info); if (strpos($user['password'], '$') === false) { + //Legacy hash that has not been replaced by a new hash yet if (self::hashPasswordLegacy($password) === $user['password']) { self::updatePassword($user['uid'], $password); + return $user['uid']; + } + } elseif (!empty($user['legacy_password'])) { + //Legacy hash that has been double-hashed and not replaced by a new hash yet + //Warning: `legacy_password` is not necessary in sync with the content of `password` + if (password_verify(self::hashPasswordLegacy($password), $user['password'])) { + self::updatePassword($user['uid'], $password); + return $user['uid']; } } elseif (password_verify($password, $user['password'])) { + //New password hash if (password_needs_rehash($user['password'], PASSWORD_DEFAULT)) { self::updatePassword($user['uid'], $password); }