forwardedForHeadersWorking should return true if the remote address matches trusted proxies

[DanielYWoo]

Because this is purely a potential logical bug so I skip all the environment and reproduce steps, since the bug is very easy to understand I think.

Nextcloud version: (see Nextcloud admin page)
13 and the master branch.

	private function forwardedForHeadersWorking() {
		$trustedProxies = $this->config->getSystemValue('trusted_proxies', []);
		$remoteAddress = $this->request->getRemoteAddress();
		if (is_array($trustedProxies) && in_array($remoteAddress, $trustedProxies)) {
			return false; // ------------- here ? return true? -----------------
		}
		// either not enabled or working correctly
		return true;
	}

If the remote address is in the trusted proxy list, here we should return true, right?

[nextcloud-bot]

GitMate.io thinks possibly related issues are #10826 (webdav address is incorrect when using reverse proxy), and #3456 (Remote wipe).

[DanielYWoo]

My dear bot, this is not related I think.

[kesselb]

If the remote address is in the trusted proxy list, here we should return true, right?

• Client: a.a.a.a
• Trusted Proxy (e.g. load balancer): b.b.b.b
• Nextcloud: c.c.c.c

When remote addr is b.b.b.b for nextcloud than your setup is not configured properly. If you ask for remoteAddr you expect the client ip (a.a.a.a). Return false when your remoteAddr is listed as trusted proxy looks correct for me.

server/lib/private/AppFramework/Http/Request.php

Lines 597 to 627 in c0a283f

	/**
	* Returns the remote address, if the connection came from a trusted proxy
	* and `forwarded_for_headers` has been configured then the IP address
	* specified in this header will be returned instead.
	* Do always use this instead of $_SERVER['REMOTE_ADDR']
	* @return string IP address
	*/
	public function getRemoteAddress(): string {
	$remoteAddress = isset($this->server['REMOTE_ADDR']) ? $this->server['REMOTE_ADDR'] : '';
	$trustedProxies = $this->config->getSystemValue('trusted_proxies', []);
	if(\is_array($trustedProxies) && \in_array($remoteAddress, $trustedProxies)) {
	$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])) {
	foreach(explode(',', $this->server[$header]) as $IP) {
	$IP = trim($IP);
	if (filter_var($IP, FILTER_VALIDATE_IP) !== false) {
	return $IP;
	}
	}
	}
	}
	}
	return $remoteAddress;
	}

When remoteAddr is trusted proxy than check if real client ip is passed by HTTP_X_FORWARDED_FOR (or any other header you specify).

cc @rullzer @MorrisJobke could you have a second look?

[MorrisJobke]

Makes sense 👍

[DanielYWoo]

@danielkesselberg I think the remote address in your example is b.b.b.b and should be b.b.b.b.

We are checking the proxy IP b.b.b.b not the client IP a.a.a.a. You can't trust client IPs which could change every hour on a mobile device with DHCP, maybe we should never do that. What we want to do here is to trust a reverse proxy whose IP is fixed in your intranet or IDC, so you need to check if $_SERVER['REMOTE_ADDR'] is in the trusted proxies list.

If you have a client desktop or mobile device with static IP a.a.a.a (which is rare) you can use $_SERVER['HTTP_X_FORWARDED_FOR'] to check against something like trusted_clients, but I don't think anybody needs this feature and it is not related to my problem.

Here is how I found this bug, I set up my nextcloud server with a reverse proxy, in the admin console it always complain about the reverse proxy mis-configuration then I check the ajax response error message and then tracked down this bug. You can use an nginx proxy with a nextcloud docker container to do the test.

[MorrisJobke]

@DanielYWoo This is also correct, but we should trust the proxy and on the other side also require the proxy to forward the correct address. And if this is not the case, then the proxy is slightly misconfigured. That's the whole point of this check. So if we get the IP of a trusted server, then it is not a real client, but a proxy that does not forward the UP properly.

[DanielYWoo]

@MorrisJobke If you want to check the forwarded address, that's fine but you should not use $_SERVER['REMOTE_ADDR'] for that purpose, you should use $_SERVER['HTTP_X_FORWARDED_FOR'] and you should do something more, split the addresses by comma then check them one by one.

Let's say, we have multiple proxies along the way, let's say client IP a.a.a.a, proxy 1 as b.b.b.b, proxy 2 and c.c.c.c, then nextcloud as d.d.d.d. In that case the x-forwarded-for headers will be something like this: a.a.a.a, b.b.b.b, you have to split x-forwarded-for then check one by one. If any proxy along the way in matches, we can say it's safe, is that right?

But I am kind of confused, nextcloud should not care about a.a.a.a or b.b.b.b, it should only care about the nearest proxy c.c.c.c is trusted, right?

[kesselb]

If you want to check the forwarded address, that's fine but you should not use $_SERVER['REMOTE_ADDR'] for that purpose, you should use $_SERVER['HTTP_X_FORWARDED_FOR'] and you should do something more, split the addresses by comma then check them one by one.

