From 71d2d3ca00f62fc44d49ced5aa8ea5c0b350fc58 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 29 Oct 2018 22:12:18 +0100 Subject: [PATCH 1/3] Reset bruteforce on token refresh OAuth When using atoken obtained via OAuth the token expires. Resulting in brute force attempts hitting the requesting IP. This resets the brute force attempts for that UID on a valid refresh of the token. Signed-off-by: Roeland Jago Douma --- .../lib/Controller/OauthApiController.php | 10 ++++- .../Controller/OauthApiControllerTest.php | 40 ++++++++++++++++++- lib/private/Server.php | 3 +- 3 files changed, 50 insertions(+), 3 deletions(-) diff --git a/apps/oauth2/lib/Controller/OauthApiController.php b/apps/oauth2/lib/Controller/OauthApiController.php index 2083741fa0c89..978ca76d75b4f 100644 --- a/apps/oauth2/lib/Controller/OauthApiController.php +++ b/apps/oauth2/lib/Controller/OauthApiController.php @@ -24,6 +24,7 @@ use OC\Authentication\Exceptions\InvalidTokenException; use OC\Authentication\Token\ExpiredTokenException; use OC\Authentication\Token\IProvider as TokenProvider; +use OC\Security\Bruteforce\Throttler; use OCA\OAuth2\Db\AccessTokenMapper; use OCA\OAuth2\Db\ClientMapper; use OCA\OAuth2\Exceptions\AccessTokenNotFoundException; @@ -49,6 +50,8 @@ class OauthApiController extends Controller { private $secureRandom; /** @var ITimeFactory */ private $time; + /** @var Throttler */ + private $throttler; /** * @param string $appName @@ -59,6 +62,7 @@ class OauthApiController extends Controller { * @param TokenProvider $tokenProvider * @param ISecureRandom $secureRandom * @param ITimeFactory $time + * @param Throttler $throttler */ public function __construct($appName, IRequest $request, @@ -67,7 +71,8 @@ public function __construct($appName, ClientMapper $clientMapper, TokenProvider $tokenProvider, ISecureRandom $secureRandom, - ITimeFactory $time) { + ITimeFactory $time, + Throttler $throttler) { parent::__construct($appName, $request); $this->crypto = $crypto; $this->accessTokenMapper = $accessTokenMapper; @@ -75,6 +80,7 @@ public function __construct($appName, $this->tokenProvider = $tokenProvider; $this->secureRandom = $secureRandom; $this->time = $time; + $this->throttler = $throttler; } /** @@ -164,6 +170,8 @@ public function getToken($grant_type, $code, $refresh_token, $client_id, $client $accessToken->setEncryptedToken($this->crypto->encrypt($newToken, $newCode)); $this->accessTokenMapper->update($accessToken); + $this->throttler->resetDelay($this->request->getRemoteAddress(), 'login', ['user' => $appToken->getUID()]); + return new JSONResponse( [ 'access_token' => $newToken, diff --git a/apps/oauth2/tests/Controller/OauthApiControllerTest.php b/apps/oauth2/tests/Controller/OauthApiControllerTest.php index 1074848597179..7d5dc9be258f7 100644 --- a/apps/oauth2/tests/Controller/OauthApiControllerTest.php +++ b/apps/oauth2/tests/Controller/OauthApiControllerTest.php @@ -27,6 +27,7 @@ use OC\Authentication\Token\ExpiredTokenException; use OC\Authentication\Token\IProvider as TokenProvider; use OC\Authentication\Token\IToken; +use OC\Security\Bruteforce\Throttler; use OCA\OAuth2\Controller\OauthApiController; use OCA\OAuth2\Db\AccessToken; use OCA\OAuth2\Db\AccessTokenMapper; @@ -57,6 +58,8 @@ class OauthApiControllerTest extends TestCase { private $secureRandom; /** @var ITimeFactory|\PHPUnit_Framework_MockObject_MockObject */ private $time; + /** @var Throttler|\PHPUnit_Framework_MockObject_MockObject */ + private $throttler; /** @var OauthApiController */ private $oauthApiController; @@ -70,6 +73,7 @@ public function setUp() { $this->tokenProvider = $this->createMock(TokenProvider::class); $this->secureRandom = $this->createMock(ISecureRandom::class); $this->time = $this->createMock(ITimeFactory::class); + $this->throttler = $this->createMock(Throttler::class); $this->oauthApiController = new OauthApiController( 'oauth2', @@ -79,7 +83,8 @@ public function setUp() { $this->clientMapper, $this->tokenProvider, $this->secureRandom, - $this->time + $this->time, + $this->throttler ); } @@ -286,6 +291,17 @@ public function testGetTokenValidAppToken() { 'user_id' => 'userId', ]); + $this->request->method('getRemoteAddress') + ->willReturn('1.2.3.4'); + + $this->throttler->expects($this->once()) + ->method('resetDelay') + ->with( + '1.2.3.4', + 'login', + ['user' => 'userId'] + ); + $this->assertEquals($expected, $this->oauthApiController->getToken('refresh_token', null, 'validrefresh', 'clientId', 'clientSecret')); } @@ -370,6 +386,17 @@ public function testGetTokenValidAppTokenBasicAuth() { $this->request->server['PHP_AUTH_USER'] = 'clientId'; $this->request->server['PHP_AUTH_PW'] = 'clientSecret'; + $this->request->method('getRemoteAddress') + ->willReturn('1.2.3.4'); + + $this->throttler->expects($this->once()) + ->method('resetDelay') + ->with( + '1.2.3.4', + 'login', + ['user' => 'userId'] + ); + $this->assertEquals($expected, $this->oauthApiController->getToken('refresh_token', null, 'validrefresh', null, null)); } @@ -451,6 +478,17 @@ public function testGetTokenExpiredAppToken() { 'user_id' => 'userId', ]); + $this->request->method('getRemoteAddress') + ->willReturn('1.2.3.4'); + + $this->throttler->expects($this->once()) + ->method('resetDelay') + ->with( + '1.2.3.4', + 'login', + ['user' => 'userId'] + ); + $this->assertEquals($expected, $this->oauthApiController->getToken('refresh_token', null, 'validrefresh', 'clientId', 'clientSecret')); } } diff --git a/lib/private/Server.php b/lib/private/Server.php index 5962012604a09..c9a2d1d0ae755 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -753,7 +753,7 @@ public function __construct($webRoot, \OC\Config $config) { $this->registerService('TrustedDomainHelper', function ($c) { return new TrustedDomainHelper($this->getConfig()); }); - $this->registerService('Throttler', function (Server $c) { + $this->registerService(Throttler::class, function (Server $c) { return new Throttler( $c->getDatabaseConnection(), new TimeFactory(), @@ -761,6 +761,7 @@ public function __construct($webRoot, \OC\Config $config) { $c->getConfig() ); }); + $this->registerAlias('Throttler', Throttler::class); $this->registerService('IntegrityCodeChecker', function (Server $c) { // IConfig and IAppManager requires a working database. This code // might however be called when ownCloud is not yet setup. From f17137883713b19800dcbe7c9e7fb4f355f81d79 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Tue, 30 Oct 2018 13:18:41 +0100 Subject: [PATCH 2/3] Move ExpiredTokenException to the correct namespace Signed-off-by: Roeland Jago Douma --- apps/oauth2/lib/Controller/OauthApiController.php | 2 +- apps/oauth2/tests/Controller/OauthApiControllerTest.php | 4 +--- lib/composer/composer/autoload_classmap.php | 2 +- lib/composer/composer/autoload_static.php | 2 +- .../Authentication/Exceptions/ExpiredTokenException.php | 4 ++-- lib/private/Authentication/Token/DefaultTokenProvider.php | 1 + lib/private/Authentication/Token/IProvider.php | 1 + lib/private/Authentication/Token/Manager.php | 1 + lib/private/Authentication/Token/PublicKeyTokenProvider.php | 1 + tests/lib/Authentication/Token/DefaultTokenProviderTest.php | 3 +-- tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php | 4 +--- 11 files changed, 12 insertions(+), 13 deletions(-) diff --git a/apps/oauth2/lib/Controller/OauthApiController.php b/apps/oauth2/lib/Controller/OauthApiController.php index 978ca76d75b4f..73fed3654d574 100644 --- a/apps/oauth2/lib/Controller/OauthApiController.php +++ b/apps/oauth2/lib/Controller/OauthApiController.php @@ -22,7 +22,7 @@ namespace OCA\OAuth2\Controller; use OC\Authentication\Exceptions\InvalidTokenException; -use OC\Authentication\Token\ExpiredTokenException; +use OC\Authentication\Exceptions\ExpiredTokenException; use OC\Authentication\Token\IProvider as TokenProvider; use OC\Security\Bruteforce\Throttler; use OCA\OAuth2\Db\AccessTokenMapper; diff --git a/apps/oauth2/tests/Controller/OauthApiControllerTest.php b/apps/oauth2/tests/Controller/OauthApiControllerTest.php index 7d5dc9be258f7..f5a8138fa2d2e 100644 --- a/apps/oauth2/tests/Controller/OauthApiControllerTest.php +++ b/apps/oauth2/tests/Controller/OauthApiControllerTest.php @@ -22,11 +22,9 @@ namespace OCA\OAuth2\Tests\Controller; use OC\Authentication\Exceptions\InvalidTokenException; +use OC\Authentication\Exceptions\ExpiredTokenException; use OC\Authentication\Token\DefaultToken; -use OC\Authentication\Token\DefaultTokenMapper; -use OC\Authentication\Token\ExpiredTokenException; use OC\Authentication\Token\IProvider as TokenProvider; -use OC\Authentication\Token\IToken; use OC\Security\Bruteforce\Throttler; use OCA\OAuth2\Controller\OauthApiController; use OCA\OAuth2\Db\AccessToken; diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index c80072e596fb2..bee556c9e4716 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -430,6 +430,7 @@ 'OC\\Archive\\Archive' => $baseDir . '/lib/private/Archive/Archive.php', 'OC\\Archive\\TAR' => $baseDir . '/lib/private/Archive/TAR.php', 'OC\\Archive\\ZIP' => $baseDir . '/lib/private/Archive/ZIP.php', + 'OC\\Authentication\\Exceptions\\ExpiredTokenException' => $baseDir . '/lib/private/Authentication/Exceptions/ExpiredTokenException.php', 'OC\\Authentication\\Exceptions\\InvalidTokenException' => $baseDir . '/lib/private/Authentication/Exceptions/InvalidTokenException.php', 'OC\\Authentication\\Exceptions\\LoginRequiredException' => $baseDir . '/lib/private/Authentication/Exceptions/LoginRequiredException.php', 'OC\\Authentication\\Exceptions\\PasswordLoginForbiddenException' => $baseDir . '/lib/private/Authentication/Exceptions/PasswordLoginForbiddenException.php', @@ -442,7 +443,6 @@ 'OC\\Authentication\\Token\\DefaultTokenCleanupJob' => $baseDir . '/lib/private/Authentication/Token/DefaultTokenCleanupJob.php', 'OC\\Authentication\\Token\\DefaultTokenMapper' => $baseDir . '/lib/private/Authentication/Token/DefaultTokenMapper.php', 'OC\\Authentication\\Token\\DefaultTokenProvider' => $baseDir . '/lib/private/Authentication/Token/DefaultTokenProvider.php', - 'OC\\Authentication\\Token\\ExpiredTokenException' => $baseDir . '/lib/private/Authentication/Exceptions/ExpiredTokenException.php', 'OC\\Authentication\\Token\\IProvider' => $baseDir . '/lib/private/Authentication/Token/IProvider.php', 'OC\\Authentication\\Token\\IToken' => $baseDir . '/lib/private/Authentication/Token/IToken.php', 'OC\\Authentication\\Token\\Manager' => $baseDir . '/lib/private/Authentication/Token/Manager.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 9e5ba1f2f1598..989265d31d1f4 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -460,6 +460,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\Archive\\Archive' => __DIR__ . '/../../..' . '/lib/private/Archive/Archive.php', 'OC\\Archive\\TAR' => __DIR__ . '/../../..' . '/lib/private/Archive/TAR.php', 'OC\\Archive\\ZIP' => __DIR__ . '/../../..' . '/lib/private/Archive/ZIP.php', + 'OC\\Authentication\\Exceptions\\ExpiredTokenException' => __DIR__ . '/../../..' . '/lib/private/Authentication/Exceptions/ExpiredTokenException.php', 'OC\\Authentication\\Exceptions\\InvalidTokenException' => __DIR__ . '/../../..' . '/lib/private/Authentication/Exceptions/InvalidTokenException.php', 'OC\\Authentication\\Exceptions\\LoginRequiredException' => __DIR__ . '/../../..' . '/lib/private/Authentication/Exceptions/LoginRequiredException.php', 'OC\\Authentication\\Exceptions\\PasswordLoginForbiddenException' => __DIR__ . '/../../..' . '/lib/private/Authentication/Exceptions/PasswordLoginForbiddenException.php', @@ -472,7 +473,6 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\Authentication\\Token\\DefaultTokenCleanupJob' => __DIR__ . '/../../..' . '/lib/private/Authentication/Token/DefaultTokenCleanupJob.php', 'OC\\Authentication\\Token\\DefaultTokenMapper' => __DIR__ . '/../../..' . '/lib/private/Authentication/Token/DefaultTokenMapper.php', 'OC\\Authentication\\Token\\DefaultTokenProvider' => __DIR__ . '/../../..' . '/lib/private/Authentication/Token/DefaultTokenProvider.php', - 'OC\\Authentication\\Token\\ExpiredTokenException' => __DIR__ . '/../../..' . '/lib/private/Authentication/Exceptions/ExpiredTokenException.php', 'OC\\Authentication\\Token\\IProvider' => __DIR__ . '/../../..' . '/lib/private/Authentication/Token/IProvider.php', 'OC\\Authentication\\Token\\IToken' => __DIR__ . '/../../..' . '/lib/private/Authentication/Token/IToken.php', 'OC\\Authentication\\Token\\Manager' => __DIR__ . '/../../..' . '/lib/private/Authentication/Token/Manager.php', diff --git a/lib/private/Authentication/Exceptions/ExpiredTokenException.php b/lib/private/Authentication/Exceptions/ExpiredTokenException.php index a45ca5b69559b..d5b2e2cbca708 100644 --- a/lib/private/Authentication/Exceptions/ExpiredTokenException.php +++ b/lib/private/Authentication/Exceptions/ExpiredTokenException.php @@ -21,9 +21,9 @@ * along with this program. If not, see . * */ -namespace OC\Authentication\Token; +namespace OC\Authentication\Exceptions; -use OC\Authentication\Exceptions\InvalidTokenException; +use OC\Authentication\Token\IToken; class ExpiredTokenException extends InvalidTokenException { /** @var IToken */ diff --git a/lib/private/Authentication/Token/DefaultTokenProvider.php b/lib/private/Authentication/Token/DefaultTokenProvider.php index ad45303fa7c3b..065a13eb51ca4 100644 --- a/lib/private/Authentication/Token/DefaultTokenProvider.php +++ b/lib/private/Authentication/Token/DefaultTokenProvider.php @@ -29,6 +29,7 @@ namespace OC\Authentication\Token; use Exception; +use OC\Authentication\Exceptions\ExpiredTokenException; use OC\Authentication\Exceptions\InvalidTokenException; use OC\Authentication\Exceptions\PasswordlessTokenException; use OCP\AppFramework\Db\DoesNotExistException; diff --git a/lib/private/Authentication/Token/IProvider.php b/lib/private/Authentication/Token/IProvider.php index ab46bd121262a..52f4b40d1c7bd 100644 --- a/lib/private/Authentication/Token/IProvider.php +++ b/lib/private/Authentication/Token/IProvider.php @@ -26,6 +26,7 @@ namespace OC\Authentication\Token; +use OC\Authentication\Exceptions\ExpiredTokenException; use OC\Authentication\Exceptions\InvalidTokenException; use OC\Authentication\Exceptions\PasswordlessTokenException; diff --git a/lib/private/Authentication/Token/Manager.php b/lib/private/Authentication/Token/Manager.php index 5fbf78eefd298..44e2ef84246c4 100644 --- a/lib/private/Authentication/Token/Manager.php +++ b/lib/private/Authentication/Token/Manager.php @@ -23,6 +23,7 @@ namespace OC\Authentication\Token; +use OC\Authentication\Exceptions\ExpiredTokenException; use OC\Authentication\Exceptions\InvalidTokenException; use OC\Authentication\Exceptions\PasswordlessTokenException; diff --git a/lib/private/Authentication/Token/PublicKeyTokenProvider.php b/lib/private/Authentication/Token/PublicKeyTokenProvider.php index fe352d29e056f..021adb4077779 100644 --- a/lib/private/Authentication/Token/PublicKeyTokenProvider.php +++ b/lib/private/Authentication/Token/PublicKeyTokenProvider.php @@ -23,6 +23,7 @@ namespace OC\Authentication\Token; +use OC\Authentication\Exceptions\ExpiredTokenException; use OC\Authentication\Exceptions\InvalidTokenException; use OC\Authentication\Exceptions\PasswordlessTokenException; use OCP\AppFramework\Db\DoesNotExistException; diff --git a/tests/lib/Authentication/Token/DefaultTokenProviderTest.php b/tests/lib/Authentication/Token/DefaultTokenProviderTest.php index 3fb11f410bac3..829d4940d4b46 100644 --- a/tests/lib/Authentication/Token/DefaultTokenProviderTest.php +++ b/tests/lib/Authentication/Token/DefaultTokenProviderTest.php @@ -22,17 +22,16 @@ namespace Test\Authentication\Token; +use OC\Authentication\Exceptions\ExpiredTokenException; use OC\Authentication\Exceptions\InvalidTokenException; use OC\Authentication\Token\DefaultToken; use OC\Authentication\Token\DefaultTokenMapper; use OC\Authentication\Token\DefaultTokenProvider; -use OC\Authentication\Token\ExpiredTokenException; use OC\Authentication\Token\IToken; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Utility\ITimeFactory; use OCP\IConfig; use OCP\ILogger; -use OCP\IUser; use OCP\Security\ICrypto; use Test\TestCase; diff --git a/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php b/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php index cd3bcb81ba641..80ee0e6d7872f 100644 --- a/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php +++ b/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php @@ -23,19 +23,17 @@ namespace Test\Authentication\Token; +use OC\Authentication\Exceptions\ExpiredTokenException; use OC\Authentication\Exceptions\InvalidTokenException; -use OC\Authentication\Exceptions\PasswordlessTokenException; use OC\Authentication\Token\DefaultToken; use OC\Authentication\Token\PublicKeyToken; use OC\Authentication\Token\PublicKeyTokenMapper; use OC\Authentication\Token\PublicKeyTokenProvider; -use OC\Authentication\Token\ExpiredTokenException; use OC\Authentication\Token\IToken; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Utility\ITimeFactory; use OCP\IConfig; use OCP\ILogger; -use OCP\IUser; use OCP\Security\ICrypto; use Test\TestCase; From e3f3212fbc5a27174d97f32c217d1399ded9fe99 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Tue, 30 Oct 2018 13:19:59 +0100 Subject: [PATCH 3/3] Error out early on an expired token Fixes #12131 If we hit an expired token there is no need to continue checking. Since we know it is a token. We also should not register this with the bruteforce throttler as it is actually a valid token. Just expired. Instead the authentication should fail. And buisness continues as usual. Signed-off-by: Roeland Jago Douma --- lib/private/User/Session.php | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index fbd6a0a78e362..2c2244f06f727 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -38,6 +38,7 @@ namespace OC\User; use OC; +use OC\Authentication\Exceptions\ExpiredTokenException; use OC\Authentication\Exceptions\InvalidTokenException; use OC\Authentication\Exceptions\PasswordlessTokenException; use OC\Authentication\Exceptions\PasswordLoginForbiddenException; @@ -401,7 +402,13 @@ public function logClientIn($user, $this->manager->emit('\OC\User', 'preLogin', array($user, $password)); } - $isTokenPassword = $this->isTokenPassword($password); + try { + $isTokenPassword = $this->isTokenPassword($password); + } catch (ExpiredTokenException $e) { + // Just return on an expired token no need to check further or record a failed login + return false; + } + if (!$isTokenPassword && $this->isTokenAuthEnforced()) { throw new PasswordLoginForbiddenException(); } @@ -474,11 +481,14 @@ protected function isTwoFactorEnforced($username) { * * @param string $password * @return boolean + * @throws ExpiredTokenException */ public function isTokenPassword($password) { try { $this->tokenProvider->getToken($password); return true; + } catch (ExpiredTokenException $e) { + throw $e; } catch (InvalidTokenException $ex) { return false; }