Merge pull request #22280 from nextcloud/bugfix/noid/429-on-brute-force-maximum

Send "429 Too Many Requests" in case of brute force protection

build/psalm-baseline.xml

@@ -6067,11 +6067,6 @@
       <code>OC_User::getUser()</code>
     </InvalidScalarArgument>
   </file>
-  <file src="lib/private/legacy/OC_Template.php">
-    <ImplementedReturnTypeMismatch occurrences="1">
-      <code>boolean|string</code>
-    </ImplementedReturnTypeMismatch>
-  </file>
   <file src="lib/private/legacy/OC_User.php">
     <UndefinedClass occurrences="1">
       <code>\Test\Util\User\Dummy</code>
@@ -6153,14 +6148,6 @@
   <file src="lib/public/AppFramework/Http/Template/PublicTemplateResponse.php">
     <InvalidScalarArgument occurrences="1"/>
   </file>
-  <file src="lib/public/AppFramework/Http/TemplateResponse.php">
-    <InvalidReturnStatement occurrences="1">
-      <code>$template-&gt;fetchPage($this-&gt;params)</code>
-    </InvalidReturnStatement>
-    <InvalidReturnType occurrences="1">
-      <code>string</code>
-    </InvalidReturnType>
-  </file>
   <file src="lib/public/AppFramework/Http/ZipResponse.php">
     <InvalidArrayAccess occurrences="5">
       <code>$resource['size']</code>

core/templates/429.php

@@ -0,0 +1,4 @@
+<div class="body-login-container update">
+	<h2><?php p($l->t('Too many requests')); ?></h2>
+	<p class="infogroup"><?php p($l->t('There were too many requests from your network. Retry later or contact your administrator if this is an error.')); ?></p>
+</div>

lib/composer/composer/autoload_classmap.php

