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

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.

Hi, I’ve reproduced this issue on both Drupal 10.3.x and 11.x with PHP 8.3 and have made the proposed changes to address the issue. Here’s a summary of the updates:

1. Flood Control Improvement: Updated the password reset functionality to ensure that both the IP address and the user account are simultaneously blocked after exceeding the allowed number of attempts. This ensures that the account is secured regardless of the IP address used.
2. Error Message: As proposed, updated the error message to be more generic, enhancing security by not revealing whether the account exists.

Attaching before & after screenshots of the "Error Message" update for reference.

Due to my local environment setup, I was unable to change my IP address dynamically. Instead, I manually updated the IP address in the database to verify the behavior. The changes appear to resolve the issue as intended. However, it would be greatly appreciated if further testing could be conducted in an environment where IP changes can be performed to confirm the fix.

Kindly review the changes and let me know if any further adjustments or updates are needed.

Thanks!

This is the code in the merge request, including the changed part.

    // Try to load by email.
    $users = $this->userStorage->loadByProperties(['mail' => $name]);
    if (empty($users)) {
      // No success, try to load by name.
      $users = $this->userStorage->loadByProperties(['name' => $name]);
    }
    $account = reset($users);
    // Blocked accounts cannot request a new password.
    if ($account && $account->id() && $account->isActive()) {
      // Register flood events based on the uid only, so they apply for any
      // IP address. This allows them to be cleared on successful reset (from
      // any IP).
      $identifier = $account->id();
      if (!$this->flood->isAllowed('user.password_request_user', $flood_config->get('user_limit'), $flood_config->get('user_window'), $identifier)) {
      $ip_blocked = !$this->flood->isAllowed('user.password_request_ip', $flood_config->get('ip_limit'), $flood_config->get('ip_window'));
      $user_blocked = !$this->flood->isAllowed('user.password_request_user', $flood_config->get('user_limit'), $flood_config->get('user_window'), $identifier);
      if ($ip_blocked || $user_blocked) {
        // Block the account and show a generic error message.
        $this->flood->register('user.password_request_user', $flood_config->get('user_window'), $identifier);
        // Add the error message to inform the user.
        $form_state->setErrorByName('name', $this->t('Too many password recovery requests. Try again later or contact the site administrator.'));
        return;
      }
      $this->flood->register('user.password_request_user', $flood_config->get('user_window'), $identifier);
      $form_state->setValueForElement(['#parents' => ['account']], $account);
    }

Since the error is shown after checking an account has been loaded from the database, that error message is indeed saying the account exists. There is no need for the message to explicitly report the username; I know well which username I entered.

When the code does not show any error for the name form element, it could mean that either there is no account with the entered username, or that account has been blocked. That is the only case where the code does not give any hint about a possible account with the entered username.

Hi @avpaderno,

Thank you for your feedback.

I understand that when no error is shown for the name form element, it might imply that either the account does not exist or it has been blocked, which could potentially reveal hints about the account status.

Given this context, should we consider adjustments to the "if()" condition mentioned in comment #9 🐛 Flood control on reset password form blocks IP address but not the account Needs work , or do you recommend another approach? I’m seeking confirmation as the "if()" condition (if ($account && $account->id() && $account->isActive()) { /* */ }) was part of the existing code and not introduced by my recent changes.

Kindly please let me know how you’d like to proceed.

Thanks!