Skip to content

Commit

Permalink
Mitigate race condition
Browse files Browse the repository at this point in the history
  • Loading branch information
@LukasReschke
LukasReschke committed Jul 20, 2016
1 parent adf67fa commit c1589f1
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 11 deletions.
5 changes: 4 additions & 1 deletion core/Controller/LoginController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ public function showLoginForm($user, $redirect_url, $remember_login) {
* @return RedirectResponse
*/
public function tryLogin($user, $password, $redirect_url) {
$currentDelay = $this->throttler->getDelay($this->request->getRemoteAddress());
$this->throttler->sleepDelay($this->request->getRemoteAddress());

$originalUser = $user;
Expand All @@ -194,7 +195,9 @@ public function tryLogin($user, $password, $redirect_url) {
}
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']
]);
Expand Down
5 changes: 4 additions & 1 deletion lib/private/User/Session.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,7 @@ public function logClientIn($user,
$password,
IRequest $request,
OC\Security\Bruteforce\Throttler $throttler) {
$currentDelay = $throttler->getDelay($request->getRemoteAddress());
$throttler->sleepDelay($request->getRemoteAddress());

$isTokenPassword = $this->isTokenPassword($password);
Expand All @@ -326,6 +327,9 @@ public function logClientIn($user,
}

$throttler->registerAttempt('login', $request->getRemoteAddress(), ['uid' => $user]);
if($currentDelay === 0) {
$throttler->sleepDelay($request->getRemoteAddress());
}
return false;
}

Expand Down Expand Up @@ -405,7 +409,6 @@ protected function prepareUserLogin() {
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)) {
/**
Expand Down
37 changes: 31 additions & 6 deletions tests/Core/Controller/LoginControllerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -289,13 +289,18 @@ public function testLoginWithInvalidCredentials() {
$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')
Expand All @@ -322,13 +327,18 @@ public function testLoginWithValidCredentials() {
$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));
Expand Down Expand Up @@ -362,13 +372,18 @@ public function testLoginWithValidCredentialsAndRedirectUrl() {
$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)
Expand Down Expand Up @@ -399,13 +414,18 @@ public function testLoginWithTwoFactorEnforced() {
$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));
Expand Down Expand Up @@ -456,9 +476,14 @@ public function testToNotLeakLoginName() {
->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')
Expand Down
21 changes: 18 additions & 3 deletions tests/lib/User/SessionTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -371,13 +371,18 @@ public function testLogClientInNoTokenPasswordWith2fa() {
->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);
}
Expand Down Expand Up @@ -407,13 +412,18 @@ public function testLogClientInWithTokenPassword() {
->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));
}
Expand Down Expand Up @@ -449,13 +459,18 @@ public function testLogClientInNoTokenPasswordNo2fa() {
->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);
}
Expand Down

0 comments on commit c1589f1

Please sign in to comment.