[3.0] Address issues with proxy lookups

[jdarwood007]

The 3.0 version of #9229

[jdarwood007]

@sbulen, Please review. I did adjust how your logic worked, but I managed to accomplish the same goals, I believe.

The matchToCIDR should be the same, while getUserIP was what I adjusted.

[albertlast]

I applied this on top of release-3.0 (c81f523) and exercised both matchToCIDR() and getUserIP() against the test cases from #9229. The CIDR rewrite is a clear win; the getUserIP() adjustment has a few problems.

The CIDR rewrite fixes three crashes

Worth noting for anyone wondering whether this is needed, matchToCIDR() on release-3.0 today:

input	today	with this PR
2a06:98c0::1 in 2a06:98c0::/29	fatal: TypeError: str_repeat(): Argument #2 ($times) must be of type int, float given	true
1.2.3.4 in 1.2.3.0/33	fatal: ArithmeticError: Bit shift by negative number	false
2405:b500::4 in 2405:b500::4 (no prefix)	Warning: Undefined array key 1 + false	true

2a06:98c0::/29 is in Cloudflare's own published IP list, so pasting that list into proxy_ip_servers fatals on every request today. The new range math gave the right answer for every case I tried.

The $valid_sender gate also does what #9143 needs: with a public REMOTE_ADDR and a spoofed HTTP_X_FORWARDED_FOR, release-3.0 returns the spoofed address, this PR returns the real one.

Blocking

1. Fatal on any install with backward compatibility off.

matchToCIDR() calls isValidIP($cidr_network). That function lives in Subs-Compat.php inside if (!empty(SMF\Config::$backward_compatibility) && !function_exists('smf_error_handler')), and other/Settings.php ships $backward_compatibility = 0. On a default install:

Error: Call to undefined function SMF\isValidIP()

reached from getUserIP() as soon as proxy_ip_servers is non-empty, so on every request, for exactly the admins this PR is for. It is a faithful copy of the 2.1 patch, where that global is always available. (new self($cidr_network))->isValid() does the same job here.

2. It now takes the last IP in the header rather than the first.

The headline item in #9229 is "Properly uses FIRST ip in header list, not last, for user IP, per RFC 7239", and 2.1 does $ip = simplify_ip($ips[0]); … break;. This port removed the array_reverse() that produced that effect on release-3.0, but kept the assign-on-every-match loop with no break, so the last match wins:

REMOTE_ADDR = 103.31.6.1 (in proxy_ip_servers), HTTP_CF_CONNECTING_IP = '173.228.74.9, 8.8.8.8'

  release-3.0    => 173.228.74.9
  this PR        => 8.8.8.8

So it is a change in behaviour against release-3.0 as well as against the PR being ported. A break after the first public match restores it.

3. Dangling loop variable in the "both IPs are private" fallback.

if (!isset(static::$user_ip) && !$remote_addr->isValid(FILTER_FLAG_GLOBAL_RANGE)) {
		static::$user_ip = $ip->simplified();
}

$ips[0] became $ip, which is whatever leaked out of the foreach ($ips as $ip) above, i.e. the last element again. (The extra tab of indentation is in the PR too; php-cs-fixer does not flag it, so CI stays green.)

4. Entries in proxy_ip_servers are no longer normalised, so ordinary lists fail closed.

2.1 runs each entry through simplify_ip($proxy), which trims whitespace and strips []. This port compares and matches the raw string:

proxy_ip_servers = '173.245.48.0/20, 103.31.4.0/22'   => valid_sender false, headers ignored
proxy_ip_servers = '[2405:b500::4]'                   => valid_sender false, headers ignored

Tests 7, 8 and 9 in #9229 are exactly these cases and they do not pass here. A space after the comma is what an admin will type.

Worth resolving before merge

5. simplified() is not equivalent to 2.1's simplify_ip(). The 2.1 function does trim($ip), trim($ip, '[]'), then the ::ffff: strip. simplified() only does the strip, and gates it on is4in6(), which returns false for [::ffff:1.2.3.4], so a bracketed value comes back unchanged. The docblock copied along with it still promises the trimming. Worth making the code match the docblock, since #4 needs that behaviour anyway.

6. The two Subs-Compat.php shims probably should not be added. Neither simplify_ip() nor valid_localhost_ip() exists in v2.1.7, and #9229 is still open, so nothing a mod can be written against calls them. Same reasoning as make_fetch_safe() on #9535.

7. isLocalhostIP() is carrying a stale note and an inaccurate name. "FILTER_FLAG_GLOBAL_RANGE … only supported in PHP 8.2+" was true for 2.1 but this branch is 8.4+, and IP::isValid() already takes those flags. As written it also returns true for a string that is not an IP at all, and it accepts any private or reserved address, not just localhost. Something like:

public function isPrivate(): bool
{
	return $this->isValid() && !$this->isValid(FILTER_FLAG_GLOBAL_RANGE);
}

Minor

