From 6780a1d3e5482c712fb5af192631104764cf19b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20=C5=A0koda?= Date: Sun, 22 Dec 2013 17:59:35 +0800 Subject: [PATCH] MDL-43481 remove unnecessary legacy password hashing --- config-dist.php | 7 +-- lib/deprecatedlib.php | 16 ++++++ lib/installlib.php | 5 -- lib/moodlelib.php | 55 +------------------ .../tests/PasswordGetInfoTest.php | 7 --- .../tests/PasswordHashTest.php | 7 --- .../tests/PasswordNeedsRehashTest.php | 7 --- .../tests/PasswordVerifyTest.php | 7 --- lib/setup.php | 1 - lib/tests/moodlelib_test.php | 51 +++++------------ 10 files changed, 35 insertions(+), 128 deletions(-) diff --git a/config-dist.php b/config-dist.php index 451fa87393e..964a646a56c 100644 --- a/config-dist.php +++ b/config-dist.php @@ -562,11 +562,8 @@ $CFG->admin = 'admin'; //========================================================================= // 10. SECRET PASSWORD SALT //========================================================================= -// A single site-wide password salt is no longer required *unless* you are -// upgrading an older version of Moodle (prior to 2.5), or if you are using -// a PHP version below 5.3.7. If upgrading, keep any values from your old -// config.php file. If you are using PHP < 5.3.7 set to a long random string -// below: +// A site-wide password salt is no longer used in new installations. +// If upgrading from 2.6 or older, keep all existing salts in config.php file. // // $CFG->passwordsaltmain = 'a_very_long_random_string_of_characters#@6&*1'; // diff --git a/lib/deprecatedlib.php b/lib/deprecatedlib.php index 394a12e1c31..a24e8ee8858 100644 --- a/lib/deprecatedlib.php +++ b/lib/deprecatedlib.php @@ -30,6 +30,22 @@ defined('MOODLE_INTERNAL') || die(); +/** + * Checks whether the password compatibility library will work with the current + * version of PHP. This cannot be done using PHP version numbers since the fix + * has been backported to earlier versions in some distributions. + * + * See https://github.com/ircmaxell/password_compat/issues/10 for more details. + * + * @deprecated since 2.7 PHP 5.4.x should be always compatible. + * + * @return bool always returns false + */ +function password_compat_not_supported() { + debugging('Do not use password_compat_not_supported() - bcrypt is now always available', DEBUG_DEVELOPER); + return false; +} + /** * Factory method that was returning moodle_session object. * diff --git a/lib/installlib.php b/lib/installlib.php index 40705f1006f..bcfc7181e51 100644 --- a/lib/installlib.php +++ b/lib/installlib.php @@ -246,11 +246,6 @@ function install_generate_configphp($database, $cfg) { } $configphp .= '$CFG->directorypermissions = ' . $chmod . ';' . PHP_EOL . PHP_EOL; - // A site-wide salt is only needed if bcrypt is not properly supported by the current version of PHP. - if (password_compat_not_supported()) { - $configphp .= '$CFG->passwordsaltmain = '.var_export(complex_random_string(), true) . ';' . PHP_EOL . PHP_EOL; - } - $configphp .= 'require_once(dirname(__FILE__) . \'/lib/setup.php\');' . PHP_EOL . PHP_EOL; $configphp .= '// There is no php closing tag in this file,' . PHP_EOL; $configphp .= '// it is intentional because it prevents trailing whitespace problems!' . PHP_EOL; diff --git a/lib/moodlelib.php b/lib/moodlelib.php index e69b3a47e83..073ecbc1775 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -4532,38 +4532,6 @@ function password_is_legacy_hash($password) { return (bool) preg_match('/^[0-9a-f]{32}$/', $password); } -/** - * Checks whether the password compatibility library will work with the current - * version of PHP. This cannot be done using PHP version numbers since the fix - * has been backported to earlier versions in some distributions. - * - * See https://github.com/ircmaxell/password_compat/issues/10 for more details. - * - * @return bool True if the library is NOT supported. - */ -function password_compat_not_supported() { - - $hash = '$2y$04$usesomesillystringfore7hnbRJHxXVLeakoG8K30oukPsA.ztMG'; - - // Create a one off application cache to store bcrypt support status as - // the support status doesn't change and crypt() is slow. - $cache = cache::make_from_params(cache_store::MODE_APPLICATION, 'core', 'password_compat'); - - if (!$bcryptsupport = $cache->get('bcryptsupport')) { - $test = crypt('password', $hash); - // Cache string instead of boolean to avoid MDL-37472. - if ($test == $hash) { - $bcryptsupport = 'supported'; - } else { - $bcryptsupport = 'not supported'; - } - $cache->set('bcryptsupport', $bcryptsupport); - } - - // Return true if bcrypt *not* supported. - return ($bcryptsupport !== 'supported'); -} - /** * Compare password against hash stored in user object to determine if it is valid. * @@ -4638,15 +4606,6 @@ function hash_internal_user_password($password, $fasthash = false) { global $CFG; require_once($CFG->libdir.'/password_compat/lib/password.php'); - // Use the legacy hashing algorithm (md5) if PHP is not new enough to support bcrypt properly. - if (password_compat_not_supported()) { - if (isset($CFG->passwordsaltmain)) { - return md5($password.$CFG->passwordsaltmain); - } else { - return md5($password); - } - } - // Set the cost factor to 4 for fast hashing, otherwise use default cost. $options = ($fasthash) ? array('cost' => 4) : array(); @@ -4679,9 +4638,6 @@ function update_internal_user_password($user, $password) { global $CFG, $DB; require_once($CFG->libdir.'/password_compat/lib/password.php'); - // Use the legacy hashing algorithm (md5) if PHP doesn't support bcrypt properly. - $legacyhash = password_compat_not_supported(); - // Figure out what the hashed password should be. $authplugin = get_auth_plugin($user->auth); if ($authplugin->prevent_local_passwords()) { @@ -4690,14 +4646,9 @@ function update_internal_user_password($user, $password) { $hashedpassword = hash_internal_user_password($password); } - if ($legacyhash) { - $passwordchanged = ($user->password !== $hashedpassword); - $algorithmchanged = false; - } else { - // If verification fails then it means the password has changed. - $passwordchanged = !password_verify($password, $user->password); - $algorithmchanged = password_needs_rehash($user->password, PASSWORD_DEFAULT); - } + // If verification fails then it means the password has changed. + $passwordchanged = !password_verify($password, $user->password); + $algorithmchanged = password_needs_rehash($user->password, PASSWORD_DEFAULT); if ($passwordchanged || $algorithmchanged) { $DB->set_field('user', 'password', $hashedpassword, array('id' => $user->id)); diff --git a/lib/password_compat/tests/PasswordGetInfoTest.php b/lib/password_compat/tests/PasswordGetInfoTest.php index 215cd3c46dd..633549d4799 100644 --- a/lib/password_compat/tests/PasswordGetInfoTest.php +++ b/lib/password_compat/tests/PasswordGetInfoTest.php @@ -5,13 +5,6 @@ require_once($CFG->dirroot . '/lib/password_compat/lib/password.php'); class PasswordGetInfoTest extends PHPUnit_Framework_TestCase { - protected function setUp() { - if (password_compat_not_supported()) { - // Skip test if password_compat is not supported. - $this->markTestSkipped('password_compat not supported'); - } - } - public static function provideInfo() { return array( array('foo', array('algo' => 0, 'algoName' => 'unknown', 'options' => array())), diff --git a/lib/password_compat/tests/PasswordHashTest.php b/lib/password_compat/tests/PasswordHashTest.php index 010ea341ec3..a19191c8df1 100644 --- a/lib/password_compat/tests/PasswordHashTest.php +++ b/lib/password_compat/tests/PasswordHashTest.php @@ -5,13 +5,6 @@ require_once($CFG->dirroot . '/lib/password_compat/lib/password.php'); class PasswordHashTest extends PHPUnit_Framework_TestCase { - protected function setUp() { - if (password_compat_not_supported()) { - // Skip test if password_compat is not supported. - $this->markTestSkipped('password_compat not supported'); - } - } - public function testFuncExists() { $this->assertTrue(function_exists('password_hash')); } diff --git a/lib/password_compat/tests/PasswordNeedsRehashTest.php b/lib/password_compat/tests/PasswordNeedsRehashTest.php index f6fa0e36b1d..e947d4d8c21 100644 --- a/lib/password_compat/tests/PasswordNeedsRehashTest.php +++ b/lib/password_compat/tests/PasswordNeedsRehashTest.php @@ -5,13 +5,6 @@ require_once($CFG->dirroot . '/lib/password_compat/lib/password.php'); class PasswordNeedsRehashTest extends PHPUnit_Framework_TestCase { - protected function setUp() { - if (password_compat_not_supported()) { - // Skip test if password_compat is not supported. - $this->markTestSkipped('password_compat not supported'); - } - } - public static function provideCases() { return array( array('foo', 0, array(), false), diff --git a/lib/password_compat/tests/PasswordVerifyTest.php b/lib/password_compat/tests/PasswordVerifyTest.php index 80b007b61b7..0e865a5c4fd 100644 --- a/lib/password_compat/tests/PasswordVerifyTest.php +++ b/lib/password_compat/tests/PasswordVerifyTest.php @@ -5,13 +5,6 @@ require_once($CFG->dirroot . '/lib/password_compat/lib/password.php'); class PasswordVerifyTest extends PHPUnit_Framework_TestCase { - protected function setUp() { - if (password_compat_not_supported()) { - // Skip test if password_compat is not supported. - $this->markTestSkipped('password_compat not supported'); - } - } - public function testFuncExists() { $this->assertTrue(function_exists('password_verify')); } diff --git a/lib/setup.php b/lib/setup.php index f1d3054e4fd..268dcd0bacb 100644 --- a/lib/setup.php +++ b/lib/setup.php @@ -128,7 +128,6 @@ if (defined('BEHAT_SITE_RUNNING')) { // Now we can begin switching $CFG->X for $CFG->behat_X. $CFG->wwwroot = $CFG->behat_wwwroot; - $CFG->passwordsaltmain = 'moodle'; $CFG->prefix = $CFG->behat_prefix; $CFG->dataroot = $CFG->behat_dataroot; } diff --git a/lib/tests/moodlelib_test.php b/lib/tests/moodlelib_test.php index 5894dada6fd..c6afa0957d0 100644 --- a/lib/tests/moodlelib_test.php +++ b/lib/tests/moodlelib_test.php @@ -2181,24 +2181,13 @@ class core_moodlelib_testcase extends advanced_testcase { * Test function validate_internal_user_password(). */ public function test_validate_internal_user_password() { - if (password_compat_not_supported()) { - // If bcrypt is not properly supported test legacy md5 hashes instead. - // Can't hardcode these as we don't know the site's password salt. - $validhashes = array( - 'pw' => hash_internal_user_password('pw'), - 'abc' => hash_internal_user_password('abc'), - 'C0mP1eX_&} hash_internal_user_password('C0mP1eX_&} hash_internal_user_password('ĩńťėŕňăţĩōŋāĹ') - ); - } else { - // Otherwise test bcrypt hashes. - $validhashes = array( - 'pw' => '$2y$10$LOSDi5eaQJhutSRun.OVJ.ZSxQZabCMay7TO1KmzMkDMPvU40zGXK', - 'abc' => '$2y$10$VWTOhVdsBbWwtdWNDRHSpewjd3aXBQlBQf5rBY/hVhw8hciarFhXa', - 'C0mP1eX_&} '$2y$10$3PJf.q.9ywNJlsInPbqc8.IFeSsvXrGvQLKRFBIhVu1h1I3vpIry6', - 'ĩńťėŕňăţĩōŋāĹ' => '$2y$10$3A2Y8WpfRAnP3czJiSv6N.6Xp0T8hW3QZz2hUCYhzyWr1kGP1yUve' - ); - } + // Otherwise test bcrypt hashes. + $validhashes = array( + 'pw' => '$2y$10$LOSDi5eaQJhutSRun.OVJ.ZSxQZabCMay7TO1KmzMkDMPvU40zGXK', + 'abc' => '$2y$10$VWTOhVdsBbWwtdWNDRHSpewjd3aXBQlBQf5rBY/hVhw8hciarFhXa', + 'C0mP1eX_&} '$2y$10$3PJf.q.9ywNJlsInPbqc8.IFeSsvXrGvQLKRFBIhVu1h1I3vpIry6', + 'ĩńťėŕňăţĩōŋāĹ' => '$2y$10$3A2Y8WpfRAnP3czJiSv6N.6Xp0T8hW3QZz2hUCYhzyWr1kGP1yUve' + ); foreach ($validhashes as $password => $hash) { $user = new stdClass(); @@ -2227,17 +2216,12 @@ class core_moodlelib_testcase extends advanced_testcase { $user->password = $hash; $this->assertTrue(validate_internal_user_password($user, $password)); - if (password_compat_not_supported()) { - // If bcrypt is not properly supported make sure the passwords are in md5 format. - $this->assertTrue(password_is_legacy_hash($hash)); - } else { - // Otherwise they should not be in md5 format. - $this->assertFalse(password_is_legacy_hash($hash)); + // Otherwise they should not be in md5 format. + $this->assertFalse(password_is_legacy_hash($hash)); - // Check that cost factor in hash is correctly set. - $this->assertRegExp('/\$10\$/', $hash); - $this->assertRegExp('/\$04\$/', $fasthash); - } + // Check that cost factor in hash is correctly set. + $this->assertRegExp('/\$10\$/', $hash); + $this->assertRegExp('/\$04\$/', $fasthash); } } @@ -2265,15 +2249,8 @@ class core_moodlelib_testcase extends advanced_testcase { // Update the password. update_internal_user_password($user, 'password'); - if (password_compat_not_supported()) { - // If bcrypt not properly supported the password should remain as an md5 hash. - $expected_hash = hash_internal_user_password('password', true); - $this->assertSame($user->password, $expected_hash); - $this->assertTrue(password_is_legacy_hash($user->password)); - } else { - // Otherwise password should have been updated to a bcrypt hash. - $this->assertFalse(password_is_legacy_hash($user->password)); - } + // Otherwise password should have been updated to a bcrypt hash. + $this->assertFalse(password_is_legacy_hash($user->password)); } public function test_fullname() {