From c87855b31c1dba91d0e3d9e7b4fe6dd8aabf5e6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Somogyi=20M=C3=A1rton?= Date: Thu, 3 Oct 2019 13:56:20 +0200 Subject: [PATCH] Fix #17573: `Request::getUserIP()` security fix for the case when `Request::$trustedHost` and `Request::$ipHeaders` are used --- framework/CHANGELOG.md | 2 +- framework/validators/IpValidator.php | 2 +- framework/web/Request.php | 76 ++++++++++++++++++++++++---- tests/framework/web/RequestTest.php | 32 ++++++++++++ 4 files changed, 101 insertions(+), 11 deletions(-) diff --git a/framework/CHANGELOG.md b/framework/CHANGELOG.md index f488c81e61..1bf0b70ede 100644 --- a/framework/CHANGELOG.md +++ b/framework/CHANGELOG.md @@ -4,7 +4,7 @@ Yii Framework 2 Change Log 2.0.28 under development ------------------------ -- no changes in this release. +- Bug #17573: `Request::getUserIP()` security fix for the case when `Request::$trustedHost` and `Request::$ipHeaders` are used (kamarton) 2.0.27 September 18, 2019 diff --git a/framework/validators/IpValidator.php b/framework/validators/IpValidator.php index d34c03bdc2..03d0c37f66 100644 --- a/framework/validators/IpValidator.php +++ b/framework/validators/IpValidator.php @@ -247,7 +247,7 @@ class IpValidator extends Validator * * @property array the IPv4 or IPv6 ranges that are allowed or forbidden. * See [[setRanges()]] for detailed description. - * @param array $ranges the IPv4 or IPv6 ranges that are allowed or forbidden. + * @param array|string $ranges the IPv4 or IPv6 ranges that are allowed or forbidden. * * When the array is empty, or the option not set, all IP addresses are allowed. * diff --git a/framework/web/Request.php b/framework/web/Request.php index 7dcc4c76b9..88a1a1271b 100644 --- a/framework/web/Request.php +++ b/framework/web/Request.php @@ -290,6 +290,23 @@ class Request extends \yii\base\Request * @since 2.0.13 */ protected function filterHeaders(HeaderCollection $headerCollection) + { + $trustedHeaders = $this->getTrustedIpHeaders(); + + // remove all secure headers unless they are trusted + foreach ($this->secureHeaders as $secureHeader) { + if (!in_array($secureHeader, $trustedHeaders)) { + $headerCollection->remove($secureHeader); + } + } + } + + /** + * Trusted Ip headers according to the [[trustedHosts]]. + * @return array + * @since 2.0.28 + */ + protected function getTrustedIpHeaders() { // do not trust any of the [[secureHeaders]] by default $trustedHeaders = []; @@ -310,13 +327,7 @@ class Request extends \yii\base\Request } } } - - // filter all secure headers unless they are trusted - foreach ($this->secureHeaders as $secureHeader) { - if (!in_array($secureHeader, $trustedHeaders)) { - $headerCollection->remove($secureHeader); - } - } + return $trustedHeaders; } /** @@ -1131,15 +1142,62 @@ class Request extends \yii\base\Request */ public function getUserIP() { - foreach ($this->ipHeaders as $ipHeader) { + foreach($this->getTrustedIpHeaders() as $ipHeader) { if ($this->headers->has($ipHeader)) { - return trim(explode(',', $this->headers->get($ipHeader))[0]); + $ip = $this->getUserIpFromIpHeader($this->headers->get($ipHeader)); + if ($ip !== null) { + return $ip; + } } } return $this->getRemoteIP(); } + /** + * Return user IP's from IP header. + * + * @param string $ips comma separated IP list + * @return string|null IP as string. Null is returned if IP can not be determined from header. + * @see $getUserHost + * @see $ipHeader + * @see $trustedHeaders + * @since 2.0.28 + */ + protected function getUserIpFromIpHeader($ips) + { + $ips = trim($ips); + if ($ips === '') { + return null; + } + $ips = preg_split('/\s*,\s*/', $ips, -1, PREG_SPLIT_NO_EMPTY); + krsort($ips); + $validator = $this->getIpValidator(); + $resultIp = null; + foreach ($ips as $ip) { + $validator->setRanges('any'); + if (!$validator->validate($ip) /* checking IP format */) { + break; + } + $resultIp = $ip; + $isTrusted = false; + foreach ($this->trustedHosts as $trustedCidr => $trustedCidrOrHeaders) { + if (!is_array($trustedCidrOrHeaders)) { + $trustedCidr = $trustedCidrOrHeaders; + } + $validator->setRanges($trustedCidr); + if ($validator->validate($ip) /* checking trusted range */) { + $isTrusted = true; + break; + } + } + if (!$isTrusted) { + break; + } + } + return $resultIp; + } + /** * Returns the user host name. * The HOST is determined using headers and / or `$_SERVER` variables. diff --git a/tests/framework/web/RequestTest.php b/tests/framework/web/RequestTest.php index 33877f55e6..3082b9544f 100644 --- a/tests/framework/web/RequestTest.php +++ b/tests/framework/web/RequestTest.php @@ -725,6 +725,38 @@ class RequestTest extends TestCase $this->assertSame('default', $request->getBodyParam('unexisting', 'default')); } + public function trustedHostAndInjectedXForwardedForDataProvider() + { + return [ + 'emptyIPs' => ['1.1.1.1', '', ['10.10.10.10'], '1.1.1.1'], + 'invalidIp' => ['1.1.1.1', '127.0.0.1, 8.8.8.8, 2.2.2.2, apple', ['10.10.10.10'], '1.1.1.1'], + 'invalidIp2' => ['1.1.1.1', '127.0.0.1, 8.8.8.8, 2.2.2.2, 300.300.300.300', ['10.10.10.10'], '1.1.1.1'], + 'invalidIp3' => ['1.1.1.1', '127.0.0.1, 8.8.8.8, 2.2.2.2, 10.0.0.0/26', ['10.0.0.0/24'], '1.1.1.1'], + 'invalidLatestIp' => ['1.1.1.1', '127.0.0.1, 8.8.8.8, 2.2.2.2, apple, 2.2.2.2', ['1.1.1.1', '2.2.2.2'], '2.2.2.2'], + 'notTrusted' => ['1.1.1.1', '127.0.0.1, 8.8.8.8, 2.2.2.2', ['10.10.10.10'], '1.1.1.1'], + 'trustedLevel1' => ['1.1.1.1', '127.0.0.1, 8.8.8.8, 2.2.2.2', ['1.1.1.1'], '2.2.2.2'], + 'trustedLevel2' => ['1.1.1.1', '127.0.0.1, 8.8.8.8, 2.2.2.2', ['1.1.1.1', '2.2.2.2'], '8.8.8.8'], + 'trustedLevel3' => ['1.1.1.1', '127.0.0.1, 8.8.8.8, 2.2.2.2', ['1.1.1.1', '2.2.2.2', '8.8.8.8'], '127.0.0.1'], + 'trustedLevel4' => ['1.1.1.1', '127.0.0.1, 8.8.8.8, 2.2.2.2', ['1.1.1.1', '2.2.2.2', '8.8.8.8', '127.0.0.1'], '127.0.0.1'], + 'trustedLevel4EmptyElements' => ['1.1.1.1', '127.0.0.1, 8.8.8.8,,,, , , 2.2.2.2', ['1.1.1.1', '2.2.2.2', '8.8.8.8', '127.0.0.1'], '127.0.0.1'], + 'trustedWithCidr' => ['10.0.0.2', '127.0.0.1, 8.8.8.8, 10.0.0.240, 10.0.0.32, 10.0.0.99', ['10.0.0.0/24'], '8.8.8.8'], + 'trustedAll' => ['10.0.0.2', '127.0.0.1, 8.8.8.8, 10.0.0.240, 10.0.0.32, 10.0.0.99', ['0.0.0.0/0'], '127.0.0.1'], + ]; + } + + /** + * @dataProvider trustedHostAndInjectedXForwardedForDataProvider + */ + public function testTrustedHostAndInjectedXForwardedFor($remoteAddress, $xForwardedFor, $trustedHosts, $expectedUserIp) + { + $_SERVER['REMOTE_ADDR'] = $remoteAddress; + $_SERVER['HTTP_X_FORWARDED_FOR'] = $xForwardedFor; + $request = new Request([ + 'trustedHosts' => $trustedHosts, + ]); + $this->assertSame($expectedUserIp, $request->getUserIP()); + } + /** * @testWith ["POST", "GET", "POST"] * ["POST", "OPTIONS", "POST"]