[11.x] Rehash user passwords when validating credentials (#48665)

* Rehash user passwords when validating credentials * Fix style violations * Remove hardcoded password when it's changable * Shift rehashing into SessionGuard The Session guard's attempt() method is a better place to apply rehashing than the validateCredentials() method on the provider. The latter shouldn't have side-effects, as per it's name. * Fix style violation * Add config option to disable rehashing on login * Clean up rehash flag injection * Fix contract in DatabaseUserProvider * Fixing return type in the docblocks * Use hash_equals() for a secure string comparison * formatting * formatting, leverage method on logoutOtherDevices * Fix spelling of passwords Co-authored-by: Chrysanthos <48060191+chrysanthos@users.noreply.github.com> --------- Co-authored-by: Taylor Otwell <taylor@laravel.com> Co-authored-by: Chrysanthos <48060191+chrysanthos@users.noreply.github.com>
laravel · Dec 10, 2023 · ceb8ed2 · Thiritin · Mar 13, 2024 · ceb8ed2
1 parent 6e302e8
commit ceb8ed2
Show file tree

Hide file tree

Showing 12 changed files with 270 additions and 14 deletions.
diff --git a/src/Illuminate/Auth/AuthManager.php b/src/Illuminate/Auth/AuthManager.php
@@ -128,6 +128,7 @@ public function createSessionDriver($name, $config)
             $name,
             $provider,
             $this->app['session.store'],
+            rehashOnLogin: $this->app['config']->get('hashing.rehash_on_login', true),
         );
 
         // When using the remember me functionality of the authentication services we

diff --git a/src/Illuminate/Auth/Authenticatable.php b/src/Illuminate/Auth/Authenticatable.php
@@ -4,6 +4,13 @@
 
 trait Authenticatable
 {
+    /**
+     * The column name of the password field using during authentication.
+     *
+     * @var string
+     */
+    protected $authPasswordName = 'password';
+
     /**
      * The column name of the "remember me" token.
      *
@@ -41,14 +48,24 @@ public function getAuthIdentifierForBroadcasting()
         return $this->getAuthIdentifier();
     }
 
+    /**
+     * Get the name of the password attribute for the user.
+     *
+     * @return string
+     */
+    public function getAuthPasswordName()
+    {
+        return $this->authPasswordName;
+    }
+
     /**
      * Get the password for the user.
      *
      * @return string
      */
     public function getAuthPassword()
     {
-        return $this->password;
+        return $this->{$this->getAuthPasswordName()};
     }
 
     /**

diff --git a/src/Illuminate/Auth/DatabaseUserProvider.php b/src/Illuminate/Auth/DatabaseUserProvider.php
@@ -158,4 +158,23 @@ public function validateCredentials(UserContract $user, array $credentials)
             $credentials['password'], $user->getAuthPassword()
         );
     }
+
+    /**
+     * Rehash the user's password if required and supported.
+     *
+     * @param  \Illuminate\Contracts\Auth\Authenticatable  $user
+     * @param  array  $credentials
+     * @param  bool  $force
+     * @return void
+     */
+    public function rehashPasswordIfRequired(UserContract $user, array $credentials, bool $force = false)
+    {
+        if (! $this->hasher->needsRehash($user->getAuthPassword()) && ! $force) {
+            return;
+        }
+
+        $this->connection->table($this->table)
+            ->where($user->getAuthIdentifierName(), $user->getAuthIdentifier())
+            ->update([$user->getAuthPasswordName() => $this->hasher->make($credentials['password'])]);
+    }
 }
diff --git a/src/Illuminate/Auth/EloquentUserProvider.php b/src/Illuminate/Auth/EloquentUserProvider.php
@@ -155,6 +155,25 @@ public function validateCredentials(UserContract $user, array $credentials)
         return $this->hasher->check($plain, $user->getAuthPassword());
     }
 
+    /**
+     * Rehash the user's password if required and supported.
+     *
+     * @param  \Illuminate\Contracts\Auth\Authenticatable  $user
+     * @param  array  $credentials
+     * @param  bool  $force
+     * @return void
+     */
+    public function rehashPasswordIfRequired(UserContract $user, array $credentials, bool $force = false)
+    {
+        if (! $this->hasher->needsRehash($user->getAuthPassword()) && ! $force) {
+            return;
+        }
+
+        $user->forceFill([
+            $user->getAuthPasswordName() => $this->hasher->make($credentials['password']),
+        ])->save();
+    }
+
     /**
      * Get a new query builder for the model instance.
      *

diff --git a/src/Illuminate/Auth/GenericUser.php b/src/Illuminate/Auth/GenericUser.php
@@ -44,6 +44,16 @@ public function getAuthIdentifier()
         return $this->attributes[$this->getAuthIdentifierName()];
     }
 
+    /**
+     * Get the name of the password attribute for the user.
+     *
+     * @return string
+     */
+    public function getAuthPasswordName()
+    {
+        return 'password';
+    }
+
     /**
      * Get the password for the user.
      *

diff --git a/src/Illuminate/Auth/SessionGuard.php b/src/Illuminate/Auth/SessionGuard.php
@@ -96,6 +96,13 @@ class SessionGuard implements StatefulGuard, SupportsBasicAuth
      */
     protected $timebox;
 
