bug #40920 [PasswordHasher] accept hashing passwords with nul bytes o…

…r longer than 72 bytes when using bcrypt (nicolas-grekas) This PR was merged into the 5.3-dev branch. Discussion ---------- [PasswordHasher] accept hashing passwords with nul bytes or longer than 72 bytes when using bcrypt | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - This limitation of bcrypt creates a risk for migrations. But we can remove it, so here we are. Commits ------- a5d3b89472 [PasswordHasher] accept hashing passwords with nul bytes or longer than 72 bytes when using bcrypt
symfony · Apr 29, 2021 · 9c0ca75 · 9c0ca75
2 parents c6de8ca + 27c2805
commit 9c0ca75
Show file tree

Hide file tree

Showing 4 changed files with 52 additions and 12 deletions.
diff --git a/Hasher/NativePasswordHasher.php b/Hasher/NativePasswordHasher.php
@@ -53,11 +53,11 @@ public function __construct(int $opsLimit = null, int $memLimit = null, int $cos
             $algorithms = [1 => \PASSWORD_BCRYPT, '2y' => \PASSWORD_BCRYPT];
 
             if (\defined('PASSWORD_ARGON2I')) {
-                $algorithms[2] = $algorithms['argon2i'] = (string) \PASSWORD_ARGON2I;
+                $algorithms[2] = $algorithms['argon2i'] = \PASSWORD_ARGON2I;
             }
 
             if (\defined('PASSWORD_ARGON2ID')) {
-                $algorithms[3] = $algorithms['argon2id'] = (string) \PASSWORD_ARGON2ID;
+                $algorithms[3] = $algorithms['argon2id'] = \PASSWORD_ARGON2ID;
             }
 
             $this->algorithm = $algorithms[$algorithm] ?? $algorithm;
@@ -73,10 +73,14 @@ public function __construct(int $opsLimit = null, int $memLimit = null, int $cos
 
     public function hash(string $plainPassword): string
     {
-        if ($this->isPasswordTooLong($plainPassword) || ((string) \PASSWORD_BCRYPT === $this->algorithm && 72 < \strlen($plainPassword))) {
+        if ($this->isPasswordTooLong($plainPassword)) {
             throw new InvalidPasswordException();
         }
 
+        if (\PASSWORD_BCRYPT === $this->algorithm && (72 < \strlen($plainPassword) || false !== strpos($plainPassword, "\0"))) {
+            $plainPassword = base64_encode(hash('sha512', $plainPassword, true));
+        }
+
         return password_hash($plainPassword, $this->algorithm, $this->options);
     }
 
@@ -87,8 +91,12 @@ public function verify(string $hashedPassword, string $plainPassword): bool
         }
 
         if (0 !== strpos($hashedPassword, '$argon')) {
-            // BCrypt encodes only the first 72 chars
-            return (72 >= \strlen($plainPassword) || 0 !== strpos($hashedPassword, '$2')) && password_verify($plainPassword, $hashedPassword);
+            // Bcrypt cuts on NUL chars and after 72 bytes
+            if (0 === strpos($hashedPassword, '$2') && (72 < \strlen($plainPassword) || false !== strpos($plainPassword, "\0"))) {
+                $plainPassword = base64_encode(hash('sha512', $plainPassword, true));
+            }
+
+            return password_verify($plainPassword, $hashedPassword);
         }
 
         if (\extension_loaded('sodium') && version_compare(\SODIUM_LIBRARY_VERSION, '1.0.14', '>=')) {

diff --git a/Hasher/SodiumPasswordHasher.php b/Hasher/SodiumPasswordHasher.php
@@ -80,8 +80,12 @@ public function verify(string $hashedPassword, string $plainPassword): bool
         }
 
         if (0 !== strpos($hashedPassword, '$argon')) {
+            if (0 === strpos($hashedPassword, '$2') && (72 < \strlen($plainPassword) || false !== strpos($plainPassword, "\0"))) {
+                $plainPassword = base64_encode(hash('sha512', $plainPassword, true));
+            }
+
             // Accept validating non-argon passwords for seamless migrations
-            return (72 >= \strlen($plainPassword) || 0 !== strpos($hashedPassword, '$2')) && password_verify($plainPassword, $hashedPassword);
+            return password_verify($plainPassword, $hashedPassword);
         }
 
         if (\function_exists('sodium_crypto_pwhash_str_verify')) {

diff --git a/Tests/Hasher/NativePasswordHasherTest.php b/Tests/Hasher/NativePasswordHasherTest.php
@@ -89,13 +89,22 @@ public function testConfiguredAlgorithmWithLegacyConstValue()
         $this->assertStringStartsWith('$2', $result);
     }
 
-    public function testCheckPasswordLength()
+    public function testBcryptWithLongPassword()
     {
-        $hasher = new NativePasswordHasher(null, null, 4);
-        $result = password_hash(str_repeat('a', 72), \PASSWORD_BCRYPT, ['cost' => 4]);
+        $hasher = new NativePasswordHasher(null, null, 4, \PASSWORD_BCRYPT);
+        $plainPassword = str_repeat('a', 100);
 
-        $this->assertFalse($hasher->verify($result, str_repeat('a', 73), 'salt'));
-        $this->assertTrue($hasher->verify($result, str_repeat('a', 72), 'salt'));
+        $this->assertFalse($hasher->verify(password_hash($plainPassword, \PASSWORD_BCRYPT, ['cost' => 4]), $plainPassword, 'salt'));
+        $this->assertTrue($hasher->verify($hasher->hash($plainPassword), $plainPassword, 'salt'));
+    }
+
+    public function testBcryptWithNulByte()
+    {
+        $hasher = new NativePasswordHasher(null, null, 4, \PASSWORD_BCRYPT);
+        $plainPassword = "a\0b";
+
+        $this->assertFalse($hasher->verify(password_hash($plainPassword, \PASSWORD_BCRYPT, ['cost' => 4]), $plainPassword, 'salt'));
+        $this->assertTrue($hasher->verify($hasher->hash($plainPassword), $plainPassword, 'salt'));
     }
 
     public function testNeedsRehash()

diff --git a/Tests/Hasher/SodiumPasswordHasherTest.php b/Tests/Hasher/SodiumPasswordHasherTest.php
@@ -13,6 +13,7 @@
 
 use PHPUnit\Framework\TestCase;
 use Symfony\Component\PasswordHasher\Exception\InvalidPasswordException;
+use Symfony\Component\PasswordHasher\Hasher\NativePasswordHasher;
 use Symfony\Component\PasswordHasher\Hasher\SodiumPasswordHasher;
 
 class SodiumPasswordHasherTest extends TestCase
@@ -33,7 +34,7 @@ public function testValidation()
         $this->assertFalse($hasher->verify($result, '', null));
     }
 
-    public function testBCryptValidation()
+    public function testBcryptValidation()
     {
         $hasher = new SodiumPasswordHasher();
         $this->assertTrue($hasher->verify('$2y$04$M8GDODMoGQLQRpkYCdoJh.lbiZPee3SZI32RcYK49XYTolDGwoRMm', 'abc', null));
@@ -63,6 +64,24 @@ public function testCheckPasswordLength()
         $this->assertTrue($hasher->verify($result, str_repeat('a', 4096), null));
     }
 
+    public function testBcryptWithLongPassword()
+    {
+        $hasher = new SodiumPasswordHasher(null, null, 4);
+        $plainPassword = str_repeat('a', 100);
+
+        $this->assertFalse($hasher->verify(password_hash($plainPassword, \PASSWORD_BCRYPT, ['cost' => 4]), $plainPassword, 'salt'));
+        $this->assertTrue($hasher->verify((new NativePasswordHasher(null, null, 4, \PASSWORD_BCRYPT))->hash($plainPassword), $plainPassword, 'salt'));
+    }
+
+    public function testBcryptWithNulByte()
+    {
+        $hasher = new SodiumPasswordHasher(null, null, 4);
+        $plainPassword = "a\0b";
+
+        $this->assertFalse($hasher->verify(password_hash($plainPassword, \PASSWORD_BCRYPT, ['cost' => 4]), $plainPassword, 'salt'));
+        $this->assertTrue($hasher->verify((new NativePasswordHasher(null, null, 4, \PASSWORD_BCRYPT))->hash($plainPassword), $plainPassword, 'salt'));
+    }
+
     public function testUserProvidedSaltIsNotUsed()
     {
         $hasher = new SodiumPasswordHasher();