Hi,

on Environment class the function ip returns the $_SERVER['HTTP_X_FORWARDED_FOR'] if it's not empty
this is fine if the visitors are behind a proxy but it's less fine if the visitors are behind a local proxy

using the dlstats extension I've found many entries like 192.168.72.5, 192.168.0.6 etc. etc.

as stated here http://tools.ietf.org/html/rfc1918

Quote:

The Internet Assigned Numbers Authority (IANA) has reserved the
following three blocks of the IP address space for private internets:

10.0.0.0 - 10.255.255.255 (10/8 prefix)
172.16.0.0 - 172.31.255.255 (172.16/12 prefix)
192.168.0.0 - 192.168.255.255 (192.168/16 prefix)

so I've found a function from php manual user comment

Code:

# function ipCIDRCheck taken from:
# http://us.php.net/manual/en/ref.network.php#74656
function ipCIDRCheck ($IP, $CIDR) {
  list ($net, $mask) = split ("/", $CIDR);
  $ip_net = ip2long ($net);
  $ip_mask = ~((1 << (32 - $mask)) - 1);
  $ip_ip = ip2long ($IP);
  $ip_ip_net = $ip_ip & $ip_mask;
  return ($ip_ip_net == $ip_net);
}

starting from above I write the ipIsPrivate function

Code:

function ipIsPrivate($ip) {
   
    return     ipCIDRCheck($ip, '10.0.0.0/8')
            || ipCIDRCheck($ip, '172.16.0.0/12')
            || ipCIDRCheck($ip, '192.168.0.0/16');
}

to fix the issue, the code in Environment class (line 322)

Code:

$this->arrCache['ip'] = strlen($_SERVER['HTTP_X_FORWARDED_FOR']) ? $_SERVER['HTTP_X_FORWARDED_FOR'] : $_SERVER['REMOTE_ADDR'];

should be replaced with

Code:

            if ( strlen($_SERVER['HTTP_X_FORWARDED_FOR']) && !ipIsPrivate($_SERVER['HTTP_X_FORWARDED_FOR']) ) 
            {
                $this->arrCache['ip'] = $_SERVER['HTTP_X_FORWARDED_FOR'];
            } else {
                $this->arrCache['ip'] = $_SERVER['REMOTE_ADDR'];
            }

but I have no idea (and do not want to break Leo's design) where to put the two functions above

a quick and dirty solution is to replace the line 322 of Environment class

Code:

$this->arrCache['ip'] = strlen($_SERVER['HTTP_X_FORWARDED_FOR']) ? $_SERVER['HTTP_X_FORWARDED_FOR'] : $_SERVER['REMOTE_ADDR'];

with

Code:

            if ( strlen($_SERVER['HTTP_X_FORWARDED_FOR']) {
                # function ipCIDRCheck taken from: http://us.php.net/manual/en/ref.network.php#74656
                $ipCIDRCheck = create_function('$IP, $CIDR', 'list ($net, $mask) = split ("/", $CIDR);$ip_net = ip2long ($net);$ip_mask = ~((1 << (32 - $mask)) - 1);$ip_ip = ip2long ($IP);$ip_ip_net = $ip_ip & $ip_mask;return ($ip_ip_net == $ip_net);');
                $ip = $_SERVER['HTTP_X_FORWARDED_FOR'];
                $ipIsPrivate  = $ipCIDRCheck($ip, '10.0.0.0/8') || $ipCIDRCheck($ip, '172.16.0.0/12') || $ipCIDRCheck($ip, '192.168.0.0/16');
                if ($ipIsPrivate) {
                    $this->arrCache['ip'] = $_SERVER['REMOTE_ADDR'];
                } else {
                    $this->arrCache['ip'] = $_SERVER['HTTP_X_FORWARDED_FOR'];
                }
            } else {
                $this->arrCache['ip'] = $_SERVER['REMOTE_ADDR'];
            }

hope this helps

Well, as HTTP_FORWARDED_FOR can easily be faked (and not only useless because of local proxies), I'd prefer to just use REMOTE_ADDR or store both information instead of one of them.

FloB:

Well, as HTTP_FORWARDED_FOR can easily be faked (and not only useless because of local proxies), I'd prefer to just use REMOTE_ADDR or store both information instead of one of them.

I support the later, see also this thread.

The most reasonable would be to increase the ip table columns of TYPOlight as suggested in the thread above, and use both server vars. As HTTP_X_FORWARDED_FOR can allready be a comma separated list when multiple proxies are in the game, my code-snippet suggestion would be:

$this->arrCache['ip'] = rtrim( $_SERVER['REMOTE_ADDR'] . ', ' . $_SERVER['HTTP_X_FORWARDED_FOR'], ', ');

Quote:

acenes wrote:
my code-snippet suggestion would be:

$this->arrCache['ip'] = rtrim( $_SERVER['REMOTE_ADDR'] . ', ' . $_SERVER['HTTP_X_FORWARDED_FOR'], ', ');

I agreed (after read the above thread )

acenes:

As HTTP_X_FORWARDED_FOR can allready be a comma separated list when multiple proxies are in the game, …

Just want to come back to this issue.

So there are two reasons for storing multiple IPs: The header can contain multiple IPs and can easily be faked. What to do?