MDL-43481 remove unnecessary legacy password hashing

Andmat7 · Dec 27, 2013 · 6780a1d · 6780a1d
1 parent bbb291b
commit 6780a1d
Show file tree

Hide file tree

Showing 10 changed files with 35 additions and 128 deletions.
diff --git a/config-dist.php b/config-dist.php
@@ -562,11 +562,8 @@
 //=========================================================================
 // 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
@@ -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
@@ -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
@@ -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
@@ -5,13 +5,6 @@
 
 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
@@ -5,13 +5,6 @@
 
 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
@@ -5,13 +5,6 @@
 
 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
@@ -5,13 +5,6 @@
 
 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
@@ -128,7 +128,6 @@
 
         // 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
@@ -2181,24 +2181,13 @@ public function test_password_is_legacy_hash() {
      * 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 @@ public function test_hash_internal_user_password() {
             $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 @@ public function test_update_internal_user_password() {
         // 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() {