diff --git a/config/config.sample.php b/config/config.sample.php index 3b5632087f..5c362e9425 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -1017,7 +1017,13 @@ $CONFIG = array( /** * Headers that should be trusted as client IP address in combination with - * `trusted_proxies` + * `trusted_proxies`. If the HTTP header looks like 'X-Forwarded-For', then use + * 'HTTP_X_FORWARDED_FOR' here. + * + * If set incorrectly, a client can spoof their IP address as visible to + * ownCloud, bypassing access controls and making logs useless! + * + * Defaults to 'HTTP_X_FORWARED_FOR' if unset */ 'forwarded_for_headers' => array('HTTP_X_FORWARDED', 'HTTP_FORWARDED_FOR'), diff --git a/core/js/setupchecks.js b/core/js/setupchecks.js index 5a5c12c85e..fd192e6563 100644 --- a/core/js/setupchecks.js +++ b/core/js/setupchecks.js @@ -77,6 +77,11 @@ t('core', 'Your PHP version ({version}) is no longer supported by PHP. We encourage you to upgrade your PHP version to take advantage of performance and security updates provided by PHP.', {version: data.phpSupported.version, phpLink: 'https://secure.php.net/supported-versions.php'}) ); } + if(!data.forwardedForHeadersWorking) { + messages.push( + t('core', 'The reverse proxy headers configuration is incorrect, or you are accessing ownCloud from a trusted proxy. If you are not accessing ownCloud from a trusted proxy, this is a security issue and can allow an attacker to spoof their IP address as visible to ownCloud. Further information can be found in our documentation.', {docLink: data.reverseProxyDocs}) + ); + } } else { messages.push(t('core', 'Error occurred while checking server setup')); } diff --git a/core/js/tests/specs/setupchecksSpec.js b/core/js/tests/specs/setupchecksSpec.js index fe12aa4544..d0efcf4b28 100644 --- a/core/js/tests/specs/setupchecksSpec.js +++ b/core/js/tests/specs/setupchecksSpec.js @@ -66,7 +66,12 @@ describe('OC.SetupChecks tests', function() { { 'Content-Type': 'application/json' }, - JSON.stringify({isUrandomAvailable: true, serverHasInternetConnection: false, memcacheDocs: 'https://doc.owncloud.org/server/go.php?to=admin-performance'}) + JSON.stringify({ + isUrandomAvailable: true, + serverHasInternetConnection: false, + memcacheDocs: 'https://doc.owncloud.org/server/go.php?to=admin-performance', + forwardedForHeadersWorking: true + }) ); async.done(function( data, s, x ){ @@ -83,7 +88,13 @@ describe('OC.SetupChecks tests', function() { { 'Content-Type': 'application/json' }, - JSON.stringify({isUrandomAvailable: true, serverHasInternetConnection: false, dataDirectoryProtected: false, memcacheDocs: 'https://doc.owncloud.org/server/go.php?to=admin-performance'}) + JSON.stringify({ + isUrandomAvailable: true, + serverHasInternetConnection: false, + dataDirectoryProtected: false, + memcacheDocs: 'https://doc.owncloud.org/server/go.php?to=admin-performance', + forwardedForHeadersWorking: true + }) ); async.done(function( data, s, x ){ @@ -100,7 +111,13 @@ describe('OC.SetupChecks tests', function() { { 'Content-Type': 'application/json', }, - JSON.stringify({isUrandomAvailable: true, serverHasInternetConnection: false, dataDirectoryProtected: false, isMemcacheConfigured: true}) + JSON.stringify({ + isUrandomAvailable: true, + serverHasInternetConnection: false, + dataDirectoryProtected: false, + isMemcacheConfigured: true, + forwardedForHeadersWorking: true + }) ); async.done(function( data, s, x ){ @@ -117,7 +134,14 @@ describe('OC.SetupChecks tests', function() { { 'Content-Type': 'application/json', }, - JSON.stringify({isUrandomAvailable: false, securityDocs: 'https://docs.owncloud.org/myDocs.html', serverHasInternetConnection: true, dataDirectoryProtected: true, isMemcacheConfigured: true}) + JSON.stringify({ + isUrandomAvailable: false, + securityDocs: 'https://docs.owncloud.org/myDocs.html', + serverHasInternetConnection: true, + dataDirectoryProtected: true, + isMemcacheConfigured: true, + forwardedForHeadersWorking: true + }) ); async.done(function( data, s, x ){ @@ -126,6 +150,30 @@ describe('OC.SetupChecks tests', function() { }); }); + it('should return an error if the forwarded for headers are not working', function(done) { + var async = OC.SetupChecks.checkSetup(); + + suite.server.requests[0].respond( + 200, + { + 'Content-Type': 'application/json', + }, + JSON.stringify({ + isUrandomAvailable: true, + serverHasInternetConnection: true, + dataDirectoryProtected: true, + isMemcacheConfigured: true, + forwardedForHeadersWorking: false, + reverseProxyDocs: 'https://docs.owncloud.org/foo/bar.html' + }) + ); + + async.done(function( data, s, x ){ + expect(data).toEqual(['The reverse proxy headers configuration is incorrect, or you are accessing ownCloud from a trusted proxy. If you are not accessing ownCloud from a trusted proxy, this is a security issue and can allow an attacker to spoof their IP address as visible to ownCloud. Further information can be found in our documentation.']); + done(); + }); + }); + it('should return an error if the response has no statuscode 200', function(done) { var async = OC.SetupChecks.checkSetup(); @@ -151,7 +199,15 @@ describe('OC.SetupChecks tests', function() { { 'Content-Type': 'application/json', }, - JSON.stringify({isUrandomAvailable: true, securityDocs: 'https://docs.owncloud.org/myDocs.html', serverHasInternetConnection: true, dataDirectoryProtected: true, isMemcacheConfigured: true, phpSupported: {eol: true, version: '5.4.0'}}) + JSON.stringify({ + isUrandomAvailable: true, + securityDocs: 'https://docs.owncloud.org/myDocs.html', + serverHasInternetConnection: true, + dataDirectoryProtected: true, + isMemcacheConfigured: true, + forwardedForHeadersWorking: true, + phpSupported: {eol: true, version: '5.4.0'} + }) ); async.done(function( data, s, x ){ diff --git a/lib/private/appframework/http/request.php b/lib/private/appframework/http/request.php index 43f01dfde3..aaad286e84 100644 --- a/lib/private/appframework/http/request.php +++ b/lib/private/appframework/http/request.php @@ -452,7 +452,10 @@ class Request implements \ArrayAccess, \Countable, IRequest { $trustedProxies = $this->config->getSystemValue('trusted_proxies', []); if(is_array($trustedProxies) && in_array($remoteAddress, $trustedProxies)) { - $forwardedForHeaders = $this->config->getSystemValue('forwarded_for_headers', []); + $forwardedForHeaders = $this->config->getSystemValue('forwarded_for_headers', [ + 'HTTP_X_FORWARDED_FOR' + // only have one default, so we cannot ship an insecure product out of the box + ]); foreach($forwardedForHeaders as $header) { if(isset($this->server[$header])) { diff --git a/settings/controller/checksetupcontroller.php b/settings/controller/checksetupcontroller.php index ff605b474e..33f94fe423 100644 --- a/settings/controller/checksetupcontroller.php +++ b/settings/controller/checksetupcontroller.php @@ -128,7 +128,7 @@ class CheckSetupController extends Controller { /** * Check if the used SSL lib is outdated. Older OpenSSL and NSS versions do * have multiple bugs which likely lead to problems in combination with - * functionalities required by ownCloud such as SNI. + * functionality required by ownCloud such as SNI. * * @link https://github.com/owncloud/core/issues/17446#issuecomment-122877546 * @link https://bugzilla.redhat.com/show_bug.cgi?id=1241172 @@ -193,6 +193,23 @@ class CheckSetupController extends Controller { return ['eol' => $eol, 'version' => PHP_VERSION]; } + /* + * Check if the reverse proxy configuration is working as expected + * + * @return bool + */ + private function forwardedForHeadersWorking() { + $trustedProxies = $this->config->getSystemValue('trusted_proxies', []); + $remoteAddress = $this->request->getRemoteAddress(); + + if (is_array($trustedProxies) && in_array($remoteAddress, $trustedProxies)) { + return false; + } + + // either not enabled or working correctly + return true; + } + /** * @return DataResponse */ @@ -207,6 +224,8 @@ class CheckSetupController extends Controller { 'securityDocs' => $this->urlGenerator->linkToDocs('admin-security'), 'isUsedTlsLibOutdated' => $this->isUsedTlsLibOutdated(), 'phpSupported' => $this->isPhpSupported(), + 'forwardedForHeadersWorking' => $this->forwardedForHeadersWorking(), + 'reverseProxyDocs' => $this->urlGenerator->linkToDocs('admin-reverse-proxy'), ] ); } diff --git a/tests/settings/controller/CheckSetupControllerTest.php b/tests/settings/controller/CheckSetupControllerTest.php index 62fedd6dd6..414b1b91e1 100644 --- a/tests/settings/controller/CheckSetupControllerTest.php +++ b/tests/settings/controller/CheckSetupControllerTest.php @@ -246,7 +246,40 @@ class CheckSetupControllerTest extends TestCase { ['eol' => false, 'version' => PHP_VERSION], self::invokePrivate($this->checkSetupController, 'isPhpSupported') ); + } + public function testForwardedForHeadersWorkingFalse() { + $this->config->expects($this->once()) + ->method('getSystemValue') + ->with('trusted_proxies', []) + ->willReturn(['1.2.3.4']); + $this->request->expects($this->once()) + ->method('getRemoteAddress') + ->willReturn('1.2.3.4'); + + $this->assertFalse( + self::invokePrivate( + $this->checkSetupController, + 'forwardedForHeadersWorking' + ) + ); + } + + public function testForwardedForHeadersWorkingTrue() { + $this->config->expects($this->once()) + ->method('getSystemValue') + ->with('trusted_proxies', []) + ->willReturn(['1.2.3.4']); + $this->request->expects($this->once()) + ->method('getRemoteAddress') + ->willReturn('4.3.2.1'); + + $this->assertTrue( + self::invokePrivate( + $this->checkSetupController, + 'forwardedForHeadersWorking' + ) + ); } public function testCheck() { @@ -258,6 +291,14 @@ class CheckSetupControllerTest extends TestCase { ->method('getSystemValue') ->with('memcache.local', null) ->will($this->returnValue('SomeProvider')); + $this->config->expects($this->at(2)) + ->method('getSystemValue') + ->with('trusted_proxies', []) + ->willReturn(['1.2.3.4']); + + $this->request->expects($this->once()) + ->method('getRemoteAddress') + ->willReturn('4.3.2.1'); $client = $this->getMockBuilder('\OCP\Http\Client\IClient') ->disableOriginalConstructor()->getMock(); @@ -285,6 +326,10 @@ class CheckSetupControllerTest extends TestCase { ->with('admin-security') ->willReturn('https://doc.owncloud.org/server/8.1/admin_manual/configuration_server/hardening.html'); self::$version_compare = -1; + $this->urlGenerator->expects($this->at(2)) + ->method('linkToDocs') + ->with('admin-reverse-proxy') + ->willReturn('reverse-proxy-doc-link'); $expected = new DataResponse( [ @@ -298,7 +343,9 @@ class CheckSetupControllerTest extends TestCase { 'phpSupported' => [ 'eol' => true, 'version' => PHP_VERSION - ] + ], + 'forwardedForHeadersWorking' => true, + 'reverseProxyDocs' => 'reverse-proxy-doc-link', ] ); $this->assertEquals($expected, $this->checkSetupController->check());