+    /**
+     * Indicates if passwords should be rehashed on login if needed.
+     *
+     * @var bool
+     */
+    protected $rehashOnLogin;
+
     /**
      * Indicates if the logout method has been called.
      *
@@ -118,19 +125,22 @@ class SessionGuard implements StatefulGuard, SupportsBasicAuth
      * @param  \Illuminate\Contracts\Session\Session  $session
      * @param  \Symfony\Component\HttpFoundation\Request|null  $request
      * @param  \Illuminate\Support\Timebox|null  $timebox
+     * @param  bool  $rehashOnLogin
      * @return void
      */
     public function __construct($name,
                                 UserProvider $provider,
                                 Session $session,
                                 Request $request = null,
-                                Timebox $timebox = null)
+                                Timebox $timebox = null,
+                                bool $rehashOnLogin = true)
     {
         $this->name = $name;
         $this->session = $session;
         $this->request = $request;
         $this->provider = $provider;
         $this->timebox = $timebox ?: new Timebox;
+        $this->rehashOnLogin = $rehashOnLogin;
     }
 
     /**
@@ -384,6 +394,8 @@ public function attempt(array $credentials = [], $remember = false)
         // to validate the user against the given credentials, and if they are in
         // fact valid we'll log the users into the application and return true.
         if ($this->hasValidCredentials($user, $credentials)) {
+            $this->rehashPasswordIfRequired($user, $credentials);
+
             $this->login($user, $remember);
 
             return true;
@@ -415,6 +427,8 @@ public function attemptWhen(array $credentials = [], $callbacks = null, $remembe
         // the user is retrieved and validated. If one of the callbacks returns falsy we do
         // not login the user. Instead, we will fail the specific authentication attempt.
         if ($this->hasValidCredentials($user, $credentials) && $this->shouldLogin($callbacks, $user)) {
+            $this->rehashPasswordIfRequired($user, $credentials);
+
             $this->login($user, $remember);
 
             return true;
@@ -465,6 +479,20 @@ protected function shouldLogin($callbacks, AuthenticatableContract $user)
         return true;
     }
 
+    /**
+     * Rehash the user's password if enabled and required.
+     *
+     * @param  \Illuminate\Contracts\Auth\Authenticatable  $user
+     * @param  array  $credentials
+     * @return void
+     */
+    protected function rehashPasswordIfRequired(AuthenticatableContract $user, array $credentials)
+    {
+        if ($this->rehashOnLogin) {
+            $this->provider->rehashPasswordIfRequired($user, $credentials);
+        }
+    }
+
     /**
      * Log the given user ID into the application.
      *
@@ -656,18 +684,17 @@ protected function cycleRememberToken(AuthenticatableContract $user)
      * The application must be using the AuthenticateSession middleware.
      *
      * @param  string  $password
-     * @param  string  $attribute
      * @return \Illuminate\Contracts\Auth\Authenticatable|null
      *
      * @throws \Illuminate\Auth\AuthenticationException
      */
