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

Conversation

geozak
Copy link
Contributor

@geozak geozak commented Jan 8, 2016

These are changes to address a potential attack method revealed in discussion within #776

These changes make it so that if an attack were to try at guessing a user's reset password verification hash they would only have one chance to do so.

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. either of the following depending on if there are any attempts remaining or not
    5.a. make expirePasswordReset update number of attempts
    5.b make expirePasswordReset update the timestamp to make it expired

Added messages to support changes in PasswordResetModel::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
@panique
Copy link
Owner

panique commented Jan 8, 2016

hi thanks, can you please add the database fields to the SQL files, so it works 100% without manual editing ? THanks a lot, cool feature!

@geozak
Copy link
Contributor Author

geozak commented Jan 9, 2016

What should the default max number of attempts be?

Added field in database user table for counting number of attempts made to reset the their password with the current code.
update database install file
@panique
Copy link
Owner

panique commented Jan 9, 2016

Hi, sorry maybe I've missed something but for me this is totally broken, the field "user_password_reset_attempts" is not used in your code. A big thanks for committing cool features, but it's get complicated when features are incomplete / unuseable, so please let's finish this feature first before going on here!

@geozak
Copy link
Contributor Author

geozak commented Jan 9, 2016

I haven't finished applying the changes with using the database like you asked.
I went to hang out with friends so I pushed the code online.
I'm going to sleep now but I probably won't have time to finish this until Sunday.
So if you want cherry pick or roll back the commits that change the database before then that's cool. Or even if you finish from the outline in pull description for this extra is cool too.

Removing DB from this branch so that it is in a completed state.
@geozak
Copy link
Contributor Author

geozak commented Jan 16, 2016

I have removed the DB changes that way this branch is atleast complete to solving the issue.
I copied the DB changes into a new branch to continue working on the improved implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants