Improved security: rate limit login attempts.

This looks sensible, we already do it for the contact form etc. I think it'd be worth making the flood limit a variable (but /not/ adding a settings page for it), so it's easy for sites who want to use more restrictive settings to do so.

That effectively means that you can DoS someone out of his/her own account.

Nothing prevent this mechanism (a more sensible one than the one proposed here, hopefully) to be implemented in contrib (inside logintoboggan for example). This is not core material, IMO.

I think that for core, it is sufficient to watchdog failed login attempts so an admin can take notice.

Regarding further security, there are so many different approaches (blocking IPs, requiring captchas like Google, notifying users about failed accesses to their account) that core cannot implement them all and should not limit users to a specific one. This is a job for contrib.

I believe core should support the ability to limit password attempts, but I do not believe that functionality should be in core.

I disagree - this should be in core as a default. DamZ says this needs a little work though due to core refacting.

http://www.usenix.org/event/hotsec07/tech/full_papers/florencio/florenci...

marking #117056: Protection against brute force login as duplicate since this has a better patch.

Here we are.

Adding even more comments (thanks Peter).

Some thoughts:

Why should flood_register_event('failed_login_attempt'); occur when the flood control mechanisms are denying access, shouldn't it just occur when the specific password is rejected?

I believe code comments need to wrap at 80 characters.

20 allowed attempts is much to high - ~3 is safer per the linked article

After discussion with Peter and Moshe on #drupal, we concluded that we need both a global hourly limit and a per-uid hourly limit.

Even more tests, and a fix for a flow error (thanks Peter).

I don't understand why in the test we can attempt 4 failed logins - we should see the flood control on the 4th?

Also, minor typo:

+    // Clear past failures for this IP address so as no to block users

should be

+    // Clear past failures for this IP address so as not to block users

+    variable_set('user_login_hourly_limit', 4);

Four failed login are allowed, the fifth one is rate-limited.

Well, I think it should be 3 not 4, and the patch code seems to show 3:

if (!flood_is_allowed('failed_login_attempt_' . $account->uid, variable_get('user_login_per_user_hourly_limit', 3))) {

New patch:

- harmonized the per_user limit to 3 in the test and in the code.
- renamed user_login_hourly_limit and user_login_per_user_hourly_limit to user_failed_login_hourly_limit and user_failed_login_per_user_hourly_limit, to clarify their meaning.
- fixed comment typo

subscribing

I'd rather omit the call to flood_clear_event('failed_login_attempt') here:

+  else {
+    // Clear past failures for this IP address so as not to block users
+    // who might log in and out more than once in an hour.
+    flood_clear_event('failed_login_attempt');
+    flood_clear_event('failed_login_attempt_' . $form_state['uid']);
   }

Otherwise, this would essentially (I think) destroy the flood protection for any site getting regular logins from a shared IP.

Other than that, it's looking good. Did you test one-time login links? I think it's important that those work even for a locked out user so as to have a way to bypass a DOS.

need to remove that one line.

Please also backport this to Drupal 6x.

I think it would be far too late to implement this in the stable Drupal 6 release branch, but there's no reason a contributed module can't do this.

That will be great if someone comes with a contrib module.
Previous versions of Drupal are in usage widely and even book on Views - Drupal 5 makes front page story even today. Drupal 6 will stay for a few years more and web site security becomes more and more critical everyday ( see unmaskparasites.com )

with the one line removed.

oops - needed to adjust the test a little to match the new behavior.

I think 3 failed logins per hour is ridiculously low for a default - people will hit that limit all the time.

Sorry, crosspost.

@kaakuu et al looking for a Drupal 6 contrib solution see http://drupal.org/project/login_security (though I haven't tried it)

@David - then they can increase the variable. 3 is a very standard limit. The goal is to provide a configuration that is secure against brute-forcing out of the box.

Also, if they fail their 3x, they can still get a login link e-mailed to them.

This looks very good to me... but I will not RTBC because I wrote most of it.

My last patches only changed 1 line of real code, so if Damien has re-reviwed, I think this is ready to go.

1.Also, if they fail their 3x, they can still get a login link e-mailed to them.

That's a very good point - but the current patch never tells them that - the text that is shown when they've triggered the maximum number of attempts should link to the "request new password" page and explain that it will still work. Extra usability bonus points for also showing them a special message when they are at N-1, which tells them that they only have one more try to get it right :)

