Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent password reset brute force #778

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Added expirePasswordReset and altered verifyPasswordReset
Added a method named expirePasswordReset to be used with verifyPasswordReset to mark existing password reset requests as expired when it is detected that fake attempts are being used.

modified verifyPasswordReset in a few ways.
1. Made the query search for just username instead of the username/verification code combo
2. Added feedback for if user does not exist.
3. Added feedback for if verification code does not exist for selected user
4. Added feedback for if verification code does not match the selected users current code
    and calls the expirePasswordReset method to current code invalid

These changes assume that one bad guess at a verification code is an attempted attack. A better solution might include keeping a tally that way if  users click on old link by mistake they don't have to start the process over, but that probably requires using altering the database structure slightly and I don't know if that would be desired, but can be easily added into here if its.

Steps need incorporate a tally system:
1. Add field in database users table for number of attempts
2. Add value in config file for max number of attempts
3. Make setPasswordResetDatabaseToken set number of attempts to 0
4. make expirePasswordReset read the current number of attempts.
5.a. make expirePasswordReset update number of attempts
5.b  make expirePasswordReset update the timestamp to make it expired
  • Loading branch information
geozak committed Jan 8, 2016
commit 4ac1aa5fdaded0937335f6ff51fa1f349ce809da
60 changes: 52 additions & 8 deletions application/model/PasswordResetModel.php
Original file line number Diff line number Diff line change
Expand Up @@ -128,27 +128,40 @@ public static function verifyPasswordReset($user_name, $verification_code)
$database = DatabaseFactory::getFactory()->getConnection();

// check if user-provided username + verification code combination exists
$sql = "SELECT user_id, user_password_reset_timestamp
$sql = "SELECT user_id, user_password_reset_hash, user_password_reset_timestamp
FROM users
WHERE user_name = :user_name
AND user_password_reset_hash = :user_password_reset_hash
AND user_provider_type = :user_provider_type
LIMIT 1";
$query = $database->prepare($sql);
$query->execute(array(
':user_password_reset_hash' => $verification_code, ':user_name' => $user_name,
':user_name' => $user_name,
':user_provider_type' => 'DEFAULT'
));

// if this user with exactly this verification hash code does NOT exist
if ($query->rowCount() != 1) {
Session::add('feedback_negative', Text::get('FEEDBACK_PASSWORD_RESET_COMBINATION_DOES_NOT_EXIST'));
//if username does not exist
if($query->rowCount() != 1) {
Session::add('feedback_negative', Text::get('FEEDBACK_USER_DOES_NOT_EXIST'));
return false;
}

// get result row (as an object)
$result_user_row = $query->fetch();


// if this user's verification hash code does NOT exist
if ($result_user_row->user_password_reset_hash == NULL) {
Session::add('feedback_negative', Text::get('FEEDBACK_PASSWORD_RESET_TOKEN_INVALID'));
return false;
}

// if this user's verification hash code does not match
// marks existing request as expired
if ($result_user_row->user_password_reset_hash != $verification_code) {
Session::add('feedback_negative', Text::get('FEEDBACK_PASSWORD_RESET_TOKEN_INVALID'));
self::expirePasswordReset($user_name);
return false;
}

// 3600 seconds are 1 hour
$timestamp_one_hour_ago = time() - 3600;

Expand All @@ -162,6 +175,37 @@ public static function verifyPasswordReset($user_name, $verification_code)
return false;
}
}

/**
* Marks existing password reset request as expired to prevent prevent guessing hash codes
* @param string $user_name Username
* @return bool Success status
*/
private static function expirePasswordReset($user_name) {
// 3600 seconds are 1 hour
$timestamp_one_hour_ago = time() - 3600;

$database = DatabaseFactory::getFactory()->getConnection();

$sql = "UPDATE users
SET user_password_reset_timestamp = :user_password_reset_timestamp
WHERE user_name = :user_name AND user_provider_type = :provider_type LIMIT 1";
$query = $database->prepare($sql);
$query->execute(array(
':user_name' => $user_name,
':user_password_reset_timestamp' => $timestamp_one_hour_ago,
':provider_type' => 'DEFAULT'
));

// check if exactly one row was successfully changed
if ($query->rowCount() == 1) {
return true;
}

// user feedback not necessary here
// but adding a note in the logs may be useful
return false;
}

/**
* Writes the new password to the database
Expand Down