Merge pull request #17868 from owncloud/x-forwarded-for

Set default 'forwarded for' headers for reverse proxy

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'),
 

core/js/setupchecks.js

@@ -77,6 +77,11 @@
 							t('core', 'Your PHP version ({version}) is no longer <a href="{phpLink}">supported by PHP</a>. 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 <a href="{docLink}">documentation</a>.', {docLink: data.reverseProxyDocs})
+						);
+					}
 				} else {
 					messages.push(t('core', 'Error occurred while checking server setup'));
 				}

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 <a href="https://docs.owncloud.org/foo/bar.html">documentation</a>.']);
+				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 ){

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])) {

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'),
 			]
 		);
 	}

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());