[3.0] Address issues with proxy lookups

Sources/Actions/Admin/Bans.php

@@ -1659,7 +1659,11 @@ protected function banLoadAdditionalIPsMember(int $member_id): array
 		);
 
 		while ($row = Db::$db->fetch_assoc($request)) {
-			$message_ips[] = new IP($row['poster_ip']);
+			$ip = new IP($row['poster_ip']);
+
+			if (!$ip->isRegisteredProxyServer()) {
+				$message_ips[] = (string) $ip;
+			}
 		}
 		Db::$db->free_result($request);
 
@@ -1688,7 +1692,11 @@ protected function banLoadAdditionalIPsError(int $member_id): array
 		);
 
 		while ($row = Db::$db->fetch_assoc($request)) {
-			$error_ips[] = new IP($row['ip']);
+			$ip = new IP($row['ip']);
+
+			if (!$ip->isRegisteredProxyServer()) {
+				$error_ips[] = (string) $ip;
+			}
 		}
 		Db::$db->free_result($request);
 

Sources/Actions/Admin/Server.php

@@ -1109,6 +1109,19 @@ public static function securityConfigVars(): array
 
 		IntegrationHook::call('integrate_general_security_settings', [&$config_vars]);
 
+		if (isset($_GET['save'])) {
+			// If we have saved the proxy_ip_servers, lets ensure that the list has valid entries
+			if (!empty($_POST['proxy_ip_servers'])) {
+				$proxy_server_ips = explode(',', $_POST['proxy_ip_servers']);
+
+				foreach ($proxy_server_ips as &$ip) {
+					$ip = trim($ip);
+				}
+
+				$_POST['proxy_ip_servers'] = implode(',', $proxy_server_ips);
+			}
+		}
+
 		return $config_vars;
 	}
 

Sources/IP.php

@@ -79,6 +79,13 @@ class IP implements \Stringable
 	 */
 	private static ?string $user_ip_alternative = null;
 
+	/**
+	 * @var string[]
+	 *
+	 * Container of all registered proxy servers.
+	 */
+	private static array $registered_proxy_servers = [];
+
 	/****************
 	 * Public methods
 	 ****************/
@@ -89,13 +96,19 @@ class IP implements \Stringable
 	 * If the passed string is not a valid IP address, it will be set to ''.
 	 *
 	 * @param ?string|self $ip The IP address in either string or binary form.
+	 * @param bool $clean Perform cleanups to the IP prior to parsing.
 	 */
-	public function __construct(self|string|null $ip)
+	public function __construct(self|string|null $ip, bool $clean = false)
 	{
 		if ($ip instanceof self) {
 			$ip = (string) $ip;
 		}
 
+		// When we are passing user provided input, we should perform cleanups to try and make sense of it.
+		if ($clean) {
+			$ip = $this->clean($ip);
+		}
+
 		// Is it in a valid IPv4 string?
 		if (filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4) !== false) {
 			$this->ip = $ip;
@@ -186,6 +199,56 @@ public function is4in6(): bool
 		return $this->isValid(FILTER_FLAG_IPV6) && (str_starts_with($this->ip, '::ffff:') || str_starts_with($this->ip, '64:ff9b::'));
 	}
 