server/lib/private/AppFramework/Http/Request.php

Lines 597 to 627 in c0a283f

	/**
	* Returns the remote address, if the connection came from a trusted proxy
	* and `forwarded_for_headers` has been configured then the IP address
	* specified in this header will be returned instead.
	* Do always use this instead of $_SERVER['REMOTE_ADDR']
	* @return string IP address
	*/
	public function getRemoteAddress(): string {
	$remoteAddress = isset($this->server['REMOTE_ADDR']) ? $this->server['REMOTE_ADDR'] : '';
	$trustedProxies = $this->config->getSystemValue('trusted_proxies', []);
	if(\is_array($trustedProxies) && \in_array($remoteAddress, $trustedProxies)) {
	$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])) {
	foreach(explode(',', $this->server[$header]) as $IP) {
	$IP = trim($IP);
	if (filter_var($IP, FILTER_VALIDATE_IP) !== false) {
	return $IP;
	}
	}
	}
	}
	}
	return $remoteAddress;
	}

Let's say, we have multiple proxies along the way, let's say client IP a.a.a.a, proxy 1 as b.b.b.b, proxy 2 and c.c.c.c, then nextcloud as d.d.d.d. In that case the x-forwarded-for headers will be something like this:
a.a.a.a, b.b.b.b, you have to split x-forwarded-for then check one by one. If any proxy along the way in matches, we can say it's safe, is that right?

No (you should set c.c.c.c as trusted proxy)

But I am kind of confused, nextcloud should not care about a.a.a.a or b.b.b.b, it should only care about the nearest proxy c.c.c.c is trusted, right?

Yes

I'm confused now 🙈 I guess getRemoteAddr works like you described? The setup check returns false when the remote addr is a trusted proxy (and implies that x_forwarded_for is not configured). I'm sorry I don't get it 😞

[DanielYWoo]

@danielkesselberg I did not read Request.php carefully so I thought it does not respect x-forwarded-for, sorry for that.

But I am still confused about the checking logic. If we only check the nearest proxy IP, why do we just check $_SERVER['REMOTE_ADDR'? If you want to utilize x-forwarded-for, getRemoteAddr should return a list of proxies and we should extract the last IP in the chain but the current implementation extracts the first one which could be a DHCP client IP, it's not a proxy at all.

Let's say x-forwarded-for is "a.a.a.a, b.b.b.b, c.c.c.c", getRemoteAddr currently returns a.a.a.a which is the client IP, but I think we should use c.c.c.c, extract the last one, right?

[kesselb]

Thank you for your patience I got it 🙈

private function forwardedForHeadersWorking() {
	$trustedProxies = $this->config->getSystemValue('trusted_proxies', []);
	$remoteAddress = $this->request->server['REMOTE_ADDR'] ?? '';

	if (is_array($trustedProxies) && in_array($remoteAddress, $trustedProxies)) {
		return $remoteAddress !== $this->request->getRemoteAddress();
	}
	// either not enabled or working correctly
	return true;
}

What do you think about the above version? When $this->server['REMOTE_ADDR'] is a trusted proxy then $this->server['REMOTE_ADDR'] should not match $this->request->getRemoteAddress() right?

[DanielYWoo]

Yes, if the x-forwarded-for is set correctly, $remoteAddress would be the nearest proxy IP in the proxy chain, getRemoteAddress() should return the farthest IP in the chain (client IP), and they should never match. Your solution is definitely much better than my pull request, so ignore my PR please.

And there are some corner cases to mention but they are all just fine with your solution:
If the user is accessing nextcloud directly without the proxy from the intranet, the code above will show a warning and I think it's expected. If the user is accessing nextcloud on the same server (let's say gnome and firefox are installed), it also show a warning, and I think it's just expected, just fine. If we are running docker network or VM NATs, if only it's DNAT, it should be fine.

Lastly, a little suggestion. To better help users troubleshoot all these kinds of network issues, maybe we can return the IP chains in x-forwarded-for and the $server['REMOTE_ADDR'] instead of a simple boolean value, then we can show them in the console along with the warning message. Without that information it's also OK, because most users can modify the nginx accesss log format or modify php code to write a log to get that information too, with that info it's would be a little easier and time-saving.

nextcloud rocks!

[kesselb]

Would you mind updating your pr? The above solution is something we developed together 👍 @nickvergessen when we use #11594 (comment) howto mock $this->request->server['REMOTE_ADDR']?

[DanielYWoo]

Sure, I will do it this weekend.

[nickvergessen]

in your unit test:

self::invokePrivate($request, 'item', [['REMOTE_ADDR' => 'fake.addr']]);

[nickvergessen]

If that doesn't work, add it to

server/lib/private/AppFramework/Http/Request.php

Lines 324 to 328 in c0a283f

	case 'CONTENT_TYPE' :
	case 'CONTENT_LENGTH' :
	if (isset($this->server[$name])) {
	return $this->server[$name];
	}

and use the method instead.