Skip to content

Commit

Permalink
Merge pull request #41438 from nextcloud/feat/migrate-forwarded-for-h…
Browse files Browse the repository at this point in the history
…eaders-check

Migrate forwarded for headers check
  • Loading branch information
come-nc authored Nov 21, 2023
2 parents 0e43350 + ad37286 commit 24c2c09
Show file tree
Hide file tree
Showing 10 changed files with 249 additions and 198 deletions.
1 change: 1 addition & 0 deletions apps/settings/composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@
'OCA\\Settings\\SetupChecks\\DefaultPhoneRegionSet' => $baseDir . '/../lib/SetupChecks/DefaultPhoneRegionSet.php',
'OCA\\Settings\\SetupChecks\\EmailTestSuccessful' => $baseDir . '/../lib/SetupChecks/EmailTestSuccessful.php',
'OCA\\Settings\\SetupChecks\\FileLocking' => $baseDir . '/../lib/SetupChecks/FileLocking.php',
'OCA\\Settings\\SetupChecks\\ForwardedForHeaders' => $baseDir . '/../lib/SetupChecks/ForwardedForHeaders.php',
'OCA\\Settings\\SetupChecks\\InternetConnectivity' => $baseDir . '/../lib/SetupChecks/InternetConnectivity.php',
'OCA\\Settings\\SetupChecks\\LegacySSEKeyFormat' => $baseDir . '/../lib/SetupChecks/LegacySSEKeyFormat.php',
'OCA\\Settings\\SetupChecks\\MemcacheConfigured' => $baseDir . '/../lib/SetupChecks/MemcacheConfigured.php',
Expand Down
1 change: 1 addition & 0 deletions apps/settings/composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ class ComposerStaticInitSettings
'OCA\\Settings\\SetupChecks\\DefaultPhoneRegionSet' => __DIR__ . '/..' . '/../lib/SetupChecks/DefaultPhoneRegionSet.php',
'OCA\\Settings\\SetupChecks\\EmailTestSuccessful' => __DIR__ . '/..' . '/../lib/SetupChecks/EmailTestSuccessful.php',
'OCA\\Settings\\SetupChecks\\FileLocking' => __DIR__ . '/..' . '/../lib/SetupChecks/FileLocking.php',
'OCA\\Settings\\SetupChecks\\ForwardedForHeaders' => __DIR__ . '/..' . '/../lib/SetupChecks/ForwardedForHeaders.php',
'OCA\\Settings\\SetupChecks\\InternetConnectivity' => __DIR__ . '/..' . '/../lib/SetupChecks/InternetConnectivity.php',
'OCA\\Settings\\SetupChecks\\LegacySSEKeyFormat' => __DIR__ . '/..' . '/../lib/SetupChecks/LegacySSEKeyFormat.php',
'OCA\\Settings\\SetupChecks\\MemcacheConfigured' => __DIR__ . '/..' . '/../lib/SetupChecks/MemcacheConfigured.php',
Expand Down
2 changes: 2 additions & 0 deletions apps/settings/lib/AppInfo/Application.php
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
use OCA\Settings\SetupChecks\DefaultPhoneRegionSet;
use OCA\Settings\SetupChecks\EmailTestSuccessful;
use OCA\Settings\SetupChecks\FileLocking;
use OCA\Settings\SetupChecks\ForwardedForHeaders;
use OCA\Settings\SetupChecks\InternetConnectivity;
use OCA\Settings\SetupChecks\LegacySSEKeyFormat;
use OCA\Settings\SetupChecks\MemcacheConfigured;
Expand Down Expand Up @@ -162,6 +163,7 @@ public function register(IRegistrationContext $context): void {
$context->registerSetupCheck(DefaultPhoneRegionSet::class);
$context->registerSetupCheck(EmailTestSuccessful::class);
$context->registerSetupCheck(FileLocking::class);
$context->registerSetupCheck(ForwardedForHeaders::class);
$context->registerSetupCheck(InternetConnectivity::class);
$context->registerSetupCheck(LegacySSEKeyFormat::class);
$context->registerSetupCheck(MemcacheConfigured::class);
Expand Down
26 changes: 0 additions & 26 deletions apps/settings/lib/Controller/CheckSetupController.php
Original file line number Diff line number Diff line change
Expand Up @@ -246,31 +246,6 @@ private function isUsedTlsLibOutdated() {
return '';
}

/**
* Check if the reverse proxy configuration is working as expected
*
* @return bool
*/
private function forwardedForHeadersWorking(): bool {
$trustedProxies = $this->config->getSystemValue('trusted_proxies', []);
$remoteAddress = $this->request->getHeader('REMOTE_ADDR');

if (empty($trustedProxies) && $this->request->getHeader('X-Forwarded-Host') !== '') {
return false;
}

if (\is_array($trustedProxies)) {
if (\in_array($remoteAddress, $trustedProxies, true) && $remoteAddress !== '127.0.0.1') {
return $remoteAddress !== $this->request->getRemoteAddress();
}
} else {
return false;
}

// either not enabled or working correctly
return true;
}

/**
* Checks if the correct memcache module for PHP is installed. Only
* fails if memcached is configured and the working module is not installed.
Expand Down Expand Up @@ -721,7 +696,6 @@ public function check() {
'cronErrors' => $this->getCronErrors(),
'isFairUseOfFreePushService' => $this->isFairUseOfFreePushService(),
'isUsedTlsLibOutdated' => $this->isUsedTlsLibOutdated(),
'forwardedForHeadersWorking' => $this->forwardedForHeadersWorking(),
'reverseProxyDocs' => $this->urlGenerator->linkToDocs('admin-reverse-proxy'),
'isCorrectMemcachedPHPModuleInstalled' => $this->isCorrectMemcachedPHPModuleInstalled(),
'hasPassedCodeIntegrityCheck' => $this->checker->hasPassedCheck(),
Expand Down
14 changes: 9 additions & 5 deletions apps/settings/lib/SetupChecks/BruteForceThrottler.php
Original file line number Diff line number Diff line change
Expand Up @@ -53,17 +53,21 @@ public function getName(): string {
public function run(): SetupResult {
$address = $this->request->getRemoteAddress();
if ($address === '') {
return SetupResult::info(
$this->l10n->t('Your remote address could not be determined.')
);
if (\OC::$CLI) {
/* We were called from CLI */
return SetupResult::info($this->l10n->t('Your remote address could not be determined.'));
} else {
/* Should never happen */
return SetupResult::error($this->l10n->t('Your remote address could not be determined.'));
}
} elseif ($this->throttler->showBruteforceWarning($address)) {
return SetupResult::error(
$this->l10n->t('Your remote address was identified as "%s" and is bruteforce throttled at the moment slowing down the performance of various requests. If the remote address is not your address this can be an indication that a proxy is not configured correctly.', $address),
$this->l10n->t('Your remote address was identified as "%s" and is bruteforce throttled at the moment slowing down the performance of various requests. If the remote address is not your address this can be an indication that a proxy is not configured correctly.', [$address]),
$this->urlGenerator->linkToDocs('admin-reverse-proxy')
);
} else {
return SetupResult::success(
$this->l10n->t('Your remote address "%s" is not bruteforce throttled.', $address)
$this->l10n->t('Your remote address "%s" is not bruteforce throttled.', [$address])
);
}
}
Expand Down
94 changes: 94 additions & 0 deletions apps/settings/lib/SetupChecks/ForwardedForHeaders.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
<?php

