Skip to content

Commit 90da5d8

Browse files
AltahrimAltahrim
committed
feat(security): restrict admin actions to IP ranges
Signed-off-by: Benjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com>
1 parent 86f5fb0 commit 90da5d8

File tree

10 files changed

+240
-67
lines changed

10 files changed

+240
-67
lines changed

‎config/config.sample.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2195,6 +2195,16 @@
21952195
*/
21962196
'forwarded_for_headers' => ['HTTP_X_FORWARDED', 'HTTP_FORWARDED_FOR'],
21972197

2198+
/**
2199+
* List of trusted IP ranges for admin actions
2200+
*
2201+
* If this list is non-empty, all admin actions must be triggered from
2202+
* IP addresses inside theses ranges.
2203+
*
2204+
* Defaults to an empty array.
2205+
*/
2206+
'allowed_admin_ranges' => ['192.0.2.42/32', '233.252.0.0/24', '2001:db8::13:37/64'],
2207+
21982208
/**
21992209
* max file size for animating gifs on public-sharing-site.
22002210
* If the gif is bigger, it'll show a static preview

‎lib/composer/composer/autoload_classmap.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -893,6 +893,7 @@
893893
'OC\\AppFramework\\Middleware\\Security\\BruteForceMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php',
894894
'OC\\AppFramework\\Middleware\\Security\\CORSMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php',
895895
'OC\\AppFramework\\Middleware\\Security\\CSPMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/CSPMiddleware.php',
896+
'OC\\AppFramework\\Middleware\\Security\\Exceptions\\AdminIpNotAllowedException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/AdminIpNotAllowedException.php',
896897
'OC\\AppFramework\\Middleware\\Security\\Exceptions\\AppNotEnabledException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/AppNotEnabledException.php',
897898
'OC\\AppFramework\\Middleware\\Security\\Exceptions\\CrossSiteRequestForgeryException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/CrossSiteRequestForgeryException.php',
898899
'OC\\AppFramework\\Middleware\\Security\\Exceptions\\ExAppRequiredException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/ExAppRequiredException.php',
@@ -1808,6 +1809,7 @@
18081809
'OC\\Security\\RateLimiting\\Exception\\RateLimitExceededException' => $baseDir . '/lib/private/Security/RateLimiting/Exception/RateLimitExceededException.php',
18091810
'OC\\Security\\RateLimiting\\Limiter' => $baseDir . '/lib/private/Security/RateLimiting/Limiter.php',
18101811
'OC\\Security\\RemoteHostValidator' => $baseDir . '/lib/private/Security/RemoteHostValidator.php',
1812+
'OC\\Security\\RemoteIpAddress' => $baseDir . '/lib/private/Security/RemoteIpAddress.php',
18111813
'OC\\Security\\SecureRandom' => $baseDir . '/lib/private/Security/SecureRandom.php',
18121814
'OC\\Security\\TrustedDomainHelper' => $baseDir . '/lib/private/Security/TrustedDomainHelper.php',
18131815
'OC\\Security\\VerificationToken\\CleanUpJob' => $baseDir . '/lib/private/Security/VerificationToken/CleanUpJob.php',

‎lib/composer/composer/autoload_static.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -926,6 +926,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
926926
'OC\\AppFramework\\Middleware\\Security\\BruteForceMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php',
927927
'OC\\AppFramework\\Middleware\\Security\\CORSMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php',
928928
'OC\\AppFramework\\Middleware\\Security\\CSPMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/CSPMiddleware.php',
929+
'OC\\AppFramework\\Middleware\\Security\\Exceptions\\AdminIpNotAllowedException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/AdminIpNotAllowedException.php',
929930
'OC\\AppFramework\\Middleware\\Security\\Exceptions\\AppNotEnabledException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/AppNotEnabledException.php',
930931
'OC\\AppFramework\\Middleware\\Security\\Exceptions\\CrossSiteRequestForgeryException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/CrossSiteRequestForgeryException.php',
931932
'OC\\AppFramework\\Middleware\\Security\\Exceptions\\ExAppRequiredException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/ExAppRequiredException.php',
@@ -1841,6 +1842,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
18411842
'OC\\Security\\RateLimiting\\Exception\\RateLimitExceededException' => __DIR__ . '/../../..' . '/lib/private/Security/RateLimiting/Exception/RateLimitExceededException.php',
18421843
'OC\\Security\\RateLimiting\\Limiter' => __DIR__ . '/../../..' . '/lib/private/Security/RateLimiting/Limiter.php',
18431844
'OC\\Security\\RemoteHostValidator' => __DIR__ . '/../../..' . '/lib/private/Security/RemoteHostValidator.php',
1845+
'OC\\Security\\RemoteIpAddress' => __DIR__ . '/../../..' . '/lib/private/Security/RemoteIpAddress.php',
18441846
'OC\\Security\\SecureRandom' => __DIR__ . '/../../..' . '/lib/private/Security/SecureRandom.php',
18451847
'OC\\Security\\TrustedDomainHelper' => __DIR__ . '/../../..' . '/lib/private/Security/TrustedDomainHelper.php',
18461848
'OC\\Security\\VerificationToken\\CleanUpJob' => __DIR__ . '/../../..' . '/lib/private/Security/VerificationToken/CleanUpJob.php',

‎lib/private/AppFramework/DependencyInjection/DIContainer.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
use OC\Core\Middleware\TwoFactorMiddleware;
2222
use OC\Diagnostics\EventLogger;
2323
use OC\Log\PsrLoggerAdapter;
24+
use OC\Security\RemoteIpAddress;
2425
use OC\ServerContainer;
2526
use OC\Settings\AuthorizedGroupMapper;
2627
use OCA\WorkflowEngine\Manager;
@@ -231,7 +232,8 @@ public function __construct(string $appName, array $urlParams = [], ?ServerConta
231232
$server->getAppManager(),
232233
$server->getL10N('lib'),
233234
$c->get(AuthorizedGroupMapper::class),
234-
$server->get(IUserSession::class)
235+
$server->get(IUserSession::class),
236+
$c->get(RemoteIpAddress::class),
235237
);
236238
$dispatcher->registerMiddleware($securityMiddleware);
237239
$dispatcher->registerMiddleware(

‎lib/private/AppFramework/Middleware/Security/Exceptions/AdminIpNotAllowedException.php‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
/**
5+
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
6+
* SPDX-License-Identifier: AGPL-3.0-only
7+
*/
8+
namespace OC\AppFramework\Middleware\Security\Exceptions;
9+
10+
use OCP\AppFramework\Http;
11+
12+
/**
13+
* Class AdminIpNotAllowed is thrown when a resource has been requested by a
14+
* an admin user connecting from an unauthorized IP address
15+
* See configuration `allowed_admin_ranges`
16+
*
17+
* @package OC\AppFramework\Middleware\Security\Exceptions
18+
*/
19+
class AdminIpNotAllowedException extends SecurityException {
20+
public function __construct(string $message) {
21+
parent::__construct($message, Http::STATUS_FORBIDDEN);
22+
}
23+
}

