nextcloud · nickvergessen · Jan 17, 2025
diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php
@@ -1942,6 +1942,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
@@ -1991,6 +1991,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
@@ -274,15 +274,7 @@ public function __construct(string $appName, array $urlParams = [], ?ServerConta
 					$c->get(LoggerInterface::class)
 				)
 			);
-			$dispatcher->registerMiddleware(
-				new RateLimitingMiddleware(
-					$c->get(IRequest::class),
-					$c->get(IUserSession::class),
-					$c->get(IControllerMethodReflector::class),
-					$c->get(OC\Security\RateLimiting\Limiter::class),
-					$c->get(ISession::class)
-				)
-			);
+			$dispatcher->registerMiddleware($c->get(RateLimitingMiddleware::class));
 			$dispatcher->registerMiddleware(
 				new OC\AppFramework\Middleware\PublicShare\PublicShareMiddleware(
 					$c->get(IRequest::class),

diff --git 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 $bruteForceAllowList,
 	) {
 	}
 
@@ -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', 'apply_allowlist_to_ratelimit')
+					&& $this->bruteForceAllowList->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
@@ -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
@@ -0,0 +1,60 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+namespace OC\Security\Ip;
+
+use OCP\IAppConfig;
+use OCP\Security\Ip\IFactory;
+
+class BruteforceAllowList {
+	/** @var array<string, bool> */
+	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/AppFramework/Middleware/Security/RateLimitingMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/RateLimitingMiddlewareTest.php
@@ -11,13 +11,15 @@
 
 use OC\AppFramework\Middleware\Security\RateLimitingMiddleware;
 use OC\AppFramework\Utility\ControllerMethodReflector;
+use OC\Security\Ip\BruteforceAllowList;
 use OC\Security\RateLimiting\Exception\RateLimitExceededException;
 use OC\Security\RateLimiting\Limiter;
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http\Attribute\AnonRateLimit;
 use OCP\AppFramework\Http\Attribute\UserRateLimit;
 use OCP\AppFramework\Http\DataResponse;
 use OCP\AppFramework\Http\TemplateResponse;
+use OCP\IAppConfig;
 use OCP\IRequest;
 use OCP\ISession;
 use OCP\IUser;
@@ -61,6 +63,8 @@ class RateLimitingMiddlewareTest extends TestCase {
 	private ControllerMethodReflector $reflector;
 	private Limiter|MockObject $limiter;
 	private ISession|MockObject $session;
+	private IAppConfig|MockObject $appConfig;
+	private BruteforceAllowList|MockObject $bruteForceAllowList;
 	private RateLimitingMiddleware $rateLimitingMiddleware;
 
 	protected function setUp(): void {
@@ -71,13 +75,17 @@ protected function setUp(): void {
 		$this->reflector = new ControllerMethodReflector();
 		$this->limiter = $this->createMock(Limiter::class);
 		$this->session = $this->createMock(ISession::class);
+		$this->appConfig = $this->createMock(IAppConfig::class);
+		$this->bruteForceAllowList = $this->createMock(BruteforceAllowList::class);
 
 		$this->rateLimitingMiddleware = new RateLimitingMiddleware(
 			$this->request,
 			$this->userSession,
 			$this->reflector,
 			$this->limiter,
-			$this->session
+			$this->session,
+			$this->appConfig,
+			$this->bruteForceAllowList,
 		);
 	}