declare(strict_types=1);

/**
* @copyright Copyright (c) 2023 Côme Chilliet <come.chilliet@nextcloud.com>
*
* @author Côme Chilliet <come.chilliet@nextcloud.com>
*
* @license GNU AGPL version 3 or any later version
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

namespace OCA\Settings\SetupChecks;

use OCP\IConfig;
use OCP\IL10N;
use OCP\IRequest;
use OCP\IURLGenerator;
use OCP\SetupCheck\ISetupCheck;
use OCP\SetupCheck\SetupResult;

class ForwardedForHeaders implements ISetupCheck {
public function __construct(
private IL10N $l10n,
private IConfig $config,
private IURLGenerator $urlGenerator,
private IRequest $request,
) {
}

public function getCategory(): string {
return 'security';
}

public function getName(): string {
return $this->l10n->t('Forwared for headers');
}

public function run(): SetupResult {
$trustedProxies = $this->config->getSystemValue('trusted_proxies', []);
$remoteAddress = $this->request->getHeader('REMOTE_ADDR');
$detectedRemoteAddress = $this->request->getRemoteAddress();

if (!\is_array($trustedProxies)) {
return SetupResult::error($this->l10n->t('Your trusted_proxies setting is not correctly set, it should be an array.'));
}

if (($remoteAddress === '') && ($detectedRemoteAddress === '')) {
if (\OC::$CLI) {
/* We were called from CLI */
return SetupResult::info($this->l10n->t('Your remote address could not be determined.'));
} else {
/* Should never happen */
return SetupResult::error($this->l10n->t('Your remote address could not be determined.'));
}
}

if (empty($trustedProxies) && $this->request->getHeader('X-Forwarded-Host') !== '') {
return SetupResult::error(
$this->l10n->t('The reverse proxy header configuration is incorrect. This is a security issue and can allow an attacker to spoof their IP address as visible to the Nextcloud.'),
$this->urlGenerator->linkToDocs('admin-reverse-proxy')
);
}