+	/**
+	 * If ipv4 has been passed inside ipv6, using ::ffff:ipv4 format, pluck the ipv4 out of there.
+	 * We'd rather use the real ipv4 for display & for lookups, etc.
+	 * Consistently trim before usage. ipv6 is sometimes enclosed in square brackets (to clarify port vs ip).
+	 *
+	 * @return string $ip
+	 */
+	public function simplified(): string
+	{
+		return $this->is4in6() ? preg_replace('/^.*:(\d+\.\d+\.\d+\.\d+)$/', '$1', (string) $this->ip) : (string) $this->ip;
+	}
+
+	/**
+	 * Checks if an IP matches an expected localhost IP, for locally hosted proxies.
+	 * Note: FILTER_FLAG_GLOBAL_RANGE is used, see RFC6890 for more information.
+	 * IPv4: https://www.iana.org/assignments/iana-ipv4-special-registry
+	 * IPv6: https://www.iana.org/assignments/iana-ipv6-special-registry
+	 *
+	 * @return bool True if the IP is a private space IP per the specification
+	 */
+	public function isPrivate(): bool
+	{
+		return $this->isValid() && !$this->isValid(FILTER_FLAG_GLOBAL_RANGE);
+	}
+
+	/**
+	 * Checks if the IP is in our registered list of Proxy IP server
+	 *
+	 * @return bool True if found, false otherwise
+	 */
+	public function isRegisteredProxyServer(): bool
+	{
+		if (empty(Config::$modSettings['proxy_ip_servers'])) {
+			return false;
+		}
+
+		// We may call this function multiple times, lets hold onto the initialization logic.
+		if (empty(static::$registered_proxy_servers)) {
+			static::$registered_proxy_servers = array_map('trim', explode(',', Config::$modSettings['proxy_ip_servers']));
+		}
+
+		foreach (static::$registered_proxy_servers as $proxy) {
+			if ($proxy == $this->ip || $this->matchToCIDR($proxy)) {
+				return true;
+			}
+		}
+
+		return false;
+	}
+
 	/**
 	 * Tries to finds the name of the host that corresponds to this IP address.
 	 *
@@ -253,49 +316,51 @@ public function getHost(int $timeout = 1000): string
 	 */
 	public function matchToCIDR(string $cidr_address): bool
 	{
-		list($cidr_network, $cidr_subnetmask) = preg_split('~/~', $cidr_address);
-
-		$ip_address = $this->ip;
-
-		// v6?
-		if (str_contains($cidr_network, ':')) {
-			if (
-				!filter_var($ip_address, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6)
-				|| !filter_var($cidr_network, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6)
-			) {
-				return false;
-			}
+		// Validate the CIDR, skip if bogus
+		$addr_split = explode('/', $cidr_address);
+		$cidr_network = new self($addr_split[0]);
 
-			$ip_address = inet_pton($ip_address);
-			$cidr_network = inet_pton($cidr_network);
-			$bin_mask = str_repeat('f', (int) $cidr_subnetmask / 4);
+		if (!$cidr_network->isValid()) {
+			return false;
+		}
+		$cidr_network_packed = inet_pton((string) $cidr_network);
+		$ip_address_packed = inet_pton($this->ip);
 
-			switch ($cidr_subnetmask % 4) {
-				case 0:
-					break;
+		// Can't find an ipv4 in ipv6 CIDRs & vice-versa...
+		if (\strlen($cidr_network_packed) !== \strlen($ip_address_packed)) {
+			return false;
+		}
 
-				case 1:
-					$bin_mask .= '8';
-					break;
+		// Prefix must make sense if provided...
+		$bits = \strlen($cidr_network_packed) * 8;
+		$single_ip = !isset($addr_split[1]);
 
-				case 2:
-					$bin_mask .= 'c';
-					break;
+		if (!$single_ip) {
+			$cidr_subnetmask = (int) $addr_split[1];
 
-				case 3:
-					$bin_mask .= 'e';
-					break;
+			if (!is_numeric($addr_split[1]) || $cidr_subnetmask < 0 || $cidr_subnetmask > $bits) {
+				return false;
 			}
+		}
 
-			$bin_mask = str_pad($bin_mask, 32, '0');
-			$bin_mask = pack('H*', $bin_mask);
+		// Now build the range to check against...
+		if ($single_ip) {
+			$cidr_from = $cidr_network_packed;
+			$cidr_to = $cidr_network_packed;
 		} else {
-			$ip_address = ip2long($ip_address);
-			$cidr_network = ip2long($cidr_network);
-			$bin_mask = ~((1 << (32 - $cidr_subnetmask)) - 1);
+			$bin_mask = str_repeat('1', $cidr_subnetmask) . str_repeat('0', $bits - $cidr_subnetmask);
+			$bin_chunks = str_split($bin_mask, 8);
+			$chr_mask = '';
+
+			foreach ($bin_chunks as $chunk) {
+				$chr_mask .= \chr(bindec($chunk));
+			}
+			$cidr_from = $cidr_network_packed & $chr_mask;
+			$cidr_to = $cidr_network_packed | ~$chr_mask;
 		}
 
-		return ($ip_address & $bin_mask) == $cidr_network;
+		// strcmp is binary safe...
+		return ((strcmp($ip_address_packed, $cidr_from) > -1) && (strcmp($ip_address_packed, $cidr_to) < 1));
 	}
 
 	/***********************
@@ -526,6 +591,10 @@ public static function range2ip(string|IP $low, string|IP $high): string
 	/**
 	 * Determines the IP of the current user.
 	 *
+	 * Users can use SMF's proxy configs if desired.  Normally, it is suggested folks use
+	 * mod_remoteip, which straightens out proxy vs user IPs.  If mod_remoteip is not
+	 * available at your host, or doesn't meet your needs, this SMF function may help.
+	 *
 	 * What it does:
 	 * - Checks REMOTE_ADDR exists
 	 * - If proxy detection is enabled, parses headers to find a valid header.
@@ -545,14 +614,23 @@ public static function getUserIP(): string
 		}
 
 		$remote_addr = new self($_SERVER['REMOTE_ADDR']);
+		$valid_sender = false;
 
 		// If we haven't specified how to handle Reverse Proxy IP headers, lets do what we always used to do.
 		if (!isset(Config::$modSettings['proxy_ip_header'])) {
 			Config::$modSettings['proxy_ip_header'] = 'autodetect';
 		}
 
+		// Proxy config? Step 1: Check if IP passed is a valid server...
+		if (!empty(Config::$modSettings['proxy_ip_servers'])) {
+			$valid_sender = (new IP($_SERVER['REMOTE_ADDR']))->isRegisteredProxyServer();
+		} else {
+			// 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();
+		}
+
 		// Which headers are we going to check for Reverse Proxy IP headers?
-		if (Config::$modSettings['proxy_ip_header'] == 'disabled') {
+		if (!$valid_sender || Config::$modSettings['proxy_ip_header'] == 'disabled') {
 			$reverseIPheaders = [];
 		} elseif (Config::$modSettings['proxy_ip_header'] == 'autodetect') {
 			$reverseIPheaders = ['HTTP_X_FORWARDED_FOR', 'HTTP_CLIENT_IP', 'HTTP_X_REAL_IP', 'HTTP_CF_CONNECTING_IP'];
@@ -563,46 +641,28 @@ public static function getUserIP(): string
 		// Find the user's IP address. (but don't let it give you 'unknown'!)
 		foreach ($reverseIPheaders as $proxyIPheader) {
 			// Ignore if this is not set.
-			if (!isset($_SERVER[$proxyIPheader])) {
+			if (empty($_SERVER[$proxyIPheader])) {
 				continue;
 			}
 
-			if (!empty(Config::$modSettings['proxy_ip_servers'])) {
-				$valid_sender = false;
-
-				foreach (explode(',', Config::$modSettings['proxy_ip_servers']) as $proxy) {
-					if (
-						$proxy == $_SERVER['REMOTE_ADDR']
-						|| $remote_addr->matchToCIDR($proxy)
-					) {
-						$valid_sender = true;
-						break;
-					}
-				}
-
-				if (!$valid_sender) {
-					continue;
-				}
-			}
-
+			// Break out each IP in order from first to last, trim it and set as a reference to this object.
 			$ips = array_map(
 				fn($ip) => new self($ip),
 				array_map(
 					'trim',
-					array_reverse(explode(',', $_SERVER[$proxyIPheader])),
+					explode(',', $_SERVER[$proxyIPheader]),
 				),
 			);
 
+			// Check each IP is a valid public IP.
+			// It may be a comma separated list.  We are only interested in the first one, per RFC 7239..
 			foreach ($ips as $ip) {
 				if ($ip->isValid(FILTER_FLAG_GLOBAL_RANGE)) {
-					static::$user_ip = $ip->is4in6() ? preg_replace('/^.*:(\d+\.\d+\.\d+\.\d+)$/', '$1', (string) $ip) : (string) $ip;
+					// If this IP is a IPv4 stuffed into a IPv6, rip out the IPv4.
+					static::$user_ip = $ip->simplified();
+					break 2;
 				}
 			}
-
-			// If both IPs are private, then I guess that's all we've got.
-			if (!isset(static::$user_ip) && !$remote_addr->isValid(FILTER_FLAG_GLOBAL_RANGE)) {
-				static::$user_ip = $ips[0]->is4in6() ? preg_replace('/^.*:(\d+\.\d+\.\d+\.\d+)$/', '$1', (string) $ips[0]) : (string) $ips[0];
-			}
 		}
 
 		// If checking proxy headers gave us nothing, use the plain remote address.
@@ -788,4 +848,15 @@ protected function getReverseDnsQuery(): string
 
 		return $query;
 	}
+
+	/**
+	 * Performs cleanup logic on a string that may be a IP.
+	 *
+	 * @param string $raw A raw string which may contain a IP
+	 * @return string The cleaned value.
+	 */
+	private function clean($raw): string
+	{
+		return trim(trim($raw), '[]');
+	}
 }

Sources/Subs-Compat.php

@@ -8886,6 +8886,19 @@ function ShowXmlFeed(): void
 		SMF\Actions\Feed::call();
 	}
 
