summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorMorris Jobke <hey@morrisjobke.de>2020-08-19 18:21:01 +0200
committerGitHub <noreply@github.com>2020-08-19 18:21:01 +0200
commit4c6eb96471b90c41e26bdf2cb3edf2cb15aec613 (patch)
tree49ea3b11915636e3f87640e24178b180939c8848
parent60be722ee8781d9e94ecc66d62c0e5fcb7e3934e (diff)
parente93bf713690047da6e48f882848c7f1ba832db4e (diff)
downloadnextcloud-server-4c6eb96471b90c41e26bdf2cb3edf2cb15aec613.tar.gz
nextcloud-server-4c6eb96471b90c41e26bdf2cb3edf2cb15aec613.zip
Merge pull request #22280 from nextcloud/bugfix/noid/429-on-brute-force-maximum
Send "429 Too Many Requests" in case of brute force protection
-rw-r--r--build/psalm-baseline.xml13
-rw-r--r--core/templates/429.php4
-rw-r--r--lib/composer/composer/autoload_classmap.php2
-rw-r--r--lib/composer/composer/autoload_static.php2
-rw-r--r--lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php29
-rw-r--r--lib/private/Security/Bruteforce/Throttler.php89
-rw-r--r--lib/private/legacy/OC_Template.php2
-rw-r--r--lib/public/AppFramework/Http/TooManyRequestsResponse.php52
-rw-r--r--lib/public/Security/Bruteforce/MaxDelayReached.php31
-rw-r--r--tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php4
-rw-r--r--tests/lib/User/SessionTest.php12
11 files changed, 188 insertions, 52 deletions
diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml
index a1d7c787c80..be5769de7ae 100644
--- a/build/psalm-baseline.xml
+++ b/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>
diff --git a/core/templates/429.php b/core/templates/429.php
new file mode 100644
index 00000000000..caf8a3675e2
--- /dev/null
+++ b/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>
diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php
index e063fe0b715..549b11c05ec 100644
--- a/lib/composer/composer/autoload_classmap.php
+++ b/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',
diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php
index 20394b4ae30..1a771332437 100644
--- a/lib/composer/composer/autoload_static.php
+++ b/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',
diff --git a/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php b/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php
index 398c2f3f3a4..e6c511537a0 100644
--- a/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php
+++ b/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;
+ }
}
diff --git a/lib/private/Security/Bruteforce/Throttler.php b/lib/private/Security/Bruteforce/Throttler.php
index 63c6361b9ce..169ad0c0623 100644
--- a/lib/private/Security/Bruteforce/Throttler.php
+++ b/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;
}
- return (int) \ceil($firstDelay * 1000);
+
+ $delay = $firstDelay * 2**$attempts;
+ if ($delay > self::MAX_DELAY) {
+ return self::MAX_DELAY_MS;
+ }
+ 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;
+ }
}
diff --git a/lib/private/legacy/OC_Template.php b/lib/private/legacy/OC_Template.php
index 18a15ad1d43..54c203a3ab6 100644
--- a/lib/private/legacy/OC_Template.php
+++ b/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.
diff --git a/lib/public/AppFramework/Http/TooManyRequestsResponse.php b/lib/public/AppFramework/Http/TooManyRequestsResponse.php
new file mode 100644
index 00000000000..b15df303bfe
--- /dev/null
+++ b/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();
+ }
+}
diff --git a/lib/public/Security/Bruteforce/MaxDelayReached.php b/lib/public/Security/Bruteforce/MaxDelayReached.php
new file mode 100644
index 00000000000..3aaa7d05159
--- /dev/null
+++ b/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 {
+}
diff --git a/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php
index f8db85a9358..786bac6d856 100644
--- a/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php
+++ b/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);
diff --git a/tests/lib/User/SessionTest.php b/tests/lib/User/SessionTest.php
index 48c4785bf71..8fd94ffc004 100644
--- a/tests/lib/User/SessionTest.php
+++ b/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')