if (\in_array($remoteAddress, $trustedProxies, true) && ($remoteAddress !== '127.0.0.1')) {
if ($remoteAddress !== $detectedRemoteAddress) {
/* Remote address was successfuly fixed */
return SetupResult::success($this->l10n->t('Your IP address was resolved as %s', [$detectedRemoteAddress]));
} else {
return SetupResult::warning(
$this->l10n->t('The reverse proxy header configuration is incorrect, or you are accessing Nextcloud from a trusted proxy. If not, this is a security issue and can allow an attacker to spoof their IP address as visible to the Nextcloud.'),
$this->urlGenerator->linkToDocs('admin-reverse-proxy')
);
}
}

/* Either not enabled or working correctly */
return SetupResult::success();
}
}
91 changes: 2 additions & 89 deletions apps/settings/tests/Controller/CheckSetupControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -204,87 +204,6 @@ public function removeTestDirectories() {
$this->dirsToRemove = [];
}

/**
* @dataProvider dataForwardedForHeadersWorking
*
* @param array $trustedProxies
* @param string $remoteAddrNotForwarded
* @param string $remoteAddr
* @param bool $result
*/
public function testForwardedForHeadersWorking(array $trustedProxies, string $remoteAddrNotForwarded, string $remoteAddr, bool $result): void {
$this->config->expects($this->once())
->method('getSystemValue')
->with('trusted_proxies', [])
->willReturn($trustedProxies);
$this->request->expects($this->atLeastOnce())
->method('getHeader')
->willReturnMap([
['REMOTE_ADDR', $remoteAddrNotForwarded],
['X-Forwarded-Host', '']
]);
$this->request->expects($this->any())
->method('getRemoteAddress')
->willReturn($remoteAddr);

$this->assertEquals(
$result,
self::invokePrivate($this->checkSetupController, 'forwardedForHeadersWorking')
);
}

public function dataForwardedForHeadersWorking(): array {
return [
// description => trusted proxies, getHeader('REMOTE_ADDR'), getRemoteAddr, expected result
'no trusted proxies' => [[], '2.2.2.2', '2.2.2.2', true],
'trusted proxy, remote addr not trusted proxy' => [['1.1.1.1'], '2.2.2.2', '2.2.2.2', true],
'trusted proxy, remote addr is trusted proxy, x-forwarded-for working' => [['1.1.1.1'], '1.1.1.1', '2.2.2.2', true],
'trusted proxy, remote addr is trusted proxy, x-forwarded-for not set' => [['1.1.1.1'], '1.1.1.1', '1.1.1.1', false],
];
}

public function testForwardedHostPresentButTrustedProxiesNotAnArray(): void {
$this->config->expects($this->once())
->method('getSystemValue')
->with('trusted_proxies', [])
->willReturn('1.1.1.1');
$this->request->expects($this->atLeastOnce())
->method('getHeader')
->willReturnMap([
['REMOTE_ADDR', '1.1.1.1'],
['X-Forwarded-Host', 'nextcloud.test']
]);
$this->request->expects($this->any())
->method('getRemoteAddress')
->willReturn('1.1.1.1');

$this->assertEquals(
false,
self::invokePrivate($this->checkSetupController, 'forwardedForHeadersWorking')
);
}

public function testForwardedHostPresentButTrustedProxiesEmpty(): void {
$this->config->expects($this->once())
->method('getSystemValue')
->with('trusted_proxies', [])
->willReturn([]);
$this->request->expects($this->atLeastOnce())
->method('getHeader')
->willReturnMap([
['REMOTE_ADDR', '1.1.1.1'],
['X-Forwarded-Host', 'nextcloud.test']
]);
$this->request->expects($this->any())
->method('getRemoteAddress')
->willReturn('1.1.1.1');

$this->assertEquals(
false,
self::invokePrivate($this->checkSetupController, 'forwardedForHeadersWorking')
);
}

public function testCheck() {
$this->config->expects($this->any())
->method('getAppValue')
Expand All @@ -302,13 +221,8 @@ public function testCheck() {
['appstoreenabled', true, false],
]);

$this->request->expects($this->atLeastOnce())
->method('getHeader')
->willReturnMap([
['REMOTE_ADDR', '4.3.2.1'],
['X-Forwarded-Host', '']
]);

$this->request->expects($this->never())
->method('getHeader');
$this->clientService->expects($this->never())
->method('newClient');
$this->checkSetupController
Expand Down Expand Up @@ -415,7 +329,6 @@ public function testCheck() {
],
'cronErrors' => [],
'isUsedTlsLibOutdated' => '',
'forwardedForHeadersWorking' => false,
'reverseProxyDocs' => 'reverse-proxy-doc-link',
'isCorrectMemcachedPHPModuleInstalled' => true,
'hasPassedCodeIntegrityCheck' => true,
Expand Down
Loading

0 comments on commit 24c2c09

Please sign in to comment.