-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Description
Problem
Considering the current implementation:
Lines 99 to 125 in a31fd86
/** | |
* Returns the last IP address in a comma separated list, subject to an optional exclusion list. | |
* | |
* @param string $csv Comma separated list of elements. | |
* @param array $excludedIps Optional list of excluded IP addresses (or IP address ranges). | |
* @return string Last (non-excluded) IP address in the list or an empty string if all given IPs are excluded. | |
*/ | |
public static function getFirstIpFromList($csv, $excludedIps = null) | |
{ | |
$p = strrpos($csv, ','); | |
if ($p !== false) { | |
$elements = explode(',', $csv); | |
foreach ($elements as $ipString) { | |
$element = trim(Common::sanitizeInputValue($ipString)); | |
if(empty($element)) { | |
continue; | |
} | |
$ip = \Matomo\Network\IP::fromStringIP(IPUtils::sanitizeIp($element)); | |
if (empty($excludedIps) || (!in_array($element, $excludedIps) && !$ip->isInRanges($excludedIps))) { | |
return $element; | |
} | |
} | |
return ''; | |
} | |
return trim(Common::sanitizeInputValue($csv)); | |
} |
With the following setup:
[client] -> [reverse proxy 1] -> [reverse proxy 2] -> [matomo Nginx]
Since each proxy appends the IP of the incoming connection according to RFC7239, X-Forwarded-For
will be this by the time the connection reaches Nginx (1):
X-Forwarded-For: (client), (proxy 1)
Matomo currently extracts the first IP from the list, since #10404. This is correct until you realized the "client" can pretended to be a proxy as well, by including its own X-Forwarded-For
header, like this:
curl -i -H"X-Forwarded-For: 8.8.8.8" "http://example.com/matomo/matomo.php?..."
When this happens, proxy 1 will be faithfully preserve the header passed in and append the client IP after it (2):
X-Forwarded-For: 8.8.8.8, (client), (proxy 1)
And Matomo will extract 8.8.8.8
as the client IP.
Solutions
Solution 1: Trust the "transparent proxies"
If Matomo decided the first "client" IP can always be trusted, we can still extract the first IP address, and this issue can be closed as "works as intended." But it will in risk of having the analytics data being polluted. Brutal force login detection can be tricked too.
Solution 2: Extract the last IP address
This would involve reverting #10404 and go back to extract the last IP address. Since the original proxy_ips[]
configuration is still intact, when facing a header like (2), above, a correctly configured Matomo will be able to exclude the "proxy 1" IP address.
Notes
- There can be a solution 1.1 where Mamoto will rely on proxy 1 (the first public-facing proxy) to remove the spoofed header. However this is not always possible (not possible for Apache AFAIK)
- It is unclear to me what other client IP headers (
HTTP_CF_CONNECTING_IP
,HTTP_CLIENT_IP
, etc) behaves and if Matomo have to do things differently between these and theX-Forwarded-For
header. - IP::getNonProxyIpFromHeader retrieves final proxy instead of client #7060 is a staled discussion related to this, but the discussion talks about the header processing in the context before Wrong IP extracted from HTTP_X_FORWARDED_FOR when there is more than one #10342.
- Better documentation of Proxy setup #16379 have since promoted
proxy_ips[]
pretty well in the documentation. We may consider doubling down on the documentation effort when we pick either solution for this issue. - Lastly, one tiny nitpick: Extract the first IP from HTTP_X_FORWARDED_FOR and HTTP_CLIENT_IP and HTTP_CF_CONNECTING_IP and HTTP_X_FORWARDED_HOST when there is more than one IP #10404 updated the name of the function
IP::getFirstIpFromList
but it didn't change the word "last" in the function comment right above it :)
Thanks!