Skip to content

Commit

Permalink
bug #21849 [HttpFoundation] Fix missing handling of for/host/proto in…
Browse files Browse the repository at this point in the history 
…fo from "Forwarded" header (nicolas-grekas)

This PR was merged into the 2.8 branch.

Discussion
----------

[HttpFoundation] Fix missing handling of for/host/proto info from "Forwarded" header

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

We're missing handling of for/host/proto info embedded in the `Forwarded` header, as eg in:
`Forwarded:  for=1.1.1.1:443, host=foo.example.com:1234, proto=https, for=2.2.2.2, host=real.example.com:8080`

Commits
-------

04caacb [HttpFoundation] Fix missing handling of for/host/proto info from "Forwarded" header
  • Loading branch information
@fabpot
fabpot committed Mar 22, 2017
2 parents 5f9d941 + 04caacb commit 8371dea
Show file tree
Hide file tree
Showing 4 changed files with 120 additions and 58 deletions.
123 changes: 68 additions & 55 deletions src/Symfony/Component/HttpFoundation/Request.php
View file
Expand Up @@ -206,6 +206,15 @@ class Request

protected static $requestFactory;

private $isForwardedValid = true;

private static $forwardedParams = array(
self::HEADER_CLIENT_IP => 'for',
self::HEADER_CLIENT_HOST => 'host',
self::HEADER_CLIENT_PROTO => 'proto',
self::HEADER_CLIENT_PORT => 'host',
);

/**
* Constructor.
*
Expand Down Expand Up @@ -806,41 +815,13 @@ public function setSession(SessionInterface $session)
*/
public function getClientIps()
{
$clientIps = array();
$ip = $this->server->get('REMOTE_ADDR');

if (!$this->isFromTrustedProxy()) {
return array($ip);
}

$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);
$forwardedClientIps = $matches[3];

$forwardedClientIps = $this->normalizeAndFilterClientIps($forwardedClientIps, $ip);
$clientIps = $forwardedClientIps;
}

if ($hasTrustedClientIpHeader) {
$xForwardedForClientIps = array_map('trim', explode(',', $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_IP])));

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

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.');
}

if (!$hasTrustedForwardedHeader && !$hasTrustedClientIpHeader) {
return $this->normalizeAndFilterClientIps(array(), $ip);
}

return $clientIps;
return $this->getTrustedValues(self::HEADER_CLIENT_IP, $ip) ?: array($ip);
}

/**
Expand Down Expand Up @@ -966,31 +947,25 @@ public function getScheme()
*/
public function getPort()
{
if ($this->isFromTrustedProxy()) {
if (self::$trustedHeaders[self::HEADER_CLIENT_PORT] && $port = $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_PORT])) {
return (int) $port;
}

if (self::$trustedHeaders[self::HEADER_CLIENT_PROTO] && 'https' === $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_PROTO], 'http')) {
return 443;
}
if ($this->isFromTrustedProxy() && $host = $this->getTrustedValues(self::HEADER_CLIENT_PORT)) {
$host = $host[0];
} elseif ($this->isFromTrustedProxy() && $host = $this->getTrustedValues(self::HEADER_CLIENT_HOST)) {
$host = $host[0];
} elseif (!$host = $this->headers->get('HOST')) {
return $this->server->get('SERVER_PORT');
}

if ($host = $this->headers->get('HOST')) {
if ($host[0] === '[') {
$pos = strpos($host, ':', strrpos($host, ']'));
} else {
$pos = strrpos($host, ':');
}

if (false !== $pos) {
return (int) substr($host, $pos + 1);
}
if ($host[0] === '[') {
$pos = strpos($host, ':', strrpos($host, ']'));
} else {
$pos = strrpos($host, ':');
}

return 'https' === $this->getScheme() ? 443 : 80;
if (false !== $pos) {
return (int) substr($host, $pos + 1);
}

return $this->server->get('SERVER_PORT');
return 'https' === $this->getScheme() ? 443 : 80;
}

