Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement brute force protection #479

Merged
merged 3 commits into from
Jul 20, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions apps/dav/appinfo/v1/caldav.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
\OC::$server->getUserSession(),
\OC::$server->getRequest(),
\OC::$server->getTwoFactorAuthManager(),
\OC::$server->getBruteForceThrottler(),
'principals/'
);
$principalBackend = new Principal(
Expand Down
1 change: 1 addition & 0 deletions apps/dav/appinfo/v1/carddav.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
\OC::$server->getUserSession(),
\OC::$server->getRequest(),
\OC::$server->getTwoFactorAuthManager(),
\OC::$server->getBruteForceThrottler(),
'principals/'
);
$principalBackend = new Principal(
Expand Down
1 change: 1 addition & 0 deletions apps/dav/appinfo/v1/webdav.php
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
\OC::$server->getUserSession(),
\OC::$server->getRequest(),
\OC::$server->getTwoFactorAuthManager(),
\OC::$server->getBruteForceThrottler(),
'principals/'
);
$requestUri = \OC::$server->getRequest()->getRequestUri();
Expand Down
9 changes: 8 additions & 1 deletion apps/dav/lib/Connector/Sabre/Auth.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
use OC\AppFramework\Http\Request;
use OC\Authentication\Exceptions\PasswordLoginForbiddenException;
use OC\Authentication\TwoFactorAuth\Manager;
use OC\Security\Bruteforce\Throttler;
use OC\User\Session;
use OCA\DAV\Connector\Sabre\Exception\PasswordLoginForbidden;
use OCP\IRequest;
Expand All @@ -58,23 +59,28 @@ class Auth extends AbstractBasic {
private $currentUser;
/** @var Manager */
private $twoFactorManager;
/** @var Throttler */
private $throttler;

/**
* @param ISession $session
* @param Session $userSession
* @param IRequest $request
* @param Manager $twoFactorManager
* @param Throttler $throttler
* @param string $principalPrefix
*/
public function __construct(ISession $session,
Session $userSession,
IRequest $request,
Manager $twoFactorManager,
Throttler $throttler,
$principalPrefix = 'principals/users/') {
$this->session = $session;
$this->userSession = $userSession;
$this->twoFactorManager = $twoFactorManager;
$this->request = $request;
$this->throttler = $throttler;
$this->principalPrefix = $principalPrefix;

// setup realm
Expand Down Expand Up @@ -107,6 +113,7 @@ public function isDavAuthenticated($username) {
* @param string $username
* @param string $password
* @return bool
* @throws PasswordLoginForbidden
*/
protected function validateUserPass($username, $password) {
if ($this->userSession->isLoggedIn() &&
Expand All @@ -118,7 +125,7 @@ protected function validateUserPass($username, $password) {
} else {
\OC_Util::setupFS(); //login hooks may need early access to the filesystem
try {
if ($this->userSession->logClientIn($username, $password, $this->request)) {
if ($this->userSession->logClientIn($username, $password, $this->request, $this->throttler)) {
\OC_Util::setupFS($this->userSession->getUser()->getUID());
$this->session->set(self::DAV_AUTHENTICATED, $this->userSession->getUser()->getUID());
$this->session->close();
Expand Down
3 changes: 2 additions & 1 deletion apps/dav/lib/Server.php
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ public function __construct(IRequest $request, $baseUri) {
\OC::$server->getSession(),
\OC::$server->getUserSession(),
\OC::$server->getRequest(),
\OC::$server->getTwoFactorAuthManager()
\OC::$server->getTwoFactorAuthManager(),
\OC::$server->getBruteForceThrottler()
);

// Set URL explicitly due to reverse-proxy situations
Expand Down
9 changes: 8 additions & 1 deletion apps/dav/tests/unit/Connector/Sabre/AuthTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
namespace OCA\DAV\Tests\unit\Connector\Sabre;

use OC\Authentication\TwoFactorAuth\Manager;
use OC\Security\Bruteforce\Throttler;
use OC\User\Session;
use OCP\IRequest;
use OCP\ISession;
Expand All @@ -51,6 +52,8 @@ class AuthTest extends TestCase {
private $request;
/** @var Manager */
private $twoFactorManager;
/** @var Throttler */
private $throttler;

public function setUp() {
parent::setUp();
Expand All @@ -63,11 +66,15 @@ public function setUp() {
$this->twoFactorManager = $this->getMockBuilder('\OC\Authentication\TwoFactorAuth\Manager')
->disableOriginalConstructor()
->getMock();
$this->throttler = $this->getMockBuilder('\OC\Security\Bruteforce\Throttler')
->disableOriginalConstructor()
->getMock();
$this->auth = new \OCA\DAV\Connector\Sabre\Auth(
$this->session,
$this->userSession,
$this->request,
$this->twoFactorManager
$this->twoFactorManager,
$this->throttler
);
}

Expand Down
3 changes: 3 additions & 0 deletions build/integration/run.sh
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ else
exit 1
fi

# Disable bruteforce protection because the integration tests do trigger them
../../occ config:system:set auth.bruteforce.protection.enabled --value false --type bool

composer install

SCENARIO_TO_RUN=$1
Expand Down
7 changes: 7 additions & 0 deletions config/config.sample.php
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,13 @@
*/
'token_auth_enforced' => false,

/**
* Whether the bruteforce protection shipped with Nextcloud should be enabled or not.
*
* Disabling this is discouraged for security reasons.
*/
'auth.bruteforce.protection.enabled' => true,

/**
* The directory where the skeleton files are located. These files will be
* copied to the data directory of new users. Leave empty to not copy any
Expand Down
3 changes: 2 additions & 1 deletion core/Application.php
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,8 @@ public function __construct(array $urlParams=array()){
$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) {
Expand Down
30 changes: 22 additions & 8 deletions core/Controller/LoginController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -37,24 +39,20 @@
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
Expand All @@ -65,16 +63,25 @@ 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;
$this->session = $session;
$this->userSession = $userSession;
$this->urlGenerator = $urlGenerator;
$this->twoFactorManager = $twoFactorManager;
$this->throttler = $throttler;
}

/**
Expand Down Expand Up @@ -171,6 +178,9 @@ 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;
// TODO: Add all the insane error handling
/* @var $loginResult IUser */
Expand All @@ -184,6 +194,10 @@ 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
18 changes: 10 additions & 8 deletions core/Controller/TokenController.php
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
<?php

/**
* @author Christoph Wurst <[email protected]>
*
Expand All @@ -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;
Expand All @@ -35,27 +35,29 @@
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;
Expand Down
78 changes: 78 additions & 0 deletions db_structure.xml
Original file line number Diff line number Diff line change
Expand Up @@ -1163,6 +1163,84 @@
</declaration>
</table>

<table>

<!--
List of usernames, their display name and login password.
-->
<name>*dbprefix*bruteforce_attempts</name>

<declaration>
<field>
<name>id</name>
<type>integer</type>
<default>0</default>
<notnull>true</notnull>
<autoincrement>1</autoincrement>
<unsigned>true</unsigned>
<length>4</length>
</field>

<field>
<name>action</name>
<type>text</type>
<default></default>
<notnull>true</notnull>
<length>64</length>
</field>

<field>
<name>occurred</name>
<type>integer</type>
<default>0</default>
<notnull>true</notnull>
<unsigned>true</unsigned>
<length>4</length>
</field>

<field>
<name>ip</name>
<type>text</type>
<default></default>
<notnull>true</notnull>
<length>255</length>
</field>

<field>
<name>subnet</name>
<type>text</type>
<default></default>
<notnull>true</notnull>
<length>255</length>
</field>

<field>
<name>metadata</name>
<type>text</type>
<default></default>
<notnull>true</notnull>
<length>255</length>
</field>

<index>
<name>bruteforce_attempts_ip</name>
<field>
<name>ip</name>
<sorting>ascending</sorting>
</field>
</index>
<index>
<name>bruteforce_attempts_subnet</name>
<field>
<name>subnet</name>
<sorting>ascending</sorting>
</field>
</index>

</declaration>

</table>

<table>

<!--
Expand Down
2 changes: 1 addition & 1 deletion lib/base.php
Original file line number Diff line number Diff line change
Expand Up @@ -913,7 +913,7 @@ static function handleLogin(OCP\IRequest $request) {
if ($userSession->tryTokenLogin($request)) {
return true;
}
if ($userSession->tryBasicAuthLogin($request)) {
if ($userSession->tryBasicAuthLogin($request, \OC::$server->getBruteForceThrottler())) {
return true;
}
return false;
Expand Down
3 changes: 2 additions & 1 deletion lib/private/AppFramework/DependencyInjection/DIContainer.php
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,8 @@ public function __construct($appName, $urlParams = array()){
return new CORSMiddleware(
$c['Request'],
$c['ControllerMethodReflector'],
$c['OCP\IUserSession']
$c['OCP\IUserSession'],
$c->getServer()->getBruteForceThrottler()
);
});

Expand Down
Loading