Details
-
Improvement
-
Status: Unverified Fix (View Workflow)
-
Major
-
Resolution: Fixed
-
3.0.7-PL1
-
None
-
PHP Version 5.2.13, MySQL(i) 5.0.77
Description
Detailed info (including demo code, results, and execution time tests) in this thread: http://area51.phpbb.com/phpBB/viewtopic.php?f=3&t=33891&p=213615#p213603
I'm setting priority to Major as I suspect that issue #5, with proper understanding, could be exploited in an attack (which might be combined with other vectors) to cause a Denial-of-Service, or at least resource starvation...
the regex done on both $this->ip and $this->forwarded_for is redundant and adds a considerable delay
$this->ip = preg_replace('#[ ]{2,}#', ' ', str_replace(array(',', ' '), ' ', $this->ip));
|
(Took: 0.046015 ms)
Modified regex:
$this->ip = preg_replace('# {2,}#', ' ', str_replace(',', ' ', $this->ip));
|
(Took: 0.015974 ms)
(And same results in all scenarios)
Additional code, which is seldom needed, is executed every time. That is, REMOTE_ADDR rarely contains spaces or commas (signifying multiple addresses), so this overhead could be avoided by using strpos in an if statement to first check if $this->ip contains a comma or a space.
Assuming REMOTE_ADDR contains standard ipv4 IP address, say '1.2.3.4':
Without strpos:
Took: 0.030994 ms
Modified:
Took: 0.007868 ms
if (strpos($this->ip, ',') !== FALSE || strpos($this->ip, ' ') !== FALSE)
|
{
|
$this->ip = preg_replace('# {2,}#', ' ', str_replace(',', ' ', $this->ip));
|
|
|
// split the list of IPs
|
$ips = explode(' ', $this->ip);
|
|
|
// Default IP if REMOTE_ADDR is invalid
|
$this->ip = '127.0.0.1';
|
|
|
foreach ($ips as $ip)
|
{
|
// check IPv4 first, the IPv6 is hopefully only going to be used very seldomly
|
if (!empty($ip) && !preg_match(get_preg_expression('ipv4'), $ip) && !preg_match(get_preg_expression('ipv6'), $ip))
|
{
|
// Just break
|
break;
|
}
|
|
|
// Use the last in chain
|
$this->ip = $ip;
|
}
|
}
|
else
|
{
|
if (empty($this->ip) || (!preg_match(get_preg_expression('ipv4'), $this->ip) && !preg_match(get_preg_expression('ipv6'), $this->ip)))
|
{
|
$this->ip = '127.0.0.1';
|
}
|
}
|
A logic issue in the foreach loop above: the way it is structured, if $ip is empty, $this->ip will get set to an empty string.
This could be avoided by checking:
if(empty($ip)) continue;
else if (!ipv4 && !ipv6) break;
or even better, use trim() before exploding, and get rid of the empty() check in the loop:
// split the list of IPs
|
$ips = explode(' ', trim($this->ip));
|
|
|
// Default IP if REMOTE_ADDR is invalid
|
$this->ip = '127.0.0.1';
|
|
|
foreach ($ips as $ip)
|
{
|
if (!preg_match(get_preg_expression('ipv4'), $ip) && !preg_match(get_preg_expression('ipv6'), $ip))
|
{
|
break;
|
}
|
$this->ip = $ip; // Use the last in chain
|
}
|
htmlspecialchars is unneeded, as $this->ip will be set to '127.0.0.1' if the IP does not validate as ipv4 or ipv6 addr, and neither can be valid and yet contain html special chars (ampersand, single/double quote, less/greater than).
$this->ip = (!empty($_SERVER['REMOTE_ADDR'])) ? htmlspecialchars((string) $_SERVER['REMOTE_ADDR']) : '';
|
same applies to $this->forwarded_for:
$this->forwarded_for = (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) ? htmlspecialchars((string) $_SERVER['HTTP_X_FORWARDED_FOR']) : '';
|
which is later double htmlspecialchar'ed:
add_log('critical', 'LOG_IP_BROWSER_FORWARDED_CHECK', $u_ip, $s_ip, $u_browser, $s_browser, htmlspecialchars($u_forwarded_for), htmlspecialchars($s_forwarded_for));
|
For 'forwarded_for_check', we should not loop through all the IPs in the HTTP_X_FORWARDED_FOR header (if there are more than one), because the possibly real address is the first IP in the $ips array ( $ips[0] ), the rest (if there are any) are most likely chained proxies, and we shouldn't really care about those. Matter of fact, we should really ignore them, to prevent possible abuse... Keep in mind that HTTP_X_FORWARDED_FOR header can be forged by the user-agent! (think DoS)..
So this modified snippet would make more sense (and less work):
// if the forwarded for header shall be checked we have to validate its contents
|
if ($config['forwarded_for_check'] && strpos($this->forwarded_for, ',') !== FALSE || strpos($this->forwarded_for, ' ') !== FALSE)
|
{
|
$this->forwarded_for = preg_replace('# {2,}#', ' ', str_replace(',', ' ', $this->forwarded_for));
|
|
|
// split the list of IPs
|
$ips = explode(' ', trim($this->forwarded_for));
|
|
|
// Possibly real address is the first IP in the array. That's all we need.
|
// The rest are most-likely chained proxy, and we should just ignore them,
|
// rather than loop through the list, to prevent possible abuse.
|
// (Yes, HTTP_X_FORWARDED_FOR header can be forged by the user-agent!)
|
if (!preg_match(get_preg_expression('ipv4'), $ips[0]) && !preg_match(get_preg_expression('ipv6'), $ips[0]))
|
{
|
// contains invalid data, don't use the forwarded for header
|
$this->forwarded_for = '';
|
}
|
else
|
{
|
$this->forwarded_for = $ips[0];
|
}
|
}
|
else
|
{
|
$this->forwarded_for = '';
|
}
|
Hope that helps..