• simplified() has no return type declaration, and isLocalhostIP() has no @return tag.
• [2.1] Address issues with proxy lookups #9229 changes the unspecified default for proxy_ip_header to disabled; this keeps autodetect. The $valid_sender gate makes it nearly moot, but it is a stated goal of the ported PR that did not come across.
• Pre-existing and unchanged, but easy to fix while in here: the header loop has no break either, so under autodetect with two headers populated the last one checked wins, not the first one found.
• No tests, which is fair while [3.0][Testing] Add a PHPUnit suite for the parts that need no database #9326 is unmerged. Once it is, matchToCIDR() is a pure method on a value object and every row in the table above is a one-line data provider case, including the two crashes. Might be worth a note in the description.

[sbulen]

Note the concern raised in #9229 is still open: the ban logic needs to be changed.

Existing ban logic enforces a ban on both IPs... Once this logic works, one of those IPs will be your proxy. Ie., a ban might shut down all proxy traffic.

[jdarwood007]

Actually, that is a fairly simple change for 3.0. Added a method to the IP class for isRegisteredProxyServer(), if the IP is in that, it returns true. We can simply ignore suggestions if it matches.

For 2.1, I think we could just do as mentioned and not check it. Or implement the same idea for 2.1 to check suggestions to ensure they don't match a proxy ip

[albertlast]

Re-checked at 07df9ee, applied on top of the current release-3.0 (3aa674e, so with #9543 in). Still applies cleanly, php-cs-fixer is clean on all four files, and matchToCIDR() still passes every case from my earlier table, including the /29 and /33 crashes it fixes.

Confirmed fixed: the isValidIP() fatal, the first-vs-last IP ordering ('173.228.74.9, 8.8.8.8' now gives 173.228.74.9), the dangling $ip, whitespace in proxy_ip_servers, and isPrivate(). Thanks.

Two new ones, both blocking, and then some smaller items.

1. ??= on a bool leaves the localhost-proxy path dead

$valid_sender = (new IP($_SERVER['REMOTE_ADDR']))->isRegisteredProxyServer();

// If a list of proxy ip servers has not been provided, we will assume its a valid sender if its from a localhost IP.
$valid_sender ??= $remote_addr->isPrivate();

isRegisteredProxyServer() returns false, never null, so the second line never executes. With proxy_ip_servers empty, which is the case that line is there for, headers are now never trusted:

HTTP_X_FORWARDED_FOR: 173.228.74.9, no server list	release-3.0	this PR
REMOTE_ADDR = 127.0.0.1	173.228.74.9	127.0.0.1
REMOTE_ADDR = 10.0.0.2	173.228.74.9	10.0.0.2

That is the plain nginx/Varnish-in-front setup, and it is a regression against release-3.0 as well as against #9229. || (or the if/else the previous revision had) does what the comment says.

2. Saving the security settings destroys every CIDR in proxy_ip_servers

securityConfigVars() runs each entry through (new IP($ip, true))->simplified(). The constructor sets $this->ip = '' for anything that is not a bare IP, and a CIDR is not:

in : 173.245.48.0/20, 103.31.4.0/22, 2400:cb00::/32, 2405:b500::4
out: ,,,2405:b500::4

So an admin who opens Server Settings -> Security and presses Save once silently loses the whole Cloudflare list, and with item 1 above everything then falls back to the proxy's own IP. The / needs splitting off before the cleanup runs.

Two smaller points about the same block: it sits in a static getter that returns config vars and mutates $_POST as a side effect, and it runs before User::$me->checkSession(). The cors_domains normalisation just below it does the same kind of job from inside security() after the session check, which looks like the pattern to follow.

Smaller items

• clean() throws on null. The constructor is self|string|null $ip and the file is strict_types=1, so new IP(null, true) gives TypeError: trim(): Argument #1 ($string) must be of type string, null given. Not reachable from the current call site, but it is a public constructor. private function clean(?string $raw): string with a cast covers it.
• simplify_ip() still does not clean. The shim calls new SMF\IP($ip) without clean: true, so simplify_ip(' [::ffff:1.2.3.4] ') returns '' where the 2.1 function returns 1.2.3.4. One word to fix, though I still think neither shim should be added: neither simplify_ip() nor valid_localhost_ip() exists in v2.1.7, and [2.1] Address issues with proxy lookups #9229 is still open, so no mod can be calling them (same reasoning as make_fetch_safe() on [3.0] Fixes broken SNI resolution #9535). valid_localhost_ip()'s docblock also still carries the "only supported in PHP 8.2+" note that isPrivate() just dropped.
• Header values are not cleaned either. getUserIP() builds new self($ip) without the flag, so a bracketed [::ffff:173.228.74.9] in a header is still discarded.
• Dropping the private-IP fallback changes LAN behaviour. With a private client behind a private proxy, release-3.0 records 192.168.1.9; this records the proxy, 10.0.0.2. 2.1 takes the first entry if it is a valid IP at all, rather than requiring global range, so it keeps the client too. Fine if deliberate, but on an intranet forum every member ends up sharing one IP.
• The ban concern is only half-addressed. isRegisteredProxyServer() filters the suggestion list on the ban form, but Security::checkBans() still queries both ip and ip2, and ip2 is the proxy. An IP typed in by hand, or a range ban, can still catch proxy traffic.
• Cosmetic: $valid_sender = false; is now a dead initial assignment; new IP($_SERVER['REMOTE_ADDR']) re-instantiates what $remote_addr already holds two lines above; static::$registered_proxy_servers is never invalidated if proxy_ip_servers changes during the request; clean($raw) has no parameter type.