2. I understand that 3 is a standard limit, but I think Drupal is used on a much wider variety of sites than the kinds of sites that usually employ that. And because there is no user interface for changing this value, the average site administrator isn't going to know how they can change it - they are basically going to be stuck with the default we give them. Honestly, if the default were ~10 instead of 3, how much does that really matter for security? I think it means that a brute force attack would succeed a few times times faster, but if it succeeds at all in a reasonable timeframe, I'm not sure I understand why a factor of 3 matters. If you're that close to the edge, then your users don't have secure enough passwords - seems like that's the real problem. I think the difference between an (effectively) infinite number as we have now and a finite number is by far the most important change.

3. Looking into how some other sites do this, e.g. Gmail, they let you continue trying to log in after a few failures, but eventually start making you enter a CAPTCHA. Obviously Drupal core can't do that, but should support a contributed module doing so. I think this patch does (I guess the module could just get rid of/replace this form validation handler?), but it's worth thinking about.

Well, I'd agree that the message after you fail should point you to getting a login link and explain that they may attempt again in an hour. That would really take the place of a captcha in this situation. It would also be nice to distinguish the per user vs per IP limiting.

I still think 3 is a very reasonable default.

I agree with David that we need to ensure that contrib module can mess up with this behavior as they see fit.

We need to move those to validation functions.

@DamZ, David - looking at the code flow, it's hard to see how to really put the flood code in a separate validation function - it would have to be both before and after the call to user_authenticate() and would have to do something like unsetting form_state elements to invalidate the login.

Note that another validation function inserted before user_login_final_validate() could readily unset the flood control flags or otherwise alter this behavior.

Ok, at the cost of 1 extra query (only on login attempts, so not a performance issue), we can move the flood control code out of user_authenticate(), thus preserving this as a true API function and allowing it to be re-used. Also re-worked the error messaged and added a link there.

I can't help myself - this reworks user_authenticate() into a sane API function instead of a hack usable only with form values.

Without bikeshedding this issue, but can we pump it up to like 5 by default? I know, other systems do 3 - and I find that incredibly annoying.

I like.

@Bojan - re-reading http://www.usenix.org/event/hotsec07/tech/full_papers/florencio/florenci... note that the protection from brute-force attacks is essentially inverse to the number of attempts allowed. In failing 3 times really that common? Note, as above, that you can bypass this also with a login link via e-mail.

Given that the lockout is only an hour (versus 24 hours in the paper - so we are allowing 24x more attempts), and is per-IP not global, we are actually not as safe from brute-forcing as we want, and need to alter the flood control mechanism.

This patch adds to the flood API an optional $hostname param - by setting this to any constant string (I choose '*') we can apply flood control regardless of IP address. This also enables more flexible flood control in general.

In a separate patch we should think about some simple core facility for requiring a minimum-length password.

from the paper:

"To consider a concrete example, if a bank allows only 6 digits PINs (a relatively weak password) and locks an account for 24 hours after three
attempts an attacker could search 3 × 365 10/1e6 0.011 or 1% of the key-space in 10 years. "

note that a random 4 character lowercase alpha-numeric (36 chars) is a little stronger than 6 digit numeric, though probably much weaker if it's a dictionary word. So I think a random non-word 4 char password would with 1-hour lockout after 3 attempts allow roughly 1% key-space coverage by brute force in 6 months.

I'm wondering if the $hostname parameter is the cleanest way to accomplish this. I think it would be OK to make hostname a required paramater and to explicitly pass in ip_address(). If we do that, we could rename 'hostname' to something a bit more generic too -- maybe 'agent', 'actor', or 'identifier'. In some cases, the hostname might not be the best identifier. Sometimes, maybe we want to flood-control on say, User-Agent (e.g. to block a distributed crawler) or Cookie (e.g. when lots of people are behind one IP address -- not super robust for this use case but maybe ok for others). I think such clean-up would make the '*' approach a bit nicer DX-wise. Just a thought.

Thought about this some more in the shower; the flood protection mechanism is really a glorified counter with a decay. We could rename it to decay_counter or something along the lines as I described in #47.

In fact, I could have been useful in #536144: Add a mollom_get_statistics() to fetch API statistics. When an XML-RPC calls fails, we might want to try a few times before storing false. Storing false instantly, as we do now, cause no data to be available for one hour. Using a decay counter, we could be slightly more elegant about it.

