From ba4f12baa02dfb55ec8822687896d643261440c4 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Wed, 20 Jul 2016 18:36:15 +0200 Subject: Implement brute force protection Class Throttler implements the bruteforce protection for security actions in Nextcloud. It is working by logging invalid login attempts to the database and slowing down all login attempts from the same subnet. The max delay is 30 seconds and the starting delay are 200 milliseconds. (after the first failed login) --- core/Application.php | 3 ++- core/Controller/LoginController.php | 27 +++++++++++++++++++-------- core/Controller/TokenController.php | 18 ++++++++++-------- 3 files changed, 31 insertions(+), 17 deletions(-) (limited to 'core') diff --git a/core/Application.php b/core/Application.php index 1485f7a7516..82ec5ad023c 100644 --- a/core/Application.php +++ b/core/Application.php @@ -103,7 +103,8 @@ class Application extends App { $c->query('Session'), $c->query('UserSession'), $c->query('URLGenerator'), - $c->query('TwoFactorAuthManager') + $c->query('TwoFactorAuthManager'), + $c->query('ServerContainer')->getBruteforceThrottler() ); }); $container->registerService('TwoFactorChallengeController', function (SimpleContainer $c) { diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index 7806e1de904..c453bd20a23 100644 --- a/core/Controller/LoginController.php +++ b/core/Controller/LoginController.php @@ -22,7 +22,9 @@ namespace OC\Core\Controller; +use OC\AppFramework\Utility\TimeFactory; use OC\Authentication\TwoFactorAuth\Manager; +use OC\Security\Bruteforce\Throttler; use OC\User\Session; use OC_App; use OC_Util; @@ -37,24 +39,20 @@ use OCP\IUser; use OCP\IUserManager; class LoginController extends Controller { - /** @var IUserManager */ private $userManager; - /** @var IConfig */ private $config; - /** @var ISession */ private $session; - /** @var Session */ private $userSession; - /** @var IURLGenerator */ private $urlGenerator; - /** @var Manager */ private $twoFactorManager; + /** @var Throttler */ + private $throttler; /** * @param string $appName @@ -65,9 +63,17 @@ class LoginController extends Controller { * @param Session $userSession * @param IURLGenerator $urlGenerator * @param Manager $twoFactorManager + * @param Throttler $throttler */ - function __construct($appName, IRequest $request, IUserManager $userManager, IConfig $config, ISession $session, - Session $userSession, IURLGenerator $urlGenerator, Manager $twoFactorManager) { + function __construct($appName, + IRequest $request, + IUserManager $userManager, + IConfig $config, + ISession $session, + Session $userSession, + IURLGenerator $urlGenerator, + Manager $twoFactorManager, + Throttler $throttler) { parent::__construct($appName, $request); $this->userManager = $userManager; $this->config = $config; @@ -75,6 +81,7 @@ class LoginController extends Controller { $this->userSession = $userSession; $this->urlGenerator = $urlGenerator; $this->twoFactorManager = $twoFactorManager; + $this->throttler = $throttler; } /** @@ -171,6 +178,8 @@ class LoginController extends Controller { * @return RedirectResponse */ public function tryLogin($user, $password, $redirect_url) { + $this->throttler->sleepDelay($this->request->getRemoteAddress()); + $originalUser = $user; // TODO: Add all the insane error handling /* @var $loginResult IUser */ @@ -184,6 +193,8 @@ class LoginController extends Controller { } } if ($loginResult === false) { + $this->throttler->registerAttempt('login', $this->request->getRemoteAddress(), ['user' => $originalUser]); + $this->session->set('loginMessages', [ ['invalidpassword'] ]); diff --git a/core/Controller/TokenController.php b/core/Controller/TokenController.php index 13b1db9044a..8401c4f23a9 100644 --- a/core/Controller/TokenController.php +++ b/core/Controller/TokenController.php @@ -1,5 +1,4 @@ * @@ -23,6 +22,7 @@ namespace OC\Core\Controller; use OC\AppFramework\Http; +use OC\AppFramework\Utility\TimeFactory; use OC\Authentication\Token\DefaultTokenProvider; use OC\Authentication\Token\IProvider; use OC\Authentication\Token\IToken; @@ -35,27 +35,29 @@ use OCP\IRequest; use OCP\Security\ISecureRandom; class TokenController extends Controller { - /** @var UserManager */ private $userManager; - /** @var IProvider */ private $tokenProvider; - /** @var TwoFactorAuthManager */ private $twoFactorAuthManager; - /** @var ISecureRandom */ private $secureRandom; /** * @param string $appName * @param IRequest $request - * @param Manager $userManager - * @param DefaultTokenProvider $tokenProvider + * @param UserManager $userManager + * @param IProvider $tokenProvider + * @param TwoFactorAuthManager $twoFactorAuthManager * @param ISecureRandom $secureRandom */ - public function __construct($appName, IRequest $request, UserManager $userManager, IProvider $tokenProvider, TwoFactorAuthManager $twoFactorAuthManager, ISecureRandom $secureRandom) { + public function __construct($appName, + IRequest $request, + UserManager $userManager, + IProvider $tokenProvider, + TwoFactorAuthManager $twoFactorAuthManager, + ISecureRandom $secureRandom) { parent::__construct($appName, $request); $this->userManager = $userManager; $this->tokenProvider = $tokenProvider; -- cgit v1.2.3 From c1589f163c44839fba9b2d3dcfb1e45ee7fa47ef Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Wed, 20 Jul 2016 23:09:27 +0200 Subject: Mitigate race condition --- core/Controller/LoginController.php | 5 +++- lib/private/User/Session.php | 5 +++- tests/Core/Controller/LoginControllerTest.php | 37 ++++++++++++++++++++++----- tests/lib/User/SessionTest.php | 21 ++++++++++++--- 4 files changed, 57 insertions(+), 11 deletions(-) (limited to 'core') diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index c453bd20a23..66bb13dbb54 100644 --- a/core/Controller/LoginController.php +++ b/core/Controller/LoginController.php @@ -178,6 +178,7 @@ class LoginController extends Controller { * @return RedirectResponse */ public function tryLogin($user, $password, $redirect_url) { + $currentDelay = $this->throttler->getDelay($this->request->getRemoteAddress()); $this->throttler->sleepDelay($this->request->getRemoteAddress()); $originalUser = $user; @@ -194,7 +195,9 @@ class LoginController extends Controller { } if ($loginResult === false) { $this->throttler->registerAttempt('login', $this->request->getRemoteAddress(), ['user' => $originalUser]); - + if($currentDelay === 0) { + $this->throttler->sleepDelay($this->request->getRemoteAddress()); + } $this->session->set('loginMessages', [ ['invalidpassword'] ]); diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index 79bd7c22848..8d12982dd1a 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -310,6 +310,7 @@ class Session implements IUserSession, Emitter { $password, IRequest $request, OC\Security\Bruteforce\Throttler $throttler) { + $currentDelay = $throttler->getDelay($request->getRemoteAddress()); $throttler->sleepDelay($request->getRemoteAddress()); $isTokenPassword = $this->isTokenPassword($password); @@ -326,6 +327,9 @@ class Session implements IUserSession, Emitter { } $throttler->registerAttempt('login', $request->getRemoteAddress(), ['uid' => $user]); + if($currentDelay === 0) { + $throttler->sleepDelay($request->getRemoteAddress()); + } return false; } @@ -405,7 +409,6 @@ class Session implements IUserSession, Emitter { public function tryBasicAuthLogin(IRequest $request, OC\Security\Bruteforce\Throttler $throttler) { if (!empty($request->server['PHP_AUTH_USER']) && !empty($request->server['PHP_AUTH_PW'])) { - $throttler->sleepDelay(\OC::$server->getRequest()->getRemoteAddress()); try { if ($this->logClientIn($request->server['PHP_AUTH_USER'], $request->server['PHP_AUTH_PW'], $request, $throttler)) { /** diff --git a/tests/Core/Controller/LoginControllerTest.php b/tests/Core/Controller/LoginControllerTest.php index 0e13485b272..f09f3c98118 100644 --- a/tests/Core/Controller/LoginControllerTest.php +++ b/tests/Core/Controller/LoginControllerTest.php @@ -289,13 +289,18 @@ class LoginControllerTest extends TestCase { $loginPageUrl = 'some url'; $this->request - ->expects($this->exactly(2)) + ->expects($this->exactly(4)) ->method('getRemoteAddress') ->willReturn('192.168.0.1'); $this->throttler - ->expects($this->once()) + ->expects($this->exactly(2)) ->method('sleepDelay') ->with('192.168.0.1'); + $this->throttler + ->expects($this->once()) + ->method('getDelay') + ->with('192.168.0.1') + ->willReturn(0); $this->throttler ->expects($this->once()) ->method('registerAttempt') @@ -322,13 +327,18 @@ class LoginControllerTest extends TestCase { $indexPageUrl = 'some url'; $this->request - ->expects($this->once()) + ->expects($this->exactly(2)) ->method('getRemoteAddress') ->willReturn('192.168.0.1'); $this->throttler ->expects($this->once()) ->method('sleepDelay') ->with('192.168.0.1'); + $this->throttler + ->expects($this->once()) + ->method('getDelay') + ->with('192.168.0.1') + ->willReturn(200); $this->userManager->expects($this->once()) ->method('checkPassword') ->will($this->returnValue($user)); @@ -362,13 +372,18 @@ class LoginControllerTest extends TestCase { $redirectUrl = 'http://localhost/another url'; $this->request - ->expects($this->once()) + ->expects($this->exactly(2)) ->method('getRemoteAddress') ->willReturn('192.168.0.1'); $this->throttler ->expects($this->once()) ->method('sleepDelay') ->with('192.168.0.1'); + $this->throttler + ->expects($this->once()) + ->method('getDelay') + ->with('192.168.0.1') + ->willReturn(200); $this->userManager->expects($this->once()) ->method('checkPassword') ->with('Jane', $password) @@ -399,13 +414,18 @@ class LoginControllerTest extends TestCase { $challengeUrl = 'challenge/url'; $this->request - ->expects($this->once()) + ->expects($this->exactly(2)) ->method('getRemoteAddress') ->willReturn('192.168.0.1'); $this->throttler ->expects($this->once()) ->method('sleepDelay') ->with('192.168.0.1'); + $this->throttler + ->expects($this->once()) + ->method('getDelay') + ->with('192.168.0.1') + ->willReturn(200); $this->userManager->expects($this->once()) ->method('checkPassword') ->will($this->returnValue($user)); @@ -456,9 +476,14 @@ class LoginControllerTest extends TestCase { ->with('core.login.showLoginForm', ['user' => 'john@doe.com']) ->will($this->returnValue('')); $this->request - ->expects($this->exactly(2)) + ->expects($this->exactly(3)) ->method('getRemoteAddress') ->willReturn('192.168.0.1'); + $this->throttler + ->expects($this->once()) + ->method('getDelay') + ->with('192.168.0.1') + ->willReturn(200); $this->throttler ->expects($this->once()) ->method('sleepDelay') diff --git a/tests/lib/User/SessionTest.php b/tests/lib/User/SessionTest.php index 33930a50ce5..379c7e39442 100644 --- a/tests/lib/User/SessionTest.php +++ b/tests/lib/User/SessionTest.php @@ -371,13 +371,18 @@ class SessionTest extends \Test\TestCase { ->with('token_auth_enforced', false) ->will($this->returnValue(true)); $request - ->expects($this->once()) + ->expects($this->exactly(2)) ->method('getRemoteAddress') ->willReturn('192.168.0.1'); $this->throttler ->expects($this->once()) ->method('sleepDelay') ->with('192.168.0.1'); + $this->throttler + ->expects($this->once()) + ->method('getDelay') + ->with('192.168.0.1') + ->willReturn(0); $userSession->logClientIn('john', 'doe', $request, $this->throttler); } @@ -407,13 +412,18 @@ class SessionTest extends \Test\TestCase { ->method('set') ->with('app_password', 'I-AM-AN-APP-PASSWORD'); $request - ->expects($this->once()) + ->expects($this->exactly(2)) ->method('getRemoteAddress') ->willReturn('192.168.0.1'); $this->throttler ->expects($this->once()) ->method('sleepDelay') ->with('192.168.0.1'); + $this->throttler + ->expects($this->once()) + ->method('getDelay') + ->with('192.168.0.1') + ->willReturn(0); $this->assertTrue($userSession->logClientIn('john', 'I-AM-AN-APP-PASSWORD', $request, $this->throttler)); } @@ -449,13 +459,18 @@ class SessionTest extends \Test\TestCase { ->will($this->returnValue(true)); $request - ->expects($this->once()) + ->expects($this->exactly(2)) ->method('getRemoteAddress') ->willReturn('192.168.0.1'); $this->throttler ->expects($this->once()) ->method('sleepDelay') ->with('192.168.0.1'); + $this->throttler + ->expects($this->once()) + ->method('getDelay') + ->with('192.168.0.1') + ->willReturn(0); $userSession->logClientIn('john', 'doe', $request, $this->throttler); } -- cgit v1.2.3