Mitigate race condition

4 files changed, 57 insertions, 11 deletions

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,15 +289,20 @@ 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')
 			->with('login', '192.168.0.1', ['user' => 'MyUserName']);
 		$this->userManager->expects($this->once())
@@ -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,11 +476,16 @@ 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')
 			->with('192.168.0.1');
 		$this->throttler
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);
 	}