Dries #48 made me realize that we probably also want to keep an eye on the performance of the current "flood" implementation.

@Dries - certainly agree that something like 'identifier' would be a better variable name.

i'd suggest against making it a required parameter since the most common use case is still IP-based.

Personally, I think the effort of explicitly writing ip_address() is small, and it makes for more readable code. Either way, that is a small detail.

Ok, here's a re-roll but not really tested - feels like something else in core is broken because install is generally failing for me even with a clean checkout.

This renames the DB column and uses the uid as the identifier for flood control per uid.

Without totally renaming everything, this patch makes the time window a parameter to flood_is_allowed - this way we can up the lockout time to 6 hours (or whatever you like as a defualt) and then we can comfortabily increase the per-use allowed fails to 5 as Bojhan suggests.

This looks OK to me. We can do the refactoring in a follow-up patch. Hopefully someone else can review it too, and mark it RTBC.

Looks very good. I saw only one issue:

 /**
+* Alter field hostnme to identifier in the {flood} table.
+ */
+function system_update_7031() {
+  $ret = array();
+  db_change_field($ret, 'flood', 'hostname', 'identifier', array('type' => 'varchar', 'length' => 128, 'not null' => TRUE, 'default' => ''));
+  db_drop_index($ret, 'flood', 'allow');
+  db_add_index($ret, 'flood', 'allow', array('event', 'identifier', 'timestamp'));
+  return $ret;
+}

The drop index needs to be before the change field.

I looked over the patch but did not try it out. I agree it looks very good, but noticed a couple other issues/questions.

1. In the flood_is_allowed() documentation, this is no longer accurate, right? (not necessarily per hour anymore):

  * @param $threshold
  *   The maximum number of the specified event per hour (per visitor).

2. Minor typo ("hostnme"):

 /**
+* Alter field hostnme to identifier in the {flood} table.
+ */

3. Regarding this functionality:

+      // Don't allow login if the limit for this user has been reached. Specify
+      // the uid as identifier to limit all attempts regardless of origin IP.

The first few comments in this issue discussed that doing it this way would allow denial of service attacks. Is that still a concern? I guess the answer is that they can still log in via requesting a new password, but it seems like someone could still make your life very annoying. Not sure if this is a problem or not.

4. user_login_final_validate() contains calls to both flood_register_event()/etc as well as to form_set_error(). Seems like it would be ideal if these could be split out to separate validation handlers. Is that possible or not? If so, a contrib module might have an easier time replacing this. Drupal would just tell them if the limits have been exceeded or not, and they would decide what to do with this information, but not have to deal with registering/clearing events.

5. The tests have code like this:

+    $incorrect_user1 = clone $user1;
+    $incorrect_user1->pass_raw = $this->randomName();

Amusingly, you could actually use the formulas discussed earlier in this issue to estimate how many times the testbot will run before it stumbles upon the correct password accidentally, thereby causing the test to fail.... :) More seriously, the code might be a little easier to understand if it explicitly made the password invalid; e.g. $incorrect_user1->pass_raw = $user1->pass_raw . 'incorrect_letters'.

