Contrib.social

Flood control on reset password form blocks IP address but not the account

Open on Drupal.org →
Created on 1 August 2022, almost 2 years ago
Updated 12 April 2024, 3 months ago

This issue has been approved for public discussion by the Drupal Security Team.

Problem/Motivation

The IP address blocking system works fine, but if the reset password for the same account is made from a different IP address, the user is still able to do the same number of attempts until the new IP address gets blocked.

In the validateForm of UserPasswordForm, these 3 lines currently don't do anything, since a simple return in validateForm doesn't trigger any error.

if (!$this->flood->isAllowed('user.password_request_user', $flood_config->get('user_limit'), $flood_config->get('user_window'), $identifier)) {
    return;
}

The setError has been removed as part of https://www.drupal.org/node/1521996 → which ultimately changed the behaviour and left redundant code (as stated before, the lines above don't do anything). And currently there is no Change Request to put the behaviour described here https://www.drupal.org/node/3085704 → back in place.

Steps to reproduce

1. Go to the reset password form
2. Attempt to reset the password for a given account 5 times (ip_limit changed to 5 for testing purposes)
3. After these 5 attempts, the "IP blocked" error message appears
4. Change the IP address (via VPN for instance)
5. Do a new attempt on the same account
6. The same amount of attempts can be done before getting the new IP blocked

Proposed resolution

The account should be appropriately blocked according to the user_limit value. In our example, the account should be blocked at the same time as the first IP address being blocked, since it is set by default to 5.
- Put back the message but with a more generic sentence, so it is unclear if an account exists or not.

if (!$this->flood->isAllowed('user.password_request_user', $flood_config->get('user_limit'), $flood_config->get('user_window'), $identifier)) {
$form_state->setErrorByName('name', $this->t('Too many password recovery requests. Try again later or contact the site administrator.'));
    return;
}
🐛 Bug report
Status

Active

Version

11.0 🔥

Component
User system  →

Last updated about 24 hours ago

Created by

carolea

Live updates comments and jobs are added and updated live.
  • Security improvements

    It makes Drupal less vulnerable to abuse or misuse. Note, this is the preferred tag, though the Security tag has a large body of issues tagged to it. Do NOT publicly disclose security vulnerabilities; contact the security team instead. Anyone (whether security team or not) can apply this tag to security improvements that do not directly present a vulnerability e.g. hardening an API to add filtering to reduce a common mistake in contributed modules.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment 3 months ago →
    luke.stewart

    So If I understand the changes I think this is what happened:

    Previously attempts per account name were recorded and if they were above the limit set the account was blocked.

    However this means that you can test for the existence of an account by triggering the block. So this was removed.

    So even if we put a generic message back - that still yields information about the attempt as because it's done within a test to see if the user exists - it's only triggered when a username exists.

    So if we want the behaviour back whereby you can trigger a block based on attempting to reset an account separate from the IP block either:

    1) We'd need to block the account but not notify the user that got blocked.
    2) We'd need to record all attempted usernames in a way that we could block by username even if the username wasn't a username corresponding to a user.

    I don't think we could assume that any non existant username is equivalent. Although this would be a nice way to speed up blocking attempts at enumerating usernames it would be possible to use this to identify valid usernames.for example. If you trigger the block and get notified on say 5 usernames that don't exist - and then retry after you are unblocked with 4 that you know don't exist and another username - you would get blocked (and notified) if and only if the fourth didn't exist. Thereby allowing you to test. Although I guess this is complicated by that this behaviour would be influence by any use of the form.

    I feel like 1) is worse behaviour. As this would mean that a legitimate user would not be notified they were blocked, leading to confusion as to why they were not getting a password reset email.

    For 2 things to consider:

    Currently we use the UID in the flood as an identifier - this doesn't work for usernames that don't have a UID because they don't exist.
    One option would be to store a list of attempted usernames and a unique id (perhaps a negative integer as this would never be a UID?)
    But that seems excessive we could store the UID for existing accounts, and the username for attempts on accounts that don't exist.

    What happens if a username that doesn't exist is blocked - is then created. At that point we'd want to remove any block. I think if we split storage between uid for usernames that exist and username for ones that don't we don't have to worry about this.

    The only downside I can see with storing non existing usernames and user ids is that it would mean that the storage would be different so would that mean you could use a timing attack to work out if a username exists? Is this something that is worth considering?

    So if we wanted to go with option 2 is all that is required to add back a generic error message, and for submissions that valid to meet the test:
    if ($account && $account->id() && $account->isActive()) {
    call isAllowed with the provided username as the indentifier and if disallowed set the same message.

contrib.social Blog FAQ Discussions
Production build 0.69.0 2024