+	/**
+	 * If ipv4 has been passed inside ipv6, using ::ffff:ipv4 format, pluck the ipv4 out of there.
+	 * We'd rather use the real ipv4 for display & for lookups, etc.
+	 * Consistently trim before usage. ipv6 is sometimes enclosed in square brackets (to clarify port vs ip).
+	 *
+	 * @param string $ip
+	 * @return string $ip
+	 */
+	function simplify_ip($ip)
+	{
+		return (new SMF\IP($ip))->simplified();
+	}
+
 	/**
 	 * Locates the most appropriate temp directory.
 	 *
@@ -10277,6 +10290,18 @@ function utf8_strtoupper(string $string): string
 		return (string) SMF\Unicode\Utf8String::create($string)->convertCase('upper');
 	}
 
+	/**
+	 * Checks if an IP matches an expected localhost IP, for locally hosted proxies.
+	 * Note: FILTER_FLAG_GLOBAL_RANGE may be more helpful here (see RFC6890), but it's only supported in PHP 8.2+
+	 *
+	 * @param string $ip
+	 * @return bool
+	 */
+	function valid_localhost_ip($ip)
+	{
+		return (new SMF\IP($ip))->isPrivate();
+	}
+
 	/**
 	 * A wrapper for `filter_var($url, FILTER_VALIDATE_URL)` that can handle URLs
 	 * with international characters (a.k.a. IRIs)