diff --git CHANGELOG.txt CHANGELOG.txt index 0486ce0..055d5b6 100644 --- CHANGELOG.txt +++ 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 includes/common.inc includes/common.inc index 7758686..260bce4 100644 --- includes/common.inc +++ 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 modules/system/system.install modules/system/system.install index c684b65..3882aca 100644 --- modules/system/system.install +++ 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'), ), ); @@ -2220,6 +2220,17 @@ function system_update_7030() { } /** +* Alter field hostname to identifier in the {flood} table. + */ +function system_update_7031() { + $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 modules/user/user.module modules/user/user.module index 668b508..bd9da42 100644 --- modules/user/user.module +++ 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_uid'] = 0; + 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_uid'] = $account->uid; + 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. Have you forgotten your password?', 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_uid'])) { + if ($form_state['flood_control_uid'] > 0) { + 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 request a new password.', 'Sorry, there have been more than @count failed login attempts for this account. It is temporarily blocked. Please try again later, or request a new password.', 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 request a new password.', array('@url' => url('user/password')))); + } + } + else { + form_set_error('name', t('Sorry, unrecognized username or password. Have you forgotten your password?', 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 modules/user/user.test modules/user/user.test index c05c38e..843a561 100644 --- modules/user/user.test +++ modules/user/user.test @@ -159,6 +159,120 @@ class UserValidationTestCase extends DrupalWebTestCase { } } +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, 0); + + // A login with the correct password should also result in a flood error + // message. + $this->assertFailedLogin($user1, 0); + } + + /** + * 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, $user1->uid); + } + + /** + * Make an unsuccessful login attempt. + * + * @param $account + * A user object with name and pass_raw attributes for the login attempt. + * @param $flood_uid + * Whether or not to expect that the flood control mechanism will be + * triggered. + */ + function assertFailedLogin($account, $flood_uid = NULL) { + $edit = array( + 'name' => $account->name, + 'pass' => $account->pass_raw, + ); + $this->drupalPost('user', $edit, t('Log in')); + if (isset($flood_uid)) { + if ($flood_uid > 0) { + $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 request a new password.', 'Sorry, there have been more than @count failed login attempts for this account. It is temporarily blocked. Please try again later, or request a new password.', 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 request a new password.', 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(