From ddc9e3892fe09b3a9ca2bb1fb14c565489c3d0cf Mon Sep 17 00:00:00 2001 From: Dmitrii Chekaliuk Date: Fri, 8 Mar 2013 22:49:38 +0200 Subject: [PATCH 1/5] Modify Request::getClientIp() to use IpUtils::checkIp() Adds the ability to use CIDR notation in the trusted proxy list --- src/Symfony/Component/HttpFoundation/Request.php | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/Symfony/Component/HttpFoundation/Request.php b/src/Symfony/Component/HttpFoundation/Request.php index 2040c8945a..d2e1597d9f 100644 --- a/src/Symfony/Component/HttpFoundation/Request.php +++ b/src/Symfony/Component/HttpFoundation/Request.php @@ -688,7 +688,16 @@ class Request $trustedProxies = !self::$trustedProxies ? array($ip) : self::$trustedProxies; $ip = $clientIps[0]; - $clientIps = array_values(array_diff($clientIps, $trustedProxies)); + + foreach ($clientIps as $key => $clientIp) { + foreach ($trustedProxies as $trustedProxy) { + if (IpUtils::checkIp($clientIp, $trustedProxy)) { + unset($clientIps[$key]); + + continue 2; + } + } + } return $clientIps ? array_reverse($clientIps) : array($ip); } From 811434f988a8af34485b3b2ebcbcb3a40f9579c6 Mon Sep 17 00:00:00 2001 From: Dmitrii Chekaliuk Date: Fri, 8 Mar 2013 22:55:36 +0200 Subject: [PATCH 2/5] Allow setting trusted_proxies using CIDR notation --- .../DependencyInjection/Configuration.php | 16 +++++++++++++++- .../DependencyInjection/ConfigurationTest.php | 2 ++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php index 2a376b24cf..d75107355a 100644 --- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php +++ b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php @@ -46,7 +46,21 @@ class Configuration implements ConfigurationInterface ->end() ->prototype('scalar') ->validate() - ->ifTrue(function($v) { return !empty($v) && !filter_var($v, FILTER_VALIDATE_IP); }) + ->ifTrue(function($v) { + if (empty($v)) { + return false; + } + + if (false !== strpos($v, '/')) { + list($v, $mask) = explode('/', $v, 2); + + if (strcmp($mask, (int) $mask) || $mask < 1 || $mask > (false !== strpos($v, ':') ? 128 : 32)) { + return true; + } + } + + return !filter_var($v, FILTER_VALIDATE_IP); + }) ->thenInvalid('Invalid proxy IP "%s"') ->end() ->end() diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php index c5582f3934..625b0229ff 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php +++ b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php @@ -51,6 +51,8 @@ class ConfigurationTest extends \PHPUnit_Framework_TestCase array(null, array()), array(false, array()), array(array(), array()), + array(array('10.0.0.0/8'), array('10.0.0.0/8')), + array(array('::ffff:0:0/96'), array('::ffff:0:0/96')), ); } From 169506743b97d23565dc54bf77174588bdeaad5c Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Sat, 20 Apr 2013 17:34:19 +0200 Subject: [PATCH 3/5] [HttpFoundation] added some unit tests for ranges of trusted IP addresses --- src/Symfony/Component/HttpFoundation/Tests/RequestTest.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php b/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php index 9d4461b1e9..18661ae513 100644 --- a/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php +++ b/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php @@ -781,11 +781,15 @@ class RequestTest extends \PHPUnit_Framework_TestCase array(array('88.88.88.88'), '127.0.0.1', '88.88.88.88', array('127.0.0.1')), // forwarded for with remote IPv4 and all FF addrs trusted array(array('88.88.88.88'), '127.0.0.1', '88.88.88.88', array('127.0.0.1', '88.88.88.88')), + // forwarded for with remote IPv4 range trusted + array(array('88.88.88.88'), '123.45.67.89', '88.88.88.88', array('123.45.67.0/24')), // forwarded for with remote IPv6 addr not trusted array(array('1620:0:1cfe:face:b00c::3'), '1620:0:1cfe:face:b00c::3', '2620:0:1cfe:face:b00c::3', null), // forwarded for with remote IPv6 addr trusted array(array('2620:0:1cfe:face:b00c::3'), '1620:0:1cfe:face:b00c::3', '2620:0:1cfe:face:b00c::3', array('1620:0:1cfe:face:b00c::3')), + // forwarded for with remote IPv6 range trusted + array(array('88.88.88.88'), '2a01:198:603:0:396e:4789:8e99:890f', '88.88.88.88', array('2a01:198:603:0::/65')), // multiple forwarded for with remote IPv4 addr trusted array(array('88.88.88.88', '87.65.43.21', '127.0.0.1'), '123.45.67.89', '127.0.0.1, 87.65.43.21, 88.88.88.88', array('123.45.67.89')), From e7c1696278202c394aac6ea21b17c7532bf43a24 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Sat, 20 Apr 2013 17:42:24 +0200 Subject: [PATCH 4/5] [HttpFoundation] refactored code to avoid code duplication --- .../Component/HttpFoundation/IpUtils.php | 20 +++++++++++++------ .../Component/HttpFoundation/Request.php | 8 +++----- .../HttpFoundation/RequestMatcher.php | 6 ++---- .../HttpFoundation/Tests/IpUtilsTest.php | 6 ++++++ .../EventListener/FragmentListener.php | 6 ++---- 5 files changed, 27 insertions(+), 19 deletions(-) diff --git a/src/Symfony/Component/HttpFoundation/IpUtils.php b/src/Symfony/Component/HttpFoundation/IpUtils.php index 2e3e1aa746..7c3742e768 100644 --- a/src/Symfony/Component/HttpFoundation/IpUtils.php +++ b/src/Symfony/Component/HttpFoundation/IpUtils.php @@ -26,18 +26,26 @@ class IpUtils /** * Validates an IPv4 or IPv6 address. * - * @param string $requestIp - * @param string $ip + * @param string $requestIp + * @param string|array $ips * * @return boolean Whether the IP is valid */ - public static function checkIp($requestIp, $ip) + public static function checkIp($requestIp, $ips) { - if (false !== strpos($requestIp, ':')) { - return self::checkIp6($requestIp, $ip); + if (!is_array($ips)) { + $ips = array($ips); } - return self::checkIp4($requestIp, $ip); + $method = false !== strpos($requestIp, ':') ? 'checkIp6': 'checkIp4'; + + foreach ($ips as $ip) { + if (self::$method($requestIp, $ip)) { + return true; + } + } + + return false; } /** diff --git a/src/Symfony/Component/HttpFoundation/Request.php b/src/Symfony/Component/HttpFoundation/Request.php index d2e1597d9f..159366d1ec 100644 --- a/src/Symfony/Component/HttpFoundation/Request.php +++ b/src/Symfony/Component/HttpFoundation/Request.php @@ -690,12 +690,10 @@ class Request $ip = $clientIps[0]; foreach ($clientIps as $key => $clientIp) { - foreach ($trustedProxies as $trustedProxy) { - if (IpUtils::checkIp($clientIp, $trustedProxy)) { - unset($clientIps[$key]); + if (IpUtils::checkIp($clientIp, $trustedProxies)) { + unset($clientIps[$key]); - continue 2; - } + continue; } } diff --git a/src/Symfony/Component/HttpFoundation/RequestMatcher.php b/src/Symfony/Component/HttpFoundation/RequestMatcher.php index cdfacf2958..a3d52d6337 100644 --- a/src/Symfony/Component/HttpFoundation/RequestMatcher.php +++ b/src/Symfony/Component/HttpFoundation/RequestMatcher.php @@ -153,10 +153,8 @@ class RequestMatcher implements RequestMatcherInterface return false; } - foreach ($this->ips as $ip) { - if (IpUtils::checkIp($request->getClientIp(), $ip)) { - return true; - } + if (IpUtils::checkIp($request->getClientIp(), $this->ips)) { + return true; } // Note to future implementors: add additional checks above the diff --git a/src/Symfony/Component/HttpFoundation/Tests/IpUtilsTest.php b/src/Symfony/Component/HttpFoundation/Tests/IpUtilsTest.php index 3aef49eb04..726ba6a347 100644 --- a/src/Symfony/Component/HttpFoundation/Tests/IpUtilsTest.php +++ b/src/Symfony/Component/HttpFoundation/Tests/IpUtilsTest.php @@ -31,6 +31,9 @@ class IpUtilsTest extends \PHPUnit_Framework_TestCase array(true, '192.168.1.1', '192.168.1.0/24'), array(false, '192.168.1.1', '1.2.3.4/1'), array(false, '192.168.1.1', '192.168.1/33'), + array(true, '192.168.1.1', array('1.2.3.4/1', '192.168.1.0/24')), + array(true, '192.168.1.1', array('192.168.1.0/24', '1.2.3.4/1')), + array(false, '192.168.1.1', array('1.2.3.4/1', '4.3.2.1/1')), ); } @@ -54,6 +57,9 @@ class IpUtilsTest extends \PHPUnit_Framework_TestCase array(false, '2a01:198:603:0:396e:4789:8e99:890f', '::1'), array(true, '0:0:0:0:0:0:0:1', '::1'), array(false, '0:0:603:0:396e:4789:8e99:0001', '::1'), + array(true, '2a01:198:603:0:396e:4789:8e99:890f', array('::1', '2a01:198:603:0::/65')), + array(true, '2a01:198:603:0:396e:4789:8e99:890f', array('2a01:198:603:0::/65', '::1')), + array(false, '2a01:198:603:0:396e:4789:8e99:890f', array('::1', '1a01:198:603:0::/65')), ); } diff --git a/src/Symfony/Component/HttpKernel/EventListener/FragmentListener.php b/src/Symfony/Component/HttpKernel/EventListener/FragmentListener.php index 1a4d83b769..ef3fad3d4c 100644 --- a/src/Symfony/Component/HttpKernel/EventListener/FragmentListener.php +++ b/src/Symfony/Component/HttpKernel/EventListener/FragmentListener.php @@ -80,10 +80,8 @@ class FragmentListener implements EventSubscriberInterface // does the Request come from a trusted IP? $trustedIps = array_merge($this->getLocalIpAddresses(), $request->getTrustedProxies()); $remoteAddress = $request->server->get('REMOTE_ADDR'); - foreach ($trustedIps as $ip) { - if (IpUtils::checkIp($remoteAddress, $ip)) { - return; - } + if (IpUtils::checkIp($remoteAddress, $trustedIps)) { + return; } // is the Request signed? From 7b32794b792180593f92d71675159f88a13652ec Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Sat, 20 Apr 2013 17:52:13 +0200 Subject: [PATCH 5/5] [HttpFoundation] updated CHANGELOG --- src/Symfony/Component/HttpFoundation/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Symfony/Component/HttpFoundation/CHANGELOG.md b/src/Symfony/Component/HttpFoundation/CHANGELOG.md index 597b65b3f6..24a55da766 100644 --- a/src/Symfony/Component/HttpFoundation/CHANGELOG.md +++ b/src/Symfony/Component/HttpFoundation/CHANGELOG.md @@ -2,7 +2,9 @@ CHANGELOG ========= 2.3.0 +----- + * added support for ranges of IPs in trusted proxies * `UploadedFile::isValid` now returns false if the file was not uploaded via HTTP (in a non-test mode) 2.2.0