/**
Expand Down Expand Up @@ -1190,8 +1165,8 @@ public function getQueryString()
*/
public function isSecure()
{
if ($this->isFromTrustedProxy() && self::$trustedHeaders[self::HEADER_CLIENT_PROTO] && $proto = $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_PROTO])) {
return in_array(strtolower(current(explode(',', $proto))), array('https', 'on', 'ssl', '1'));
if ($this->isFromTrustedProxy() && $proto = $this->getTrustedValues(self::HEADER_CLIENT_PROTO)) {
return in_array(strtolower($proto[0]), array('https', 'on', 'ssl', '1'), true);
}

$https = $this->server->get('HTTPS');
Expand All @@ -1216,10 +1191,8 @@ public function isSecure()
*/
public function getHost()
{
if ($this->isFromTrustedProxy() && self::$trustedHeaders[self::HEADER_CLIENT_HOST] && $host = $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_HOST])) {
$elements = explode(',', $host, 2);

$host = $elements[0];
if ($this->isFromTrustedProxy() && $host = $this->getTrustedValues(self::HEADER_CLIENT_HOST)) {
$host = $host[0];
} elseif (!$host = $this->headers->get('HOST')) {
if (!$host = $this->server->get('SERVER_NAME')) {
$host = $this->server->get('SERVER_ADDR', '');
Expand Down Expand Up @@ -1948,8 +1921,48 @@ private function isFromTrustedProxy()
return self::$trustedProxies && IpUtils::checkIp($this->server->get('REMOTE_ADDR'), self::$trustedProxies);
}

private function getTrustedValues($type, $ip = null)
{
$clientValues = array();
$forwardedValues = array();

if (self::$trustedHeaders[$type] && $this->headers->has(self::$trustedHeaders[$type])) {
foreach (explode(',', $this->headers->get(self::$trustedHeaders[$type])) as $v) {
$clientValues[] = (self::HEADER_CLIENT_PORT === $type ? '0.0.0.0:' : '').trim($v);
}
}

if (self::$trustedHeaders[self::HEADER_FORWARDED] && $this->headers->has(self::$trustedHeaders[self::HEADER_FORWARDED])) {
$forwardedValues = $this->headers->get(self::$trustedHeaders[self::HEADER_FORWARDED]);
$forwardedValues = preg_match_all(sprintf('{(?:%s)=(?:"?\[?)([a-zA-Z0-9\.:_\-/]*+)}', self::$forwardedParams[$type]), $forwardedValues, $matches) ? $matches[1] : array();
}

if (null !== $ip) {
$clientValues = $this->normalizeAndFilterClientIps($clientValues, $ip);
$forwardedValues = $this->normalizeAndFilterClientIps($forwardedValues, $ip);
}

if ($forwardedValues === $clientValues || !$clientValues) {
return $forwardedValues;
}

if (!$forwardedValues) {
return $clientValues;
}

if (!$this->isForwardedValid) {
return null !== $ip ? array('0.0.0.0', $ip) : array();
}
$this->isForwardedValid = false;

throw new ConflictingHeadersException(sprintf('The request has both a trusted "%s" header and a trusted "%s" header, conflicting with each other. You should either configure your proxy to remove one of them, or configure your project to distrust the offending one.', self::$trustedHeaders[self::HEADER_FORWARDED], self::$trustedHeaders[$type]));
}

private function normalizeAndFilterClientIps(array $clientIps, $ip)
{
if (!$clientIps) {
return array();
}
$clientIps[] = $ip; // Complete the IP chain with the IP the request actually came from
$firstTrustedIp = null;

Expand Down
51 changes: 50 additions & 1 deletion src/Symfony/Component/HttpFoundation/Tests/RequestTest.php
View file
Expand Up @@ -1631,7 +1631,7 @@ private function getRequestInstanceForClientIpsForwardedTests($remoteAddr, $http
return $request;
}

public function testTrustedProxies()
public function testTrustedProxiesXForwardedFor()
{
$request = Request::create('http://example.com/');
$request->server->set('REMOTE_ADDR', '3.3.3.3');
Expand Down Expand Up @@ -1714,6 +1714,55 @@ public function testTrustedProxies()
Request::setTrustedHeaderName(Request::HEADER_CLIENT_PROTO, 'X_FORWARDED_PROTO');
}

public function testTrustedProxiesForwarded()
{
$request = Request::create('http://example.com/');
$request->server->set('REMOTE_ADDR', '3.3.3.3');
$request->headers->set('FORWARDED', 'for=1.1.1.1, host=foo.example.com:8080, proto=https, for=2.2.2.2, host=real.example.com:8080');

// no trusted proxies
$this->assertEquals('3.3.3.3', $request->getClientIp());
$this->assertEquals('example.com', $request->getHost());
$this->assertEquals(80, $request->getPort());
$this->assertFalse($request->isSecure());

// disabling proxy trusting
Request::setTrustedProxies(array());
$this->assertEquals('3.3.3.3', $request->getClientIp());
$this->assertEquals('example.com', $request->getHost());
$this->assertEquals(80, $request->getPort());
$this->assertFalse($request->isSecure());

// request is forwarded by a non-trusted proxy
Request::setTrustedProxies(array('2.2.2.2'));
$this->assertEquals('3.3.3.3', $request->getClientIp());
$this->assertEquals('example.com', $request->getHost());
$this->assertEquals(80, $request->getPort());
$this->assertFalse($request->isSecure());

// trusted proxy via setTrustedProxies()
Request::setTrustedProxies(array('3.3.3.3', '2.2.2.2'));
$this->assertEquals('1.1.1.1', $request->getClientIp());
$this->assertEquals('foo.example.com', $request->getHost());
$this->assertEquals(8080, $request->getPort());
$this->assertTrue($request->isSecure());

// trusted proxy via setTrustedProxies()
Request::setTrustedProxies(array('3.3.3.4', '2.2.2.2'));
$this->assertEquals('3.3.3.3', $request->getClientIp());
$this->assertEquals('example.com', $request->getHost());
$this->assertEquals(80, $request->getPort());
$this->assertFalse($request->isSecure());

// check various X_FORWARDED_PROTO header values
Request::setTrustedProxies(array('3.3.3.3', '2.2.2.2'));
$request->headers->set('FORWARDED', 'proto=ssl');
$this->assertTrue($request->isSecure());

$request->headers->set('FORWARDED', 'proto=https, proto=http');
$this->assertTrue($request->isSecure());
}

/**
* @expectedException \InvalidArgumentException
*/
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestListenerTest.php
View file
Expand Up @@ -32,7 +32,7 @@ public function testListenerThrowsWhenMasterRequestHasInconsistentClientIps()
$request = new Request();
$request->setTrustedProxies(array('1.1.1.1'));
$request->server->set('REMOTE_ADDR', '1.1.1.1');
$request->headers->set('FORWARDED', '2.2.2.2');
$request->headers->set('FORWARDED', 'for=2.2.2.2');
$request->headers->set('X_FORWARDED_FOR', '3.3.3.3');

$dispatcher->addListener(KernelEvents::REQUEST, array(new ValidateRequestListener(), 'onKernelRequest'));
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/HttpKernel/Tests/HttpKernelTest.php
View file
Expand Up @@ -287,7 +287,7 @@ public function testInconsistentClientIpsOnMasterRequests()
$request = new Request();
$request->setTrustedProxies(array('1.1.1.1'));
$request->server->set('REMOTE_ADDR', '1.1.1.1');
$request->headers->set('FORWARDED', '2.2.2.2');
$request->headers->set('FORWARDED', 'for=2.2.2.2');
$request->headers->set('X_FORWARDED_FOR', '3.3.3.3');

$kernel->handle($request, $kernel::MASTER_REQUEST, false);
Expand Down

0 comments on commit 8371dea

Please sign in to comment.