Commit 57b1af03 authored by Dries's avatar Dries
Browse files

- Patch #991270 by carlos8f, chx: password_count_log2 var out of bounds is a sorry mess.

parent 4b687afc
......@@ -99,17 +99,37 @@ 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.
*
......@@ -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);
}
......@@ -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
......
<?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.'));
}
}
......@@ -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));
}
}
}
......
......@@ -366,6 +366,31 @@ class UserLoginTestCase extends DrupalWebTestCase {
$this->assertFailedLogin($user1, 'user');
}
/**
* 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.
*
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment