Fix #17573: `Request::getUserIP()` security fix for the case when `Request::$trustedHost` and `Request::$ipHeaders` are used

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

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

framework/web/Request.php

@@ -291,6 +291,23 @@ class Request extends \yii\base\Request
      */
     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,9 +1142,12 @@ 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;
+                }
             }
         }
 
@@ -1141,6 +1155,50 @@ class Request extends \yii\base\Request
     }
 
     /**
+     * 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.
      * @return string|null user host name, null if not available

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"]