From 4c2448b3b3195dc2b5cf6809a0cd9579f18b3e34 Mon Sep 17 00:00:00 2001 From: Michael Chernyshev Date: Fri, 18 Oct 2019 13:57:26 +0300 Subject: [PATCH 1/4] Security fix: Waiting before retrying password reset --- .../Passwords/DatabaseTokenRepository.php | 37 +++++++++++++++++- .../Auth/Passwords/PasswordBroker.php | 6 +++ .../Auth/Passwords/PasswordBrokerManager.php | 3 +- .../Contracts/Auth/PasswordBroker.php | 7 ++++ .../Auth/AuthDatabaseTokenRepositoryTest.php | 38 +++++++++++++++++++ tests/Auth/AuthPasswordBrokerTest.php | 17 +++++++-- 6 files changed, 102 insertions(+), 6 deletions(-) diff --git a/src/Illuminate/Auth/Passwords/DatabaseTokenRepository.php b/src/Illuminate/Auth/Passwords/DatabaseTokenRepository.php index 6d609da18978..e4e5729faef9 100755 --- a/src/Illuminate/Auth/Passwords/DatabaseTokenRepository.php +++ b/src/Illuminate/Auth/Passwords/DatabaseTokenRepository.php @@ -45,6 +45,13 @@ class DatabaseTokenRepository implements TokenRepositoryInterface */ protected $expires; + /** + * Minimum number of seconds before re-redefining the token. + * + * @var int + */ + protected $timeout; + /** * Create a new token repository instance. * @@ -53,15 +60,17 @@ class DatabaseTokenRepository implements TokenRepositoryInterface * @param string $table * @param string $hashKey * @param int $expires + * @param int $timeout * @return void */ public function __construct(ConnectionInterface $connection, HasherContract $hasher, - $table, $hashKey, $expires = 60) + $table, $hashKey, $expires = 60, $timeout = 60) { $this->table = $table; $this->hasher = $hasher; $this->hashKey = $hashKey; $this->expires = $expires * 60; + $this->timeout = $timeout; $this->connection = $connection; } @@ -139,6 +148,32 @@ protected function tokenExpired($createdAt) return Carbon::parse($createdAt)->addSeconds($this->expires)->isPast(); } + /** + * Determine if a token record exists and was recently created. + * + * @param \Illuminate\Contracts\Auth\CanResetPassword $user + * @return bool + */ + public function recentlyCreated(CanResetPasswordContract $user) + { + $record = (array) $this->getTable()->where( + 'email', $user->getEmailForPasswordReset() + )->first(); + + return $record && $this->tokenRecentlyCreated($record['created_at']); + } + + /** + * Determine if the token was recently created. + * + * @param string $createdAt + * @return bool + */ + protected function tokenRecentlyCreated($createdAt) + { + return Carbon::parse($createdAt)->addSeconds($this->timeout)->isFuture(); + } + /** * Delete a token record by user. * diff --git a/src/Illuminate/Auth/Passwords/PasswordBroker.php b/src/Illuminate/Auth/Passwords/PasswordBroker.php index 75ad3ee076a0..d58f155669ce 100755 --- a/src/Illuminate/Auth/Passwords/PasswordBroker.php +++ b/src/Illuminate/Auth/Passwords/PasswordBroker.php @@ -55,6 +55,12 @@ public function sendResetLink(array $credentials) return static::INVALID_USER; } + // An attacker can make a lot of password reset requests, + // which will lead to spam in user's mailbox. + if ($this->tokens->recentlyCreated($user)) { + return static::RESEND_TIMEOUT; + } + // Once we have the reset token, we are ready to send the message out to this // user with a link to reset their password. We will then redirect back to // the current URI having nothing set in the session to indicate errors. diff --git a/src/Illuminate/Auth/Passwords/PasswordBrokerManager.php b/src/Illuminate/Auth/Passwords/PasswordBrokerManager.php index 310f0fd1626d..613099633399 100644 --- a/src/Illuminate/Auth/Passwords/PasswordBrokerManager.php +++ b/src/Illuminate/Auth/Passwords/PasswordBrokerManager.php @@ -95,7 +95,8 @@ protected function createTokenRepository(array $config) $this->app['hash'], $config['table'], $key, - $config['expire'] + $config['expire'], + $config['timeout'] ); } diff --git a/src/Illuminate/Contracts/Auth/PasswordBroker.php b/src/Illuminate/Contracts/Auth/PasswordBroker.php index 1df3fe884a3d..3d9a7e257701 100644 --- a/src/Illuminate/Contracts/Auth/PasswordBroker.php +++ b/src/Illuminate/Contracts/Auth/PasswordBroker.php @@ -34,6 +34,13 @@ interface PasswordBroker */ const INVALID_TOKEN = 'passwords.token'; + /** + * Constant representing the wait before password reset link resending. + * + * @var string + */ + const RESEND_TIMEOUT = 'passwords.timeout'; + /** * Send a password reset link to a user. * diff --git a/tests/Auth/AuthDatabaseTokenRepositoryTest.php b/tests/Auth/AuthDatabaseTokenRepositoryTest.php index 52877cb5ddd2..9c66908e9d63 100755 --- a/tests/Auth/AuthDatabaseTokenRepositoryTest.php +++ b/tests/Auth/AuthDatabaseTokenRepositoryTest.php @@ -98,6 +98,44 @@ public function testExistReturnsFalseIfInvalidToken() $this->assertFalse($repo->exists($user, 'wrong-token')); } + public function testRecentlyCreatedReturnsFalseIfNoRowFoundForUser() + { + $repo = $this->getRepo(); + $repo->getConnection()->shouldReceive('table')->once()->with('table')->andReturn($query = m::mock(stdClass::class)); + $query->shouldReceive('where')->once()->with('email', 'email')->andReturn($query); + $query->shouldReceive('first')->once()->andReturn(null); + $user = m::mock(CanResetPassword::class); + $user->shouldReceive('getEmailForPasswordReset')->once()->andReturn('email'); + + $this->assertFalse($repo->recentlyCreated($user)); + } + + public function testRecentlyCreatedReturnsTrueIfRecordIsRecentlyCreated() + { + $repo = $this->getRepo(); + $repo->getConnection()->shouldReceive('table')->once()->with('table')->andReturn($query = m::mock(stdClass::class)); + $query->shouldReceive('where')->once()->with('email', 'email')->andReturn($query); + $date = Carbon::now()->subSeconds(59)->toDateTimeString(); + $query->shouldReceive('first')->once()->andReturn((object) ['created_at' => $date, 'token' => 'hashed-token']); + $user = m::mock(CanResetPassword::class); + $user->shouldReceive('getEmailForPasswordReset')->once()->andReturn('email'); + + $this->assertTrue($repo->recentlyCreated($user)); + } + + public function testRecentlyCreatedReturnsFalseIfValidRecordExists() + { + $repo = $this->getRepo(); + $repo->getConnection()->shouldReceive('table')->once()->with('table')->andReturn($query = m::mock(stdClass::class)); + $query->shouldReceive('where')->once()->with('email', 'email')->andReturn($query); + $date = Carbon::now()->subSeconds(61)->toDateTimeString(); + $query->shouldReceive('first')->once()->andReturn((object) ['created_at' => $date, 'token' => 'hashed-token']); + $user = m::mock(CanResetPassword::class); + $user->shouldReceive('getEmailForPasswordReset')->once()->andReturn('email'); + + $this->assertFalse($repo->recentlyCreated($user)); + } + public function testDeleteMethodDeletesByToken() { $repo = $this->getRepo(); diff --git a/tests/Auth/AuthPasswordBrokerTest.php b/tests/Auth/AuthPasswordBrokerTest.php index 7bf375ec6611..7a76c39d4284 100755 --- a/tests/Auth/AuthPasswordBrokerTest.php +++ b/tests/Auth/AuthPasswordBrokerTest.php @@ -29,6 +29,17 @@ public function testIfUserIsNotFoundErrorRedirectIsReturned() $this->assertEquals(PasswordBrokerContract::INVALID_USER, $broker->sendResetLink(['credentials'])); } + public function testIfTokenIsRecentlyCreated() + { + $mocks = $this->getMocks(); + $broker = $this->getMockBuilder(PasswordBroker::class)->setMethods(['emailResetLink', 'getUri'])->setConstructorArgs(array_values($mocks))->getMock(); + $mocks['users']->shouldReceive('retrieveByCredentials')->once()->with(['foo'])->andReturn($user = m::mock(CanResetPassword::class)); + $mocks['tokens']->shouldReceive('recentlyCreated')->once()->with($user)->andReturn(true); + $user->shouldReceive('sendPasswordResetNotification')->with('token'); + + $this->assertEquals(PasswordBrokerContract::RESEND_TIMEOUT, $broker->sendResetLink(['foo'])); + } + public function testGetUserThrowsExceptionIfUserDoesntImplementCanResetPassword() { $this->expectException(UnexpectedValueException::class); @@ -53,13 +64,11 @@ public function testBrokerCreatesTokenAndRedirectsWithoutError() $mocks = $this->getMocks(); $broker = $this->getMockBuilder(PasswordBroker::class)->setMethods(['emailResetLink', 'getUri'])->setConstructorArgs(array_values($mocks))->getMock(); $mocks['users']->shouldReceive('retrieveByCredentials')->once()->with(['foo'])->andReturn($user = m::mock(CanResetPassword::class)); + $mocks['tokens']->shouldReceive('recentlyCreated')->once()->with($user)->andReturn(false); $mocks['tokens']->shouldReceive('create')->once()->with($user)->andReturn('token'); - $callback = function () { - // - }; $user->shouldReceive('sendPasswordResetNotification')->with('token'); - $this->assertEquals(PasswordBrokerContract::RESET_LINK_SENT, $broker->sendResetLink(['foo'], $callback)); + $this->assertEquals(PasswordBrokerContract::RESET_LINK_SENT, $broker->sendResetLink(['foo'])); } public function testRedirectIsReturnedByResetWhenUserCredentialsInvalid() From af4522ece5c9b8ffde67ae5d078a2d70f7bedaa8 Mon Sep 17 00:00:00 2001 From: Michael Chernyshev Date: Sun, 20 Oct 2019 23:07:41 +0300 Subject: [PATCH 2/4] Checks for safe inclusion in the 6.x branch --- src/Illuminate/Auth/Passwords/PasswordBroker.php | 12 ++++++++---- .../Auth/Passwords/PasswordBrokerManager.php | 4 +++- tests/Auth/AuthPasswordBrokerTest.php | 13 +++++++++++++ 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/src/Illuminate/Auth/Passwords/PasswordBroker.php b/src/Illuminate/Auth/Passwords/PasswordBroker.php index d58f155669ce..968096cc3f4a 100755 --- a/src/Illuminate/Auth/Passwords/PasswordBroker.php +++ b/src/Illuminate/Auth/Passwords/PasswordBroker.php @@ -55,10 +55,14 @@ public function sendResetLink(array $credentials) return static::INVALID_USER; } - // An attacker can make a lot of password reset requests, - // which will lead to spam in user's mailbox. - if ($this->tokens->recentlyCreated($user)) { - return static::RESEND_TIMEOUT; + // Before 7.x we have to check the existence of a new method. + // In 7.x, this code must be removed. + if (method_exists($this->tokens, 'recentlyCreated')) { + // An attacker can make a lot of password reset requests, + // which will lead to spam in user's mailbox. + if ($this->tokens->recentlyCreated($user)) { + return static::RESEND_TIMEOUT; + } } // Once we have the reset token, we are ready to send the message out to this diff --git a/src/Illuminate/Auth/Passwords/PasswordBrokerManager.php b/src/Illuminate/Auth/Passwords/PasswordBrokerManager.php index 613099633399..39e9d0073f07 100644 --- a/src/Illuminate/Auth/Passwords/PasswordBrokerManager.php +++ b/src/Illuminate/Auth/Passwords/PasswordBrokerManager.php @@ -96,7 +96,9 @@ protected function createTokenRepository(array $config) $config['table'], $key, $config['expire'], - $config['timeout'] + // Before 7.x this element in the configuration may not exist. + // In 7.x, this check must be removed. + $config['timeout'] ?? 0 ); } diff --git a/tests/Auth/AuthPasswordBrokerTest.php b/tests/Auth/AuthPasswordBrokerTest.php index 7a76c39d4284..5047e9fa6e1b 100755 --- a/tests/Auth/AuthPasswordBrokerTest.php +++ b/tests/Auth/AuthPasswordBrokerTest.php @@ -124,3 +124,16 @@ protected function getMocks() ]; } } + +// Before 7.x we have to check the existence of a new method. In 7.x, this code must be removed. + +namespace Illuminate\Auth\Passwords; + +function method_exists($object, $method_name) +{ + if ($method_name == 'recentlyCreated') { + return true; + } + + return \method_exists($object, $method_name); +} From 5111a182d96925f495375cbba0c6481d4ece9d12 Mon Sep 17 00:00:00 2001 From: Michael Chernyshev Date: Mon, 21 Oct 2019 12:37:36 +0300 Subject: [PATCH 3/4] AuthPasswordBrokerTest in another way --- tests/Auth/AuthPasswordBrokerTest.php | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/tests/Auth/AuthPasswordBrokerTest.php b/tests/Auth/AuthPasswordBrokerTest.php index 5047e9fa6e1b..aa67fefeb154 100755 --- a/tests/Auth/AuthPasswordBrokerTest.php +++ b/tests/Auth/AuthPasswordBrokerTest.php @@ -117,7 +117,7 @@ protected function getBroker($mocks) protected function getMocks() { return [ - 'tokens' => m::mock(TokenRepositoryInterface::class), + 'tokens' => m::mock(TestTokenRepositoryInterface::class), 'users' => m::mock(UserProvider::class), 'mailer' => m::mock(Mailer::class), 'view' => 'resetLinkView', @@ -125,15 +125,10 @@ protected function getMocks() } } -// Before 7.x we have to check the existence of a new method. In 7.x, this code must be removed. +// Before 7.x we have to check the existence of a new method. In 7.x, this code must be moved to +// Illuminate\Auth\Passwords\TokenRepositoryInterface -namespace Illuminate\Auth\Passwords; - -function method_exists($object, $method_name) +interface TestTokenRepositoryInterface extends TokenRepositoryInterface { - if ($method_name == 'recentlyCreated') { - return true; - } - - return \method_exists($object, $method_name); + public function recentlyCreated(CanResetPassword $user); } From c4c5f4ad54eb23d2bbff366672445fac652fd192 Mon Sep 17 00:00:00 2001 From: Michael Chernyshev Date: Mon, 21 Oct 2019 13:36:40 +0300 Subject: [PATCH 4/4] AuthPasswordBrokerTest without changing old tests --- tests/Auth/AuthPasswordBrokerTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Auth/AuthPasswordBrokerTest.php b/tests/Auth/AuthPasswordBrokerTest.php index aa67fefeb154..c495702fb94d 100755 --- a/tests/Auth/AuthPasswordBrokerTest.php +++ b/tests/Auth/AuthPasswordBrokerTest.php @@ -32,6 +32,7 @@ public function testIfUserIsNotFoundErrorRedirectIsReturned() public function testIfTokenIsRecentlyCreated() { $mocks = $this->getMocks(); + $mocks['tokens'] = m::mock(TestTokenRepositoryInterface::class); $broker = $this->getMockBuilder(PasswordBroker::class)->setMethods(['emailResetLink', 'getUri'])->setConstructorArgs(array_values($mocks))->getMock(); $mocks['users']->shouldReceive('retrieveByCredentials')->once()->with(['foo'])->andReturn($user = m::mock(CanResetPassword::class)); $mocks['tokens']->shouldReceive('recentlyCreated')->once()->with($user)->andReturn(true); @@ -64,7 +65,6 @@ public function testBrokerCreatesTokenAndRedirectsWithoutError() $mocks = $this->getMocks(); $broker = $this->getMockBuilder(PasswordBroker::class)->setMethods(['emailResetLink', 'getUri'])->setConstructorArgs(array_values($mocks))->getMock(); $mocks['users']->shouldReceive('retrieveByCredentials')->once()->with(['foo'])->andReturn($user = m::mock(CanResetPassword::class)); - $mocks['tokens']->shouldReceive('recentlyCreated')->once()->with($user)->andReturn(false); $mocks['tokens']->shouldReceive('create')->once()->with($user)->andReturn('token'); $user->shouldReceive('sendPasswordResetNotification')->with('token'); @@ -117,7 +117,7 @@ protected function getBroker($mocks) protected function getMocks() { return [ - 'tokens' => m::mock(TestTokenRepositoryInterface::class), + 'tokens' => m::mock(TokenRepositoryInterface::class), 'users' => m::mock(UserProvider::class), 'mailer' => m::mock(Mailer::class), 'view' => 'resetLinkView',