Optimize session_begin REMOTE_ADDR/FORWARDED_FOR validation

[jasmineaura]

Per this, as of date: http://code.phpbb.com/projects/phpbb/re ... ession.php

2 issues in session_begin()'s handling of REMOTE_ADDR and HTTP_X_FORWARDED_FOR:
1. the regex done on both $this->ip and $this->forwarded_for is redundant and adds a considerable delay

Code: Select all

$this->ip = preg_replace('#[ ]{2,}#', ' ', str_replace(array(',', ' '), ' ', $this->ip)); 

passing an array to str_replace (as the search/pattern) is needless, as the 2nd array element is a space, and the replace parameter is a space, too. This adds a considerable delay to that line of code.

Consider the follow demo (phpbb's regex vs. modified regex):

Code: Select all


<?php

function show_ips($ip_ary, $start, $end)
{
    echo "<pre>\n";
    foreach ($ip_ary as $ip) echo $ip . "\n";
    echo "</pre>\n";

    $time = round(($end-$start)*1000,6);
    echo 'Took: '.$time.' ms<br /><br /><br />'."\n\n\n";
}

$ips = array(
    '1.2.3.4',
    '1.2.3.4 1.2.3.5',
    '1.2.3.4  1.2.3.5',
    '1.2.3.4   1.2.3.5',
    '1.2.3.4,1.2.3.5',
    '1.2.3.4, 1.2.3.5',
    '1.2.3.4 , 1.2.3.5',
    '1.2.3.4  ,  1.2.3.5  ,  1.2.3.6',
    "1.2.3.4,1.2.3.5, 1.2.3.6 1.2.3.7",
);

echo '<b>phpBB:</b><br />'."\n";

$s = microtime(true);
$ips_ary = preg_replace('#[ ]{2,}#', ' ', str_replace(array(',', ' '), ' ', $ips));
$e = microtime(true);
show_ips($ips_ary, $s, $e);

// Cleanup
unset($s, $e, $ips_ary);

echo '<b>MOD:</b><br />'."\n";

$s = microtime(true);
$ips_ary = preg_replace('# {2,}#', ' ', str_replace(',', ' ', $ips));
$e = microtime(true);
show_ips($ips_ary, $s, $e);

?>


Example output:

phpBB:

1.2.3.4
1.2.3.4 1.2.3.5
1.2.3.4 1.2.3.5
1.2.3.4 1.2.3.5
1.2.3.4 1.2.3.5
1.2.3.4 1.2.3.5
1.2.3.4 1.2.3.5
1.2.3.4 1.2.3.5 1.2.3.6
1.2.3.4 1.2.3.5 1.2.3.6 1.2.3.7

Took: 0.046015 ms


MOD:

1.2.3.4
1.2.3.4 1.2.3.5
1.2.3.4 1.2.3.5
1.2.3.4 1.2.3.5
1.2.3.4 1.2.3.5
1.2.3.4 1.2.3.5
1.2.3.4 1.2.3.5
1.2.3.4 1.2.3.5 1.2.3.6
1.2.3.4 1.2.3.5 1.2.3.6 1.2.3.7

Took: 0.015974 ms

2. Additional code, which is seldom needed, is executed every time. That is, REMOTE_ADDR rarely contains spaces or commas (and multiple addresses). This could be alleviated by using strpos in an if statement to check if $this->ip contains a comma or a space.

Consider the follow demo (emulating session_begin()'s current behavior vs. modified):

Code: Select all


<?php

define('IN_PHPBB', true);
$phpbb_root_path = 'phpbb3/';
$phpEx = substr(strrchr(__FILE__, '.'), 1);
include($phpbb_root_path . 'common.' . $phpEx);

$this_ip = '1.2.3.4';

echo '<b>phpBB:</b><br />'."\n";

$s = microtime(true);
$this_ip = preg_replace('#[ ]{2,}#', ' ', str_replace(array(',', ' '), ' ', $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;
}
$e = microtime(true);
$time = round(($e-$s)*1000,6);
echo '$this_ip = ' . $this_ip . '<br />'."\n";
echo 'Took: '.$time.' ms<br /><br /><br />'."\n\n\n";

// Cleanup
unset($s, $e, $time);

echo '<b>MOD:</b><br />'."\n";

$s = microtime(true);
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';
    }
}
$e = microtime(true);
$time = round(($e-$s)*1000,6);
echo '$this_ip = ' . $this_ip . '<br />'."\n";
echo 'Took: '.$time.' ms<br /><br /><br />'."\n\n\n";

?>


Example output:

phpBB:
$this_ip = 1.2.3.4
Took: 0.030994 ms


MOD:
$this_ip = 1.2.3.4
Took: 0.007868 ms

Hope that helps

[jasmineaura]

On a side note: I know I probably should have posted this on the tracker (AJIRA), but I have been unable to login - per this post

[jasmineaura]

Additional note, also on session_begin():

Code: Select all

$this->ip = (!empty($_SERVER['REMOTE_ADDR'])) ? htmlspecialchars((string) $_SERVER['REMOTE_ADDR']) : ''; 

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).

Same applies to:

Code: Select all

$this->forwarded_for        = (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) ? htmlspecialchars((string) $_SERVER['HTTP_X_FORWARDED_FOR']) : ''; 

and later:

Code: Select all

add_log('critical', 'LOG_IP_BROWSER_FORWARDED_CHECK', $u_ip, $s_ip, $u_browser, $s_browser, htmlspecialchars($u_forwarded_for), htmlspecialchars($s_forwarded_for)); 

(doubling up htmlspecialchars() on HTTP_X_FORWARDED_FOR)

[jasmineaura]

A logic issue as well:

Code: Select all

        // 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;
        } 

the way that foreach loop is structured, if $ip is empty, $this->ip will get set to an empty string. So I think it should rather be:

Code: Select all

    foreach ($ips as $ip)
    {
        if (empty($ip))
        {
            continue;
        }
        else if (!preg_match(get_preg_expression('ipv4'), $ip) && !preg_match(get_preg_expression('ipv6'), $ip))
        {
            break;
        }
        $user_ip = $ip;    // Use the last in chain
    }
 

I think that would make more sense..

Edit: even better, use trim() before exploding, and get rid of the empty() check in the loop

Code: Select all

        // 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
        } 

[jasmineaura]

Finally able to login to the tracker
Issues described above, as well as possible abuse of HTTP_X_FORWARDED_FOR header issue, reported here: http://tracker.phpbb.com/browse/PHPBB3-9802