-    public function logoutOtherDevices($password, $attribute = 'password')
+    public function logoutOtherDevices($password)
     {
         if (! $this->user()) {
             return;
         }
 
-        $result = $this->rehashUserPassword($password, $attribute);
+        $result = $this->rehashUserPasswordForDeviceLogout($password);
 
         if ($this->recaller() ||
             $this->getCookieJar()->hasQueued($this->getRecallerName())) {
@@ -680,23 +707,24 @@ public function logoutOtherDevices($password, $attribute = 'password')
     }
 
     /**
-     * Rehash the current user's password.
+     * Rehash the current user's password for logging out other devices via AuthenticateSession.
      *
      * @param  string  $password
-     * @param  string  $attribute
      * @return \Illuminate\Contracts\Auth\Authenticatable|null
      *
      * @throws \InvalidArgumentException
      */
-    protected function rehashUserPassword($password, $attribute)
+    protected function rehashUserPasswordForDeviceLogout($password)
     {
-        if (! Hash::check($password, $this->user()->{$attribute})) {
+        $user = $this->user();
+
+        if (! Hash::check($password, $user->getAuthPassword())) {
             throw new InvalidArgumentException('The given password does not match the current password.');
         }
 
-        return tap($this->user()->forceFill([
-            $attribute => Hash::make($password),
-        ]))->save();
+        $this->provider->rehashPasswordIfRequired(
+            $user, ['password' => $password], force: true
+        );
     }
 
     /**

diff --git a/src/Illuminate/Contracts/Auth/Authenticatable.php b/src/Illuminate/Contracts/Auth/Authenticatable.php
@@ -18,6 +18,13 @@ public function getAuthIdentifierName();
      */
     public function getAuthIdentifier();
 
+    /**
+     * Get the name of the password attribute for the user.
+     *
+     * @return string
+     */
+    public function getAuthPasswordName();
+
     /**
      * Get the password for the user.
      *

diff --git a/src/Illuminate/Contracts/Auth/UserProvider.php b/src/Illuminate/Contracts/Auth/UserProvider.php
@@ -46,4 +46,14 @@ public function retrieveByCredentials(array $credentials);
      * @return bool
      */
     public function validateCredentials(Authenticatable $user, array $credentials);
+
+    /**
+     * Rehash the user's password if required and supported.
+     *
+     * @param  \Illuminate\Contracts\Auth\Authenticatable  $user
+     * @param  array  $credentials
+     * @param  bool  $force
+     * @return void
+     */
+    public function rehashPasswordIfRequired(Authenticatable $user, array $credentials, bool $force = false);
 }
diff --git a/src/Illuminate/Session/Middleware/AuthenticateSession.php b/src/Illuminate/Session/Middleware/AuthenticateSession.php
@@ -51,7 +51,7 @@ public function handle($request, Closure $next)
         if ($this->guard()->viaRemember()) {
             $passwordHash = explode('|', $request->cookies->get($this->guard()->getRecallerName()))[2] ?? null;
 
-            if (! $passwordHash || $passwordHash != $request->user()->getAuthPassword()) {
+            if (! $passwordHash || ! hash_equals($request->user()->getAuthPassword(), $passwordHash)) {
                 $this->logout($request);
             }
         }
@@ -60,7 +60,7 @@ public function handle($request, Closure $next)
             $this->storePasswordHashInSession($request);
         }
 
-        if ($request->session()->get('password_hash_'.$this->auth->getDefaultDriver()) !== $request->user()->getAuthPassword()) {
+        if (! hash_equals($request->session()->get('password_hash_'.$this->auth->getDefaultDriver()), $request->user()->getAuthPassword())) {
             $this->logout($request);
         }
 

diff --git a/tests/Auth/AuthDatabaseUserProviderTest.php b/tests/Auth/AuthDatabaseUserProviderTest.php
@@ -7,6 +7,7 @@
 use Illuminate\Contracts\Auth\Authenticatable;
 use Illuminate\Contracts\Hashing\Hasher;
 use Illuminate\Database\Connection;
+use Illuminate\Database\ConnectionInterface;
 use Mockery as m;
 use PHPUnit\Framework\TestCase;
 use stdClass;
@@ -160,4 +161,61 @@ public function testCredentialValidation()
 
         $this->assertTrue($result);
     }
+
+    public function testCredentialValidationFailed()
+    {
+        $conn = m::mock(Connection::class);
+        $hasher = m::mock(Hasher::class);
+        $hasher->shouldReceive('check')->once()->with('plain', 'hash')->andReturn(false);
+        $provider = new DatabaseUserProvider($conn, $hasher, 'foo');
+        $user = m::mock(Authenticatable::class);
+        $user->shouldReceive('getAuthPassword')->once()->andReturn('hash');
+        $result = $provider->validateCredentials($user, ['password' => 'plain']);
+
+        $this->assertFalse($result);
+    }
+
+    public function testRehashPasswordIfRequired()
+    {
+        $hasher = m::mock(Hasher::class);
+        $hasher->shouldReceive('needsRehash')->once()->with('hash')->andReturn(true);
+        $hasher->shouldReceive('make')->once()->with('plain')->andReturn('rehashed');
+
+        $conn = m::mock(Connection::class);
+        $table = m::mock(ConnectionInterface::class);
+        $conn->shouldReceive('table')->once()->with('foo')->andReturn($table);
+        $table->shouldReceive('where')->once()->with('id', 1)->andReturnSelf();
+        $table->shouldReceive('update')->once()->with(['password_attribute' => 'rehashed']);
+
+        $user = m::mock(Authenticatable::class);
+        $user->shouldReceive('getAuthIdentifierName')->once()->andReturn('id');
+        $user->shouldReceive('getAuthIdentifier')->once()->andReturn(1);
+        $user->shouldReceive('getAuthPassword')->once()->andReturn('hash');
+        $user->shouldReceive('getAuthPasswordName')->once()->andReturn('password_attribute');
+
+        $provider = new DatabaseUserProvider($conn, $hasher, 'foo');
+        $provider->rehashPasswordIfRequired($user, ['password' => 'plain']);
+    }
+
+    public function testDontRehashPasswordIfNotRequired()
+    {
+        $hasher = m::mock(Hasher::class);
+        $hasher->shouldReceive('needsRehash')->once()->with('hash')->andReturn(false);
+        $hasher->shouldNotReceive('make');
+
+        $conn = m::mock(Connection::class);
+        $table = m::mock(ConnectionInterface::class);
+        $conn->shouldNotReceive('table');
+        $table->shouldNotReceive('where');
+        $table->shouldNotReceive('update');
+
+        $user = m::mock(Authenticatable::class);
+        $user->shouldReceive('getAuthPassword')->once()->andReturn('hash');
+        $user->shouldNotReceive('getAuthIdentifierName');
+        $user->shouldNotReceive('getAuthIdentifier');
+        $user->shouldNotReceive('getAuthPasswordName');
+
+        $provider = new DatabaseUserProvider($conn, $hasher, 'foo');
+        $provider->rehashPasswordIfRequired($user, ['password' => 'plain']);
+    }
 }