diff options
17 files changed, 336 insertions, 101 deletions
diff --git a/apps/settings/composer/composer/autoload_classmap.php b/apps/settings/composer/composer/autoload_classmap.php index 488aaee264d..27c1496008e 100644 --- a/apps/settings/composer/composer/autoload_classmap.php +++ b/apps/settings/composer/composer/autoload_classmap.php @@ -78,6 +78,7 @@ return array( 'OCA\\Settings\\Settings\\Personal\\Security\\TwoFactor' => $baseDir . '/../lib/Settings/Personal/Security/TwoFactor.php', 'OCA\\Settings\\Settings\\Personal\\Security\\WebAuthn' => $baseDir . '/../lib/Settings/Personal/Security/WebAuthn.php', 'OCA\\Settings\\Settings\\Personal\\ServerDevNotice' => $baseDir . '/../lib/Settings/Personal/ServerDevNotice.php', + 'OCA\\Settings\\SetupChecks\\AllowedAdminRanges' => $baseDir . '/../lib/SetupChecks/AllowedAdminRanges.php', 'OCA\\Settings\\SetupChecks\\AppDirsWithDifferentOwner' => $baseDir . '/../lib/SetupChecks/AppDirsWithDifferentOwner.php', 'OCA\\Settings\\SetupChecks\\BruteForceThrottler' => $baseDir . '/../lib/SetupChecks/BruteForceThrottler.php', 'OCA\\Settings\\SetupChecks\\CheckServerResponseTrait' => $baseDir . '/../lib/SetupChecks/CheckServerResponseTrait.php', diff --git a/apps/settings/composer/composer/autoload_static.php b/apps/settings/composer/composer/autoload_static.php index ac2e4645239..14e4c362887 100644 --- a/apps/settings/composer/composer/autoload_static.php +++ b/apps/settings/composer/composer/autoload_static.php @@ -93,6 +93,7 @@ class ComposerStaticInitSettings 'OCA\\Settings\\Settings\\Personal\\Security\\TwoFactor' => __DIR__ . '/..' . '/../lib/Settings/Personal/Security/TwoFactor.php', 'OCA\\Settings\\Settings\\Personal\\Security\\WebAuthn' => __DIR__ . '/..' . '/../lib/Settings/Personal/Security/WebAuthn.php', 'OCA\\Settings\\Settings\\Personal\\ServerDevNotice' => __DIR__ . '/..' . '/../lib/Settings/Personal/ServerDevNotice.php', + 'OCA\\Settings\\SetupChecks\\AllowedAdminRanges' => __DIR__ . '/..' . '/../lib/SetupChecks/AllowedAdminRanges.php', 'OCA\\Settings\\SetupChecks\\AppDirsWithDifferentOwner' => __DIR__ . '/..' . '/../lib/SetupChecks/AppDirsWithDifferentOwner.php', 'OCA\\Settings\\SetupChecks\\BruteForceThrottler' => __DIR__ . '/..' . '/../lib/SetupChecks/BruteForceThrottler.php', 'OCA\\Settings\\SetupChecks\\CheckServerResponseTrait' => __DIR__ . '/..' . '/../lib/SetupChecks/CheckServerResponseTrait.php', diff --git a/apps/settings/composer/composer/installed.php b/apps/settings/composer/composer/installed.php index d2b87e1bdfd..651d70adcf8 100644 --- a/apps/settings/composer/composer/installed.php +++ b/apps/settings/composer/composer/installed.php @@ -3,7 +3,7 @@ 'name' => '__root__', 'pretty_version' => 'dev-master', 'version' => 'dev-master', - 'reference' => '4ff660ca2e0baa02440ba07296ed7e75fa544c0e', + 'reference' => '071fe73d0a28f44c6e24cc87fbd00e54a3b92b57', 'type' => 'library', 'install_path' => __DIR__ . '/../', 'aliases' => array(), @@ -13,7 +13,7 @@ '__root__' => array( 'pretty_version' => 'dev-master', 'version' => 'dev-master', - 'reference' => '4ff660ca2e0baa02440ba07296ed7e75fa544c0e', + 'reference' => '071fe73d0a28f44c6e24cc87fbd00e54a3b92b57', 'type' => 'library', 'install_path' => __DIR__ . '/../', 'aliases' => array(), diff --git a/apps/settings/lib/AppInfo/Application.php b/apps/settings/lib/AppInfo/Application.php index f62555f0cdb..80420cb3335 100644 --- a/apps/settings/lib/AppInfo/Application.php +++ b/apps/settings/lib/AppInfo/Application.php @@ -22,6 +22,7 @@ use OCA\Settings\Middleware\SubadminMiddleware; use OCA\Settings\Search\AppSearch; use OCA\Settings\Search\SectionSearch; use OCA\Settings\Search\UserSearch; +use OCA\Settings\SetupChecks\AllowedAdminRanges; use OCA\Settings\SetupChecks\AppDirsWithDifferentOwner; use OCA\Settings\SetupChecks\BruteForceThrottler; use OCA\Settings\SetupChecks\CheckUserCertificates; @@ -154,6 +155,7 @@ class Application extends App implements IBootstrap { Util::getDefaultEmailAddress('no-reply') ); }); + $context->registerSetupCheck(AllowedAdminRanges::class); $context->registerSetupCheck(AppDirsWithDifferentOwner::class); $context->registerSetupCheck(BruteForceThrottler::class); $context->registerSetupCheck(CheckUserCertificates::class); diff --git a/apps/settings/lib/SetupChecks/AllowedAdminRanges.php b/apps/settings/lib/SetupChecks/AllowedAdminRanges.php new file mode 100644 index 00000000000..7c50c261dea --- /dev/null +++ b/apps/settings/lib/SetupChecks/AllowedAdminRanges.php @@ -0,0 +1,66 @@ +<?php + +declare(strict_types=1); + +/** + * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ +namespace OCA\Settings\SetupChecks; + +use IPLib\Factory; +use OC\Security\RemoteIpAddress; +use OCP\IConfig; +use OCP\IL10N; +use OCP\SetupCheck\ISetupCheck; +use OCP\SetupCheck\SetupResult; + +class AllowedAdminRanges implements ISetupCheck { + public function __construct( + private IConfig $config, + private IL10N $l10n, + ) { + } + + public function getCategory(): string { + return 'system'; + } + + public function getName(): string { + return $this->l10n->t('Allowed admin IP ranges'); + } + + public function run(): SetupResult { + $allowedAdminRanges = $this->config->getSystemValue(RemoteIpAddress::SETTING_NAME, false); + if ($allowedAdminRanges === false) { + return SetupResult::success($this->l10n->t('Admin IP filtering isn’t applied.')); + } + + if (!is_array($allowedAdminRanges)) { + 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)], + ) + ); + } + + $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) { + return SetupResult::warning( + $this->l10n->t( + 'Configuration key "%1$s" contains invalid IP range(s): "%2$s"', + [RemoteIpAddress::SETTING_NAME, implode('", "', $invalidRanges)], + ), + ); + } + + return SetupResult::success($this->l10n->t('Admin IP filtering is correctly configured.')); + } +} diff --git a/config/config.sample.php b/config/config.sample.php index 67110a1844a..9840fcfc97c 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -2208,6 +2208,16 @@ $CONFIG = [ 'forwarded_for_headers' => ['HTTP_X_FORWARDED', 'HTTP_FORWARDED_FOR'], /** + * List of trusted IP ranges for admin actions + * + * If this list is non-empty, all admin actions must be triggered from + * IP addresses inside theses ranges. + * + * Defaults to an empty array. + */ +'allowed_admin_ranges' => ['192.0.2.42/32', '233.252.0.0/24', '2001:db8::13:37/64'], + +/** * max file size for animating gifs on public-sharing-site. * If the gif is bigger, it'll show a static preview * diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index fbc61bcb1a6..9519dccd109 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -896,6 +896,7 @@ return array( 'OC\\AppFramework\\Middleware\\Security\\BruteForceMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\CORSMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\CSPMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/CSPMiddleware.php', + 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\AdminIpNotAllowedException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/AdminIpNotAllowedException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\AppNotEnabledException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/AppNotEnabledException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\CrossSiteRequestForgeryException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/CrossSiteRequestForgeryException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\ExAppRequiredException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/ExAppRequiredException.php', @@ -1814,6 +1815,7 @@ 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 dd25d62f7e3..1b886b0483e 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -929,6 +929,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\AppFramework\\Middleware\\Security\\BruteForceMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\CORSMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\CSPMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/CSPMiddleware.php', + 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\AdminIpNotAllowedException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/AdminIpNotAllowedException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\AppNotEnabledException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/AppNotEnabledException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\CrossSiteRequestForgeryException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/CrossSiteRequestForgeryException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\ExAppRequiredException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/ExAppRequiredException.php', @@ -1847,6 +1848,7 @@ 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 c25b6994b4f..3c5c6ad433c 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -21,6 +21,7 @@ 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; @@ -232,7 +233,8 @@ class DIContainer extends SimpleContainer implements IAppContainer { $server->getAppManager(), $server->getL10N('lib'), $c->get(AuthorizedGroupMapper::class), - $server->get(IUserSession::class) + $server->get(IUserSession::class), + $c->get(RemoteIpAddress::class), ); $dispatcher->registerMiddleware($securityMiddleware); $dispatcher->registerMiddleware( diff --git a/lib/private/AppFramework/Middleware/Security/Exceptions/AdminIpNotAllowedException.php b/lib/private/AppFramework/Middleware/Security/Exceptions/AdminIpNotAllowedException.php new file mode 100644 index 00000000000..36eb8f18928 --- /dev/null +++ b/lib/private/AppFramework/Middleware/Security/Exceptions/AdminIpNotAllowedException.php @@ -0,0 +1,23 @@ +<?php + +declare(strict_types=1); +/** + * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-only + */ +namespace OC\AppFramework\Middleware\Security\Exceptions; + +use OCP\AppFramework\Http; + +/** + * Class AdminIpNotAllowed is thrown when a resource has been requested by a + * an admin user connecting from an unauthorized IP address + * See configuration `allowed_admin_ranges` + * + * @package OC\AppFramework\Middleware\Security\Exceptions + */ +class AdminIpNotAllowedException extends SecurityException { + public function __construct(string $message) { + parent::__construct($message, Http::STATUS_FORBIDDEN); + } +} diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index d7479f674d9..df20c131e03 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -8,6 +8,7 @@ declare(strict_types=1); */ namespace OC\AppFramework\Middleware\Security; +use OC\AppFramework\Middleware\Security\Exceptions\AdminIpNotAllowedException; use OC\AppFramework\Middleware\Security\Exceptions\AppNotEnabledException; use OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException; use OC\AppFramework\Middleware\Security\Exceptions\ExAppRequiredException; @@ -16,6 +17,7 @@ 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; @@ -51,60 +53,22 @@ use ReflectionMethod; * check fails */ class SecurityMiddleware extends Middleware { - /** @var INavigationManager */ - private $navigationManager; - /** @var IRequest */ - private $request; - /** @var ControllerMethodReflector */ - private $reflector; - /** @var string */ - private $appName; - /** @var IURLGenerator */ - private $urlGenerator; - /** @var LoggerInterface */ - private $logger; - /** @var bool */ - private $isLoggedIn; - /** @var bool */ - private $isAdminUser; - /** @var bool */ - private $isSubAdmin; - /** @var IAppManager */ - private $appManager; - /** @var IL10N */ - private $l10n; - /** @var AuthorizedGroupMapper */ - private $groupAuthorizationMapper; - /** @var IUserSession */ - private $userSession; - - public function __construct(IRequest $request, - ControllerMethodReflector $reflector, - INavigationManager $navigationManager, - IURLGenerator $urlGenerator, - LoggerInterface $logger, - string $appName, - bool $isLoggedIn, - bool $isAdminUser, - bool $isSubAdmin, - IAppManager $appManager, - IL10N $l10n, - AuthorizedGroupMapper $mapper, - IUserSession $userSession + public function __construct( + private IRequest $request, + private ControllerMethodReflector $reflector, + private INavigationManager $navigationManager, + private IURLGenerator $urlGenerator, + private LoggerInterface $logger, + private string $appName, + private bool $isLoggedIn, + private bool $isAdminUser, + private bool $isSubAdmin, + private IAppManager $appManager, + private IL10N $l10n, + private AuthorizedGroupMapper $groupAuthorizationMapper, + private IUserSession $userSession, + private RemoteIpAddress $remoteIpAddress, ) { - $this->navigationManager = $navigationManager; - $this->request = $request; - $this->reflector = $reflector; - $this->appName = $appName; - $this->urlGenerator = $urlGenerator; - $this->logger = $logger; - $this->isLoggedIn = $isLoggedIn; - $this->isAdminUser = $isAdminUser; - $this->isSubAdmin = $isSubAdmin; - $this->appManager = $appManager; - $this->l10n = $l10n; - $this->groupAuthorizationMapper = $mapper; - $this->userSession = $userSession; } /** @@ -170,6 +134,9 @@ 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()) { + 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->isSubAdmin @@ -183,6 +150,16 @@ class SecurityMiddleware extends Middleware { && !$authorized) { throw new NotAdminException($this->l10n->t('Logged in account must be an admin')); } + if ($this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) + && !$this->remoteIpAddress->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()) { + throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn’t allow you to perform admin actions')); + } + } // Check for strict cookie requirement diff --git a/lib/private/Group/Manager.php b/lib/private/Group/Manager.php index 0ab64907c8b..67d6f8cbe9c 100644 --- a/lib/private/Group/Manager.php +++ b/lib/private/Group/Manager.php @@ -8,6 +8,7 @@ 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; @@ -41,11 +42,6 @@ class Manager extends PublicEmitter implements IGroupManager { /** @var GroupInterface[] */ private $backends = []; - /** @var \OC\User\Manager */ - private $userManager; - private IEventDispatcher $dispatcher; - private LoggerInterface $logger; - /** @var array<string, IGroup> */ private $cachedGroups = []; @@ -59,13 +55,13 @@ class Manager extends PublicEmitter implements IGroupManager { private const MAX_GROUP_LENGTH = 255; - public function __construct(\OC\User\Manager $userManager, - IEventDispatcher $dispatcher, - LoggerInterface $logger, - ICacheFactory $cacheFactory) { - $this->userManager = $userManager; - $this->dispatcher = $dispatcher; - $this->logger = $logger; + public function __construct( + private \OC\User\Manager $userManager, + private IEventDispatcher $dispatcher, + private LoggerInterface $logger, + ICacheFactory $cacheFactory, + private RemoteIpAddress $remoteIpAddress, + ) { $this->displayNameCache = new DisplayNameCache($cacheFactory, $this); $this->listen('\OC\Group', 'postDelete', function (IGroup $group): void { @@ -325,6 +321,10 @@ class Manager extends PublicEmitter implements IGroupManager { * @return bool if admin */ public function isAdmin($userId) { + if (!$this->remoteIpAddress->allowsAdminActions()) { + return false; + } + foreach ($this->backends as $backend) { if (is_string($userId) && $backend->implementsActions(Backend::IS_ADMIN) && $backend->isAdmin($userId)) { return true; diff --git a/lib/private/Security/RemoteIpAddress.php b/lib/private/Security/RemoteIpAddress.php new file mode 100644 index 00000000000..ef9debf4a65 --- /dev/null +++ b/lib/private/Security/RemoteIpAddress.php @@ -0,0 +1,64 @@ +<?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 4d549918a8f..4e29b4c6ec0 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -101,6 +101,7 @@ use OC\Security\CSRF\CsrfTokenManager; use OC\Security\CSRF\TokenStorage\SessionStorage; use OC\Security\Hasher; use OC\Security\RateLimiting\Limiter; +use OC\Security\RemoteIpAddress; use OC\Security\SecureRandom; use OC\Security\TrustedDomainHelper; use OC\Security\VerificationToken\VerificationToken; @@ -464,7 +465,8 @@ class Server extends ServerContainer implements IServerContainer { $this->get(IUserManager::class), $this->get(IEventDispatcher::class), $this->get(LoggerInterface::class), - $this->get(ICacheFactory::class) + $this->get(ICacheFactory::class), + $this->get(RemoteIpAddress::class), ); return $groupManager; }); diff --git a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php index bda71c4e8ed..1aa7ad456a8 100644 --- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php @@ -18,6 +18,7 @@ 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; @@ -90,6 +91,8 @@ class SecurityMiddlewareTest extends \Test\TestCase { $this->appManager->expects($this->any()) ->method('isEnabledForUser') ->willReturn($isAppEnabledForUser); + $remoteIpAddress = $this->createMock(RemoteIpAddress::class); + $remoteIpAddress->method('allowsAdminActions')->willReturn(true); return new SecurityMiddleware( $this->request, @@ -104,7 +107,8 @@ class SecurityMiddlewareTest extends \Test\TestCase { $this->appManager, $this->l10n, $this->authorizedGroupMapper, - $this->userSession + $this->userSession, + $remoteIpAddress ); } diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index 81fd77cc78a..e738f424065 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -9,6 +9,7 @@ namespace Test\Group; use OC\Group\Database; +use OC\Security\RemoteIpAddress; use OC\User\Manager; use OC\User\User; use OCP\EventDispatcher\IEventDispatcher; @@ -32,6 +33,8 @@ class ManagerTest extends TestCase { protected $logger; /** @var ICacheFactory|MockObject */ private $cache; + /** @var RemoteIpAddress|MockObject */ + private $remoteIpAddress; protected function setUp(): void { parent::setUp(); @@ -40,6 +43,9 @@ class ManagerTest extends TestCase { $this->dispatcher = $this->createMock(IEventDispatcher::class); $this->logger = $this->createMock(LoggerInterface::class); $this->cache = $this->createMock(ICacheFactory::class); + + $this->remoteIpAddress = $this->createMock(RemoteIpAddress::class); + $this->remoteIpAddress->method('allowsAdminActions')->willReturn(true); } private function getTestUser($userId) { @@ -103,7 +109,7 @@ class ManagerTest extends TestCase { ->with('group1') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $group = $manager->get('group1'); @@ -112,7 +118,7 @@ class ManagerTest extends TestCase { } public function testGetNoBackend() { - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $this->assertNull($manager->get('group1')); } @@ -127,7 +133,7 @@ class ManagerTest extends TestCase { ->with('group1') ->willReturn(false); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $this->assertNull($manager->get('group1')); @@ -137,7 +143,7 @@ class ManagerTest extends TestCase { $backend = new \Test\Util\Group\Dummy(); $backend->createGroup('group1'); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $group = $manager->get('group1'); @@ -164,7 +170,7 @@ class ManagerTest extends TestCase { ->with('group1') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -190,7 +196,7 @@ class ManagerTest extends TestCase { return true; }); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $group = $manager->createGroup('group1'); @@ -219,7 +225,7 @@ class ManagerTest extends TestCase { ->method('getGroupDetails') ->willReturn([]); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $group = $manager->createGroup('group1'); @@ -243,7 +249,7 @@ class ManagerTest extends TestCase { ->with($groupName) ->willReturn(false); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $this->expectException(\Exception::class); @@ -260,7 +266,7 @@ class ManagerTest extends TestCase { $backend->expects($this->never()) ->method('createGroup'); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $group = $manager->createGroup('group1'); @@ -281,7 +287,7 @@ class ManagerTest extends TestCase { ->with('group1') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $groups = $manager->search('1'); @@ -315,7 +321,7 @@ class ManagerTest extends TestCase { ->method('groupExists') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -352,7 +358,7 @@ class ManagerTest extends TestCase { ->method('groupExists') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -381,7 +387,7 @@ class ManagerTest extends TestCase { /** @var \OC\User\Manager $userManager */ $userManager = $this->createMock(Manager::class); - $manager = new \OC\Group\Manager($userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $groups = $manager->search('1'); @@ -402,7 +408,7 @@ class ManagerTest extends TestCase { ->with('group1') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $groups = $manager->getUserGroups($this->getTestUser('user1')); @@ -420,7 +426,7 @@ class ManagerTest extends TestCase { ->with('myUID') ->willReturn(['123', 'abc']); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); /** @var \OC\User\User|\PHPUnit\Framework\MockObject\MockObject $user */ @@ -450,7 +456,7 @@ class ManagerTest extends TestCase { ->with('group1') ->willReturn(false); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); /** @var \OC\User\User|\PHPUnit\Framework\MockObject\MockObject $user */ @@ -476,7 +482,7 @@ class ManagerTest extends TestCase { ->method('groupExists') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $this->assertTrue($manager->isInGroup('user1', 'group1')); @@ -495,7 +501,7 @@ class ManagerTest extends TestCase { ->method('groupExists') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $this->assertTrue($manager->isAdmin('user1')); @@ -514,7 +520,7 @@ class ManagerTest extends TestCase { ->method('groupExists') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $this->assertFalse($manager->isAdmin('user1')); @@ -545,7 +551,7 @@ class ManagerTest extends TestCase { ->method('groupExists') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -604,7 +610,7 @@ class ManagerTest extends TestCase { } }); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', 'user3'); @@ -664,7 +670,7 @@ class ManagerTest extends TestCase { } }); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', 'user3', 1); @@ -728,7 +734,7 @@ class ManagerTest extends TestCase { } }); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', 'user3', 1, 1); @@ -757,7 +763,7 @@ class ManagerTest extends TestCase { $this->userManager->expects($this->never())->method('get'); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', ''); @@ -785,7 +791,7 @@ class ManagerTest extends TestCase { $this->userManager->expects($this->never())->method('get'); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', '', 1); @@ -813,7 +819,7 @@ class ManagerTest extends TestCase { $this->userManager->expects($this->never())->method('get'); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', '', 1, 1); @@ -841,7 +847,7 @@ class ManagerTest extends TestCase { ->with('group1') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); // prime cache @@ -884,7 +890,7 @@ class ManagerTest extends TestCase { ->method('removeFromGroup') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); // prime cache @@ -914,7 +920,7 @@ class ManagerTest extends TestCase { ->with('user1') ->willReturn(null); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $groups = $manager->getUserIdGroups('user1'); @@ -939,8 +945,7 @@ class ManagerTest extends TestCase { ['group1', ['gid' => 'group1', 'displayName' => 'Group One']], ['group2', ['gid' => 'group2']], ]); - - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); // group with display name diff --git a/tests/lib/Security/RemoteIpAddressTest.php b/tests/lib/Security/RemoteIpAddressTest.php new file mode 100644 index 00000000000..9cf9458156a --- /dev/null +++ b/tests/lib/Security/RemoteIpAddressTest.php @@ -0,0 +1,74 @@ +<?php + +declare(strict_types=1); + +/** + * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +namespace Test\Security; + +use OC\Security\RemoteIpAddress; +use OCP\IConfig; +use OCP\IRequest; +use Psr\Log\LoggerInterface; + +class RemoteIpAddressTest 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 { + $this->request + ->method('getRemoteAddress') + ->willReturn($remoteIp); + $this->config + ->method('getSystemValue') + ->with('allowed_admin_ranges', false) + ->willReturn($allowedRanges); + + $this->assertEquals($expected, $this->remoteIpAddress->allowsAdminActions()); + } + + /** + * @return array<string, mixed, bool> + */ + public function dataProvider(): array { + return [ + // No configuration + ['1.2.3.4', false, true], + ['1234:4567:8910::', false, true], + // Empty configuration + ['1.2.3.4', [], true], + ['1234:4567:8910::', [], true], + // Invalid configuration + ['1.2.3.4', 'hello', true], + ['1234:4567:8910::', 'world', true], + // Allowed IP + ['1.2.3.4', ['1.2.3.*'], true], + ['fc00:1:2:3::1', ['fc00::/7'], true], + ['1.2.3.4', ['192.168.1.2/24', '1.2.3.0/24'], true], + ['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], + ['192.168.2.1', ['192.168.1.2/24', '1.2.3.0/24'], false], + ['9234:4567:8910::', ['fe80/8','1234:4567/16'], false], + ]; + } +} |