Background.

I know this is an 'old' issue, but all solutions I've seen are simply to recover the admin's password versus a regular user. Previous threads are as follows:
http://drupal.org/node/729064
http://drupal.org/node/1023440

Here's the situation. A user tries to login 5 times. They get blocked. They see a warning to simply try again later or request new password. They request said password and get a one time URL. They successfully login and change password. They logout and then log back in. They are still blocked by flood control.

Desired result.

Upon successful login via remote URL, remove a users record from flood control checking.

Steps to recreate:

  1. Download and install vanilla version of Drupal 7.10
  2. Create test user (non-admin).
  3. Try logging in 5 times with random passwords
  4. Go until you get the following error message: "Sorry, there have been more than 5 failed login attempts for this account. It is temporarily blocked. Try again later or request a new password."
  5. Request a new password and receive via email.
  6. Use one time URL, login, enter new password, save.
  7. Logout
  8. Try logging in again and see the error message again: "Sorry, there have been more than 5 failed login attempts for this account. It is temporarily blocked. Try again later or request a new password."

Motivation

For a normal user, this is surprising behavior. They did all the necessary steps and are still not able to access their account. Any attempts to access after a reset are merely adding more failed attempts to the flood control table and making it worse.

If there is a known solution, please send along. Thanks!

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rickmanelius’s picture

Actually I think a better outcome would be to remove any failed attempts upon successful reset of a password... because it's possible to use a one time login and not change that, leaving flood control as a viable option (particularly if the person registering the password reset wasn't actually in control of said email/account).

rickmanelius’s picture

in function user_login_final_validate($form, &$form_state) {

We see the following to clear a user's flood control

// Clear past failures for this user so as not to block a user who might
// log in and out more than once in an hour.
flood_clear_event('failed_login_attempt_user', $form_state['flood_control_user_identifier']);

However in the actual user edit form
user_account_form

I don't see where the password submission is occurring. Upon successfully updating one's profile, would it be prudent to run another flood_clear_event so that someone updating their information through a one-time login can prevent lock out?

lukus’s picture

Same problems here.

rickmanelius’s picture

I don't mind writing a patch here, but if any core contributor can at least chime in and say what would or would not be acceptable... that would be helpful so I don't goof and submit something that wouldn't pass a security analysis, etc.

My proposal is every submission of the user edit form would result in a clearing of the flood tables for that user, but I'd like to know if there are any use cases that this would create a problem.

diedlikeahero’s picture

Status: Active » Needs review
FileSize
1.52 KB

Here's a small patch to resolve this. This will clear out the flood control when someone changes their password.

Status: Needs review » Needs work

The last submitted patch, failed_login_attempts-1423158.patch, failed testing.

rickmanelius’s picture

When I first tried to patch user.module, I received the following:

patching file user.module
patch: **** malformed patch at line 30:   }

So I just manually dropped it in for now. I tried a reset password link and now when I go to the user/%uid/edit page after one time login, I see:

Notice: Undefined index: values in user_account_form() (line 1075 of /Library/WebServer/htdocs/devel7/public/modules/user/user.module).
Notice: Trying to get property of non-object in user_account_form() (line 1085 of /Library/WebServer/htdocs/devel7/public/modules/user/user.module).
Notice: Trying to get property of non-object in user_account_form() (line 1107 of /Library/WebServer/htdocs/devel7/public/modules/user/user.module).

The values field of $form_state is not set yet, but thankfully the $account->name value is.

Therefore I tested this and it seems to work now:

    // bugreport: 1423158
    // Reset the flood control when someone changes their password.
    $account = db_query("SELECT * FROM {users} WHERE name = :name AND status = 1", array(':name' => $account->name))->fetchObject();
    if (variable_get('user_failed_login_identifier_uid_only', FALSE)) {
      // Register flood events based on the uid only, so they apply for any
      // IP address. This is the most secure option.
      $identifier = $account->uid;
    }
    else {
      // The default identifier is a combination of uid and IP address. This
      // is less secure but more resistant to denial-of-service attacks that
      // could lock out all users with public user names.
      $identifier = $account->uid . '-' . ip_address();
    }
    $form_state['flood_control_user_identifier'] = $identifier;
    
    // Clear past failures for this user so as not to block a user who might
    // log in and out more than once in an hour.
    flood_clear_event('failed_login_attempt_user', $form_state['flood_control_user_identifier']);
rickmanelius’s picture

Version: 7.10 » 7.12
Status: Needs work » Needs review
FileSize
1.54 KB

Converting #7 into a patch for review

Status: Needs review » Needs work

The last submitted patch, failed_login_attempts-1423158-8.patch, failed testing.

rickmanelius’s picture

I don't fully understand why the patch failed because I used git diff to create the patch. Oddly enough, the previous patch had the same issue when trying to apply it.

rickmanelius’s picture

Status: Needs work » Needs review
FileSize
1.53 KB

Trying one more time... and stripping out superfluous docroot directory.

rickmanelius’s picture

Ignore #11. I realized we're trying to load the $account object again even though it's already there... so I simply switched it to a conditional if statement to check the status. Use this one instead.

rickmanelius’s picture

Status: Needs review » Needs work

Hmm... I think I may have introduced a security hole in #12. I just attempted to reset my password on a test user and it somehow logged me in as user #1. I'm not sure if I simply was mistaken or by using the cached form, something went amiss. It may have been smarter in the end to use the method outlined in the earlier patch.

Putting to needs work until I get more time to test.

PS. My production server is load balanced w/memcache enabled, so I'm not sure if it's related to that and therefore not recreatable on my local machine, etc.

rickmanelius’s picture

Status: Needs work » Needs review

Reviewing the code again, it shouldn't be an issue because of the $user->uid == $account->uid. And I think I must have accidentially logged in as admin instead of used the reset password function. I've tried to recreate the glitch a few times and I don't see it, so I think it's fine.

Any other eyeballs for #12 would be appreciated.

rickmanelius’s picture

For what it's worth, I now have it working on my setup now with #12.

jlgreen2012’s picture

If it helps, here's diedlikeahero's patch reformatted for the bot testing.

Status: Needs review » Needs work

The last submitted patch, user-login-attempts-1423158-16.patch, failed testing.

jlgreen2012’s picture

Status: Needs work » Needs review
FileSize
1.34 KB

Status: Needs review » Needs work

The last submitted patch, user-login-attempts-1423158-18.patch, failed testing.

Matt V.’s picture

Status: Needs work » Closed (duplicate)

This appears to be a duplicate of #992540: Nothing clears the "5 failed login attempts" security message when a user resets their own password, which has a slightly longer history, tests, and a bit more momentum. Plus, the patch there is now written against the most recent development version, which should help it get applied (and backported) more quickly.

Andrei.Sapeshko’s picture

flood_clear_event() after new password was saved

Andrei.Sapeshko’s picture

Status: Closed (duplicate) » Needs work
Andrei.Sapeshko’s picture

Here's a small patch to resolve this. This will clear out the flood control when someone changes their password.

Version: 7.22 » 7.x-dev

Core issues are now filed against the dev versions where changes will be made. Document the specific release you are using in your issue comment. More information about choosing a version.

poker10’s picture

poker10’s picture