symfony · magnusnordlander · May 2, 2016 · May 3, 2016 · May 5, 2016 · May 5, 2016
diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml b/src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml
@@ -46,5 +46,9 @@
            <argument type="service" id="request_stack" />
            <tag name="kernel.event_subscriber" />
        </service>
+
+        <service id="validate_request_client_ip_listener" class="Symfony\Component\HttpKernel\EventListener\ValidateRequestClientIpListener">
+            <tag name="kernel.event_subscriber" />
+        </service>
    </services>
 </container>
diff --git a/src/Symfony/Component/HttpFoundation/Exception/ConflictingHeadersException.php b/src/Symfony/Component/HttpFoundation/Exception/ConflictingHeadersException.php
@@ -0,0 +1,23 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpFoundation\Exception;
+
+/**
+ * The HTTP request contains headers with conflicting information.
+ *
+ * This exception should trigger an HTTP 400 response in your application code.
+ *
+ * @author Magnus Nordlander <magnus@fervo.se>
+ */
+class ConflictingHeadersException extends \RuntimeException
+{
+}
diff --git a/src/Symfony/Component/HttpFoundation/Request.php b/src/Symfony/Component/HttpFoundation/Request.php
@@ -11,6 +11,7 @@

 namespace Symfony\Component\HttpFoundation;

