From 7964f338dc3346dd3907b80d85c992789a229f95 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 2 May 2025 11:27:04 +0200 Subject: [PATCH] fix(throttler): Remove the sleep from the throttler that throws The sleep is not adding benefit when it's being aborted with 429 in other cases anyway. Signed-off-by: Joas Schilling --- apps/dav/lib/Connector/Sabre/PublicAuth.php | 9 ++++-- apps/dav/lib/Direct/DirectHome.php | 2 +- apps/dav/lib/Files/BrowserErrorPagePlugin.php | 6 +++- apps/dav/tests/unit/Direct/DirectHomeTest.php | 2 +- .../PublicShare/PublicShareMiddleware.php | 2 +- lib/private/Security/Bruteforce/Throttler.php | 29 +++++++++++++------ 6 files changed, 34 insertions(+), 16 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/PublicAuth.php b/apps/dav/lib/Connector/Sabre/PublicAuth.php index ea59d9efc8f93..b5d9ce3db7211 100644 --- a/apps/dav/lib/Connector/Sabre/PublicAuth.php +++ b/apps/dav/lib/Connector/Sabre/PublicAuth.php @@ -15,6 +15,7 @@ use OCP\IRequest; use OCP\ISession; use OCP\Security\Bruteforce\IThrottler; +use OCP\Security\Bruteforce\MaxDelayReached; use OCP\Share\Exceptions\ShareNotFound; use OCP\Share\IManager; use OCP\Share\IShare; @@ -56,6 +57,7 @@ public function __construct( * * @return array * @throws NotAuthenticated + * @throws MaxDelayReached * @throws ServiceUnavailable */ public function check(RequestInterface $request, ResponseInterface $response): array { @@ -75,7 +77,8 @@ public function check(RequestInterface $request, ResponseInterface $response): a } return $this->checkToken(); - } catch (NotAuthenticated $e) { + } catch (NotAuthenticated|MaxDelayReached $e) { + $this->throttler->registerAttempt(self::BRUTEFORCE_ACTION, $this->request->getRemoteAddress()); throw $e; } catch (\Exception $e) { $class = get_class($e); @@ -94,7 +97,7 @@ private function getToken(): string { $path = $this->request->getPathInfo() ?: ''; // ['', 'dav', 'files', 'token'] $splittedPath = explode('/', $path); - + if (count($splittedPath) < 4 || $splittedPath[3] === '') { throw new NotFound(); } @@ -176,7 +179,7 @@ protected function validateUserPass($username, $password) { } return true; } - + if ($this->session->exists(PublicAuth::DAV_AUTHENTICATED) && $this->session->get(PublicAuth::DAV_AUTHENTICATED) === $share->getId()) { return true; diff --git a/apps/dav/lib/Direct/DirectHome.php b/apps/dav/lib/Direct/DirectHome.php index 10e1017f5a463..ac411c9b52fb3 100644 --- a/apps/dav/lib/Direct/DirectHome.php +++ b/apps/dav/lib/Direct/DirectHome.php @@ -53,7 +53,7 @@ public function getChild($name): DirectFile { } catch (DoesNotExistException $e) { // Since the token space is so huge only throttle on non-existing token $this->throttler->registerAttempt('directlink', $this->request->getRemoteAddress()); - $this->throttler->sleepDelay($this->request->getRemoteAddress(), 'directlink'); + $this->throttler->sleepDelayOrThrowOnMax($this->request->getRemoteAddress(), 'directlink'); throw new NotFound(); } diff --git a/apps/dav/lib/Files/BrowserErrorPagePlugin.php b/apps/dav/lib/Files/BrowserErrorPagePlugin.php index de86c4995e293..85ed975a40930 100644 --- a/apps/dav/lib/Files/BrowserErrorPagePlugin.php +++ b/apps/dav/lib/Files/BrowserErrorPagePlugin.php @@ -11,6 +11,7 @@ use OCP\AppFramework\Http\ContentSecurityPolicy; use OCP\AppFramework\Http\TemplateResponse; use OCP\IRequest; +use OCP\Security\Bruteforce\MaxDelayReached; use OCP\Template\ITemplateManager; use Sabre\DAV\Exception; use Sabre\DAV\Server; @@ -60,6 +61,9 @@ public function logException(\Throwable $ex): void { if ($ex instanceof Exception) { $httpCode = $ex->getHTTPCode(); $headers = $ex->getHTTPHeaders($this->server); + } elseif ($ex instanceof MaxDelayReached) { + $httpCode = 429; + $headers = []; } else { $httpCode = 500; $headers = []; @@ -81,7 +85,7 @@ public function generateBody(int $httpCode) { $request = \OCP\Server::get(IRequest::class); $templateName = 'exception'; - if ($httpCode === 403 || $httpCode === 404) { + if ($httpCode === 403 || $httpCode === 404 || $httpCode === 429) { $templateName = (string)$httpCode; } diff --git a/apps/dav/tests/unit/Direct/DirectHomeTest.php b/apps/dav/tests/unit/Direct/DirectHomeTest.php index 1134f0cd3afed..06fb48a64d807 100644 --- a/apps/dav/tests/unit/Direct/DirectHomeTest.php +++ b/apps/dav/tests/unit/Direct/DirectHomeTest.php @@ -160,7 +160,7 @@ public function testGetChildInvalid(): void { '1.2.3.4' ); $this->throttler->expects($this->once()) - ->method('sleepDelay') + ->method('sleepDelayOrThrowOnMax') ->with( '1.2.3.4', 'directlink' diff --git a/lib/private/AppFramework/Middleware/PublicShare/PublicShareMiddleware.php b/lib/private/AppFramework/Middleware/PublicShare/PublicShareMiddleware.php index 2b3025fccff5b..b3040673d0f25 100644 --- a/lib/private/AppFramework/Middleware/PublicShare/PublicShareMiddleware.php +++ b/lib/private/AppFramework/Middleware/PublicShare/PublicShareMiddleware.php @@ -120,7 +120,7 @@ private function isLinkSharingEnabled(): bool { private function throttle($bruteforceProtectionAction, $token): void { $ip = $this->request->getRemoteAddress(); - $this->throttler->sleepDelay($ip, $bruteforceProtectionAction); + $this->throttler->sleepDelayOrThrowOnMax($ip, $bruteforceProtectionAction); $this->throttler->registerAttempt($bruteforceProtectionAction, $ip, ['token' => $token]); } } diff --git a/lib/private/Security/Bruteforce/Throttler.php b/lib/private/Security/Bruteforce/Throttler.php index 21d50848641a8..065f720ba7269 100644 --- a/lib/private/Security/Bruteforce/Throttler.php +++ b/lib/private/Security/Bruteforce/Throttler.php @@ -127,6 +127,13 @@ public function getAttempts(string $ip, string $action = '', float $maxAgeHours */ public function getDelay(string $ip, string $action = ''): int { $attempts = $this->getAttempts($ip, $action); + return $this->calculateDelay($attempts); + } + + /** + * {@inheritDoc} + */ + public function calculateDelay(int $attempts): int { if ($attempts === 0) { return 0; } @@ -199,25 +206,29 @@ public function sleepDelay(string $ip, string $action = ''): int { * {@inheritDoc} */ public function sleepDelayOrThrowOnMax(string $ip, string $action = ''): int { - $delay = $this->getDelay($ip, $action); - if (($delay === self::MAX_DELAY_MS) && $this->getAttempts($ip, $action, 0.5) > $this->config->getSystemValueInt('auth.bruteforce.max-attempts', self::MAX_ATTEMPTS)) { - $this->logger->info('IP address blocked because it reached the maximum failed attempts in the last 30 minutes [action: {action}, ip: {ip}]', [ + $attempts = $this->getAttempts($ip, $action, 0.5); + if ($attempts > $this->config->getSystemValueInt('auth.bruteforce.max-attempts', self::MAX_ATTEMPTS)) { + $this->logger->info('IP address blocked because it reached the maximum failed attempts in the last 30 minutes [action: {action}, attempts: {attempts}, ip: {ip}]', [ 'action' => $action, 'ip' => $ip, + 'attempts' => $attempts, ]); // If the ip made too many attempts within the last 30 mins we don't execute anymore throw new MaxDelayReached('Reached maximum delay'); } - if ($delay > 100) { - $this->logger->info('IP address throttled because it reached the attempts limit in the last 30 minutes [action: {action}, delay: {delay}, ip: {ip}]', [ + + $attempts = $this->getAttempts($ip, $action); + if ($attempts > 10) { + $this->logger->info('IP address throttled because it reached the attempts limit in the last 12 hours [action: {action}, attempts: {attempts}, ip: {ip}]', [ 'action' => $action, 'ip' => $ip, - 'delay' => $delay, + 'attempts' => $attempts, ]); } - if (!$this->config->getSystemValueBool('auth.bruteforce.protection.testing')) { - usleep($delay * 1000); + if ($attempts > 0) { + return $this->calculateDelay($attempts); } - return $delay; + + return 0; } }