From ba4f12baa02dfb55ec8822687896d643261440c4 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Wed, 20 Jul 2016 18:36:15 +0200 Subject: [PATCH] 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) --- apps/dav/appinfo/v1/caldav.php | 1 + apps/dav/appinfo/v1/carddav.php | 1 + apps/dav/appinfo/v1/webdav.php | 1 + apps/dav/lib/Connector/Sabre/Auth.php | 9 +- apps/dav/lib/Server.php | 3 +- .../tests/unit/Connector/Sabre/AuthTest.php | 9 +- build/integration/run.sh | 3 + config/config.sample.php | 7 + core/Application.php | 3 +- core/Controller/LoginController.php | 27 +- core/Controller/TokenController.php | 18 +- db_structure.xml | 78 ++++++ lib/base.php | 2 +- .../DependencyInjection/DIContainer.php | 3 +- .../Middleware/Security/CORSMiddleware.php | 25 +- lib/private/Security/Bruteforce/Throttler.php | 230 ++++++++++++++++++ lib/private/Server.php | 16 ++ lib/private/User/Session.php | 23 +- lib/private/legacy/api.php | 2 +- tests/Core/Controller/LoginControllerTest.php | 59 ++++- .../DependencyInjection/DIContainerTest.php | 4 +- .../Security/CORSMiddlewareTest.php | 28 ++- .../lib/Security/Bruteforce/ThrottlerTest.php | 123 ++++++++++ tests/lib/User/SessionTest.php | 42 +++- version.php | 2 +- 25 files changed, 653 insertions(+), 66 deletions(-) create mode 100644 lib/private/Security/Bruteforce/Throttler.php create mode 100644 tests/lib/Security/Bruteforce/ThrottlerTest.php diff --git a/apps/dav/appinfo/v1/caldav.php b/apps/dav/appinfo/v1/caldav.php index 50348a6020..975fd34ae8 100644 --- a/apps/dav/appinfo/v1/caldav.php +++ b/apps/dav/appinfo/v1/caldav.php @@ -35,6 +35,7 @@ $authBackend = new Auth( \OC::$server->getUserSession(), \OC::$server->getRequest(), \OC::$server->getTwoFactorAuthManager(), + \OC::$server->getBruteForceThrottler(), 'principals/' ); $principalBackend = new Principal( diff --git a/apps/dav/appinfo/v1/carddav.php b/apps/dav/appinfo/v1/carddav.php index fc7aff4a63..e2d8944fcb 100644 --- a/apps/dav/appinfo/v1/carddav.php +++ b/apps/dav/appinfo/v1/carddav.php @@ -36,6 +36,7 @@ $authBackend = new Auth( \OC::$server->getUserSession(), \OC::$server->getRequest(), \OC::$server->getTwoFactorAuthManager(), + \OC::$server->getBruteForceThrottler(), 'principals/' ); $principalBackend = new Principal( diff --git a/apps/dav/appinfo/v1/webdav.php b/apps/dav/appinfo/v1/webdav.php index 3b733c0fbd..2af49177ce 100644 --- a/apps/dav/appinfo/v1/webdav.php +++ b/apps/dav/appinfo/v1/webdav.php @@ -43,6 +43,7 @@ $authBackend = new \OCA\DAV\Connector\Sabre\Auth( \OC::$server->getUserSession(), \OC::$server->getRequest(), \OC::$server->getTwoFactorAuthManager(), + \OC::$server->getBruteForceThrottler(), 'principals/' ); $requestUri = \OC::$server->getRequest()->getRequestUri(); diff --git a/apps/dav/lib/Connector/Sabre/Auth.php b/apps/dav/lib/Connector/Sabre/Auth.php index 28e4ae2bcd..3f9e16b04c 100644 --- a/apps/dav/lib/Connector/Sabre/Auth.php +++ b/apps/dav/lib/Connector/Sabre/Auth.php @@ -33,6 +33,7 @@ use Exception; 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; @@ -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 @@ -107,6 +113,7 @@ class Auth extends AbstractBasic { * @param string $username * @param string $password * @return bool + * @throws PasswordLoginForbidden */ protected function validateUserPass($username, $password) { if ($this->userSession->isLoggedIn() && @@ -118,7 +125,7 @@ class Auth extends AbstractBasic { } 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(); diff --git a/apps/dav/lib/Server.php b/apps/dav/lib/Server.php index 0715d39049..c0cb5ecd62 100644 --- a/apps/dav/lib/Server.php +++ b/apps/dav/lib/Server.php @@ -64,7 +64,8 @@ class Server { \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 diff --git a/apps/dav/tests/unit/Connector/Sabre/AuthTest.php b/apps/dav/tests/unit/Connector/Sabre/AuthTest.php index 92798797d6..142b83a45b 100644 --- a/apps/dav/tests/unit/Connector/Sabre/AuthTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/AuthTest.php @@ -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; @@ -51,6 +52,8 @@ class AuthTest extends TestCase { private $request; /** @var Manager */ private $twoFactorManager; + /** @var Throttler */ + private $throttler; public function setUp() { parent::setUp(); @@ -63,11 +66,15 @@ class AuthTest extends TestCase { $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 ); } diff --git a/build/integration/run.sh b/build/integration/run.sh index 2abceaa1fa..eccb378eec 100755 --- a/build/integration/run.sh +++ b/build/integration/run.sh @@ -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 diff --git a/config/config.sample.php b/config/config.sample.php index 051e5422fe..c9f5fecf5f 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -207,6 +207,13 @@ $CONFIG = array( */ '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 diff --git a/core/Application.php b/core/Application.php index 1485f7a751..82ec5ad023 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 7806e1de90..c453bd20a2 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 13b1db9044..8401c4f23a 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; diff --git a/db_structure.xml b/db_structure.xml index 1127f0d82d..04c91ea494 100644 --- a/db_structure.xml +++ b/db_structure.xml @@ -1163,6 +1163,84 @@ + + + + *dbprefix*bruteforce_attempts + + + + id + integer + 0 + true + 1 + true + 4 + + + + action + text + + true + 64 + + + + occurred + integer + 0 + true + true + 4 + + + + ip + text + + true + 255 + + + + subnet + text + + true + 255 + + + + metadata + text + + true + 255 + + + + bruteforce_attempts_ip + + ip + ascending + + + + bruteforce_attempts_subnet + + subnet + ascending + + + + + +
+