From b1e3d09533fc613e74b86a1c8fd750ed4a839dab Mon Sep 17 00:00:00 2001 From: Hypolite Petovan Date: Fri, 19 Jan 2018 22:27:31 -0500 Subject: [PATCH 1/8] Fix formatting mod/lostpass --- mod/lostpass.php | 150 +++++++++++++++++++---------------------------- 1 file changed, 61 insertions(+), 89 deletions(-) diff --git a/mod/lostpass.php b/mod/lostpass.php index 3f480ea0f3..5934556a8e 100644 --- a/mod/lostpass.php +++ b/mod/lostpass.php @@ -1,47 +1,41 @@ $pwdreset_token], ['uid' => $user['uid']]); + if ($result) { + info(t('Password reset request issued. Check your email.') . EOL); + } $sitename = $a->config['sitename']; - $resetlink = System::baseUrl() . '/lostpass?verify=' . $new_password; + $resetlink = System::baseUrl() . '/lostpass?verify=' . $pwdreset_token; $preamble = deindent(t(' Dear %1$s, @@ -53,7 +47,7 @@ function lostpass_post(App $a) { provided and ignore and/or delete this email. Your password will not be changed unless we can verify that you - issued this request.')); + issued this request.', $user['username'], $sitename)); $body = deindent(t(' Follow this link to verify your identity: @@ -65,74 +59,58 @@ function lostpass_post(App $a) { The login details are as follows: Site Location: %2$s - Login Name: %3$s')); - - $preamble = sprintf($preamble, $username, $sitename); - $body = sprintf($body, $resetlink, System::baseUrl(), $email); + Login Name: %3$s', $resetlink, System::baseUrl(), $user['email'])); notification([ - 'type' => SYSTEM_EMAIL, - 'to_email' => $email, - 'subject'=> sprintf( t('Password reset requested at %s'),$sitename), - 'preamble'=> $preamble, - 'body' => $body]); + 'type' => SYSTEM_EMAIL, + 'to_email' => $user['email'], + 'subject' => t('Password reset requested at %s', $sitename), + 'preamble' => $preamble, + 'body' => $body + ]); goaway(System::baseUrl()); - } +function lostpass_content(App $a) +{ + $o = ''; + if (x($_GET, 'verify')) { + $pwdreset_token = $_GET['verify']; -function lostpass_content(App $a) { - - - if(x($_GET,'verify')) { - $verify = $_GET['verify']; - $hash = hash('whirlpool', $verify); - - $r = q("SELECT * FROM `user` WHERE `pwdreset` = '%s' LIMIT 1", - dbesc($hash) - ); - if (! DBM::is_result($r)) { - $o = t("Request could not be verified. \x28You may have previously submitted it.\x29 Password reset failed."); + $user = dba::selectFirst('user', ['uid', 'username', 'email'], ['pwdreset' => $pwdreset_token]); + if (!DBM::is_result($user)) { + $o = t("Request could not be verified. \x28You may have previously submitted it.\x29 Password reset failed."); return $o; } - $uid = $r[0]['uid']; - $username = $r[0]['username']; - $email = $r[0]['email']; - $new_password = autoname(6) . mt_rand(100,9999); - $new_password_encoded = hash('whirlpool',$new_password); + $new_password = autoname(6) . mt_rand(100, 9999); + $new_password_encoded = hash('whirlpool', $new_password); - $r = q("UPDATE `user` SET `password` = '%s', `pwdreset` = '' WHERE `uid` = %d", - dbesc($new_password_encoded), - intval($uid) - ); - - /// @TODO Is DBM::is_result() okay here? - if ($r) { + $result = dba::update('user', ['password' => $new_password_encoded, 'pwdreset' => ''], ['uid' => $user['uid']]); + if (DBM::is_result($result)) { $tpl = get_markup_template('pwdreset.tpl'); - $o .= replace_macros($tpl,[ - '$lbl1' => t('Password Reset'), - '$lbl2' => t('Your password has been reset as requested.'), - '$lbl3' => t('Your new password is'), - '$lbl4' => t('Save or copy your new password - and then'), - '$lbl5' => '' . t('click here to login') . '.', - '$lbl6' => t('Your password may be changed from the Settings page after successful login.'), + $o .= replace_macros($tpl, + [ + '$lbl1' => t('Password Reset'), + '$lbl2' => t('Your password has been reset as requested.'), + '$lbl3' => t('Your new password is'), + '$lbl4' => t('Save or copy your new password - and then'), + '$lbl5' => '' . t('click here to login') . '.', + '$lbl6' => t('Your password may be changed from the Settings page after successful login.'), '$newpass' => $new_password, '$baseurl' => System::baseUrl() - ]); - info("Your password has been reset." . EOL); + info("Your password has been reset." . EOL); $sitename = $a->config['sitename']; - // $username, $email, $new_password $preamble = deindent(t(' Dear %1$s, Your password has been changed as requested. Please retain this information for your records (or change your password immediately to something that you will remember). - ')); + ', $user['username'])); $body = deindent(t(' Your login details are as follows: @@ -141,33 +119,27 @@ function lostpass_content(App $a) { Password: %3$s You may change that password from your account settings page after logging in. - ')); - - $preamble = sprintf($preamble, $username); - $body = sprintf($body, System::baseUrl(), $email, $new_password); + ', System::baseUrl(), $user['email'], $new_password)); notification([ - 'type' => SYSTEM_EMAIL, - 'to_email' => $email, - 'subject'=> sprintf( t('Your password has been changed at %s'),$sitename), - 'preamble'=> $preamble, - 'body' => $body]); + 'type' => SYSTEM_EMAIL, + 'to_email' => $user['email'], + 'subject' => t('Your password has been changed at %s', $sitename), + 'preamble' => $preamble, + 'body' => $body + ]); return $o; } - - } - else { + } else { $tpl = get_markup_template('lostpass.tpl'); - - $o .= replace_macros($tpl,[ - '$title' => t('Forgot your Password?'), - '$desc' => t('Enter your email address and submit to have your password reset. Then check your email for further instructions.'), - '$name' => t('Nickname or Email: '), + $o .= replace_macros($tpl, [ + '$title' => t('Forgot your Password?'), + '$desc' => t('Enter your email address and submit to have your password reset. Then check your email for further instructions.'), + '$name' => t('Nickname or Email: '), '$submit' => t('Reset') ]); return $o; } - } From 209c43ebbc5300133f3d72158c5cdd6e088a4882 Mon Sep 17 00:00:00 2001 From: Hypolite Petovan Date: Fri, 19 Jan 2018 22:49:06 -0500 Subject: [PATCH 2/8] Centralize password hashing in Model\User --- mod/lostpass.php | 7 +++---- mod/settings.php | 13 +++++------- src/Model/User.php | 52 +++++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 57 insertions(+), 15 deletions(-) diff --git a/mod/lostpass.php b/mod/lostpass.php index 5934556a8e..abe67f2de2 100644 --- a/mod/lostpass.php +++ b/mod/lostpass.php @@ -7,6 +7,7 @@ use Friendica\App; use Friendica\Core\System; use Friendica\Database\DBM; +use Friendica\Model\User; require_once 'include/boot.php'; require_once 'include/enotify.php'; @@ -84,10 +85,8 @@ function lostpass_content(App $a) return $o; } - $new_password = autoname(6) . mt_rand(100, 9999); - $new_password_encoded = hash('whirlpool', $new_password); - - $result = dba::update('user', ['password' => $new_password_encoded, 'pwdreset' => ''], ['uid' => $user['uid']]); + $new_password = User::generateNewPassword(); + $result = User::updatePassword($user['uid'], $new_password); if (DBM::is_result($result)) { $tpl = get_markup_template('pwdreset.tpl'); $o .= replace_macros($tpl, diff --git a/mod/settings.php b/mod/settings.php index a5a4d4ad50..5193c4a046 100644 --- a/mod/settings.php +++ b/mod/settings.php @@ -2,14 +2,15 @@ /** * @file mod/settings.php */ + use Friendica\App; use Friendica\Content\Feature; use Friendica\Content\Nav; use Friendica\Core\Addon; -use Friendica\Core\System; -use Friendica\Core\Worker; use Friendica\Core\Config; use Friendica\Core\PConfig; +use Friendica\Core\System; +use Friendica\Core\Worker; use Friendica\Database\DBM; use Friendica\Model\GContact; use Friendica\Model\Group; @@ -391,12 +392,8 @@ function settings_post(App $a) } if (!$err) { - $password = hash('whirlpool', $newpass); - $r = q("UPDATE `user` SET `password` = '%s' WHERE `uid` = %d", - dbesc($password), - intval(local_user()) - ); - if (DBM::is_result($r)) { + $result = User::updatePassword(local_user(), $newpass); + if (DBM::is_result($result)) { info(t('Password changed.') . EOL); } else { notice(t('Password update failed. Please try again.') . EOL); diff --git a/src/Model/User.php b/src/Model/User.php index 862a9d4084..0979c2275d 100644 --- a/src/Model/User.php +++ b/src/Model/User.php @@ -142,7 +142,7 @@ class User return false; } - $password_hashed = hash('whirlpool', $password); + $password_hashed = self::hashPassword($password); if ($password_hashed !== $user['password']) { return false; @@ -151,6 +151,52 @@ class User return $user['uid']; } + /** + * Generates a human-readable random password + * + * @return string + */ + public static function generateNewPassword() + { + return autoname(6) . mt_rand(100, 9999); + } + + /** + * Global user password hashing function + * + * @param string $password + * @return string + */ + private static function hashPassword($password) + { + return hash('whirlpool', $password); + } + + /** + * Updates a user row with a new plaintext password + * + * @param int $uid + * @param string $password + * @return bool + */ + public static function updatePassword($uid, $password) + { + return self::updatePasswordHashed($uid, self::hashPassword($password)); + } + + /** + * Updates a user row with a new hashed password. + * Empties the password reset token field just in case. + * + * @param int $uid + * @param string $pasword_hashed + * @return bool + */ + private static function updatePasswordHashed($uid, $pasword_hashed) + { + return dba::update('user', ['password' => $pasword_hashed, 'pwdreset' => ''], ['uid' => $uid]); + } + /** * @brief Catch-all user creation function * @@ -290,8 +336,8 @@ class User throw new Exception(t('Nickname is already registered. Please choose another.')); } - $new_password = strlen($password) ? $password : autoname(6) . mt_rand(100, 9999); - $new_password_encoded = hash('whirlpool', $new_password); + $new_password = strlen($password) ? $password : User::generateNewPassword(); + $new_password_encoded = self::hashPassword($new_password); $return['password'] = $new_password; From 391c5913227c7f62f19b4f08906b0b1b0b618b33 Mon Sep 17 00:00:00 2001 From: Hypolite Petovan Date: Sat, 20 Jan 2018 18:06:58 -0500 Subject: [PATCH 3/8] Add pwdreset_time field to user table --- boot.php | 2 +- src/Database/DBStructure.php | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/boot.php b/boot.php index 776cf49fa8..4d2eac35a3 100644 --- a/boot.php +++ b/boot.php @@ -41,7 +41,7 @@ define('FRIENDICA_PLATFORM', 'Friendica'); define('FRIENDICA_CODENAME', 'Asparagus'); define('FRIENDICA_VERSION', '3.6-dev'); define('DFRN_PROTOCOL_VERSION', '2.23'); -define('DB_UPDATE_VERSION', 1242); +define('DB_UPDATE_VERSION', 1243); define('NEW_UPDATE_ROUTINE_VERSION', 1170); /** diff --git a/src/Database/DBStructure.php b/src/Database/DBStructure.php index 3f4614b6c7..cb69b49525 100644 --- a/src/Database/DBStructure.php +++ b/src/Database/DBStructure.php @@ -1733,7 +1733,8 @@ class DBStructure { "page-flags" => ["type" => "tinyint", "not null" => "1", "default" => "0", "comment" => ""], "account-type" => ["type" => "tinyint", "not null" => "1", "default" => "0", "comment" => ""], "prvnets" => ["type" => "boolean", "not null" => "1", "default" => "0", "comment" => ""], - "pwdreset" => ["type" => "varchar(255)", "not null" => "1", "default" => "", "comment" => ""], + "pwdreset" => ["type" => "varchar(255)", "comment" => "Password reset request token"], + "pwdreset_time" => ["type" => "datetime", "comment" => "Timestamp of the last password reset request"], "maxreq" => ["type" => "int", "not null" => "1", "default" => "10", "comment" => ""], "expire" => ["type" => "int", "not null" => "1", "default" => "0", "comment" => ""], "account_removed" => ["type" => "boolean", "not null" => "1", "default" => "0", "comment" => ""], @@ -1783,6 +1784,6 @@ class DBStructure { ] ]; - return($database); + return $database; } } From 0888f51b4b4e24ff1968b3a6dc474971c934b2b0 Mon Sep 17 00:00:00 2001 From: Hypolite Petovan Date: Sat, 20 Jan 2018 18:15:55 -0500 Subject: [PATCH 4/8] Add password reset request expiration - Change password reset link to /lostpass/[pwdreset] --- mod/lostpass.php | 148 +++++++++++++++++++++++++++------------------ src/Model/User.php | 7 ++- 2 files changed, 95 insertions(+), 60 deletions(-) diff --git a/mod/lostpass.php b/mod/lostpass.php index abe67f2de2..900e129b70 100644 --- a/mod/lostpass.php +++ b/mod/lostpass.php @@ -9,7 +9,8 @@ use Friendica\Core\System; use Friendica\Database\DBM; use Friendica\Model\User; -require_once 'include/boot.php'; +require_once 'boot.php'; +require_once 'include/datetime.php'; require_once 'include/enotify.php'; require_once 'include/text.php'; require_once 'include/pgettext.php'; @@ -30,13 +31,17 @@ function lostpass_post(App $a) $pwdreset_token = autoname(12) . mt_rand(1000, 9999); - $result = dba::update('user', ['pwdreset' => $pwdreset_token], ['uid' => $user['uid']]); + $fields = [ + 'pwdreset' => $pwdreset_token, + 'pwdreset_time' => datetime_convert() + ]; + $result = dba::update('user', $fields, ['uid' => $user['uid']]); if ($result) { info(t('Password reset request issued. Check your email.') . EOL); } $sitename = $a->config['sitename']; - $resetlink = System::baseUrl() . '/lostpass?verify=' . $pwdreset_token; + $resetlink = System::baseUrl() . '/lostpass/' . $pwdreset_token; $preamble = deindent(t(' Dear %1$s, @@ -76,69 +81,94 @@ function lostpass_post(App $a) function lostpass_content(App $a) { $o = ''; - if (x($_GET, 'verify')) { - $pwdreset_token = $_GET['verify']; + if ($a->argc > 1) { + $pwdreset_token = $a->argv[1]; - $user = dba::selectFirst('user', ['uid', 'username', 'email'], ['pwdreset' => $pwdreset_token]); + $user = dba::selectFirst('user', ['uid', 'username', 'email', 'pwdreset_time'], ['pwdreset' => $pwdreset_token]); if (!DBM::is_result($user)) { - $o = t("Request could not be verified. \x28You may have previously submitted it.\x29 Password reset failed."); - return $o; + notice(t("Request could not be verified. \x28You may have previously submitted it.\x29 Password reset failed.")); + + return lostpass_form(); } - $new_password = User::generateNewPassword(); - $result = User::updatePassword($user['uid'], $new_password); - if (DBM::is_result($result)) { - $tpl = get_markup_template('pwdreset.tpl'); - $o .= replace_macros($tpl, - [ - '$lbl1' => t('Password Reset'), - '$lbl2' => t('Your password has been reset as requested.'), - '$lbl3' => t('Your new password is'), - '$lbl4' => t('Save or copy your new password - and then'), - '$lbl5' => '' . t('click here to login') . '.', - '$lbl6' => t('Your password may be changed from the Settings page after successful login.'), - '$newpass' => $new_password, - '$baseurl' => System::baseUrl() - ]); + // Password reset requests expire in 20 minutes + if ($user['pwdreset_time'] < datetime_convert('UTC', 'UTC', 'now - 20 minutes')) { + $fields = [ + 'pwdreset' => null, + 'pwdreset_time' => null + ]; + dba::update('user', $fields, ['uid' => $user['uid']]); - info("Your password has been reset." . EOL); + notice(t('Request has expired, please make a new one.')); - $sitename = $a->config['sitename']; - $preamble = deindent(t(' - Dear %1$s, - Your password has been changed as requested. Please retain this - information for your records (or change your password immediately to - something that you will remember). - ', $user['username'])); - $body = deindent(t(' - Your login details are as follows: - - Site Location: %1$s - Login Name: %2$s - Password: %3$s - - You may change that password from your account settings page after logging in. - ', System::baseUrl(), $user['email'], $new_password)); - - notification([ - 'type' => SYSTEM_EMAIL, - 'to_email' => $user['email'], - 'subject' => t('Your password has been changed at %s', $sitename), - 'preamble' => $preamble, - 'body' => $body - ]); - - return $o; + return lostpass_form(); } + + return lostpass_generate_password($user); } else { - $tpl = get_markup_template('lostpass.tpl'); - $o .= replace_macros($tpl, [ - '$title' => t('Forgot your Password?'), - '$desc' => t('Enter your email address and submit to have your password reset. Then check your email for further instructions.'), - '$name' => t('Nickname or Email: '), - '$submit' => t('Reset') - ]); - - return $o; + return lostpass_form(); } } + +function lostpass_form() +{ + $tpl = get_markup_template('lostpass.tpl'); + $o = replace_macros($tpl, [ + '$title' => t('Forgot your Password?'), + '$desc' => t('Enter your email address and submit to have your password reset. Then check your email for further instructions.'), + '$name' => t('Nickname or Email: '), + '$submit' => t('Reset') + ]); + + return $o; +} + +function lostpass_generate_password($user) +{ + $o = ''; + + $new_password = User::generateNewPassword(); + $result = User::updatePassword($user['uid'], $new_password); + if (DBM::is_result($result)) { + $tpl = get_markup_template('pwdreset.tpl'); + $o .= replace_macros($tpl, [ + '$lbl1' => t('Password Reset'), + '$lbl2' => t('Your password has been reset as requested.'), + '$lbl3' => t('Your new password is'), + '$lbl4' => t('Save or copy your new password - and then'), + '$lbl5' => '' . t('click here to login') . '.', + '$lbl6' => t('Your password may be changed from the Settings page after successful login.'), + '$newpass' => $new_password, + '$baseurl' => System::baseUrl() + ]); + + info("Your password has been reset." . EOL); + + $sitename = $a->config['sitename']; + $preamble = deindent(t(' + Dear %1$s, + Your password has been changed as requested. Please retain this + information for your records (or change your password immediately to + something that you will remember). + ', $user['username'])); + $body = deindent(t(' + Your login details are as follows: + + Site Location: %1$s + Login Name: %2$s + Password: %3$s + + You may change that password from your account settings page after logging in. + ', System::baseUrl(), $user['email'], $new_password)); + + notification([ + 'type' => SYSTEM_EMAIL, + 'to_email' => $user['email'], + 'subject' => t('Your password has been changed at %s', $sitename), + 'preamble' => $preamble, + 'body' => $body + ]); + } + + return $o; +} diff --git a/src/Model/User.php b/src/Model/User.php index 0979c2275d..382ec62cc2 100644 --- a/src/Model/User.php +++ b/src/Model/User.php @@ -194,7 +194,12 @@ class User */ private static function updatePasswordHashed($uid, $pasword_hashed) { - return dba::update('user', ['password' => $pasword_hashed, 'pwdreset' => ''], ['uid' => $uid]); + $fields = [ + 'password' => $pasword_hashed, + 'pwdreset' => null, + 'pwdreset_time' => null + ]; + return dba::update('user', $fields, ['uid' => $uid]); } /** From 0bacff3994f333084fab71e9ccc04a63b12d5a96 Mon Sep 17 00:00:00 2001 From: Hypolite Petovan Date: Sat, 20 Jan 2018 19:09:40 -0500 Subject: [PATCH 5/8] Update database.sql with user.pwdreset_time field --- database.sql | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/database.sql b/database.sql index b55353994a..054bcd9a17 100644 --- a/database.sql +++ b/database.sql @@ -1,6 +1,6 @@ -- ------------------------------------------ -- Friendica 3.6-dev (Asparagus) --- DB_UPDATE_VERSION 1242 +-- DB_UPDATE_VERSION 1243 -- ------------------------------------------ @@ -1031,7 +1031,8 @@ CREATE TABLE IF NOT EXISTS `user` ( `page-flags` tinyint NOT NULL DEFAULT 0 COMMENT '', `account-type` tinyint NOT NULL DEFAULT 0 COMMENT '', `prvnets` boolean NOT NULL DEFAULT '0' COMMENT '', - `pwdreset` varchar(255) NOT NULL DEFAULT '' COMMENT '', + `pwdreset` varchar(255) COMMENT 'Password reset request token', + `pwdreset_time` datetime COMMENT 'Timestamp of the last password reset request', `maxreq` int NOT NULL DEFAULT 10 COMMENT '', `expire` int NOT NULL DEFAULT 0 COMMENT '', `account_removed` boolean NOT NULL DEFAULT '0' COMMENT '', From ed6f4cdbde338169e629e95799a4e0c180fc6303 Mon Sep 17 00:00:00 2001 From: Hypolite Petovan Date: Sat, 20 Jan 2018 19:12:14 -0500 Subject: [PATCH 6/8] Change password reset wording to acknowledge the expiration --- mod/lostpass.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mod/lostpass.php b/mod/lostpass.php index 900e129b70..3ac1164ef2 100644 --- a/mod/lostpass.php +++ b/mod/lostpass.php @@ -50,12 +50,12 @@ function lostpass_post(App $a) below or paste it into your web browser address bar. If you did NOT request this change, please DO NOT follow the link - provided and ignore and/or delete this email. + provided and ignore and/or delete this email, the request will expire shortly. Your password will not be changed unless we can verify that you issued this request.', $user['username'], $sitename)); $body = deindent(t(' - Follow this link to verify your identity: + Follow this link soon to verify your identity: %1$s From 071b1c038addc94178eff22a47f0bf126b4a9d28 Mon Sep 17 00:00:00 2001 From: Hypolite Petovan Date: Sat, 20 Jan 2018 19:14:41 -0500 Subject: [PATCH 7/8] Update expiration delay to 60 minutes --- mod/lostpass.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mod/lostpass.php b/mod/lostpass.php index 3ac1164ef2..c8254d6ca3 100644 --- a/mod/lostpass.php +++ b/mod/lostpass.php @@ -92,7 +92,7 @@ function lostpass_content(App $a) } // Password reset requests expire in 20 minutes - if ($user['pwdreset_time'] < datetime_convert('UTC', 'UTC', 'now - 20 minutes')) { + if ($user['pwdreset_time'] < datetime_convert('UTC', 'UTC', 'now - 1 hour')) { $fields = [ 'pwdreset' => null, 'pwdreset_time' => null From a827522f530b11972141d8bf3069e2ae5911efb4 Mon Sep 17 00:00:00 2001 From: Hypolite Petovan Date: Sat, 20 Jan 2018 19:15:05 -0500 Subject: [PATCH 8/8] Update comment with updated expiration time --- mod/lostpass.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mod/lostpass.php b/mod/lostpass.php index c8254d6ca3..e29295a714 100644 --- a/mod/lostpass.php +++ b/mod/lostpass.php @@ -91,7 +91,7 @@ function lostpass_content(App $a) return lostpass_form(); } - // Password reset requests expire in 20 minutes + // Password reset requests expire in 60 minutes if ($user['pwdreset_time'] < datetime_convert('UTC', 'UTC', 'now - 1 hour')) { $fields = [ 'pwdreset' => null,