Optimize session_begin REMOTE_ADDR/FORWARDED_FOR validation

Discussion of general topics related to the new version and its place in the world. Don't discuss new features, report bugs, ask for support, et cetera. Don't use this to spam for other boards or attack those boards!
Forum rules
Discussion of general topics related to the new release and its place in the world. Don't discuss new features, report bugs, ask for support, et cetera. Don't use this to spam for other boards or attack those boards!
Post Reply
jasmineaura
Registered User
Posts: 6
Joined: Tue Jun 29, 2010 6:38 pm

Optimize session_begin REMOTE_ADDR/FORWARDED_FOR validation

Post by 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
Registered User
Posts: 6
Joined: Tue Jun 29, 2010 6:38 pm

Re: Optimize session_begin REMOTE_ADDR/FORWARDED_FOR validat

Post by 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
Registered User
Posts: 6
Joined: Tue Jun 29, 2010 6:38 pm

Re: Optimize session_begin REMOTE_ADDR/FORWARDED_FOR validat

Post by 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
Registered User
Posts: 6
Joined: Tue Jun 29, 2010 6:38 pm

Re: Optimize session_begin REMOTE_ADDR/FORWARDED_FOR validat

Post by 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
Registered User
Posts: 6
Joined: Tue Jun 29, 2010 6:38 pm

Re: Optimize session_begin REMOTE_ADDR/FORWARDED_FOR validat

Post by 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

Post Reply