From 9bef113b0ecebc3616974c7b810f8f0cea6de962 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 17 Jan 2025 15:01:07 +0100 Subject: [PATCH] fix(ratelimit): Allow to bypass rate-limit from bruteforce allowlist Signed-off-by: Joas Schilling --- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + .../DependencyInjection/DIContainer.php | 5 +- .../Security/RateLimitingMiddleware.php | 9 + lib/private/Security/Bruteforce/Throttler.php | 69 +----- .../Security/Ip/BruteforceAllowList.php | 60 +++++ .../lib/Security/Bruteforce/ThrottlerTest.php | 212 ------------------ .../Security/Ip/BruteforceAllowListTest.php | 162 +++++++++++++ 8 files changed, 240 insertions(+), 279 deletions(-) create mode 100644 lib/private/Security/Ip/BruteforceAllowList.php delete mode 100644 tests/lib/Security/Bruteforce/ThrottlerTest.php create mode 100644 tests/lib/Security/Ip/BruteforceAllowListTest.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index d20e3f9f50130..89fa8f8ba8184 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1941,6 +1941,7 @@ 'OC\\Security\\IdentityProof\\Manager' => $baseDir . '/lib/private/Security/IdentityProof/Manager.php', 'OC\\Security\\IdentityProof\\Signer' => $baseDir . '/lib/private/Security/IdentityProof/Signer.php', 'OC\\Security\\Ip\\Address' => $baseDir . '/lib/private/Security/Ip/Address.php', + 'OC\\Security\\Ip\\BruteforceAllowList' => $baseDir . '/lib/private/Security/Ip/BruteforceAllowList.php', 'OC\\Security\\Ip\\Factory' => $baseDir . '/lib/private/Security/Ip/Factory.php', 'OC\\Security\\Ip\\Range' => $baseDir . '/lib/private/Security/Ip/Range.php', 'OC\\Security\\Ip\\RemoteAddress' => $baseDir . '/lib/private/Security/Ip/RemoteAddress.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index ead08210def82..5c3b8c25811c1 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1990,6 +1990,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Security\\IdentityProof\\Manager' => __DIR__ . '/../../..' . '/lib/private/Security/IdentityProof/Manager.php', 'OC\\Security\\IdentityProof\\Signer' => __DIR__ . '/../../..' . '/lib/private/Security/IdentityProof/Signer.php', 'OC\\Security\\Ip\\Address' => __DIR__ . '/../../..' . '/lib/private/Security/Ip/Address.php', + 'OC\\Security\\Ip\\BruteforceAllowList' => __DIR__ . '/../../..' . '/lib/private/Security/Ip/BruteforceAllowList.php', 'OC\\Security\\Ip\\Factory' => __DIR__ . '/../../..' . '/lib/private/Security/Ip/Factory.php', 'OC\\Security\\Ip\\Range' => __DIR__ . '/../../..' . '/lib/private/Security/Ip/Range.php', 'OC\\Security\\Ip\\RemoteAddress' => __DIR__ . '/../../..' . '/lib/private/Security/Ip/RemoteAddress.php', diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index 2083a0e7195f0..16a8f96ec6eb5 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -21,6 +21,7 @@ use OC\Core\Middleware\TwoFactorMiddleware; use OC\Diagnostics\EventLogger; use OC\Log\PsrLoggerAdapter; +use OC\Security\Ip\BruteforceAllowList; use OC\ServerContainer; use OC\Settings\AuthorizedGroupMapper; use OC\User\Manager as UserManager; @@ -280,7 +281,9 @@ public function __construct(string $appName, array $urlParams = [], ?ServerConta $c->get(IUserSession::class), $c->get(IControllerMethodReflector::class), $c->get(OC\Security\RateLimiting\Limiter::class), - $c->get(ISession::class) + $c->get(ISession::class), + $c->get(\OCP\IAppConfig::class), + $c->get(BruteforceAllowList::class), ) ); $dispatcher->registerMiddleware( diff --git a/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php b/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php index f4d120ebc30ac..404ee0781844e 100644 --- a/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php @@ -9,6 +9,7 @@ namespace OC\AppFramework\Middleware\Security; use OC\AppFramework\Utility\ControllerMethodReflector; +use OC\Security\Ip\BruteforceAllowList; use OC\Security\RateLimiting\Exception\RateLimitExceededException; use OC\Security\RateLimiting\Limiter; use OC\User\Session; @@ -20,6 +21,7 @@ use OCP\AppFramework\Http\Response; use OCP\AppFramework\Http\TemplateResponse; use OCP\AppFramework\Middleware; +use OCP\IAppConfig; use OCP\IRequest; use OCP\ISession; use OCP\IUserSession; @@ -53,6 +55,8 @@ public function __construct( protected ControllerMethodReflector $reflector, protected Limiter $limiter, protected ISession $session, + protected IAppConfig $appConfig, + protected BruteforceAllowList $allowList, ) { } @@ -73,6 +77,11 @@ public function beforeController(Controller $controller, string $methodName): vo $rateLimit = $this->readLimitFromAnnotationOrAttribute($controller, $methodName, 'UserRateThrottle', UserRateLimit::class); if ($rateLimit !== null) { + if ($this->appConfig->getValueBool('bruteforcesettings', 'allowlist_ratelimit') + && $this->allowList->isBypassListed($this->request->getRemoteAddress())) { + return; + } + $this->limiter->registerUserRequest( $rateLimitIdentifier, $rateLimit->getLimit(), diff --git a/lib/private/Security/Bruteforce/Throttler.php b/lib/private/Security/Bruteforce/Throttler.php index 924ae3685f353..21d50848641a8 100644 --- a/lib/private/Security/Bruteforce/Throttler.php +++ b/lib/private/Security/Bruteforce/Throttler.php @@ -9,6 +9,7 @@ namespace OC\Security\Bruteforce; use OC\Security\Bruteforce\Backend\IBackend; +use OC\Security\Ip\BruteforceAllowList; use OC\Security\Normalizer\IpAddress; use OCP\AppFramework\Utility\ITimeFactory; use OCP\IConfig; @@ -32,14 +33,13 @@ class Throttler implements IThrottler { /** @var bool[] */ private array $hasAttemptsDeleted = []; - /** @var bool[] */ - private array $ipIsWhitelisted = []; public function __construct( private ITimeFactory $timeFactory, private LoggerInterface $logger, private IConfig $config, private IBackend $backend, + private BruteforceAllowList $allowList, ) { } @@ -83,70 +83,7 @@ public function registerAttempt(string $action, * Check if the IP is whitelisted */ public function isBypassListed(string $ip): bool { - if (isset($this->ipIsWhitelisted[$ip])) { - return $this->ipIsWhitelisted[$ip]; - } - - if (!$this->config->getSystemValueBool('auth.bruteforce.protection.enabled', true)) { - $this->ipIsWhitelisted[$ip] = true; - return true; - } - - $keys = $this->config->getAppKeys('bruteForce'); - $keys = array_filter($keys, function ($key) { - return str_starts_with($key, 'whitelist_'); - }); - - if (filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)) { - $type = 4; - } elseif (filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6)) { - $type = 6; - } else { - $this->ipIsWhitelisted[$ip] = false; - return false; - } - - $ip = inet_pton($ip); - - foreach ($keys as $key) { - $cidr = $this->config->getAppValue('bruteForce', $key, null); - - $cx = explode('/', $cidr); - $addr = $cx[0]; - $mask = (int)$cx[1]; - - // Do not compare ipv4 to ipv6 - if (($type === 4 && !filter_var($addr, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)) || - ($type === 6 && !filter_var($addr, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6))) { - continue; - } - - $addr = inet_pton($addr); - - $valid = true; - for ($i = 0; $i < $mask; $i++) { - $part = ord($addr[(int)($i / 8)]); - $orig = ord($ip[(int)($i / 8)]); - - $bitmask = 1 << (7 - ($i % 8)); - - $part = $part & $bitmask; - $orig = $orig & $bitmask; - - if ($part !== $orig) { - $valid = false; - break; - } - } - - if ($valid === true) { - $this->ipIsWhitelisted[$ip] = true; - return true; - } - } - - $this->ipIsWhitelisted[$ip] = false; - return false; + return $this->allowList->isBypassListed($ip); } /** diff --git a/lib/private/Security/Ip/BruteforceAllowList.php b/lib/private/Security/Ip/BruteforceAllowList.php new file mode 100644 index 0000000000000..cc4f0ceebe562 --- /dev/null +++ b/lib/private/Security/Ip/BruteforceAllowList.php @@ -0,0 +1,60 @@ + */ + protected array $ipIsAllowListed = []; + + public function __construct( + private readonly IAppConfig $appConfig, + private readonly IFactory $factory, + ) { + } + + /** + * Check if the IP is allowed to bypass bruteforce protection + */ + public function isBypassListed(string $ip): bool { + if (isset($this->ipIsAllowListed[$ip])) { + return $this->ipIsAllowListed[$ip]; + } + + try { + $address = $this->factory->addressFromString($ip); + } catch (\InvalidArgumentException) { + $this->ipIsAllowListed[$ip] = false; + return false; + } + + $keys = $this->appConfig->getKeys('bruteForce'); + $keys = array_filter($keys, static fn ($key): bool => str_starts_with($key, 'whitelist_')); + + foreach ($keys as $key) { + $rangeString = $this->appConfig->getValueString('bruteForce', $key); + try { + $range = $this->factory->rangeFromString($rangeString); + } catch (\InvalidArgumentException) { + continue; + } + + $allowed = $range->contains($address); + if ($allowed) { + $this->ipIsAllowListed[$ip] = true; + return true; + } + } + + $this->ipIsAllowListed[$ip] = false; + return false; + } +} diff --git a/tests/lib/Security/Bruteforce/ThrottlerTest.php b/tests/lib/Security/Bruteforce/ThrottlerTest.php deleted file mode 100644 index a94f1849c8a15..0000000000000 --- a/tests/lib/Security/Bruteforce/ThrottlerTest.php +++ /dev/null @@ -1,212 +0,0 @@ -dbConnection = $this->createMock(IDBConnection::class); - $this->timeFactory = $this->createMock(ITimeFactory::class); - $this->logger = $this->createMock(LoggerInterface::class); - $this->config = $this->createMock(IConfig::class); - - $this->throttler = new Throttler( - $this->timeFactory, - $this->logger, - $this->config, - new DatabaseBackend($this->dbConnection) - ); - parent::setUp(); - } - - public function dataIsIPWhitelisted() { - return [ - [ - '10.10.10.10', - [ - 'whitelist_0' => '10.10.10.0/24', - ], - true, - ], - [ - '10.10.10.10', - [ - 'whitelist_0' => '192.168.0.0/16', - ], - false, - ], - [ - '10.10.10.10', - [ - 'whitelist_0' => '192.168.0.0/16', - 'whitelist_1' => '10.10.10.0/24', - ], - true, - ], - [ - '10.10.10.10', - [ - 'whitelist_0' => '10.10.10.11/31', - ], - true, - ], - [ - '10.10.10.10', - [ - 'whitelist_0' => '10.10.10.9/31', - ], - false, - ], - [ - '10.10.10.10', - [ - 'whitelist_0' => '10.10.10.15/29', - ], - true, - ], - [ - 'dead:beef:cafe::1', - [ - 'whitelist_0' => '192.168.0.0/16', - 'whitelist_1' => '10.10.10.0/24', - 'whitelist_2' => 'deaf:beef:cafe:1234::/64' - ], - false, - ], - [ - 'dead:beef:cafe::1', - [ - 'whitelist_0' => '192.168.0.0/16', - 'whitelist_1' => '10.10.10.0/24', - 'whitelist_2' => 'deaf:beef::/64' - ], - false, - ], - [ - 'dead:beef:cafe::1', - [ - 'whitelist_0' => '192.168.0.0/16', - 'whitelist_1' => '10.10.10.0/24', - 'whitelist_2' => 'deaf:cafe::/8' - ], - true, - ], - [ - 'dead:beef:cafe::1111', - [ - 'whitelist_0' => 'dead:beef:cafe::1100/123', - - ], - true, - ], - [ - 'invalid', - [], - false, - ], - ]; - } - - /** - * @param string $ip - * @param string[] $whitelists - * @param bool $isWhiteListed - * @param bool $enabled - */ - private function isIpWhiteListedHelper($ip, - $whitelists, - $isWhiteListed, - $enabled) { - $this->config->method('getAppKeys') - ->with($this->equalTo('bruteForce')) - ->willReturn(array_keys($whitelists)); - $this->config - ->expects($this->once()) - ->method('getSystemValueBool') - ->with('auth.bruteforce.protection.enabled', true) - ->willReturn($enabled); - - $this->config->method('getAppValue') - ->willReturnCallback(function ($app, $key, $default) use ($whitelists) { - if ($app !== 'bruteForce') { - return $default; - } - if (isset($whitelists[$key])) { - return $whitelists[$key]; - } - return $default; - }); - - $this->assertSame( - ($enabled === false) ? true : $isWhiteListed, - self::invokePrivate($this->throttler, 'isBypassListed', [$ip]) - ); - } - - /** - * @dataProvider dataIsIPWhitelisted - * - * @param string $ip - * @param string[] $whitelists - * @param bool $isWhiteListed - */ - public function testIsIpWhiteListedWithEnabledProtection($ip, - $whitelists, - $isWhiteListed): void { - $this->isIpWhiteListedHelper( - $ip, - $whitelists, - $isWhiteListed, - true - ); - } - - /** - * @dataProvider dataIsIPWhitelisted - * - * @param string $ip - * @param string[] $whitelists - * @param bool $isWhiteListed - */ - public function testIsIpWhiteListedWithDisabledProtection($ip, - $whitelists, - $isWhiteListed): void { - $this->isIpWhiteListedHelper( - $ip, - $whitelists, - $isWhiteListed, - false - ); - } -} diff --git a/tests/lib/Security/Ip/BruteforceAllowListTest.php b/tests/lib/Security/Ip/BruteforceAllowListTest.php new file mode 100644 index 0000000000000..b2f7134be9223 --- /dev/null +++ b/tests/lib/Security/Ip/BruteforceAllowListTest.php @@ -0,0 +1,162 @@ +appConfig = $this->createMock(IAppConfig::class); + $this->factory = new Factory(); + + $this->allowList = new BruteforceAllowList( + $this->appConfig, + $this->factory, + ); + } + + public function dataIsBypassListed(): array { + return [ + [ + '10.10.10.10', + [ + 'whitelist_0' => '10.10.10.0/24', + ], + true, + ], + [ + '10.10.10.10', + [ + 'whitelist_0' => '192.168.0.0/16', + ], + false, + ], + [ + '10.10.10.10', + [ + 'whitelist_0' => '192.168.0.0/16', + 'whitelist_1' => '10.10.10.0/24', + ], + true, + ], + [ + '10.10.10.10', + [ + 'whitelist_0' => '10.10.10.11/31', + ], + true, + ], + [ + '10.10.10.10', + [ + 'whitelist_0' => '10.10.10.9/31', + ], + false, + ], + [ + '10.10.10.10', + [ + 'whitelist_0' => '10.10.10.15/29', + ], + true, + ], + [ + 'dead:beef:cafe::1', + [ + 'whitelist_0' => '192.168.0.0/16', + 'whitelist_1' => '10.10.10.0/24', + 'whitelist_2' => 'deaf:beef:cafe:1234::/64' + ], + false, + ], + [ + 'dead:beef:cafe::1', + [ + 'whitelist_0' => '192.168.0.0/16', + 'whitelist_1' => '10.10.10.0/24', + 'whitelist_2' => 'deaf:beef::/64' + ], + false, + ], + [ + 'dead:beef:cafe::1', + [ + 'whitelist_0' => '192.168.0.0/16', + 'whitelist_1' => '10.10.10.0/24', + 'whitelist_2' => 'deaf:cafe::/8' + ], + true, + ], + [ + 'dead:beef:cafe::1111', + [ + 'whitelist_0' => 'dead:beef:cafe::1100/123', + ], + true, + ], + [ + 'invalid', + [], + false, + ], + ]; + } + + /** + * @dataProvider dataIsBypassListed + * + * @param string[] $allowList + */ + public function testIsBypassListed( + string $ip, + array $allowList, + bool $isAllowListed + ): void { + $this->appConfig->method('getKeys') + ->with($this->equalTo('bruteForce')) + ->willReturn(array_keys($allowList)); + + $this->appConfig->method('getValueString') + ->willReturnCallback(function ($app, $key, $default) use ($allowList) { + if ($app !== 'bruteForce') { + return $default; + } + if (isset($allowList[$key])) { + return $allowList[$key]; + } + return $default; + }); + + $this->assertSame( + $isAllowListed, + $this->allowList->isBypassListed($ip) + ); + } +}