+use Symfony\Component\HttpFoundation\Exception\ConflictingHeadersException;
 use Symfony\Component\HttpFoundation\Session\SessionInterface;

 /**
@@ -805,41 +806,34 @@ public function getClientIps()
            return array($ip);
        }

-        if (self::$trustedHeaders[self::HEADER_FORWARDED] && $this->headers->has(self::$trustedHeaders[self::HEADER_FORWARDED])) {
+        $hasTrustedForwardedHeader = self::$trustedHeaders[self::HEADER_FORWARDED] && $this->headers->has(self::$trustedHeaders[self::HEADER_FORWARDED]);
+        $hasTrustedClientIpHeader = self::$trustedHeaders[self::HEADER_CLIENT_IP] && $this->headers->has(self::$trustedHeaders[self::HEADER_CLIENT_IP]);
+
+        if ($hasTrustedForwardedHeader) {
            $forwardedHeader = $this->headers->get(self::$trustedHeaders[self::HEADER_FORWARDED]);
            preg_match_all('{(for)=("?\[?)([a-z0-9\.:_\-/]*)}', $forwardedHeader, $matches);
-            $clientIps = $matches[3];
-        } elseif (self::$trustedHeaders[self::HEADER_CLIENT_IP] && $this->headers->has(self::$trustedHeaders[self::HEADER_CLIENT_IP])) {
-            $clientIps = array_map('trim', explode(',', $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_IP])));
-        }
+            $forwardedClientIps = $matches[3];

-        $clientIps[] = $ip; // Complete the IP chain with the IP the request actually came from
-        $firstTrustedIp = null;
-
-        foreach ($clientIps as $key => $clientIp) {
-            // Remove port (unfortunately, it does happen)
-            if (preg_match('{((?:\d+\.){3}\d+)\:\d+}', $clientIp, $match)) {
-                $clientIps[$key] = $clientIp = $match[1];
-            }
+            $forwardedClientIps = $this->normalizeAndFilterClientIps($forwardedClientIps, $ip);
+            $clientIps = $forwardedClientIps;
+        }

-            if (!filter_var($clientIp, FILTER_VALIDATE_IP)) {
-                unset($clientIps[$key]);
+        if ($hasTrustedClientIpHeader) {
+            $xForwardedForClientIps = array_map('trim', explode(',', $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_IP])));

-                continue;
-            }
+            $xForwardedForClientIps = $this->normalizeAndFilterClientIps($xForwardedForClientIps, $ip);
+            $clientIps = $xForwardedForClientIps;
+        }

-            if (IpUtils::checkIp($clientIp, self::$trustedProxies)) {
-                unset($clientIps[$key]);
+        if ($hasTrustedForwardedHeader && $hasTrustedClientIpHeader && $forwardedClientIps !== $xForwardedForClientIps) {
+            throw new ConflictingHeadersException('The request has both a trusted Forwarded header and a trusted Client IP header, conflicting with each other with regards to the originating IP addresses of the request. This is the result of a misconfiguration. You should either configure your proxy only to send one of these headers, or configure Symfony to distrust one of them.');
+        }

-                // Fallback to this when the client IP falls into the range of trusted proxies
-                if (null ===  $firstTrustedIp) {
-                    $firstTrustedIp = $clientIp;
-                }
-            }
+        if (!$hasTrustedForwardedHeader && !$hasTrustedClientIpHeader) {
+            return $this->normalizeAndFilterClientIps(array(), $ip);
        }

-        // Now the IP chain contains only untrusted proxies and the client IP
-        return $clientIps ? array_reverse($clientIps) : array($firstTrustedIp);
+        return $clientIps;
    }

    /**
@@ -1930,4 +1924,35 @@ private function isFromTrustedProxy()
    {
        return self::$trustedProxies && IpUtils::checkIp($this->server->get('REMOTE_ADDR'), self::$trustedProxies);
    }
+
+    private function normalizeAndFilterClientIps(array $clientIps, $ip)
+    {
+        $clientIps[] = $ip; // Complete the IP chain with the IP the request actually came from
+        $firstTrustedIp = null;
+
+        foreach ($clientIps as $key => $clientIp) {
+            // Remove port (unfortunately, it does happen)
+            if (preg_match('{((?:\d+\.){3}\d+)\:\d+}', $clientIp, $match)) {
+                $clientIps[$key] = $clientIp = $match[1];
+            }
+
+            if (!filter_var($clientIp, FILTER_VALIDATE_IP)) {
+                unset($clientIps[$key]);
+
+                continue;
+            }
+
+            if (IpUtils::checkIp($clientIp, self::$trustedProxies)) {
+                unset($clientIps[$key]);
+
+                // Fallback to this when the client IP falls into the range of trusted proxies
+                if (null === $firstTrustedIp) {
+                    $firstTrustedIp = $clientIp;
+                }
+            }
+        }
+
+        // Now the IP chain contains only untrusted proxies and the client IP
+        return $clientIps ? array_reverse($clientIps) : array($firstTrustedIp);
+    }
 }
diff --git a/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php b/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php
@@ -923,6 +923,74 @@ public function testGetClientIpsProvider()
        );
    }

+    /**
+     * @expectedException \Symfony\Component\HttpFoundation\Exception\ConflictingHeadersException
+     * @dataProvider testGetClientIpsWithConflictingHeadersProvider
+     */
+    public function testGetClientIpsWithConflictingHeaders($httpForwarded, $httpXForwardedFor)
+    {
+        $request = new Request();
+
+        $server = array(
+            'REMOTE_ADDR' => '88.88.88.88',
+            'HTTP_FORWARDED' => $httpForwarded,
+            'HTTP_X_FORWARDED_FOR' => $httpXForwardedFor,
+        );
+
+        Request::setTrustedProxies(array('88.88.88.88'));
+
+        $request->initialize(array(), array(), array(), array(), array(), $server);
+
+        $request->getClientIps();
+    }
+
+    public function testGetClientIpsWithConflictingHeadersProvider()
+    {
+        //        $httpForwarded                   $httpXForwardedFor
+        return array(
+            array('for=87.65.43.21',                 '192.0.2.60'),
+            array('for=87.65.43.21, for=192.0.2.60', '192.0.2.60'),
+            array('for=192.0.2.60',                  '192.0.2.60,87.65.43.21'),
+            array('for="::face", for=192.0.2.60',    '192.0.2.60,192.0.2.43'),
+            array('for=87.65.43.21, for=192.0.2.60', '192.0.2.60,87.65.43.21'),
+        );
+    }
+
+    /**
+     * @dataProvider testGetClientIpsWithAgreeingHeadersProvider
+     */
+    public function testGetClientIpsWithAgreeingHeaders($httpForwarded, $httpXForwardedFor)
+    {
+        $request = new Request();
+
+        $server = array(
+            'REMOTE_ADDR' => '88.88.88.88',
+            'HTTP_FORWARDED' => $httpForwarded,
+            'HTTP_X_FORWARDED_FOR' => $httpXForwardedFor,
+        );
+
+        Request::setTrustedProxies(array('88.88.88.88'));
+
+        $request->initialize(array(), array(), array(), array(), array(), $server);
+
+        $request->getClientIps();
+
+        Request::setTrustedProxies(array());
+    }
+
+    public function testGetClientIpsWithAgreeingHeadersProvider()
+    {
+        //        $httpForwarded                               $httpXForwardedFor
+        return array(
+            array('for="192.0.2.60"',                          '192.0.2.60'),
+            array('for=192.0.2.60, for=87.65.43.21',           '192.0.2.60,87.65.43.21'),
+            array('for="[::face]", for=192.0.2.60',            '::face,192.0.2.60'),
+            array('for="192.0.2.60:80"',                       '192.0.2.60'),
+            array('for=192.0.2.60;proto=http;by=203.0.113.43', '192.0.2.60'),
+            array('for="[2001:db8:cafe::17]:4711"',            '2001:db8:cafe::17'),
+        );
+    }
+
    public function testGetContentWorksTwiceInDefaultMode()
    {
        $req = new Request();

diff --git a/src/Symfony/Component/HttpKernel/EventListener/ValidateRequestClientIpListener.php b/src/Symfony/Component/HttpKernel/EventListener/ValidateRequestClientIpListener.php
@@ -0,0 +1,57 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpKernel\EventListener;
+
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Symfony\Component\HttpFoundation\Exception\ConflictingHeadersException;
+use Symfony\Component\HttpKernel\Event\GetResponseEvent;
+use Symfony\Component\HttpKernel\Exception\HttpException;
+use Symfony\Component\HttpKernel\HttpKernelInterface;
+use Symfony\Component\HttpKernel\KernelEvents;
+
+/**
+ * Validates that the headers and other information indicating the
+ * client IP address of a request are consistent.
+ *
+ * @author Magnus Nordlander <magnus@fervo.se>
+ */
+class ValidateRequestClientIpListener implements EventSubscriberInterface
+{
+    /**
+     * Performs the validation.
+     *
+     * @param GetResponseEvent $event
+     */
+    public function onKernelRequest(GetResponseEvent $event)
+    {
+        if ($event->getRequestType() == HttpKernelInterface::MASTER_REQUEST) {
+            try {
+                // This will throw an exception if the headers are inconsistent.
+                $event->getRequest()->getClientIps();
+            } catch (ConflictingHeadersException $e) {
+                throw new HttpException(400, 'The request headers contain conflicting information regarding the origin of this request.', $e);
+            }
+        }
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public static function getSubscribedEvents()
+    {
+        return array(
+            KernelEvents::REQUEST => array(
+                array('onKernelRequest', 256),
+            ),
+        );
+    }
+}
diff --git a/src/Symfony/Component/HttpKernel/Profiler/Profiler.php b/src/Symfony/Component/HttpKernel/Profiler/Profiler.php
@@ -11,6 +11,7 @@

 namespace Symfony\Component\HttpKernel\Profiler;

+use Symfony\Component\HttpFoundation\Exception\ConflictingHeadersException;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\HttpKernel\DataCollector\DataCollectorInterface;
@@ -200,9 +201,13 @@ public function collect(Request $request, Response $response, \Exception $except
        $profile = new Profile(substr(hash('sha256', uniqid(mt_rand(), true)), 0, 6));
        $profile->setTime(time());
        $profile->setUrl($request->getUri());
-        $profile->setIp($request->getClientIp());
        $profile->setMethod($request->getMethod());
        $profile->setStatusCode($response->getStatusCode());
+        try {
+            $profile->setIp($request->getClientIp());
+        } catch (ConflictingHeadersException $e) {
+            $profile->setIp('Unknown');
+        }

        $response->headers->set('X-Debug-Token', $profile->getToken());


diff --git a/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestClientIpTest.php b/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestClientIpTest.php
@@ -0,0 +1,67 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpKernel\Tests\EventListener;
+
+use Symfony\Component\EventDispatcher\EventDispatcher;
+use Symfony\Component\HttpFoundation\Exception\ConflictingHeadersException;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpKernel\EventListener\ValidateRequestClientIpListener;
+use Symfony\Component\HttpKernel\Event\GetResponseEvent;
+use Symfony\Component\HttpKernel\HttpKernelInterface;
+use Symfony\Component\HttpKernel\KernelEvents;
+
+class ValidateRequestClientIpTest extends \PHPUnit_Framework_TestCase
+{
+    public function testListenerThrowsOnInconsistentMasterRequests()
+    {
+        $dispatcher = new EventDispatcher();
+        $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface');
+        $listener = new ValidateRequestClientIpListener();
+        $request = $this->getMock('Symfony\Component\HttpFoundation\Request');
+        $request->method('getClientIps')
+            ->will($this->throwException(new ConflictingHeadersException()));
+
+        $dispatcher->addListener(KernelEvents::REQUEST, array($listener, 'onKernelRequest'));
+        $event = new GetResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST);
+
+        $this->setExpectedException('Symfony\Component\HttpKernel\Exception\HttpException');
+        $dispatcher->dispatch(KernelEvents::REQUEST, $event);
+    }
+
+    public function testListenerDoesNothingOnConsistentRequests()
+    {
+        $dispatcher = new EventDispatcher();
+        $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface');
+        $listener = new ValidateRequestClientIpListener();
+        $request = $this->getMock('Symfony\Component\HttpFoundation\Request');
+        $request->method('getClientIps')
+            ->willReturn(array('127.0.0.1'));
+
+        $dispatcher->addListener(KernelEvents::REQUEST, array($listener, 'onKernelRequest'));
+        $event = new GetResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST);
+        $dispatcher->dispatch(KernelEvents::REQUEST, $event);
+    }
+
+    public function testListenerDoesNothingOnSubrequests()
+    {
+        $dispatcher = new EventDispatcher();
+        $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface');
+        $listener = new ValidateRequestClientIpListener();
+        $request = $this->getMock('Symfony\Component\HttpFoundation\Request');
+        $request->method('getClientIps')
+            ->will($this->throwException(new ConflictingHeadersException()));
+
+        $dispatcher->addListener(KernelEvents::REQUEST, array($listener, 'onKernelRequest'));
+        $event = new GetResponseEvent($kernel, $request, HttpKernelInterface::SUB_REQUEST);
+        $dispatcher->dispatch(KernelEvents::REQUEST, $event);
+    }
+}
-Original file line number
+Diff line change
@@ -46,5 +46,9 @@
                 <argument type="service" id="request_stack" />
                 <tag name="kernel.event_subscriber" />
             </service>
+            <service id="validate_request_client_ip_listener" class="Symfony\Component\HttpKernel\EventListener\ValidateRequestClientIpListener">
             Copy link

  
      
    
  

  
      

  
  Member


      

  

  
    
      

      
            fabpot
  

      

      

      


        Jun 28, 2016


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      If we rename the class as I suggest, this should be renamed accordingly (validate_request_listener).
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
+                <tag name="kernel.event_subscriber" />
+            </service>
         </services>
     </container>