@@ -64,6 +64,7 @@ return array(
     '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',
@@ -448,6 +449,7 @@ return array(
     'OCP\\Search\\Result' => $baseDir . '/lib/public/Search/Result.php',
     'OCP\\Search\\SearchResult' => $baseDir . '/lib/public/Search/SearchResult.php',
     'OCP\\Search\\SearchResultEntry' => $baseDir . '/lib/public/Search/SearchResultEntry.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',

lib/composer/composer/autoload_static.php

@@ -93,6 +93,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',
@@ -477,6 +478,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c
         'OCP\\Search\\Result' => __DIR__ . '/../../..' . '/lib/public/Search/Result.php',
         'OCP\\Search\\SearchResult' => __DIR__ . '/../../..' . '/lib/public/Search/SearchResult.php',
         'OCP\\Search\\SearchResultEntry' => __DIR__ . '/../../..' . '/lib/public/Search/SearchResultEntry.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',

lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php

@@ -1,4 +1,6 @@
 <?php
+
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2017 Lukas Reschke <lukas@statuscode.ch>
  *
@@ -26,9 +28,15 @@ namespace OC\AppFramework\Middleware\Security;
 
 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 +74,7 @@ class BruteForceMiddleware extends Middleware {
 
 		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 +91,23 @@ class BruteForceMiddleware extends Middleware {
 
 		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;
+	}
 }

lib/private/Security/Bruteforce/Throttler.php

@@ -1,4 +1,6 @@
 <?php
+
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2016 Lukas Reschke <lukas@statuscode.ch>
  *
@@ -34,6 +36,7 @@ use OCP\AppFramework\Utility\ITimeFactory;
 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 +53,9 @@ use OCP\ILogger;
  */
 class Throttler {
 	public const LOGIN_ACTION = 'login';
+	public const MAX_DELAY = 25;
+	public const MAX_DELAY_MS = 25000; // in milliseconds
+	public const MAX_ATTEMPTS = 10;
 
 	/** @var IDBConnection */
 	private $db;
@@ -82,7 +88,7 @@ class Throttler {
 	 * @param int $expire
 	 * @return \DateInterval
 	 */
-	private function getCutoff($expire) {
+	private function getCutoff(int $expire): \DateInterval {
 		$d1 = new \DateTime();
 		$d2 = clone $d1;
 		$d2->sub(new \DateInterval('PT' . $expire . 'S'));
@@ -92,11 +98,12 @@ class Throttler {
 	/**
 	 *  Calculate the cut off timestamp
 	 *
+	 * @param float $maxAgeHours
 	 * @return int
 	 */
-	private function getCutoffTimestamp(): int {
+	private function getCutoffTimestamp(float $maxAgeHours = 12.0): int {
 		return (new \DateTime())
-			->sub($this->getCutoff(43200))
+			->sub($this->getCutoff((int) ($maxAgeHours * 3600)))
 			->getTimestamp();
 	}
 
@@ -108,9 +115,9 @@ class Throttler {
 	 * @param array $metadata Optional metadata logged to the database
 	 * @suppress SqlInjectionChecker
 	 */
-	public function registerAttempt($action,
-									$ip,
-									array $metadata = []) {
+	public function registerAttempt(string $action,
+									string $ip,
+									array $metadata = []): void {
 		// No need to log if the bruteforce protection is disabled
 		if ($this->config->getSystemValue('auth.bruteforce.protection.enabled', true) === false) {
 			return;
@@ -150,15 +157,14 @@ class Throttler {
 	 * @param string $ip
 	 * @return bool
 	 */
-	private function isIPWhitelisted($ip) {
+	private function isIPWhitelisted(string $ip): bool {
 		if ($this->config->getSystemValue('auth.bruteforce.protection.enabled', true) === false) {
 			return true;
 		}
 
 		$keys = $this->config->getAppKeys('bruteForce');
 		$keys = array_filter($keys, function ($key) {
-			$regex = '/^whitelist_/S';
-			return preg_match($regex, $key) === 1;
+			return 0 === strpos($key, 'whitelist_');
 		});
 
 		if (filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)) {
@@ -215,18 +221,19 @@ class Throttler {
 	 *
 	 * @param string $ip
 	 * @param string $action optionally filter by action
+	 * @param float $maxAgeHours
 	 * @return int
 	 */
-	public function getDelay($ip, $action = '') {
+	public function getAttempts(string $ip, string $action = '', float $maxAgeHours = 12): int {
 		$ipAddress = new IpAddress($ip);
 		if ($this->isIPWhitelisted((string)$ipAddress)) {
 			return 0;
 		}
 
-		$cutoffTime = $this->getCutoffTimestamp();
+		$cutoffTime = $this->getCutoffTimestamp($maxAgeHours);
 
 		$qb = $this->db->getQueryBuilder();
-		$qb->select('*')
+		$qb->select($qb->func()->count('*', 'attempts'))
 			->from('bruteforce_attempts')
 			->where($qb->expr()->gt('occurred', $qb->createNamedParameter($cutoffTime)))
 			->andWhere($qb->expr()->eq('subnet', $qb->createNamedParameter($ipAddress->getSubnet())));
@@ -235,24 +242,37 @@ class Throttler {
 			$qb->andWhere($qb->expr()->eq('action', $qb->createNamedParameter($action)));
 		}
 
-		$attempts = count($qb->execute()->fetchAll());
+		$result = $qb->execute();
+		$row = $result->fetch();
+		$result->closeCursor();
 
+		return (int) $row['attempts'];
+	}
+
+	/**
+	 * Get the throttling delay (in milliseconds)
+	 *
+	 * @param string $ip
+	 * @param string $action optionally filter by action
+	 * @return int
+	 */
+	public function getDelay(string $ip, string $action = ''): int {
+		$attempts = $this->getAttempts($ip, $action);
 		if ($attempts === 0) {
 			return 0;
 		}
 
-		$maxDelay = 25;
 		$firstDelay = 0.1;
-		if ($attempts > (8 * PHP_INT_SIZE - 1)) {
+		if ($attempts > self::MAX_ATTEMPTS) {
 			// Don't ever overflow. Just assume the maxDelay time:s
-			$firstDelay = $maxDelay;
-		} else {
-			$firstDelay *= pow(2, $attempts);
-			if ($firstDelay > $maxDelay) {
-				$firstDelay = $maxDelay;
+			return self::MAX_DELAY_MS;
 		}
+
+		$delay = $firstDelay * 2**$attempts;
+		if ($delay > self::MAX_DELAY) {
+			return self::MAX_DELAY_MS;
 		}
-		return (int) \ceil($firstDelay * 1000);
+		return (int) \ceil($delay * 1000);
 	}
 
 	/**
@@ -260,9 +280,9 @@ class Throttler {
 	 *
 	 * @param string $ip
 	 * @param string $action
-	 * @param string $metadata
+	 * @param array $metadata
 	 */
-	public function resetDelay($ip, $action, $metadata) {
+	public function resetDelay(string $ip, string $action, array $metadata): void {
 		$ipAddress = new IpAddress($ip);
 		if ($this->isIPWhitelisted((string)$ipAddress)) {
 			return;
@@ -303,9 +323,28 @@ class Throttler {
 	 * @param string $action optionally filter by action
 	 * @return int the time spent sleeping
 	 */
-	public function sleepDelay($ip, $action = '') {
+	public function sleepDelay(string $ip, string $action = ''): int {
 		$delay = $this->getDelay($ip, $action);
 		usleep($delay * 1000);
 		return $delay;
 	}
+
+	/**
+	 * Will sleep for the defined amount of time unless maximum was reached in the last 30 minutes
+	 * In this case a "429 Too Many Request" exception 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(string $ip, string $action = ''): int {
+		$delay = $this->getDelay($ip, $action);
+		if (($delay === self::MAX_DELAY_MS) && $this->getAttempts($ip, $action, 0.5) > self::MAX_ATTEMPTS) {
+			// If the ip made too many attempts within the last 30 mins we don't execute anymore
+			throw new MaxDelayReached('Reached maximum delay');
+		}
+		usleep($delay * 1000);
+		return $delay;
+	}
 }

lib/private/legacy/OC_Template.php

@@ -171,7 +171,7 @@ class OC_Template extends \OC\Template\Base {
 
 	/**
 	 * Process the template
-	 * @return boolean|string
+	 * @return string
 	 *
 	 * This function process the template. If $this->renderAs is set, it
 	 * will produce a full page.

lib/public/AppFramework/Http/TooManyRequestsResponse.php

@@ -0,0 +1,52 @@
+<?php
+
+declare(strict_types=1);
+/**
+ * @copyright Copyright (c) 2020 Joas Schilling <coding@schilljs.com>
+ *
+ * @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 <http://www.gnu.org/licenses/>.
+ *
+ */
+
+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();
+	}
+}

lib/public/Security/Bruteforce/MaxDelayReached.php

@@ -0,0 +1,31 @@
+<?php
+
+declare(strict_types=1);
+/**
+ * @copyright Copyright (c) 2020 Joas Schilling <coding@schilljs.com>
+ *
+ * @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 <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace OCP\Security\Bruteforce;
+
+/**
+ * Class MaxDelayReached
+ * @since 19.0
+ */
+class MaxDelayReached extends \RuntimeException {
+}

tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php

@@ -70,7 +70,7 @@ class BruteForceMiddlewareTest extends TestCase {
 			->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 @@ class BruteForceMiddlewareTest extends TestCase {
 			->method('getRemoteAddress');
 		$this->throttler
 			->expects($this->never())
-			->method('sleepDelay');
+			->method('sleepDelayOrThrowOnMax');
 
 		/** @var Controller|\PHPUnit\Framework\MockObject\MockObject $controller */
 		$controller = $this->createMock(Controller::class);

tests/lib/User/SessionTest.php

@@ -1264,12 +1264,8 @@ class SessionTest extends \Test\TestCase {
 		$this->throttler
 			->expects($this->once())
 			->method('sleepDelay')
-			->with('192.168.0.1');
-		$this->throttler
-			->expects($this->any())
-			->method('getDelay')
 			->with('192.168.0.1')
-			->willReturn(0);
+			->willReturn(5);
 		$this->timeFactory
 			->expects($this->any())
 			->method('getTime')
@@ -1318,12 +1314,8 @@ class SessionTest extends \Test\TestCase {
 		$this->throttler
 			->expects($this->once())
 			->method('sleepDelay')
-			->with('192.168.0.1');
-		$this->throttler
-			->expects($this->any())
-			->method('getDelay')
 			->with('192.168.0.1')
-			->willReturn(0);
+			->willReturn(5);
 		$this->timeFactory
 			->expects($this->any())
 			->method('getTime')