diff options
author | Dries Buytaert <dries@buytaert.net> | 2010-12-18 00:56:18 +0000 |
---|---|---|
committer | Dries Buytaert <dries@buytaert.net> | 2010-12-18 00:56:18 +0000 |
commit | 57b1af03188120e4e76b8e1304123b724dd25aca (patch) | |
tree | d72679b007bd727e5a64db107b797188da00ede0 | |
parent | 4b687afc002b0608a730e82d4ad5d605347e55bc (diff) | |
download | brdo-57b1af03188120e4e76b8e1304123b724dd25aca.tar.gz brdo-57b1af03188120e4e76b8e1304123b724dd25aca.tar.bz2 |
- Patch #991270 by carlos8f, chx: password_count_log2 var out of bounds is a sorry mess.
-rw-r--r-- | includes/password.inc | 32 | ||||
-rw-r--r-- | modules/simpletest/simpletest.info | 1 | ||||
-rw-r--r-- | modules/simpletest/tests/password.test | 61 | ||||
-rw-r--r-- | modules/user/user.module | 7 | ||||
-rw-r--r-- | modules/user/user.test | 25 |
5 files changed, 116 insertions, 10 deletions
diff --git a/includes/password.inc b/includes/password.inc index 4940c50f9..1c6672af1 100644 --- a/includes/password.inc +++ b/includes/password.inc @@ -99,18 +99,38 @@ function _password_base64_encode($input, $count) { */ function _password_generate_salt($count_log2) { $output = '$S$'; - // Minimum log2 iterations is DRUPAL_MIN_HASH_COUNT. - $count_log2 = max($count_log2, DRUPAL_MIN_HASH_COUNT); - // Maximum log2 iterations is DRUPAL_MAX_HASH_COUNT. + // Ensure that $count_log2 is within set bounds. + $count_log2 = _password_enforce_log2_boundaries($count_log2); // We encode the final log2 iteration count in base 64. $itoa64 = _password_itoa64(); - $output .= $itoa64[min($count_log2, DRUPAL_MAX_HASH_COUNT)]; + $output .= $itoa64[$count_log2]; // 6 bytes is the standard salt for a portable phpass hash. $output .= _password_base64_encode(drupal_random_bytes(6), 6); return $output; } /** + * Ensures that $count_log2 is within set bounds. + * + * @param $count_log2 + * Integer that determines the number of iterations used in the hashing + * process. A larger value is more secure, but takes more time to complete. + * + * @return + * Integer within set bounds that is closest to $count_log2. + */ +function _password_enforce_log2_boundaries($count_log2) { + if ($count_log2 < DRUPAL_MIN_HASH_COUNT) { + return DRUPAL_MIN_HASH_COUNT; + } + elseif ($count_log2 > DRUPAL_MAX_HASH_COUNT) { + return DRUPAL_MAX_HASH_COUNT; + } + + return (int) $count_log2; +} + +/** * Hash a password using a secure stretched hash. * * By using a salt and repeated hashing the password is "stretched". Its @@ -261,7 +281,9 @@ function user_needs_new_hash($account) { if ((substr($account->pass, 0, 3) != '$S$') || (strlen($account->pass) != DRUPAL_HASH_LENGTH)) { return TRUE; } + // Ensure that $count_log2 is within set bounds. + $count_log2 = _password_enforce_log2_boundaries(variable_get('password_count_log2', DRUPAL_HASH_COUNT)); // Check whether the iteration count used differs from the standard number. - return (_password_get_count_log2($account->pass) != variable_get('password_count_log2', DRUPAL_HASH_COUNT)); + return (_password_get_count_log2($account->pass) !== $count_log2); } diff --git a/modules/simpletest/simpletest.info b/modules/simpletest/simpletest.info index dded764ee..69f9d6e3d 100644 --- a/modules/simpletest/simpletest.info +++ b/modules/simpletest/simpletest.info @@ -31,6 +31,7 @@ files[] = tests/lock.test files[] = tests/mail.test files[] = tests/menu.test files[] = tests/module.test +files[] = tests/password.test files[] = tests/path.test files[] = tests/registry.test files[] = tests/schema.test diff --git a/modules/simpletest/tests/password.test b/modules/simpletest/tests/password.test new file mode 100644 index 000000000..20e77f28d --- /dev/null +++ b/modules/simpletest/tests/password.test @@ -0,0 +1,61 @@ +<?php +// $Id$ + +/** + * @file + * Provides unit tests for password.inc. + */ + +/** + * Unit tests for password hashing API. + */ +class PasswordHashingTest extends DrupalWebTestCase { + protected $profile = 'testing'; + + public static function getInfo() { + return array( + 'name' => 'Password hashing', + 'description' => 'Password hashing unit tests.', + 'group' => 'System', + ); + } + + function setUp() { + require_once DRUPAL_ROOT . '/' . variable_get('password_inc', 'includes/password.inc'); + parent::setUp(); + } + + /** + * Test password hashing. + */ + function testPasswordHashing() { + // Set a log2 iteration count that is deliberately out of bounds to test + // that it is corrected to be within bounds. + variable_set('password_count_log2', 1); + // Set up a fake $account with a password 'baz', hashed with md5. + $password = 'baz'; + $account = (object) array('name' => 'foo', 'pass' => md5($password)); + // The md5 password should be flagged as needing an update. + $this->assertTrue(user_needs_new_hash($account), t('User with md5 password needs a new hash.')); + // Re-hash the password. + $old_hash = $account->pass; + $account->pass = user_hash_password($password); + $this->assertIdentical(_password_get_count_log2($account->pass), DRUPAL_MIN_HASH_COUNT, t('Re-hashed password has the minimum number of log2 iterations.')); + $this->assertTrue($account->pass != $old_hash, t('Password hash changed.')); + $this->assertTrue(user_check_password($password, $account), t('Password check succeeds.')); + // Since the log2 setting hasn't changed and the user has a valid password, + // user_needs_new_hash() should return FALSE. + $this->assertFalse(user_needs_new_hash($account), t('User does not need a new hash.')); + // Increment the log2 iteration to MIN + 1. + variable_set('password_count_log2', DRUPAL_MIN_HASH_COUNT + 1); + $this->assertTrue(user_needs_new_hash($account), t('User needs a new hash after incrementing the log2 count.')); + // Re-hash the password. + $old_hash = $account->pass; + $account->pass = user_hash_password($password); + $this->assertIdentical(_password_get_count_log2($account->pass), DRUPAL_MIN_HASH_COUNT + 1, t('Re-hashed password has the correct number of log2 iterations.')); + $this->assertTrue($account->pass != $old_hash, t('Password hash changed again.')); + // Now the hash should be OK. + $this->assertFalse(user_needs_new_hash($account), t('Re-hashed password does not need a new hash.')); + $this->assertTrue(user_check_password($password, $account), t('Password check succeeds with re-hashed password.')); + } +} diff --git a/modules/user/user.module b/modules/user/user.module index d464a7a7b..5411d35e9 100644 --- a/modules/user/user.module +++ b/modules/user/user.module @@ -2169,7 +2169,7 @@ function user_login_final_validate($form, &$form_state) { 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(); + $account = user_load_by_name($name); if ($account) { // Allow alternate password hashing schemes. require_once DRUPAL_ROOT . '/' . variable_get('password_inc', 'includes/password.inc'); @@ -2181,10 +2181,7 @@ function user_authenticate($name, $password) { if (user_needs_new_hash($account)) { $new_hash = user_hash_password($password); if ($new_hash) { - db_update('users') - ->fields(array('pass' => $new_hash)) - ->condition('uid', $account->uid) - ->execute(); + user_save($account, array('pass' => $new_hash)); } } } diff --git a/modules/user/user.test b/modules/user/user.test index a49a89b5c..d999c85e2 100644 --- a/modules/user/user.test +++ b/modules/user/user.test @@ -367,6 +367,31 @@ class UserLoginTestCase extends DrupalWebTestCase { } /** + * Test that user password is re-hashed upon login after changing $count_log2. + */ + function testPasswordRehashOnLogin() { + // Load password hashing API. + require_once DRUPAL_ROOT . '/' . variable_get('password_inc', 'includes/password.inc'); + // Set initial $count_log2 to the default, DRUPAL_HASH_COUNT. + variable_set('password_count_log2', DRUPAL_HASH_COUNT); + // Create a new user and authenticate. + $account = $this->drupalCreateUser(array()); + $password = $account->pass_raw; + $this->drupalLogin($account); + $this->drupalLogout(); + // Load the stored user. The password hash should reflect $count_log2. + $account = user_load($account->uid); + $this->assertIdentical(_password_get_count_log2($account->pass), DRUPAL_HASH_COUNT); + // Change $count_log2 and log in again. + variable_set('password_count_log2', DRUPAL_HASH_COUNT + 1); + $account->pass_raw = $password; + $this->drupalLogin($account); + // Load the stored user, which should have a different password hash now. + $account = user_load($account->uid, TRUE); + $this->assertIdentical(_password_get_count_log2($account->pass), DRUPAL_HASH_COUNT + 1); + } + + /** * Make an unsuccessful login attempt. * * @param $account |