From ee6167879ecba11dea945f129c026ce0f3cf7514 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Thu, 17 Mar 2011 01:20:44 +0100 Subject: [PATCH 1/5] [ticket/9802] Fix redundant str_replace call. No need to replace ' ' with ' '. PHPBB3-9802 --- phpBB/includes/session.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/phpBB/includes/session.php b/phpBB/includes/session.php index d803f8d799..9ab53d38ab 100644 --- a/phpBB/includes/session.php +++ b/phpBB/includes/session.php @@ -221,7 +221,7 @@ class session // if the forwarded for header shall be checked we have to validate its contents if ($config['forwarded_for_check']) { - $this->forwarded_for = preg_replace('#[ ]{2,}#', ' ', str_replace(array(',', ' '), ' ', $this->forwarded_for)); + $this->forwarded_for = preg_replace('#[ ]{2,}#', ' ', str_replace(',', ' ', $this->forwarded_for)); // split the list of IPs $ips = explode(' ', $this->forwarded_for); @@ -268,7 +268,7 @@ class session // Why no forwarded_for et al? Well, too easily spoofed. With the results of my recent requests // it's pretty clear that in the majority of cases you'll at least be left with a proxy/cache ip. $this->ip = (!empty($_SERVER['REMOTE_ADDR'])) ? htmlspecialchars((string) $_SERVER['REMOTE_ADDR']) : ''; - $this->ip = preg_replace('#[ ]{2,}#', ' ', str_replace(array(',', ' '), ' ', $this->ip)); + $this->ip = preg_replace('#[ ]{2,}#', ' ', str_replace(',', ' ', $this->ip)); // split the list of IPs $ips = explode(' ', $this->ip); From fd805358592162cf05c8808caca0bdf788fb7088 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Thu, 17 Mar 2011 01:29:10 +0100 Subject: [PATCH 2/5] [ticket/9802] Remove redundant character class definition from preg_replace. PHPBB3-9802 --- phpBB/includes/session.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/phpBB/includes/session.php b/phpBB/includes/session.php index 9ab53d38ab..e1e315035b 100644 --- a/phpBB/includes/session.php +++ b/phpBB/includes/session.php @@ -221,7 +221,7 @@ class session // if the forwarded for header shall be checked we have to validate its contents if ($config['forwarded_for_check']) { - $this->forwarded_for = preg_replace('#[ ]{2,}#', ' ', str_replace(',', ' ', $this->forwarded_for)); + $this->forwarded_for = preg_replace('# {2,}#', ' ', str_replace(',', ' ', $this->forwarded_for)); // split the list of IPs $ips = explode(' ', $this->forwarded_for); @@ -268,7 +268,7 @@ class session // Why no forwarded_for et al? Well, too easily spoofed. With the results of my recent requests // it's pretty clear that in the majority of cases you'll at least be left with a proxy/cache ip. $this->ip = (!empty($_SERVER['REMOTE_ADDR'])) ? htmlspecialchars((string) $_SERVER['REMOTE_ADDR']) : ''; - $this->ip = preg_replace('#[ ]{2,}#', ' ', str_replace(',', ' ', $this->ip)); + $this->ip = preg_replace('# {2,}#', ' ', str_replace(',', ' ', $this->ip)); // split the list of IPs $ips = explode(' ', $this->ip); From bef2540d9ce3b429837c7e67c5f3f7f254aa1920 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Tue, 19 Apr 2011 13:46:00 +0200 Subject: [PATCH 3/5] [ticket/9802] Fix tiny logic bug in loop determining REMOTE_ADDR. When $ip is empty() it was assigned to $this->ip. PHPBB3-9802 --- phpBB/includes/session.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/phpBB/includes/session.php b/phpBB/includes/session.php index e1e315035b..f2aa47d84e 100644 --- a/phpBB/includes/session.php +++ b/phpBB/includes/session.php @@ -271,7 +271,7 @@ class session $this->ip = preg_replace('# {2,}#', ' ', str_replace(',', ' ', $this->ip)); // split the list of IPs - $ips = explode(' ', $this->ip); + $ips = explode(' ', trim($this->ip)); // Default IP if REMOTE_ADDR is invalid $this->ip = '127.0.0.1'; @@ -279,7 +279,7 @@ class session 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)) + if (!preg_match(get_preg_expression('ipv4'), $ip) && !preg_match(get_preg_expression('ipv6'), $ip)) { // Just break break; From 5ca7121ed2f698963387f5f9fb7ffe16d3781447 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Tue, 19 Apr 2011 13:53:09 +0200 Subject: [PATCH 4/5] [ticket/9802] Only check for IPv4-mapped address when address is IPv6. PHPBB3-9802 --- phpBB/includes/session.php | 39 +++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/phpBB/includes/session.php b/phpBB/includes/session.php index f2aa47d84e..b2772696f1 100644 --- a/phpBB/includes/session.php +++ b/phpBB/includes/session.php @@ -278,26 +278,31 @@ class session foreach ($ips as $ip) { - // check IPv4 first, the IPv6 is hopefully only going to be used very seldomly - if (!preg_match(get_preg_expression('ipv4'), $ip) && !preg_match(get_preg_expression('ipv6'), $ip)) + if (preg_match(get_preg_expression('ipv4'), $ip)) { - // Just break + $this->ip = $ip; + } + else if (preg_match(get_preg_expression('ipv6'), $ip)) + { + // Quick check for IPv4-mapped address in IPv6 + if (stripos($ip, '::ffff:') === 0) + { + $ipv4 = substr($ip, 7); + + if (preg_match(get_preg_expression('ipv4'), $ipv4)) + { + $ip = $ipv4; + } + } + + $this->ip = $ip; + } + else + { + // We want to use the last valid address in the chain + // Leave foreach loop when address is invalid break; } - - // Quick check for IPv4-mapped address in IPv6 - if (stripos($ip, '::ffff:') === 0) - { - $ipv4 = substr($ip, 7); - - if (preg_match(get_preg_expression('ipv4'), $ipv4)) - { - $ip = $ipv4; - } - } - - // Use the last in chain - $this->ip = $ip; } $this->load = false; From d1f1d8ade7ab98bde70451874c94bb35584f9192 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Tue, 19 Apr 2011 14:10:23 +0200 Subject: [PATCH 5/5] [ticket/9802] Remove unnecessary htmlspecialchars() call on REMOTE_ADDR. The value in $_SERVER['REMOTE_ADDR'] is either validated to be a valid IP address or is replaced by our default value. Valid IP addresses do not contain HTML special characters, thus the htmlspecialchars() call is unnecessary. PHPBB3-9802 --- phpBB/includes/session.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpBB/includes/session.php b/phpBB/includes/session.php index b2772696f1..79d94e7780 100644 --- a/phpBB/includes/session.php +++ b/phpBB/includes/session.php @@ -267,7 +267,7 @@ class session // Why no forwarded_for et al? Well, too easily spoofed. With the results of my recent requests // it's pretty clear that in the majority of cases you'll at least be left with a proxy/cache ip. - $this->ip = (!empty($_SERVER['REMOTE_ADDR'])) ? htmlspecialchars((string) $_SERVER['REMOTE_ADDR']) : ''; + $this->ip = (!empty($_SERVER['REMOTE_ADDR'])) ? (string) $_SERVER['REMOTE_ADDR'] : ''; $this->ip = preg_replace('# {2,}#', ' ', str_replace(',', ' ', $this->ip)); // split the list of IPs