From c1589f163c44839fba9b2d3dcfb1e45ee7fa47ef Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Wed, 20 Jul 2016 23:09:27 +0200 Subject: [PATCH] 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(-) diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index c453bd20a2..66bb13dbb5 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 79bd7c2284..8d12982dd1 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 0e13485b27..f09f3c9811 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 33930a50ce..379c7e3944 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); }