When using feature "Increase delay for each attempt" no logins are possible anymore

Let me take a look to the code. In the meantime I would like to know:

- have you cron enabled, and what is the frequency of cron execution?
- Had you tried more times to fail the login before enabling the delay?

The difference between beta and RC1 that may affect the delay is just a patch about cron cleaning functions: #499788: Time track depending on cron is not accurate.. I'll review it as soon as I can and submit a fix for you to test during the following hours..

Just one note.. (as in RC1)..

  // Expire old tracked entries
  _login_security_remove_events();

  // Populate variables to be used in any module message or login operation
  $variables = _login_security_get_variables_by_name($name);

  // Start with login Delay
  if ($delay = variable_get('login_security_delay_base_time', LOGIN_SECURITY_BASE_TIME)) {
    $secs = (variable_get('login_security_delay_increase', LOGIN_SECURITY_DELAY_INCREASE) == 1) ? intval($variables['%user_ip_current_count']-1) * intval($delay) : intval($delay);
    if ($secs >= ini_get('max_execution_time')) {
      $secs = ini_get('max_execution_time') - 3;
    }

    @sleep($secs);
  }

The delay is computed using the %user_ip_current_count variable, so the operation should not be affected when using other login name, or using a different IP address. I need you to check this to be able to find the problem. Also, it's not possible to have reached the max execution time of PHP (unless your site takes more than 3 seconds to complete a request).

Lastly, are you trying again and again from the same host? if all the testing request you are doing come from the same IP address, the tracking table would be full of login entries.. Do you have a very big tracking time value (of several hours maybe)? The code first remove pending events and then computes the delay.. waiting for the tracking time to pass would clean the table.

Hope this could also help us to find where the problem is, because I can't reproduce the problem..

Also.. are you using other login modules in these sites? (login destination?)

Thanks a lot for the analysis Helmut. I did review a little few things to be sure you were completely right at all. I did a mistake not checking that secs could be negative.

In the patch I've included some validation functions to the settings form elements, and verified that sleep never gets a negative value again.. The patch is for DRUPAL-6--1 branch.

Forget previous patch, allways delays (max_execution_time - 3). This one fixes that error..

I forgot to mention, with this change, the Delay base time is introduced in any login attempt. This could be a unexpected behaviour turning into a new feature, because we have already separated "login delay base time" and "Increase delay for each attempt". In the current implementation, the "login delay base time" is ignored if the "Increase delay" option is set to "No".

With this patch, every login attempt is delayed at least the "login delay base time" value in seconds, being harder for bruteforce attempts to guess passwords. So, to clarify, I would include in the documentation:

---
login delay base time: the value of this parameter sets the ammount of seconds a login attempt would take. This way, you may protect your site of bruteforce attacks making the process of guessing slow.

Increase delay for each attempt: If you want to make the password guessing even harder, you may increase the delay of the login operation for each new attempt, being computed in the form of....... bla bla bla..
---

This new alternative looks good for me.. I would like to hear also your opinions..

I will wait a little for Deekayen to tell about the new "functionality" exposed by this patch before commiting. Thanks for testing.

Sorry Helmut, I didn't notice this comment until now..

The patch at #6 includes the following lines for the Delay operation, I've commented a little bit:

  // Start with login Delay.. (Always occurs if delay_base_time is set)
  if ($delay = variable_get('login_security_delay_base_time', LOGIN_SECURITY_BASE_TIME)) {
    //if delay should be increased in each login attempt, then multiply, otherwise will sleep at least delay_base_time
    $secs = (variable_get('login_security_delay_increase', LOGIN_SECURITY_DELAY_INCREASE) == 1) ? intval($variables['%user_ip_current_count']-1) * intval($delay) : intval($delay);
    //Included 0 just in case of 'max_execution_time' being lower than 3
    //This ping-pong max/min is to get always a value of 0 <= penalty secs <= max_execution_time
    $sleep_time  = max(0, min(ini_get('max_execution_time') - 3, $secs));
    @sleep($sleep_time);
  }

"It does, however, delay successful logins when option "Increase delay for each attempt" has NOT been selected (even if the successful login is not preceded by a failed login attempt)."

This is the unexpected behaviour I was talking about.

We can make two fixes here:

1.- Avoid sleep when "Increase delay for each attempt" has not been set:

...
    $secs = (variable_get('login_security_delay_increase', LOGIN_SECURITY_DELAY_INCREASE) == 1) ? intval($variables['%user_ip_current_count']-1) * intval($delay) : 0;

2.- DO sleep delay_base_time on any login attempt:

...
    $secs = (variable_get('login_security_delay_increase', LOGIN_SECURITY_DELAY_INCREASE) == 1) ? intval($variables['%user_ip_current_count']) * intval($delay) : intval($delay);

Assuming that "DELAY_BASE_TIME" was first defined only for delays, I would say 1 is the prefered option. But as we have two different configuration options (delay_base_time and delay_increase) I guess we can take profit of the first one to just settle a default login delay for any attempt. +1 extra feature against bruteforce attacks.

If you have any preference about which of the fixes apply please share..

Commited to 6.x-1.x-dev. Will be documented in README in the next commit.

Hi Helmut, this was already commited with the 2nd option as the winner. Only failed logins are punished with the delay base time, so it will not affect regular logins, while does protect the wrong login attempts with a little penalty to avoid password guessing operations.

So now, when delay base time has a value > 0, every invalid login is punished at least this ammount of seconds, and if "increase delay" is selected, the ammount will be multiplied by the number of invalid login attempts. This is the final behaviour of the delaying options.

I guess it's time to check also other changes in the module. I kindly suggest you to take a look at: http://drupal.org/node/397890#comment-2068410 and also review the new README file with updated instructions about the module, somethings have changed since your last comment in this issue.

Cheers!
ilo