From 80a40fd5e31daf73fcb41273f441789c41d3e14c Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Tue, 22 Jun 2021 19:54:13 +0200 Subject: [PATCH 1/6] Throttle on public DAV endpoint We should throttle whenever an invalid request is sent to the public WebDAV endpoint. Signed-off-by: Lukas Reschke --- apps/dav/appinfo/v1/publicwebdav.php | 3 ++- apps/dav/lib/Connector/PublicAuth.php | 16 +++++++++++++++- apps/dav/tests/unit/Connector/PublicAuthTest.php | 9 ++++++++- 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/apps/dav/appinfo/v1/publicwebdav.php b/apps/dav/appinfo/v1/publicwebdav.php index 556dd35de582b..32451e02af6b5 100644 --- a/apps/dav/appinfo/v1/publicwebdav.php +++ b/apps/dav/appinfo/v1/publicwebdav.php @@ -42,7 +42,8 @@ $authBackend = new OCA\DAV\Connector\PublicAuth( \OC::$server->getRequest(), \OC::$server->getShareManager(), - \OC::$server->getSession() + \OC::$server->getSession(), + \OC::$server->getBruteForceThrottler() ); $authPlugin = new \Sabre\DAV\Auth\Plugin($authBackend); diff --git a/apps/dav/lib/Connector/PublicAuth.php b/apps/dav/lib/Connector/PublicAuth.php index 450217722937a..9d4ef8ba9b251 100644 --- a/apps/dav/lib/Connector/PublicAuth.php +++ b/apps/dav/lib/Connector/PublicAuth.php @@ -31,6 +31,7 @@ namespace OCA\DAV\Connector; +use OC\Security\Bruteforce\Throttler; use OCP\IRequest; use OCP\ISession; use OCP\Share\Exceptions\ShareNotFound; @@ -44,6 +45,7 @@ * @package OCA\DAV\Connector */ class PublicAuth extends AbstractBasic { + private const BRUTEFORCE_ACTION = 'public_webdav_auth'; /** @var \OCP\Share\IShare */ private $share; @@ -57,17 +59,23 @@ class PublicAuth extends AbstractBasic { /** @var IRequest */ private $request; + /** @var Throttler */ + private $throttler; + /** * @param IRequest $request * @param IManager $shareManager * @param ISession $session + * @param Throttler $throttler */ public function __construct(IRequest $request, IManager $shareManager, - ISession $session) { + ISession $session, + Throttler $throttler) { $this->request = $request; $this->shareManager = $shareManager; $this->session = $session; + $this->throttler = $throttler; // setup realm $defaults = new \OCP\Defaults(); @@ -87,9 +95,12 @@ public function __construct(IRequest $request, * @throws \Sabre\DAV\Exception\NotAuthenticated */ protected function validateUserPass($username, $password) { + $this->throttler->sleepDelayOrThrowOnMax($this->request->getRemoteAddress(), self::BRUTEFORCE_ACTION); + try { $share = $this->shareManager->getShareByToken($username); } catch (ShareNotFound $e) { + $this->throttler->registerAttempt(self::BRUTEFORCE_ACTION, $this->request->getRemoteAddress()); return false; } @@ -114,11 +125,14 @@ protected function validateUserPass($username, $password) { header('WWW-Authenticate: DummyBasic realm="' . $this->realm . '"'); throw new \Sabre\DAV\Exception\NotAuthenticated('Cannot authenticate over ajax calls'); } + + $this->throttler->registerAttempt(self::BRUTEFORCE_ACTION, $this->request->getRemoteAddress()); return false; } } elseif ($share->getShareType() === IShare::TYPE_REMOTE) { return true; } else { + $this->throttler->registerAttempt(self::BRUTEFORCE_ACTION, $this->request->getRemoteAddress()); return false; } } else { diff --git a/apps/dav/tests/unit/Connector/PublicAuthTest.php b/apps/dav/tests/unit/Connector/PublicAuthTest.php index bcec84d414c43..0bbad1c1ff9e3 100644 --- a/apps/dav/tests/unit/Connector/PublicAuthTest.php +++ b/apps/dav/tests/unit/Connector/PublicAuthTest.php @@ -27,6 +27,7 @@ namespace OCA\DAV\Tests\unit\Connector; +use OC\Security\Bruteforce\Throttler; use OCP\IRequest; use OCP\ISession; use OCP\Share\Exceptions\ShareNotFound; @@ -50,6 +51,8 @@ class PublicAuthTest extends \Test\TestCase { private $shareManager; /** @var \OCA\DAV\Connector\PublicAuth */ private $auth; + /** @var Throttler|\PHPUnit\Framework\MockObject\MockObject */ + private $throttler; /** @var string */ private $oldUser; @@ -66,11 +69,15 @@ protected function setUp(): void { $this->shareManager = $this->getMockBuilder(IManager::class) ->disableOriginalConstructor() ->getMock(); + $this->shareManager = $this->getMockBuilder(Throttler::class) + ->disableOriginalConstructor() + ->getMock(); $this->auth = new \OCA\DAV\Connector\PublicAuth( $this->request, $this->shareManager, - $this->session + $this->session, + $this->throttler ); // Store current user From 6ad4ef850f8e45965751edf940933c3af74c12f7 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Tue, 22 Jun 2021 20:41:38 +0200 Subject: [PATCH 2/6] Reference correct private variable Signed-off-by: Lukas Reschke --- apps/dav/tests/unit/Connector/PublicAuthTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/dav/tests/unit/Connector/PublicAuthTest.php b/apps/dav/tests/unit/Connector/PublicAuthTest.php index 0bbad1c1ff9e3..0ac41295cb033 100644 --- a/apps/dav/tests/unit/Connector/PublicAuthTest.php +++ b/apps/dav/tests/unit/Connector/PublicAuthTest.php @@ -69,7 +69,7 @@ protected function setUp(): void { $this->shareManager = $this->getMockBuilder(IManager::class) ->disableOriginalConstructor() ->getMock(); - $this->shareManager = $this->getMockBuilder(Throttler::class) + $this->throttler = $this->getMockBuilder(Throttler::class) ->disableOriginalConstructor() ->getMock(); From 0b443a7adf73d1c57506dc1f7cfaacb054986b29 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 19 Mar 2020 12:09:57 +0100 Subject: [PATCH 3/6] Send "429 Too Many Requests" in case of brute force protection Signed-off-by: Joas Schilling --- core/templates/429.php | 4 ++ .../Security/BruteForceMiddleware.php | 28 +++++++++- lib/private/Security/Bruteforce/Throttler.php | 22 +++++++- .../Http/TooManyRequestsResponse.php | 51 +++++++++++++++++++ .../Security/Bruteforce/MaxDelayReached.php | 30 +++++++++++ 5 files changed, 133 insertions(+), 2 deletions(-) create mode 100644 core/templates/429.php create mode 100644 lib/public/AppFramework/Http/TooManyRequestsResponse.php create mode 100644 lib/public/Security/Bruteforce/MaxDelayReached.php diff --git a/core/templates/429.php b/core/templates/429.php new file mode 100644 index 0000000000000..caf8a3675e297 --- /dev/null +++ b/core/templates/429.php @@ -0,0 +1,4 @@ + diff --git a/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php b/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php index 398c2f3f3a447..e9b03266462ab 100644 --- a/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php @@ -1,4 +1,5 @@ * @@ -26,9 +27,15 @@ use OC\AppFramework\Utility\ControllerMethodReflector; use OC\Security\Bruteforce\Throttler; +use OCP\AppFramework\Controller; +use OCP\AppFramework\Http; use OCP\AppFramework\Http\Response; +use OCP\AppFramework\Http\TooManyRequestsResponse; use OCP\AppFramework\Middleware; +use OCP\AppFramework\OCS\OCSException; +use OCP\AppFramework\OCSController; use OCP\IRequest; +use OCP\Security\Bruteforce\MaxDelayReached; /** * Class BruteForceMiddleware performs the bruteforce protection for controllers @@ -66,7 +73,7 @@ public function beforeController($controller, $methodName) { if ($this->reflector->hasAnnotation('BruteForceProtection')) { $action = $this->reflector->getAnnotationParameter('BruteForceProtection', 'action'); - $this->throttler->sleepDelay($this->request->getRemoteAddress(), $action); + $this->throttler->sleepDelayOrThrowOnMax($this->request->getRemoteAddress(), $action); } } @@ -83,4 +90,23 @@ public function afterController($controller, $methodName, Response $response) { return parent::afterController($controller, $methodName, $response); } + + /** + * @param Controller $controller + * @param string $methodName + * @param \Exception $exception + * @throws \Exception + * @return Response + */ + public function afterException($controller, $methodName, \Exception $exception): Response { + if ($exception instanceof MaxDelayReached) { + if ($controller instanceof OCSController) { + throw new OCSException($exception->getMessage(), Http::STATUS_TOO_MANY_REQUESTS); + } + + return new TooManyRequestsResponse(); + } + + throw $exception; + } } diff --git a/lib/private/Security/Bruteforce/Throttler.php b/lib/private/Security/Bruteforce/Throttler.php index d1a5f6d44889a..4db80cd426690 100644 --- a/lib/private/Security/Bruteforce/Throttler.php +++ b/lib/private/Security/Bruteforce/Throttler.php @@ -34,6 +34,7 @@ use OCP\IConfig; use OCP\IDBConnection; use OCP\ILogger; +use OCP\Security\Bruteforce\MaxDelayReached; /** * Class Throttler implements the bruteforce protection for security actions in @@ -50,6 +51,7 @@ */ class Throttler { public const LOGIN_ACTION = 'login'; + public const MAX_DELAY = 25; /** @var IDBConnection */ private $db; @@ -236,7 +238,7 @@ public function getDelay($ip, $action = '') { return 0; } - $maxDelay = 25; + $maxDelay = self::MAX_DELAY; $firstDelay = 0.1; if ($attempts > (8 * PHP_INT_SIZE - 1)) { // Don't ever overflow. Just assume the maxDelay time:s @@ -289,4 +291,22 @@ public function sleepDelay($ip, $action = '') { usleep($delay * 1000); return $delay; } + + /** + * Will sleep for the defined amount of time unless maximum is reached + * In case of maximum a "429 Too Many Request" response is thrown + * + * @param string $ip + * @param string $action optionally filter by action + * @return int the time spent sleeping + * @throws MaxDelayReached when reached the maximum + */ + public function sleepDelayOrThrowOnMax($ip, $action = '') { + $delay = $this->getDelay($ip, $action); + if ($delay === self::MAX_DELAY * 1000) { + throw new MaxDelayReached(); + } + usleep($delay * 1000); + return $delay; + } } diff --git a/lib/public/AppFramework/Http/TooManyRequestsResponse.php b/lib/public/AppFramework/Http/TooManyRequestsResponse.php new file mode 100644 index 0000000000000..160614ab33ec0 --- /dev/null +++ b/lib/public/AppFramework/Http/TooManyRequestsResponse.php @@ -0,0 +1,51 @@ + + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OCP\AppFramework\Http; + +use OCP\Template; + +/** + * A generic 429 response showing an 404 error page as well to the end-user + * @since 19.0.0 + */ +class TooManyRequestsResponse extends Response { + + /** + * @since 19.0.0 + */ + public function __construct() { + parent::__construct(); + + $this->setContentSecurityPolicy(new ContentSecurityPolicy()); + $this->setStatus(429); + } + + /** + * @return string + * @since 19.0.0 + */ + public function render() { + $template = new Template('core', '429', 'blank'); + return $template->fetchPage(); + } +} diff --git a/lib/public/Security/Bruteforce/MaxDelayReached.php b/lib/public/Security/Bruteforce/MaxDelayReached.php new file mode 100644 index 0000000000000..817ef0e60c3f0 --- /dev/null +++ b/lib/public/Security/Bruteforce/MaxDelayReached.php @@ -0,0 +1,30 @@ + + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OCP\Security\Bruteforce; + +/** + * Class MaxDelayReached + * @since 19.0 + */ +class MaxDelayReached extends \RuntimeException { +} From 6f23221efb3e9425140bb522381914961be30522 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Wed, 23 Jun 2021 12:23:43 +0200 Subject: [PATCH 4/6] Add newline to satisfy phpcs Signed-off-by: Lukas Reschke --- .../AppFramework/Middleware/Security/BruteForceMiddleware.php | 1 + lib/public/AppFramework/Http/TooManyRequestsResponse.php | 1 + lib/public/Security/Bruteforce/MaxDelayReached.php | 1 + 3 files changed, 3 insertions(+) diff --git a/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php b/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php index e9b03266462ab..e6c511537a055 100644 --- a/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php @@ -1,4 +1,5 @@ diff --git a/lib/public/AppFramework/Http/TooManyRequestsResponse.php b/lib/public/AppFramework/Http/TooManyRequestsResponse.php index 160614ab33ec0..b15df303bfee2 100644 --- a/lib/public/AppFramework/Http/TooManyRequestsResponse.php +++ b/lib/public/AppFramework/Http/TooManyRequestsResponse.php @@ -1,4 +1,5 @@ diff --git a/lib/public/Security/Bruteforce/MaxDelayReached.php b/lib/public/Security/Bruteforce/MaxDelayReached.php index 817ef0e60c3f0..3aaa7d0515932 100644 --- a/lib/public/Security/Bruteforce/MaxDelayReached.php +++ b/lib/public/Security/Bruteforce/MaxDelayReached.php @@ -1,4 +1,5 @@ From 9588fb0424ce74c5a8db78e99bd80b8921677640 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Wed, 23 Jun 2021 15:06:30 +0200 Subject: [PATCH 5/6] Update autoloader map Signed-off-by: Lukas Reschke --- lib/composer/composer/autoload_classmap.php | 2 ++ lib/composer/composer/autoload_static.php | 2 ++ 2 files changed, 4 insertions(+) diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 37c6bc3fdf503..8998038eb1775 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -59,6 +59,7 @@ 'OCP\\AppFramework\\Http\\Template\\LinkMenuAction' => $baseDir . '/lib/public/AppFramework/Http/Template/LinkMenuAction.php', 'OCP\\AppFramework\\Http\\Template\\PublicTemplateResponse' => $baseDir . '/lib/public/AppFramework/Http/Template/PublicTemplateResponse.php', 'OCP\\AppFramework\\Http\\Template\\SimpleMenuAction' => $baseDir . '/lib/public/AppFramework/Http/Template/SimpleMenuAction.php', + 'OCP\\AppFramework\\Http\\TooManyRequestsResponse' => $baseDir . '/lib/public/AppFramework/Http/TooManyRequestsResponse.php', 'OCP\\AppFramework\\Http\\ZipResponse' => $baseDir . '/lib/public/AppFramework/Http/ZipResponse.php', 'OCP\\AppFramework\\IAppContainer' => $baseDir . '/lib/public/AppFramework/IAppContainer.php', 'OCP\\AppFramework\\Middleware' => $baseDir . '/lib/public/AppFramework/Middleware.php', @@ -415,6 +416,7 @@ 'OCP\\Search\\PagedProvider' => $baseDir . '/lib/public/Search/PagedProvider.php', 'OCP\\Search\\Provider' => $baseDir . '/lib/public/Search/Provider.php', 'OCP\\Search\\Result' => $baseDir . '/lib/public/Search/Result.php', + 'OCP\\Security\\Bruteforce\\MaxDelayReached' => $baseDir . '/lib/public/Security/Bruteforce/MaxDelayReached.php', 'OCP\\Security\\CSP\\AddContentSecurityPolicyEvent' => $baseDir . '/lib/public/Security/CSP/AddContentSecurityPolicyEvent.php', 'OCP\\Security\\Events\\GenerateSecurePasswordEvent' => $baseDir . '/lib/public/Security/Events/GenerateSecurePasswordEvent.php', 'OCP\\Security\\Events\\ValidatePasswordPolicyEvent' => $baseDir . '/lib/public/Security/Events/ValidatePasswordPolicyEvent.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index abb132ab88584..51daaefeee6f3 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -88,6 +88,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OCP\\AppFramework\\Http\\Template\\LinkMenuAction' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Template/LinkMenuAction.php', 'OCP\\AppFramework\\Http\\Template\\PublicTemplateResponse' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Template/PublicTemplateResponse.php', 'OCP\\AppFramework\\Http\\Template\\SimpleMenuAction' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Template/SimpleMenuAction.php', + 'OCP\\AppFramework\\Http\\TooManyRequestsResponse' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/TooManyRequestsResponse.php', 'OCP\\AppFramework\\Http\\ZipResponse' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/ZipResponse.php', 'OCP\\AppFramework\\IAppContainer' => __DIR__ . '/../../..' . '/lib/public/AppFramework/IAppContainer.php', 'OCP\\AppFramework\\Middleware' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Middleware.php', @@ -444,6 +445,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OCP\\Search\\PagedProvider' => __DIR__ . '/../../..' . '/lib/public/Search/PagedProvider.php', 'OCP\\Search\\Provider' => __DIR__ . '/../../..' . '/lib/public/Search/Provider.php', 'OCP\\Search\\Result' => __DIR__ . '/../../..' . '/lib/public/Search/Result.php', + 'OCP\\Security\\Bruteforce\\MaxDelayReached' => __DIR__ . '/../../..' . '/lib/public/Security/Bruteforce/MaxDelayReached.php', 'OCP\\Security\\CSP\\AddContentSecurityPolicyEvent' => __DIR__ . '/../../..' . '/lib/public/Security/CSP/AddContentSecurityPolicyEvent.php', 'OCP\\Security\\Events\\GenerateSecurePasswordEvent' => __DIR__ . '/../../..' . '/lib/public/Security/Events/GenerateSecurePasswordEvent.php', 'OCP\\Security\\Events\\ValidatePasswordPolicyEvent' => __DIR__ . '/../../..' . '/lib/public/Security/Events/ValidatePasswordPolicyEvent.php', From 4b558cdf895827c3d052a086045e396281f96aaa Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 19 Aug 2020 12:40:25 +0200 Subject: [PATCH 6/6] More test fixing Signed-off-by: Joas Schilling --- .../Middleware/Security/BruteForceMiddlewareTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php index 55c874ccdb52b..84cbddff46c07 100644 --- a/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php @@ -70,7 +70,7 @@ public function testBeforeControllerWithAnnotation() { ->willReturn('127.0.0.1'); $this->throttler ->expects($this->once()) - ->method('sleepDelay') + ->method('sleepDelayOrThrowOnMax') ->with('127.0.0.1', 'login'); /** @var Controller|\PHPUnit_Framework_MockObject_MockObject $controller */ @@ -92,7 +92,7 @@ public function testBeforeControllerWithoutAnnotation() { ->method('getRemoteAddress'); $this->throttler ->expects($this->never()) - ->method('sleepDelay'); + ->method('sleepDelayOrThrowOnMax'); /** @var Controller|\PHPUnit_Framework_MockObject_MockObject $controller */ $controller = $this->createMock(Controller::class);