From 98b8ed0742104ae468b48eb04cb71c128788470a 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 | 45 +++- version.php | 2 +- 25 files changed, 654 insertions(+), 68 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 50348a6020236..975fd34ae8ec9 100644 --- a/apps/dav/appinfo/v1/caldav.php +++ b/apps/dav/appinfo/v1/caldav.php @@ -35,6 +35,7 @@ \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 fc7aff4a63c2f..e2d8944fcb6a3 100644 --- a/apps/dav/appinfo/v1/carddav.php +++ b/apps/dav/appinfo/v1/carddav.php @@ -36,6 +36,7 @@ \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 3b733c0fbd502..2af49177ce146 100644 --- a/apps/dav/appinfo/v1/webdav.php +++ b/apps/dav/appinfo/v1/webdav.php @@ -43,6 +43,7 @@ \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 28e4ae2bcdea9..3f9e16b04c506 100644 --- a/apps/dav/lib/Connector/Sabre/Auth.php +++ b/apps/dav/lib/Connector/Sabre/Auth.php @@ -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; @@ -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 @@ public function isDavAuthenticated($username) { * @param string $username * @param string $password * @return bool + * @throws PasswordLoginForbidden */ protected function validateUserPass($username, $password) { if ($this->userSession->isLoggedIn() && @@ -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(); diff --git a/apps/dav/lib/Server.php b/apps/dav/lib/Server.php index 0715d39049cf2..c0cb5ecd62db6 100644 --- a/apps/dav/lib/Server.php +++ b/apps/dav/lib/Server.php @@ -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 diff --git a/apps/dav/tests/unit/Connector/Sabre/AuthTest.php b/apps/dav/tests/unit/Connector/Sabre/AuthTest.php index 92798797d6ce7..142b83a45b8e3 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 @@ 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 ); } diff --git a/build/integration/run.sh b/build/integration/run.sh index 2abceaa1fad62..eccb378eec491 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 051e5422fe5cd..c9f5fecf5f9db 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -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 diff --git a/core/Application.php b/core/Application.php index 1485f7a7516b3..82ec5ad023c02 100644 --- a/core/Application.php +++ b/core/Application.php @@ -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) { diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index 7806e1de904dd..c453bd20a23e5 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\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 @@ function __construct($appName, IRequest $request, IUserManager $userManager, ICo $this->userSession = $userSession; $this->urlGenerator = $urlGenerator; $this->twoFactorManager = $twoFactorManager; + $this->throttler = $throttler; } /** @@ -171,6 +178,8 @@ public function showLoginForm($user, $redirect_url, $remember_login) { * @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 @@ public function tryLogin($user, $password, $redirect_url) { } } 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 13b1db9044aef..8401c4f23a9f7 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\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 1127f0d82d440..04c91ea494f70 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 + + + + + +
+