From 1da6ef52c44fd38785391d3a94af8e969344bc12 Mon Sep 17 00:00:00 2001
From: Angie Byron <webchick@24967.no-reply.drupal.org>
Date: Sat, 8 Aug 2009 20:52:33 +0000
Subject: [PATCH] #485974 by pwolanin, Damien Tournoud, mr.baileys: Improved
 security by limiting the number of allowed login attempts.

---
 CHANGELOG.txt                 |   9 +--
 includes/common.inc           |  51 ++++++++++++---
 modules/system/system.install |  17 ++++-
 modules/user/user.module      |  98 +++++++++++++++++++++++-----
 modules/user/user.test        | 117 ++++++++++++++++++++++++++++++++++
 5 files changed, 258 insertions(+), 34 deletions(-)

diff --git a/CHANGELOG.txt b/CHANGELOG.txt
index 344d9e8a52c9..9d3d8075c4ae 100644
--- a/CHANGELOG.txt
+++ b/CHANGELOG.txt
@@ -13,10 +13,11 @@ Drupal 7.0, xxxx-xx-xx (development version)
       This offers increased scalability and data integrity.
 - Security:
     * Protected cron.php -- cron will only run if the proper key is provided.
-    * 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 a pluggable password system and much stronger password hashes
+      that are compatible with the Portable PHP password hashing framework.
+    * Rate limited login attempts to prevent brute-force password guessing, and
+      improved the flood control API to allow variable time windows and
+      identifiers for limiting user access to resources.
 - Usability:
     * Improved installer requirements check.
     * Improved support for integration of WYSIWYG editors.