6. Personally, I'm still a little uncomfortable with the defaults, although that's a small detail in terms of the code, of course. 5 attempts is a lot better than 3, but 6 hours can be long ("request new password" isn't foolproof, because sometimes people don't have access to their email, if it's not stored on the web, etc). I'd suggest we think more about this, but since it doesn't really change the code, it's a small detail in that sense.

fixing errors noted above.

David, re: #4, I'm not sure what you're suggesting - would we ever register the event but not set the form error? This user_login_final_validate() function is supposed to come last and some other function might come in between and validate the user via a different mechanism. If you want to prevent the even from being registered, you could always unset the $form_state elements, right?

re: #6 - if you restrict to the per-user flood to the current IP you are immediately vulnerable to brute-forcing by someone with many IP addresses at their disposal. Given the sad prevalence of botnets, this in not an unlikely scenario. So, I don't see how else to make logins secure from reasonably serious brute-forcing attempts. I'd agree that this could be used to make a site hard to use - so do we need an additional option to turn on a less-secure (but more robust against DOS) IP + uid identifier?

I think this is worthy of a CHANGELOG.txt entry in the "Security" section. Looking at the security section, it feels like item 2 and 3 are somewhat identical?

@Dries - yes would certainly contribute to security.

I'd say the current #3 is more about APIs/DX than security.

Reworked the identifier based on David's concerns - the default is now a combination of uid + IP, with the option to toggle via a variable to the more secure uid-only.

Added a CHANGELOG entry and consolidated the two existing ones referring to the password hashing.

@@ -13,10 +13,11 @@ Drupal 7.0, xxxx-xx-xx (development version)
-    * Implemented much stronger password hashes that are also compatible with the
-      Portable PHP password hashing framework.
-    * Implemented a pluggable password hashing API supporting alternative
-      hashing and authentication schemes.
+    * Implemented as a pluggable mechanism much stronger password hashes that
+      are also compatible with the Portable PHP password hashing framework.

Hm... the reworked entry sounds a bit strange to me.

@@ -13,10 +13,11 @@ Drupal 7.0, xxxx-xx-xx (development version)
+    * Rate limit login attempts to prevent brute-force password guessing, and
+      improve the flood control API to allow variable time windows and

I think it should be "improved" (past tense).

@@ -1276,39 +1276,70 @@ function valid_url($url, $absolute = FALSE) {
- * than $threshold times per hour.
+ * than $threshold times in the specified window.

s/window/time window/.

@@ -1276,39 +1276,70 @@ function valid_url($url, $absolute = FALSE) {
+ *   Optional number of seconds over which to look for events.  Defaults to
@@ -1618,39 +1618,101 @@ function user_login_name_validate($form, &$form_state) {
+        // Register flood events based on the uid only, so they apply for any
+        // IP address.  This is the most secure option.

Albeit grammatically correct, we do not use double-spaces between sentences in PHPDoc.

@@ -759,8 +759,8 @@ function system_schema() {
+      'identifier' => array(
+        'description' => 'Identifier of the visitor, such as IP address or hostname.',

s/such as IP/such as an IP/.

@@ -1618,39 +1618,101 @@ function user_login_name_validate($form, &$form_state) {
+  // Name and pass keys are required.
+  if (!empty($form_state['values']['name']) && !empty($password)) {

I think we can drop the comment here (it's obvious from the next line).

@@ -1618,39 +1618,101 @@ function user_login_name_validate($form, &$form_state) {
+    // Don't allow any login from that IP if the limit has been reached.

"that IP"? Which?

Also, please do not use abbreviations in comments ("don't").

@@ -1618,39 +1618,101 @@ function user_login_name_validate($form, &$form_state) {
+    // Default is 50 failed attempts allowed in one hour.
+    if (!flood_is_allowed('failed_login_attempt_ip', variable_get('user_failed_login_ip_limit', 50), variable_get('user_failed_login_ip_window', 3600))) {
+      $form_state['flood_control_uid'] = 0;
+      return;
+    }

50/h? Why 50? If that is explained/reasoned in the issue, then please also put a note in here.

...oh, after reading further, I see that this applies independent of the user. Could still use some explanation/reasoning, I think.

Also, shouldn't we return FALSE or alter $form_state somehow, so that contributed modules (i.e. further form submit/authentication handlers) know about this fact?

@@ -1618,39 +1618,101 @@ function user_login_name_validate($form, &$form_state) {
+ * A validate handler on the login form. Should be the last validator. 
...
- * A validate handler on the login form. Should be the last validator. Sets an
- * error if user has not been authenticated yet.
...
+ * 

Although the existing summary already contained this, the first sentence sounds a bit too generic and superfluous, and could be merged into the second sentence, so we get a nice summary. Otherwise, the second sentence should be moved into the description.

There is also trailing white-space on those lines.

@@ -1618,39 +1618,101 @@ function user_login_name_validate($form, &$form_state) {
+    // Register a global failed login event.
+    flood_register_event('failed_login_attempt_ip');
+
+    if (isset($form_state['flood_register_event_user'])) {
+      // Register a per-user failed login event. 
+      flood_register_event('failed_login_attempt_user', $form_state['flood_register_event_user']);
+    }

Can we move the second inline comment above the if condition, so both comments are before?

@@ -1618,39 +1618,101 @@ function user_login_name_validate($form, &$form_state) {
+        // We didn't find a uid, so limit is IP-based.
+        form_set_error('name', t('Sorry, too many failed login attempts from your IP address.  This IP address is temporarily blocked. Please try again later, or <a href="@url">request a new password</a>.', array('@url' => url('user/password'))));

Abbreviations and double-spaces in here.

@@ -1618,39 +1618,101 @@ function user_login_name_validate($form, &$form_state) {
+        // Successful authentication..

Two periods here.

@@ -159,6 +159,113 @@ class UserValidationTestCase extends DrupalWebTestCase {
+   * @param $user
+   *   The user.
...
+  function assertFailedLogin($user, $flood_uid = NULL) {
...
+   * Make an unsuccessful login attempt.
+   *

s/The user./A user object of an account to login./ ?

This review is powered by Dreditor.

Change code comments as suggested, and improved a test to check that we properly reset the flood control when a user logs in.

oops - diffed wrong - ignore #62

This doesn't make any sense, the linked results show all tests passing.

http://testing.drupal.org/pifr/file/1/485974-password-rate-limit-63.patch

I can do an install locally and run tests, but just in case - here's a fresh patch against HEAD. No changes from the last patch.

Ah, apparently the github mirror is out of date - re-rolling against CVS to include a new system update function that was conflicting.

added doxygen for the test class, and change the name of the $form_state element indicating that we tirggered flood control per DamZ.

Yay!

This patch is awesome! :D Cleans up a lot of hard-coded cruft, adds tests, AND makes my most favourite change in all the world:

-function user_authenticate(&$form_state) {
...
+function user_authenticate($name, $password) {

I looked and couldn't find anything to complain about, so committed to HEAD. Great job, all!

Let's document this wonderful change to user_authenticate() and whatever else needs documenting from this patch.

Note: the changes to the flood control API needs to be documented too.

http://drupal.org/update/modules/6/7#user_authenticate

documented the function change

I don't use to look for "security" neither "login" within the issue queue, so I just found this today. Sorry. I wonder Why nobody took a look to an existing module with just this functionality, even for consideration. In any case, Just wanted to point that the pretty "Sorry, invalid username or password" message has a new friend now: "Sorry, there has been more than one failed login attempt for this account..." where the bruteforce attempt could lead to guess usernames in the site. Deekayen created an issue in the login_security module to remove that kind of messages some time ago.

You did a good and quick work with the patch.

Sorry, also I forgot to mention that proxy based users would find problems if the proxy IP address is blocked. Here in spain there was an ISP that used a proxy to accelerate internet navigation. But this is something hard to deal in any case. This were the reasons why I decided to make several protection options, and be able to disable/enable any of them. This is something to be considered in the documentation, I guess.

BTW I've started http://drupal.org/project/flood_control to add an admin interface for these flood control variables and any others.

Thanks @Dave Reid for providing the Admin interface module for the flood control.

However, it boogles me why we would want this feature included in Drupal at all without also providing the Admin Interface?

Can this feature request please be re-opened so that the Admin Interface can also be added?

@Agileware, I would suggest creating a new issue for that if you think it should be added?

Personally, the current behavior makes sense to me though... The feature was added for security and a reasonable default behavior was added along with it, but an admin interface to adjust the defaults doesn't sound like something the majority of sites would need or want (nor would it be a great idea to expose this to people who don't understand what the settings mean), so I think think it works very well as a contrib module.

Current behaviour for flood control makes sense in the same way that no admin interface for filter_xss_bad_protocol() makes sense.

Most sites simply aren't going to require the control over which protocols are filtered on output. Similarly, as long as Drupal allows contrib modules to be created just in case the site manager/admin is required to change them (http://drupal.org/project/filter_protocols for filter_xss_bad_protocol()).

Thanks to everyone @ drupal big hugs... drupalup:)
just another note on this issue from the start not to go backwards:( but i am really not sure this is needed at all due to there are more wholes on most servers just due to the fact of PHPMYADMIN or CPANEL @ EXAMPLE.COM/ Users have to evolve too as we can not protect them from stupidity as well. I am not trying to be critical of every ones hard work. But if the host is willing to allow this behavior and not lock it down with a firewall like Fail2ban on ubuntu:)

Further a recent client kicked us their host details for a site transfer to us with "username: admin" "passcode: mypass" how can we protect against hardened sites like this:) One of the biggest issues on the net is the example.com/phpmyadmin this is crazy:)

Thanks to every one in the drupal community everyday it just keeps getting better and better:)