‎lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php‎

Lines changed: 30 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
*/
99
namespace OC\AppFramework\Middleware\Security;
1010

11+
use OC\AppFramework\Middleware\Security\Exceptions\AdminIpNotAllowedException;
1112
use OC\AppFramework\Middleware\Security\Exceptions\AppNotEnabledException;
1213
use OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException;
1314
use OC\AppFramework\Middleware\Security\Exceptions\ExAppRequiredException;
@@ -16,6 +17,7 @@
1617
use OC\AppFramework\Middleware\Security\Exceptions\SecurityException;
1718
use OC\AppFramework\Middleware\Security\Exceptions\StrictCookieMissingException;
1819
use OC\AppFramework\Utility\ControllerMethodReflector;
20+
use OC\Security\RemoteIpAddress;
1921
use OC\Settings\AuthorizedGroupMapper;
2022
use OC\User\Session;
2123
use OCP\App\AppPathNotFoundException;
@@ -50,60 +52,22 @@
5052
* check fails
5153
*/
5254
class SecurityMiddleware extends Middleware {
53-
/** @var INavigationManager */
54-
private $navigationManager;
55-
/** @var IRequest */
56-
private $request;
57-
/** @var ControllerMethodReflector */
58-
private $reflector;
59-
/** @var string */
60-
private $appName;
61-
/** @var IURLGenerator */
62-
private $urlGenerator;
63-
/** @var LoggerInterface */
64-
private $logger;
65-
/** @var bool */
66-
private $isLoggedIn;
67-
/** @var bool */
68-
private $isAdminUser;
69-
/** @var bool */
70-
private $isSubAdmin;
71-
/** @var IAppManager */
72-
private $appManager;
73-
/** @var IL10N */
74-
private $l10n;
75-
/** @var AuthorizedGroupMapper */
76-
private $groupAuthorizationMapper;
77-
/** @var IUserSession */
78-
private $userSession;
79-
80-
public function __construct(IRequest $request,
81-
ControllerMethodReflector $reflector,
82-
INavigationManager $navigationManager,
83-
IURLGenerator $urlGenerator,
84-
LoggerInterface $logger,
85-
string $appName,
86-
bool $isLoggedIn,
87-
bool $isAdminUser,
88-
bool $isSubAdmin,
89-
IAppManager $appManager,
90-
IL10N $l10n,
91-
AuthorizedGroupMapper $mapper,
92-
IUserSession $userSession
55+
public function __construct(
56+
private IRequest $request,
57+
private ControllerMethodReflector $reflector,
58+
private INavigationManager $navigationManager,
59+
private IURLGenerator $urlGenerator,
60+
private LoggerInterface $logger,
61+
private string $appName,
62+
private bool $isLoggedIn,
63+
private bool $isAdminUser,
64+
private bool $isSubAdmin,
65+
private IAppManager $appManager,
66+
private IL10N $l10n,
67+
private AuthorizedGroupMapper $groupAuthorizationMapper,
68+
private IUserSession $userSession,
69+
private RemoteIpAddress $remoteIpAddress,
9370
) {
94-
$this->navigationManager = $navigationManager;
95-
$this->request = $request;
96-
$this->reflector = $reflector;
97-
$this->appName = $appName;
98-
$this->urlGenerator = $urlGenerator;
99-
$this->logger = $logger;
100-
$this->isLoggedIn = $isLoggedIn;
101-
$this->isAdminUser = $isAdminUser;
102-
$this->isSubAdmin = $isSubAdmin;
103-
$this->appManager = $appManager;
104-
$this->l10n = $l10n;
105-
$this->groupAuthorizationMapper = $mapper;
106-
$this->userSession = $userSession;
10771
}
10872

10973
/**
@@ -161,6 +125,9 @@ public function beforeController($controller, $methodName) {
161125
if (!$authorized) {
162126
throw new NotAdminException($this->l10n->t('Logged in account must be an admin, a sub admin or gotten special right to access this setting'));
163127
}
128+
if (!$this->remoteIpAddress->allowsAdminActions()) {
129+
throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn’t allow you to perform admin actions'));
130+
}
164131
}
165132
if ($this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)
166133
&& !$this->isSubAdmin
@@ -174,6 +141,16 @@ public function beforeController($controller, $methodName) {
174141
&& !$authorized) {
175142
throw new NotAdminException($this->l10n->t('Logged in account must be an admin'));
176143
}
144+
if ($this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)
145+
&& !$this->remoteIpAddress->allowsAdminActions()) {
146+
throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn’t allow you to perform admin actions'));
147+
}
148+
if (!$this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)
149+
&& !$this->hasAnnotationOrAttribute($reflectionMethod, 'NoAdminRequired', NoAdminRequired::class)
150+
&& !$this->remoteIpAddress->allowsAdminActions()) {
151+
throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn’t allow you to perform admin actions'));
152+
}
153+
177154
}
178155

179156
// Check for strict cookie requirement

‎lib/private/Group/Manager.php‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
namespace OC\Group;
99

1010
use OC\Hooks\PublicEmitter;
11+
use OC\Security\RemoteIpAddress;
1112
use OCP\EventDispatcher\IEventDispatcher;
1213
use OCP\Group\Backend\IBatchMethodsBackend;
1314
use OCP\Group\Backend\ICreateNamedGroupBackend;
@@ -41,11 +42,6 @@ class Manager extends PublicEmitter implements IGroupManager {
4142
/** @var GroupInterface[] */
4243
private $backends = [];
4344

44-
/** @var \OC\User\Manager */
45-
private $userManager;
46-
private IEventDispatcher $dispatcher;
47-
private LoggerInterface $logger;
48-
4945
/** @var array<string, IGroup> */
5046
private $cachedGroups = [];
5147

@@ -59,13 +55,13 @@ class Manager extends PublicEmitter implements IGroupManager {
5955

6056
private const MAX_GROUP_LENGTH = 255;
6157

62-
public function __construct(\OC\User\Manager $userManager,
63-
IEventDispatcher $dispatcher,
64-
LoggerInterface $logger,
65-
ICacheFactory $cacheFactory) {
66-
$this->userManager = $userManager;
67-
$this->dispatcher = $dispatcher;
68-
$this->logger = $logger;
58+
public function __construct(
59+
private \OC\User\Manager $userManager,
60+
private IEventDispatcher $dispatcher,
61+
private LoggerInterface $logger,
62+
ICacheFactory $cacheFactory,
63+
private RemoteIpAddress $remoteIpAddress,
64+
) {
6965
$this->displayNameCache = new DisplayNameCache($cacheFactory, $this);
7066

7167
$this->listen('\OC\Group', 'postDelete', function (IGroup $group): void {
@@ -325,6 +321,10 @@ public function getUserIdGroups(string $uid): array {
325321
* @return bool if admin
326322
*/
327323
public function isAdmin($userId) {
324+
if (!$this->remoteIpAddress->allowsAdminActions()) {
325+
return false;
326+
}
327+
328328
foreach ($this->backends as $backend) {
329329
if (is_string($userId) && $backend->implementsActions(Backend::IS_ADMIN) && $backend->isAdmin($userId)) {
330330
return true;

‎lib/private/Security/RemoteIpAddress.php‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OC\Security;
11+
12+
use IPLib\Factory;
13+
use OCP\IConfig;
14+
use OCP\IRequest;
15+
use Psr\Log\LoggerInterface;
16+
17+
/**
18+
* @internal
19+
*/
20+
final class RemoteIpAddress {
21+
private const SETTING_NAME = 'allowed_admin_ranges';
22+
23+
public function __construct(
24+
private IConfig $config,
25+
private IRequest $request,
26+
private LoggerInterface $logger,
27+
) {
28+
}
29+
30+
public function allowsAdminActions(): bool {
31+
$allowedAdminRanges = $this->config->getSystemValue(self::SETTING_NAME, false);
32+
if ($allowedAdminRanges === false) {
33+
// No restriction applied
34+
return true;
35+
}
36+
37+
if (!is_array($allowedAdminRanges)) {
38+
$this->logger->warning(
39+
'Invalid configuration for "{setting}". Found "{type}" instead of "array". Please check your config.php.',
40+
[
41+
'setting' => self::SETTING_NAME,
42+
'type' => gettype($allowedAdminRanges),
43+
],
44+
);
45+
return true;
46+
}
47+
48+
if (count($allowedAdminRanges) === 0) {
49+
return true;
50+
}
51+
52+
$ipAddress = Factory::parseAddressString($this->request->getRemoteAddress());
53+
if ($ipAddress === null) {
54+
$this->logger->warning(
55+
'Unable to parse remote IP "{ip}"',
56+
['ip' => $ipAddress,]
57+
);
58+
59+
return false;
60+
}
61+
62+
foreach ($allowedAdminRanges as $rangeString) {
63+
$range = Factory::parseRangeString($rangeString);
64+
if ($range === null) {
65+
$this->logger->warning(
66+
'Invalid range "{range}" found in "{setting}". Please check your config.php.',
67+
[
68+
'range' => $rangeString,
69+
'setting' => self::SETTING_NAME,
70+
]
71+
);
72+
continue;
73+
}
74+
if ($range->contains($ipAddress)) {
75+
return true;
76+
}
77+
}
78+
79+
return false;
80+
}
81+
}

‎lib/private/Server.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@
101101
use OC\Security\CSRF\TokenStorage\SessionStorage;
102102
use OC\Security\Hasher;
103103
use OC\Security\RateLimiting\Limiter;
104+
use OC\Security\RemoteIpAddress;
104105
use OC\Security\SecureRandom;
105106
use OC\Security\TrustedDomainHelper;
106107
use OC\Security\VerificationToken\VerificationToken;
@@ -464,7 +465,8 @@ public function __construct($webRoot, \OC\Config $config) {
464465
$this->get(IUserManager::class),
465466
$this->get(IEventDispatcher::class),
466467
$this->get(LoggerInterface::class),
467-
$this->get(ICacheFactory::class)
468+
$this->get(ICacheFactory::class),
469+
$this->get(RemoteIpAddress::class),
468470
);
469471
return $groupManager;
470472
});

0 commit comments

Comments
 (0)