diff --git a/includes/common.inc b/includes/common.inc
index 7a94a79729da..f04ce4a94836 100644
--- a/includes/common.inc
+++ b/includes/common.inc
@@ -1276,39 +1276,70 @@ function valid_url($url, $absolute = FALSE) {
  */
 
 /**
- * Register an event for the current visitor (hostname/IP) to the flood control mechanism.
+ * Register an event for the current visitor to the flood control mechanism.
  *
  * @param $name
  *   The name of an event.
+ * @param $identifier
+ *   Optional identifier (defaults to the current user's IP address).
  */
-function flood_register_event($name) {
+function flood_register_event($name, $identifier = NULL) {
+  if (!isset($identifier)) {
+    $identifier = ip_address();
+  }
   db_insert('flood')
     ->fields(array(
       'event' => $name,
-      'hostname' => ip_address(),
+      'identifier' => $identifier,
       'timestamp' => REQUEST_TIME,
     ))
     ->execute();
 }
 
 /**
- * Check if the current visitor (hostname/IP) is allowed to proceed with the specified event.
+ * Make the flood control mechanism forget about an event for the current visitor.
+ *
+ * @param $name
+ *   The name of an event.
+ * @param $identifier
+ *   Optional identifier (defaults to the current user's IP address).
+ */
+function flood_clear_event($name, $identifier = NULL) {
+  if (!isset($identifier)) {
+    $identifier = ip_address();
+  }
+  db_delete('flood')
+    ->condition('event', $name)
+    ->condition('identifier', $identifier)
+    ->execute();
+}
+
+/**
+ * Check if the current visitor is allowed to proceed with the specified event.
  *
  * The user is allowed to proceed if he did not trigger the specified event more
- * than $threshold times per hour.
+ * than $threshold times in the specified time window.
  *
  * @param $name
  *   The name of the event.
  * @param $threshold
- *   The maximum number of the specified event per hour (per visitor).
+ *   The maximum number of the specified event allowed per time window.
+ * @param $window
+ *   Optional number of seconds over which to look for events.  Defaults to
+ *   3600 (1 hour).
+ * @param $identifier
+ *   Optional identifier (defaults to the current user's IP address).
  * @return
  *   True if the user did not exceed the hourly threshold. False otherwise.
  */
-function flood_is_allowed($name, $threshold) {
-  $number = db_query("SELECT COUNT(*) FROM {flood} WHERE event = :event AND hostname = :hostname AND timestamp > :timestamp", array(
+function flood_is_allowed($name, $threshold, $window = 3600, $identifier = NULL) {
+  if (!isset($identifier)) {
+    $identifier = ip_address();
+  }
+  $number = db_query("SELECT COUNT(*) FROM {flood} WHERE event = :event AND identifier = :identifier AND timestamp > :timestamp", array(
     ':event' => $name,
-    ':hostname' => ip_address(),
-    ':timestamp' => REQUEST_TIME - 3600))
+    ':identifier' => $identifier,
+    ':timestamp' => REQUEST_TIME - $window))
     ->fetchField();
   return ($number < $threshold);
 }
diff --git a/modules/system/system.install b/modules/system/system.install
index 46ded8dbfd95..77c566bed852 100644
--- a/modules/system/system.install
+++ b/modules/system/system.install
@@ -759,8 +759,8 @@ function system_schema() {
         'not null' => TRUE,
         'default' => '',
       ),
-      'hostname' => array(
-        'description' => 'Hostname of the visitor.',
+      'identifier' => array(
+        'description' => 'Identifier of the visitor, such as an IP address or hostname.',
         'type' => 'varchar',
         'length' => 128,
         'not null' => TRUE,
@@ -775,7 +775,7 @@ function system_schema() {
     ),
     'primary key' => array('fid'),
     'indexes' => array(
-      'allow' => array('event', 'hostname', 'timestamp'),
+      'allow' => array('event', 'identifier', 'timestamp'),
     ),
   );
 
@@ -2233,6 +2233,17 @@ function system_update_7031() {
   return $ret;
 }
 
+/**
+* Alter field hostname to identifier in the {flood} table.
+ */
+function system_update_7032() {
+  $ret = array();
+  db_drop_index($ret, 'flood', 'allow');
+  db_change_field($ret, 'flood', 'hostname', 'identifier', array('type' => 'varchar', 'length' => 128, 'not null' => TRUE, 'default' => ''));
+  db_add_index($ret, 'flood', 'allow', array('event', 'identifier', 'timestamp'));
+  return $ret;
+}
+
 /**
  * @} End of "defgroup updates-6.x-to-7.x"
  * The next series of updates should start at 8000.
diff --git a/modules/user/user.module b/modules/user/user.module
index 584e7ce74077..ae21574ce95b 100644
--- a/modules/user/user.module
+++ b/modules/user/user.module
@@ -1618,39 +1618,102 @@ function user_login_name_validate($form, &$form_state) {
  * is set to the matching user ID.
  */
 function user_login_authenticate_validate($form, &$form_state) {
-  user_authenticate($form_state);
+  $password = trim($form_state['values']['pass']);
+  if (!empty($form_state['values']['name']) && !empty($password)) {
+    // Do not allow any login from the current user's IP if the limit has been
+    // reached. Default is 50 failed attempts allowed in one hour. This is
+    // independent of the per-user limit to catch attempts from one IP to log
+    // in to many different user accounts.  We have a reasonably high limit
+    // since there may be only one apparent IP for all users at an institution.
+    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_triggered'] = 'ip';
+      return;
+    }
+    $account = db_query("SELECT * FROM {users} WHERE name = :name AND status = 1", array(':name' => $form_state['values']['name']))->fetchObject();
+    if ($account) {
+      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;
+
+      // Don't allow login if the limit for this user has been reached.
+      // Default is to allow 5 failed attempts every 6 hours.
+      if (!flood_is_allowed('failed_login_attempt_user', variable_get('user_failed_login_user_limit', 5), variable_get('user_failed_login_user_window', 21600), $identifier)) {
+        $form_state['flood_control_triggered'] = 'user';
+        return;
+      }
+    }
+    // We are not limited by flood control, so try to authenticate.
+    // Set $form_state['uid'] as a flag for user_login_final_validate().
+    $form_state['uid'] = user_authenticate($form_state['values']['name'], $password);
+  }
 }
 
 /**
- * A validate handler on the login form. Should be the last validator. Sets an
- * error if user has not been authenticated yet.
+ * The final validation handler on the login form.
+ * 
+ * Sets a form error if user has not been authenticated, or if too many
+ * logins have been attempted. This validation function should always
+ * be the last one.
  */
 function user_login_final_validate($form, &$form_state) {
   if (empty($form_state['uid'])) {
-    form_set_error('name', t('Sorry, unrecognized username or password. <a href="@password">Have you forgotten your password?</a>', array('@password' => url('user/password'))));
-    watchdog('user', 'Login attempt failed for %user.', array('%user' => $form_state['values']['name']));
+    // Always register an IP-based failed login event.
+    flood_register_event('failed_login_attempt_ip');
+    // Register a per-user failed login event.
+    if (isset($form_state['flood_control_user_identifier'])) {
+      flood_register_event('failed_login_attempt_user', $form_state['flood_control_user_identifier']);
+    }
+
+    if (isset($form_state['flood_control_triggered'])) {
+      if ($form_state['flood_control_triggered'] == 'user') {
+        form_set_error('name', format_plural(variable_get('user_failed_login_user_limit', 5), 'Sorry, there has been more than one failed login attempt for this account. It is temporarily blocked. Please try again later, or <a href="@url">request a new password</a>.', 'Sorry, there have been more than @count failed login attempts for this account. It is temporarily blocked. Please try again later, or <a href="@url">request a new password</a>.', array('@url' => url('user/password'))));
+      }
+      else {
+        // We did not find a uid, so the 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'))));
+      }
+    }
+    else {
+      form_set_error('name', t('Sorry, unrecognized username or password. <a href="@password">Have you forgotten your password?</a>', array('@password' => url('user/password'))));
+      watchdog('user', 'Login attempt failed for %user.', array('%user' => $form_state['values']['name']));
+    }
+  }
+  elseif (isset($form_state['flood_control_user_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']);
   }
 }
 
 /**
- * Try to log in the user locally. $form_state['uid'] is set to
- * a user ID if successful.
+ * Try to validate the user's login credentials locally.
  *
- * @param $form_state
- *   Form submission state with at least 'name' and 'pass' keys.
+ * @param $name
+ *   User name to authenticate.
+ * @param $password
+ *   A plain-text password, such as trimmed text from form values.
+ * @return
+ *   The user's uid on success, or FALSE on failure to authenticate.
  */
-function user_authenticate(&$form_state) {
-  $password = trim($form_state['values']['pass']);
-  // Name and pass keys are required.
-  if (!empty($form_state['values']['name']) && !empty($password)) {
-    $account = db_query("SELECT * FROM {users} WHERE name = :name AND status = 1", array(':name' => $form_state['values']['name']))->fetchObject();
+function user_authenticate($name, $password) {
+  $uid = FALSE;
+  if (!empty($name) && !empty($password)) {
+    $account = db_query("SELECT * FROM {users} WHERE name = :name AND status = 1", array(':name' => $name))->fetchObject();
     if ($account) {
       // Allow alternate password hashing schemes.
       require_once DRUPAL_ROOT . '/' . variable_get('password_inc', 'includes/password.inc');
       if (user_check_password($password, $account)) {
-
-        // Successful authentication. Set a flag for user_login_final_validate().
-        $form_state['uid'] = $account->uid;
+        // Successful authentication.
+        $uid = $account->uid;
 
         // Update user to new password scheme if needed.
         if (user_needs_new_hash($account)) {
@@ -1665,6 +1728,7 @@ function user_authenticate(&$form_state) {
       }
     }
   }
+  return $uid;
 }
 
 /**
diff --git a/modules/user/user.test b/modules/user/user.test
index 82d0bcf5fdac..42fe6bf42d7c 100644
--- a/modules/user/user.test
+++ b/modules/user/user.test
@@ -159,6 +159,123 @@ class UserValidationTestCase extends DrupalWebTestCase {
   }
 }
 
+/**
+ * Functional tests for user logins, including rate limiting of login attempts.
+ */
+class UserLoginTestCase extends DrupalWebTestCase {
+  public static function getInfo() {
+    return array(
+      'name' => 'User login',
+      'description' => 'Ensure that login works as expected.',
+      'group' => 'User',
+    );
+  }
+
+  /**
+   * Test the global login flood control.
+   */
+  function testGlobalLoginFloodControl() {
+    // Set the global login limit.
+    variable_set('user_failed_login_ip_limit', 10);
+    // Set a high per-user limit out so that it is not relevant in the test.
+    variable_set('user_failed_login_user_limit', 4000);
+
+    $user1 = $this->drupalCreateUser(array());
+    $incorrect_user1 = clone $user1;
+    $incorrect_user1->pass_raw .= 'incorrect';
+
+    // Try 2 failed logins.
+    for ($i = 0; $i < 2; $i++) {
+      $this->assertFailedLogin($incorrect_user1);
+    }
+
+    // A successful login will not reset the IP-based flood control count.
+    $this->drupalLogin($user1);
+    $this->drupalLogout();
+
+    // Try 8 more failed logins, they should not trigger the flood control
+    // mechanism.
+    for ($i = 0; $i < 8; $i++) {
+      $this->assertFailedLogin($incorrect_user1);
+    }
+
+    // The next login trial should result in an IP-based flood error message.
+    $this->assertFailedLogin($incorrect_user1, 'ip');
+
+    // A login with the correct password should also result in a flood error
+    // message.
+    $this->assertFailedLogin($user1, 'ip');
+  }
+
+  /**
+   * Test the per-user login flood control.
+   */
+  function testPerUserLoginFloodControl() {
+    // Set a high global limit out so that it is not relevant in the test.
+    variable_set('user_failed_login_ip_limit', 4000);
+    // Set the per-user login limit.
+    variable_set('user_failed_login_user_limit', 3);
+
+    $user1 = $this->drupalCreateUser(array());
+    $incorrect_user1 = clone $user1;
+    $incorrect_user1->pass_raw .= 'incorrect';
+
+    $user2 = $this->drupalCreateUser(array());
+
+    // Try 2 failed logins.
+    for ($i = 0; $i < 2; $i++) {
+      $this->assertFailedLogin($incorrect_user1);
+    }
+
+    // A successful login will reset the per-user flood control count.
+    $this->drupalLogin($user1);
+    $this->drupalLogout();
+
+    // Try 3 failed logins for user 1, they will not trigger flood control.
+    for ($i = 0; $i < 3; $i++) {
+      $this->assertFailedLogin($incorrect_user1);
+    }
+
+    // Try one successful attempt for user 2, it should not trigger any
+    // flood control.
+    $this->drupalLogin($user2);
+    $this->drupalLogout();
+
+    // Try one more attempt for user 1, it should be rejected, even if the
+    // correct password has been used.
+    $this->assertFailedLogin($user1, 'user');
+  }
+
+  /**
+   * Make an unsuccessful login attempt.
+   *
+   * @param $account
+   *   A user object with name and pass_raw attributes for the login attempt.
+   * @param $flood_trigger
+   *   Whether or not to expect that the flood control mechanism will be
+   *   triggered.
+   */
+  function assertFailedLogin($account, $flood_trigger = NULL) {
+    $edit = array(
+      'name' => $account->name,
+      'pass' => $account->pass_raw,
+    );
+    $this->drupalPost('user', $edit, t('Log in'));
+    if (isset($flood_trigger)) {
+      if ($flood_trigger == 'user') {
+        $this->assertRaw(format_plural(variable_get('user_failed_login_user_limit', 5), 'Sorry, there has been more than one failed login attempt for this account. It is temporarily blocked. Please try again later, or <a href="@url">request a new password</a>.', 'Sorry, there have been more than @count failed login attempts for this account. It is temporarily blocked. Please try again later, or <a href="@url">request a new password</a>.', array('@url' => url('user/password'))));
+      }
+      else {
+        // No uid, so the limit is IP-based.
+        $this->assertRaw(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'))));
+      }
+    }
+    else {
+      $this->assertText(t('Sorry, unrecognized username or password. Have you forgotten your password?'));
+    }
+  }
+}
+
 class UserCancelTestCase extends DrupalWebTestCase {
   public static function getInfo() {
     return array(
-- 
GitLab