diff options
author | Joas Schilling <coding@schilljs.com> | 2024-07-17 15:25:51 +0200 |
---|---|---|
committer | Benjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com> | 2024-07-19 16:28:03 +0200 |
commit | 047479ccf9ff332cc249cd08d5c315394f3e48da (patch) | |
tree | 1001b114f3857338bba5e520e941fca4914a2be4 | |
parent | 202e5b1e957a7692165a313710e38406ca4f6ff3 (diff) | |
download | nextcloud-server-047479ccf9ff332cc249cd08d5c315394f3e48da.tar.gz nextcloud-server-047479ccf9ff332cc249cd08d5c315394f3e48da.zip |
feat(security): Add public API to allow validating IP Ranges and checking for "in range"
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Benjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com>
-rw-r--r-- | apps/settings/lib/SetupChecks/AllowedAdminRanges.php | 25 | ||||
-rw-r--r-- | lib/composer/composer/autoload_classmap.php | 7 | ||||
-rw-r--r-- | lib/composer/composer/autoload_static.php | 7 | ||||
-rw-r--r-- | lib/private/AppFramework/DependencyInjection/DIContainer.php | 4 | ||||
-rw-r--r-- | lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php | 10 | ||||
-rw-r--r-- | lib/private/Group/Manager.php | 6 | ||||
-rw-r--r-- | lib/private/Security/Ip/Address.php | 48 | ||||
-rw-r--r-- | lib/private/Security/Ip/Range.php | 39 | ||||
-rw-r--r-- | lib/private/Security/Ip/RemoteAddress.php | 71 | ||||
-rw-r--r-- | lib/private/Security/RemoteIpAddress.php | 64 | ||||
-rw-r--r-- | lib/private/Server.php | 7 | ||||
-rw-r--r-- | lib/public/Security/Ip/IAddress.php | 35 | ||||
-rw-r--r-- | lib/public/Security/Ip/IRange.php | 37 | ||||
-rw-r--r-- | lib/public/Security/Ip/IRemoteAddress.php | 22 | ||||
-rw-r--r-- | tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php | 4 | ||||
-rw-r--r-- | tests/lib/Group/ManagerTest.php | 6 | ||||
-rw-r--r-- | tests/lib/Security/Ip/RemoteAddressTest.php (renamed from tests/lib/Security/RemoteIpAddressTest.php) | 29 |
17 files changed, 311 insertions, 110 deletions
diff --git a/apps/settings/lib/SetupChecks/AllowedAdminRanges.php b/apps/settings/lib/SetupChecks/AllowedAdminRanges.php index 7c50c261dea..87e11b06be7 100644 --- a/apps/settings/lib/SetupChecks/AllowedAdminRanges.php +++ b/apps/settings/lib/SetupChecks/AllowedAdminRanges.php @@ -8,8 +8,8 @@ declare(strict_types=1); */ namespace OCA\Settings\SetupChecks; -use IPLib\Factory; -use OC\Security\RemoteIpAddress; +use OC\Security\Ip\Range; +use OC\Security\Ip\RemoteAddress; use OCP\IConfig; use OCP\IL10N; use OCP\SetupCheck\ISetupCheck; @@ -31,8 +31,11 @@ class AllowedAdminRanges implements ISetupCheck { } public function run(): SetupResult { - $allowedAdminRanges = $this->config->getSystemValue(RemoteIpAddress::SETTING_NAME, false); - if ($allowedAdminRanges === false) { + $allowedAdminRanges = $this->config->getSystemValue(RemoteAddress::SETTING_NAME, false); + if ( + $allowedAdminRanges === false + || (is_array($allowedAdminRanges) && empty($allowedAdminRanges)) + ) { return SetupResult::success($this->l10n->t('Admin IP filtering isn’t applied.')); } @@ -40,23 +43,17 @@ class AllowedAdminRanges implements ISetupCheck { return SetupResult::error( $this->l10n->t( 'Configuration key "%1$s" expects an array (%2$s found). Admin IP range validation will not be applied.', - [RemoteIpAddress::SETTING_NAME, gettype($allowedAdminRanges)], + [RemoteAddress::SETTING_NAME, gettype($allowedAdminRanges)], ) ); } - $invalidRanges = array_reduce($allowedAdminRanges, static function (array $carry, mixed $range) { - if (!is_string($range) || Factory::parseRangeString($range) === null) { - $carry[] = $range; - } - - return $carry; - }, []); - if (count($invalidRanges) > 0) { + $invalidRanges = array_filter($allowedAdminRanges, static fn (mixed $range): bool => !is_string($range) || !Range::isValid($range)); + if (!empty($invalidRanges)) { return SetupResult::warning( $this->l10n->t( 'Configuration key "%1$s" contains invalid IP range(s): "%2$s"', - [RemoteIpAddress::SETTING_NAME, implode('", "', $invalidRanges)], + [RemoteAddress::SETTING_NAME, implode('", "', $invalidRanges)], ), ); } diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 9519dccd109..8a49ede0e06 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -644,6 +644,9 @@ return array( 'OCP\\Security\\IRemoteHostValidator' => $baseDir . '/lib/public/Security/IRemoteHostValidator.php', 'OCP\\Security\\ISecureRandom' => $baseDir . '/lib/public/Security/ISecureRandom.php', 'OCP\\Security\\ITrustedDomainHelper' => $baseDir . '/lib/public/Security/ITrustedDomainHelper.php', + 'OCP\\Security\\Ip\\IAddress' => $baseDir . '/lib/public/Security/Ip/IAddress.php', + 'OCP\\Security\\Ip\\IRange' => $baseDir . '/lib/public/Security/Ip/IRange.php', + 'OCP\\Security\\Ip\\IRemoteAddress' => $baseDir . '/lib/public/Security/Ip/IRemoteAddress.php', 'OCP\\Security\\RateLimiting\\ILimiter' => $baseDir . '/lib/public/Security/RateLimiting/ILimiter.php', 'OCP\\Security\\RateLimiting\\IRateLimitExceededException' => $baseDir . '/lib/public/Security/RateLimiting/IRateLimitExceededException.php', 'OCP\\Security\\VerificationToken\\IVerificationToken' => $baseDir . '/lib/public/Security/VerificationToken/IVerificationToken.php', @@ -1808,6 +1811,9 @@ return array( 'OC\\Security\\IdentityProof\\Key' => $baseDir . '/lib/private/Security/IdentityProof/Key.php', '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\\Range' => $baseDir . '/lib/private/Security/Ip/Range.php', + 'OC\\Security\\Ip\\RemoteAddress' => $baseDir . '/lib/private/Security/Ip/RemoteAddress.php', 'OC\\Security\\Normalizer\\IpAddress' => $baseDir . '/lib/private/Security/Normalizer/IpAddress.php', 'OC\\Security\\RateLimiting\\Backend\\DatabaseBackend' => $baseDir . '/lib/private/Security/RateLimiting/Backend/DatabaseBackend.php', 'OC\\Security\\RateLimiting\\Backend\\IBackend' => $baseDir . '/lib/private/Security/RateLimiting/Backend/IBackend.php', @@ -1815,7 +1821,6 @@ return array( 'OC\\Security\\RateLimiting\\Exception\\RateLimitExceededException' => $baseDir . '/lib/private/Security/RateLimiting/Exception/RateLimitExceededException.php', 'OC\\Security\\RateLimiting\\Limiter' => $baseDir . '/lib/private/Security/RateLimiting/Limiter.php', 'OC\\Security\\RemoteHostValidator' => $baseDir . '/lib/private/Security/RemoteHostValidator.php', - 'OC\\Security\\RemoteIpAddress' => $baseDir . '/lib/private/Security/RemoteIpAddress.php', 'OC\\Security\\SecureRandom' => $baseDir . '/lib/private/Security/SecureRandom.php', 'OC\\Security\\TrustedDomainHelper' => $baseDir . '/lib/private/Security/TrustedDomainHelper.php', 'OC\\Security\\VerificationToken\\CleanUpJob' => $baseDir . '/lib/private/Security/VerificationToken/CleanUpJob.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 1b886b0483e..ab10c53da69 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -677,6 +677,9 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OCP\\Security\\IRemoteHostValidator' => __DIR__ . '/../../..' . '/lib/public/Security/IRemoteHostValidator.php', 'OCP\\Security\\ISecureRandom' => __DIR__ . '/../../..' . '/lib/public/Security/ISecureRandom.php', 'OCP\\Security\\ITrustedDomainHelper' => __DIR__ . '/../../..' . '/lib/public/Security/ITrustedDomainHelper.php', + 'OCP\\Security\\Ip\\IAddress' => __DIR__ . '/../../..' . '/lib/public/Security/Ip/IAddress.php', + 'OCP\\Security\\Ip\\IRange' => __DIR__ . '/../../..' . '/lib/public/Security/Ip/IRange.php', + 'OCP\\Security\\Ip\\IRemoteAddress' => __DIR__ . '/../../..' . '/lib/public/Security/Ip/IRemoteAddress.php', 'OCP\\Security\\RateLimiting\\ILimiter' => __DIR__ . '/../../..' . '/lib/public/Security/RateLimiting/ILimiter.php', 'OCP\\Security\\RateLimiting\\IRateLimitExceededException' => __DIR__ . '/../../..' . '/lib/public/Security/RateLimiting/IRateLimitExceededException.php', 'OCP\\Security\\VerificationToken\\IVerificationToken' => __DIR__ . '/../../..' . '/lib/public/Security/VerificationToken/IVerificationToken.php', @@ -1841,6 +1844,9 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Security\\IdentityProof\\Key' => __DIR__ . '/../../..' . '/lib/private/Security/IdentityProof/Key.php', '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\\Range' => __DIR__ . '/../../..' . '/lib/private/Security/Ip/Range.php', + 'OC\\Security\\Ip\\RemoteAddress' => __DIR__ . '/../../..' . '/lib/private/Security/Ip/RemoteAddress.php', 'OC\\Security\\Normalizer\\IpAddress' => __DIR__ . '/../../..' . '/lib/private/Security/Normalizer/IpAddress.php', 'OC\\Security\\RateLimiting\\Backend\\DatabaseBackend' => __DIR__ . '/../../..' . '/lib/private/Security/RateLimiting/Backend/DatabaseBackend.php', 'OC\\Security\\RateLimiting\\Backend\\IBackend' => __DIR__ . '/../../..' . '/lib/private/Security/RateLimiting/Backend/IBackend.php', @@ -1848,7 +1854,6 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Security\\RateLimiting\\Exception\\RateLimitExceededException' => __DIR__ . '/../../..' . '/lib/private/Security/RateLimiting/Exception/RateLimitExceededException.php', 'OC\\Security\\RateLimiting\\Limiter' => __DIR__ . '/../../..' . '/lib/private/Security/RateLimiting/Limiter.php', 'OC\\Security\\RemoteHostValidator' => __DIR__ . '/../../..' . '/lib/private/Security/RemoteHostValidator.php', - 'OC\\Security\\RemoteIpAddress' => __DIR__ . '/../../..' . '/lib/private/Security/RemoteIpAddress.php', 'OC\\Security\\SecureRandom' => __DIR__ . '/../../..' . '/lib/private/Security/SecureRandom.php', 'OC\\Security\\TrustedDomainHelper' => __DIR__ . '/../../..' . '/lib/private/Security/TrustedDomainHelper.php', 'OC\\Security\\VerificationToken\\CleanUpJob' => __DIR__ . '/../../..' . '/lib/private/Security/VerificationToken/CleanUpJob.php', diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index 3c5c6ad433c..81365900d9b 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -21,7 +21,6 @@ use OC\AppFramework\Utility\SimpleContainer; use OC\Core\Middleware\TwoFactorMiddleware; use OC\Diagnostics\EventLogger; use OC\Log\PsrLoggerAdapter; -use OC\Security\RemoteIpAddress; use OC\ServerContainer; use OC\Settings\AuthorizedGroupMapper; use OCA\WorkflowEngine\Manager; @@ -47,6 +46,7 @@ use OCP\ISession; use OCP\IURLGenerator; use OCP\IUserSession; use OCP\Security\Bruteforce\IThrottler; +use OCP\Security\Ip\IRemoteAddress; use Psr\Container\ContainerInterface; use Psr\Log\LoggerInterface; @@ -234,7 +234,7 @@ class DIContainer extends SimpleContainer implements IAppContainer { $server->getL10N('lib'), $c->get(AuthorizedGroupMapper::class), $server->get(IUserSession::class), - $c->get(RemoteIpAddress::class), + $c->get(IRemoteAddress::class), ); $dispatcher->registerMiddleware($securityMiddleware); $dispatcher->registerMiddleware( diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index df20c131e03..b8de09072ce 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -17,7 +17,6 @@ use OC\AppFramework\Middleware\Security\Exceptions\NotLoggedInException; use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; use OC\AppFramework\Middleware\Security\Exceptions\StrictCookieMissingException; use OC\AppFramework\Utility\ControllerMethodReflector; -use OC\Security\RemoteIpAddress; use OC\Settings\AuthorizedGroupMapper; use OC\User\Session; use OCP\App\AppPathNotFoundException; @@ -42,6 +41,7 @@ use OCP\INavigationManager; use OCP\IRequest; use OCP\IURLGenerator; use OCP\IUserSession; +use OCP\Security\Ip\IRemoteAddress; use OCP\Util; use Psr\Log\LoggerInterface; use ReflectionMethod; @@ -67,7 +67,7 @@ class SecurityMiddleware extends Middleware { private IL10N $l10n, private AuthorizedGroupMapper $groupAuthorizationMapper, private IUserSession $userSession, - private RemoteIpAddress $remoteIpAddress, + private IRemoteAddress $remoteAddress, ) { } @@ -134,7 +134,7 @@ class SecurityMiddleware extends Middleware { if (!$authorized) { throw new NotAdminException($this->l10n->t('Logged in account must be an admin, a sub admin or gotten special right to access this setting')); } - if (!$this->remoteIpAddress->allowsAdminActions()) { + if (!$this->remoteAddress->allowsAdminActions()) { throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn’t allow you to perform admin actions')); } } @@ -151,12 +151,12 @@ class SecurityMiddleware extends Middleware { throw new NotAdminException($this->l10n->t('Logged in account must be an admin')); } if ($this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) - && !$this->remoteIpAddress->allowsAdminActions()) { + && !$this->remoteAddress->allowsAdminActions()) { throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn’t allow you to perform admin actions')); } if (!$this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) && !$this->hasAnnotationOrAttribute($reflectionMethod, 'NoAdminRequired', NoAdminRequired::class) - && !$this->remoteIpAddress->allowsAdminActions()) { + && !$this->remoteAddress->allowsAdminActions()) { throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn’t allow you to perform admin actions')); } diff --git a/lib/private/Group/Manager.php b/lib/private/Group/Manager.php index 67d6f8cbe9c..092c56fe00a 100644 --- a/lib/private/Group/Manager.php +++ b/lib/private/Group/Manager.php @@ -8,7 +8,6 @@ namespace OC\Group; use OC\Hooks\PublicEmitter; -use OC\Security\RemoteIpAddress; use OCP\EventDispatcher\IEventDispatcher; use OCP\Group\Backend\IBatchMethodsBackend; use OCP\Group\Backend\ICreateNamedGroupBackend; @@ -20,6 +19,7 @@ use OCP\ICacheFactory; use OCP\IGroup; use OCP\IGroupManager; use OCP\IUser; +use OCP\Security\Ip\IRemoteAddress; use Psr\Log\LoggerInterface; use function is_string; @@ -60,7 +60,7 @@ class Manager extends PublicEmitter implements IGroupManager { private IEventDispatcher $dispatcher, private LoggerInterface $logger, ICacheFactory $cacheFactory, - private RemoteIpAddress $remoteIpAddress, + private IRemoteAddress $remoteAddress, ) { $this->displayNameCache = new DisplayNameCache($cacheFactory, $this); @@ -321,7 +321,7 @@ class Manager extends PublicEmitter implements IGroupManager { * @return bool if admin */ public function isAdmin($userId) { - if (!$this->remoteIpAddress->allowsAdminActions()) { + if (!$this->remoteAddress->allowsAdminActions()) { return false; } diff --git a/lib/private/Security/Ip/Address.php b/lib/private/Security/Ip/Address.php new file mode 100644 index 00000000000..b4f12a7e295 --- /dev/null +++ b/lib/private/Security/Ip/Address.php @@ -0,0 +1,48 @@ +<?php + +declare(strict_types=1); +/** + * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +namespace OC\Security\Ip; + +use InvalidArgumentException; +use IPLib\Address\AddressInterface; +use IPLib\Factory; +use OCP\Security\Ip\IAddress; +use OCP\Security\Ip\IRange; + +/** + * @since 30.0.0 + */ +class Address implements IAddress { + private readonly AddressInterface $ip; + + public function __construct(string $ip) { + $ip = Factory::parseAddressString($ip); + if ($ip === null) { + throw new InvalidArgumentException('Given IP address can’t be parsed'); + } + $this->ip = $ip; + } + + public static function isValid(string $ip): bool { + return Factory::parseAddressString($ip) !== null; + } + + public function matches(IRange... $ranges): bool { + foreach($ranges as $range) { + if ($range->contains($this)) { + return true; + } + } + + return false; + } + + public function __toString(): string { + return $this->ip->toString(); + } +} diff --git a/lib/private/Security/Ip/Range.php b/lib/private/Security/Ip/Range.php new file mode 100644 index 00000000000..4f960166d6a --- /dev/null +++ b/lib/private/Security/Ip/Range.php @@ -0,0 +1,39 @@ +<?php + +declare(strict_types=1); +/** + * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +namespace OC\Security\Ip; + +use InvalidArgumentException; +use IPLib\Factory; +use IPLib\Range\RangeInterface; +use OCP\Security\Ip\IAddress; +use OCP\Security\Ip\IRange; + +class Range implements IRange { + private readonly RangeInterface $range; + + public function __construct(string $range) { + $range = Factory::parseRangeString($range); + if ($range === null) { + throw new InvalidArgumentException('Given range can’t be parsed'); + } + $this->range = $range; + } + + public static function isValid(string $range): bool { + return Factory::parseRangeString($range) !== null; + } + + public function contains(IAddress $address): bool { + return $this->range->contains(Factory::parseAddressString((string) $address)); + } + + public function __toString(): string { + return $this->range->toString(); + } +} diff --git a/lib/private/Security/Ip/RemoteAddress.php b/lib/private/Security/Ip/RemoteAddress.php new file mode 100644 index 00000000000..54cdb96132a --- /dev/null +++ b/lib/private/Security/Ip/RemoteAddress.php @@ -0,0 +1,71 @@ +<?php + +declare(strict_types=1); + +/** + * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +namespace OC\Security\Ip; + +use OCP\IConfig; +use OCP\IRequest; +use OCP\Security\Ip\IAddress; +use OCP\Security\Ip\IRange; +use OCP\Security\Ip\IRemoteAddress; + +class RemoteAddress implements IRemoteAddress, IAddress { + public const SETTING_NAME = 'allowed_admin_ranges'; + + private readonly ?IAddress $ip; + + public function __construct( + private IConfig $config, + IRequest $request, + ) { + $remoteAddress = $request->getRemoteAddress(); + $this->ip = $remoteAddress === '' + ? null + : new Address($remoteAddress); + } + + public static function isValid(string $ip): bool { + return Address::isValid($ip); + } + + public function matches(IRange... $ranges): bool { + return $this->ip === null + ? true + : $this->ip->matches(... $ranges); + } + + public function allowsAdminActions(): bool { + if ($this->ip === null) { + return true; + } + + $allowedAdminRanges = $this->config->getSystemValue(self::SETTING_NAME, false); + + // Don't apply restrictions on empty or invalid configuration + if ( + $allowedAdminRanges === false + || !is_array($allowedAdminRanges) + || empty($allowedAdminRanges) + ) { + return true; + } + + foreach ($allowedAdminRanges as $allowedAdminRange) { + if ((new Range($allowedAdminRange))->contains($this->ip)) { + return true; + } + } + + return false; + } + + public function __toString(): string { + return (string) $this->ip; + } +} diff --git a/lib/private/Security/RemoteIpAddress.php b/lib/private/Security/RemoteIpAddress.php deleted file mode 100644 index ef9debf4a65..00000000000 --- a/lib/private/Security/RemoteIpAddress.php +++ /dev/null @@ -1,64 +0,0 @@ -<?php - -declare(strict_types=1); - -/** - * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors - * SPDX-License-Identifier: AGPL-3.0-or-later - */ - -namespace OC\Security; - -use IPLib\Factory; -use OCP\IConfig; -use OCP\IRequest; -use Psr\Log\LoggerInterface; - -class RemoteIpAddress { - public const SETTING_NAME = 'allowed_admin_ranges'; - - public function __construct( - private IConfig $config, - private IRequest $request, - private LoggerInterface $logger, - ) { - } - - public function allowsAdminActions(): bool { - $allowedAdminRanges = $this->config->getSystemValue(self::SETTING_NAME, false); - if ($allowedAdminRanges === false) { - // No restriction applied - return true; - } - - if (!is_array($allowedAdminRanges)) { - return true; - } - - if (empty($allowedAdminRanges)) { - return true; - } - - $ipAddress = Factory::parseAddressString($this->request->getRemoteAddress()); - if ($ipAddress === null) { - $this->logger->warning( - 'Unable to parse remote IP "{ip}"', - ['ip' => $ipAddress,] - ); - - return false; - } - - foreach ($allowedAdminRanges as $rangeString) { - $range = Factory::parseRangeString($rangeString); - if ($range === null) { - continue; - } - if ($range->contains($ipAddress)) { - return true; - } - } - - return false; - } -} diff --git a/lib/private/Server.php b/lib/private/Server.php index 4e29b4c6ec0..511c5bf5451 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -100,8 +100,8 @@ use OC\Security\CSP\ContentSecurityPolicyNonceManager; use OC\Security\CSRF\CsrfTokenManager; use OC\Security\CSRF\TokenStorage\SessionStorage; use OC\Security\Hasher; +use OC\Security\Ip\RemoteAddress; use OC\Security\RateLimiting\Limiter; -use OC\Security\RemoteIpAddress; use OC\Security\SecureRandom; use OC\Security\TrustedDomainHelper; use OC\Security\VerificationToken\VerificationToken; @@ -214,6 +214,7 @@ use OCP\Security\IContentSecurityPolicyManager; use OCP\Security\ICredentialsManager; use OCP\Security\ICrypto; use OCP\Security\IHasher; +use OCP\Security\Ip\IRemoteAddress; use OCP\Security\ISecureRandom; use OCP\Security\ITrustedDomainHelper; use OCP\Security\RateLimiting\ILimiter; @@ -466,7 +467,7 @@ class Server extends ServerContainer implements IServerContainer { $this->get(IEventDispatcher::class), $this->get(LoggerInterface::class), $this->get(ICacheFactory::class), - $this->get(RemoteIpAddress::class), + $this->get(IRemoteAddress::class), ); return $groupManager; }); @@ -1405,6 +1406,8 @@ class Server extends ServerContainer implements IServerContainer { $this->registerAlias(\OCP\TaskProcessing\IManager::class, \OC\TaskProcessing\Manager::class); + $this->registerAlias(IRemoteAddress::class, RemoteAddress::class); + $this->connectDispatcher(); } diff --git a/lib/public/Security/Ip/IAddress.php b/lib/public/Security/Ip/IAddress.php new file mode 100644 index 00000000000..242418962fc --- /dev/null +++ b/lib/public/Security/Ip/IAddress.php @@ -0,0 +1,35 @@ +<?php + +declare(strict_types=1); +/** + * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +namespace OCP\Security\Ip; + +/** + * @since 30.0.0 + */ +interface IAddress { + /** + * Check if a given IP address is valid + * + * @since 30.0.0 + */ + public static function isValid(string $ip): bool; + + /** + * Check if current address is contained by given ranges + * + * @since 30.0.0 + */ + public function matches(IRange... $ranges): bool; + + /** + * Normalized IP address + * + * @since 30.0.0 + */ + public function __toString(): string; +} diff --git a/lib/public/Security/Ip/IRange.php b/lib/public/Security/Ip/IRange.php new file mode 100644 index 00000000000..70e1815c75e --- /dev/null +++ b/lib/public/Security/Ip/IRange.php @@ -0,0 +1,37 @@ +<?php + +declare(strict_types=1); +/** + * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +namespace OCP\Security\Ip; + +/** + * IP Range (IPv4 or IPv6) + * + * @since 30.0.0 + */ +interface IRange { + /** + * Check if a given range is valid + * + * @since 30.0.0 + */ + public static function isValid(string $range): bool; + + /** + * Check if an address is in the current range + * + * @since 30.0.0 + */ + public function contains(IAddress $address): bool; + + /** + * Normalized IP range + * + * @since 30.0.0 + */ + public function __toString(): string; +} diff --git a/lib/public/Security/Ip/IRemoteAddress.php b/lib/public/Security/Ip/IRemoteAddress.php new file mode 100644 index 00000000000..19a1dab9734 --- /dev/null +++ b/lib/public/Security/Ip/IRemoteAddress.php @@ -0,0 +1,22 @@ +<?php + +declare(strict_types=1); +/** + * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +namespace OCP\Security\Ip; + +/** + * IP address of the connected client + * + * @since 30.0.0 + */ +interface IRemoteAddress { + /** + * Check if the current remote address is allowed to perform admin actions + * @since 30.0.0 + */ + public function allowsAdminActions(): bool; +} diff --git a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php index 1aa7ad456a8..fb457579fac 100644 --- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php @@ -18,7 +18,6 @@ use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; use OC\Appframework\Middleware\Security\Exceptions\StrictCookieMissingException; use OC\AppFramework\Middleware\Security\SecurityMiddleware; use OC\AppFramework\Utility\ControllerMethodReflector; -use OC\Security\RemoteIpAddress; use OC\Settings\AuthorizedGroupMapper; use OC\User\Session; use OCP\App\IAppManager; @@ -33,6 +32,7 @@ use OCP\IRequestId; use OCP\ISession; use OCP\IURLGenerator; use OCP\IUserSession; +use OCP\Security\Ip\IRemoteAddress; use Psr\Log\LoggerInterface; use Test\AppFramework\Middleware\Security\Mock\NormalController; use Test\AppFramework\Middleware\Security\Mock\OCSController; @@ -91,7 +91,7 @@ class SecurityMiddlewareTest extends \Test\TestCase { $this->appManager->expects($this->any()) ->method('isEnabledForUser') ->willReturn($isAppEnabledForUser); - $remoteIpAddress = $this->createMock(RemoteIpAddress::class); + $remoteIpAddress = $this->createMock(IRemoteAddress::class); $remoteIpAddress->method('allowsAdminActions')->willReturn(true); return new SecurityMiddleware( diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index e738f424065..4f42e10537d 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -9,7 +9,6 @@ namespace Test\Group; use OC\Group\Database; -use OC\Security\RemoteIpAddress; use OC\User\Manager; use OC\User\User; use OCP\EventDispatcher\IEventDispatcher; @@ -17,6 +16,7 @@ use OCP\Group\Backend\ISearchableGroupBackend; use OCP\GroupInterface; use OCP\ICacheFactory; use OCP\IUser; +use OCP\Security\Ip\IRemoteAddress; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; use Test\TestCase; @@ -33,7 +33,7 @@ class ManagerTest extends TestCase { protected $logger; /** @var ICacheFactory|MockObject */ private $cache; - /** @var RemoteIpAddress|MockObject */ + /** @var IRemoteAddress|MockObject */ private $remoteIpAddress; protected function setUp(): void { @@ -44,7 +44,7 @@ class ManagerTest extends TestCase { $this->logger = $this->createMock(LoggerInterface::class); $this->cache = $this->createMock(ICacheFactory::class); - $this->remoteIpAddress = $this->createMock(RemoteIpAddress::class); + $this->remoteIpAddress = $this->createMock(IRemoteAddress::class); $this->remoteIpAddress->method('allowsAdminActions')->willReturn(true); } diff --git a/tests/lib/Security/RemoteIpAddressTest.php b/tests/lib/Security/Ip/RemoteAddressTest.php index 9cf9458156a..22f38f62356 100644 --- a/tests/lib/Security/RemoteIpAddressTest.php +++ b/tests/lib/Security/Ip/RemoteAddressTest.php @@ -7,33 +7,27 @@ declare(strict_types=1); * SPDX-License-Identifier: AGPL-3.0-or-later */ -namespace Test\Security; +namespace Test\Security\Ip; -use OC\Security\RemoteIpAddress; +use OC\Security\Ip\RemoteAddress; use OCP\IConfig; use OCP\IRequest; -use Psr\Log\LoggerInterface; -class RemoteIpAddressTest extends \Test\TestCase { +class RemoteAddressTest extends \Test\TestCase { private IConfig $config; private IRequest $request; - private LoggerInterface $logger; - - private RemoteIpAddress $remoteIpAddress; protected function setUp(): void { parent::setUp(); $this->config = $this->createMock(IConfig::class); $this->request = $this->createMock(IRequest::class); - $this->logger = $this->createMock(LoggerInterface::class); - $this->remoteIpAddress = new RemoteIpAddress($this->config, $this->request, $this->logger); } /** * @param mixed $allowedRanges * @dataProvider dataProvider */ - public function testEmptyConfig(string $remoteIp, $allowedRanges, bool $expected): void { + public function testAllowedIps(string $remoteIp, $allowedRanges, bool $expected): void { $this->request ->method('getRemoteAddress') ->willReturn($remoteIp); @@ -42,7 +36,9 @@ class RemoteIpAddressTest extends \Test\TestCase { ->with('allowed_admin_ranges', false) ->willReturn($allowedRanges); - $this->assertEquals($expected, $this->remoteIpAddress->allowsAdminActions()); + $remoteAddress = new RemoteAddress($this->config, $this->request); + + $this->assertEquals($expected, $remoteAddress->allowsAdminActions()); } /** @@ -50,6 +46,9 @@ class RemoteIpAddressTest extends \Test\TestCase { */ public function dataProvider(): array { return [ + // No IP (ie. CLI) + ['', ['192.168.1.2/24'], true], + ['', ['fe80/8'], true], // No configuration ['1.2.3.4', false, true], ['1234:4567:8910::', false, true], @@ -59,6 +58,10 @@ class RemoteIpAddressTest extends \Test\TestCase { // Invalid configuration ['1.2.3.4', 'hello', true], ['1234:4567:8910::', 'world', true], + // Mixed configuration + ['192.168.1.5', ['1.2.3.*', '1234::/8'], false], + ['::1', ['127.0.0.1', '1234::/8'], false], + ['192.168.1.5', ['192.168.1.0/24', '1234::/8'], true], // Allowed IP ['1.2.3.4', ['1.2.3.*'], true], ['fc00:1:2:3::1', ['fc00::/7'], true], @@ -66,9 +69,9 @@ class RemoteIpAddressTest extends \Test\TestCase { ['1234:4567:8910::1', ['fe80::/8','1234:4567::/16'], true], // Blocked IP ['192.168.1.5', ['1.2.3.*'], false], - ['9234:4567:8910::', ['1234:4567:*'], false], + ['9234:4567:8910::', ['1234:4567::1'], false], ['192.168.2.1', ['192.168.1.2/24', '1.2.3.0/24'], false], - ['9234:4567:8910::', ['fe80/8','1234:4567/16'], false], + ['9234:4567:8910::', ['fe80::/8','1234:4567::/16'], false], ]; } } |