Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[HttpKernel] InlineFragmentRenderer should add the X-Forwarded-For of real client IP #7557

Closed
wants to merge 3 commits into from
Closed

[HttpKernel] InlineFragmentRenderer should add the X-Forwarded-For of real client IP #7557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
736fedf
Getting the real customer address for the getClientIp
kinncj Apr 3, 2013
9fbd9ff
[HttpKernel] Fixed request getClientIp according to The general forma…
kinncj Apr 3, 2013
8d0a0b0
[HttpKernel] Removing unecessary validation
kinncj Apr 3, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/Symfony/Component/HttpFoundation/Request.php
View file
Open in desktop
Expand Up @@ -696,6 +696,21 @@ public function getClientIp()
$clientIps[] = $ip;

$trustedProxies = self::$trustProxy && !self::$trustedProxies ? array($ip) : self::$trustedProxies;

//If is there any forward_for IP address, this is the real client IP address
if ($this->headers->has(self::$trustedHeaders[self::HEADER_CLIENT_IP])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

At this point the condition inside if always evaluates to true, so the code following this block will never be reached. Anyway this block just duplicates the original logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Moreover, it brings back the vulnerability fixed by introducing the list of trusted proxies, since it bypasses the check.

Copy link
Author

Choose a reason for hiding this comment

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

$this->headers->has(self::$trustedHeaders[self::HEADER_CLIENT_IP])

This just work if the X-Forwarded-For exists...
This dont duplicates the original logic, it just get the client ip address from the X-Forwarded-For header (client, proxy1, proxy2, ...) instead of the hardcoded 127.0.0.1... this was made to avoid this hardcoded IP address

It does not bypass the check... there is no check for this, once a time that the real client IP address is not at the trustedProxies.

Copy link
Member

Choose a reason for hiding this comment

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

@kinncj Getting it from the X-Forwarded-For when trusting proxies is already handled by the existing code. The returned ip should always be the closest untrusted ip, not the original IP (as headers can be forged by the client)

Copy link
Author

Choose a reason for hiding this comment

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

X-Forwarded-For should return always the original IP.

The general format of the field is:
X-Forwarded-For: client, proxy1, proxy2
where the value is a comma+space separated list of IP addresses, the left-most being the original client, and each successive proxy that passed the request adding the IP address where it received the request from. In this example, the request passed through proxy1, proxy2, and then proxy3 (not shown in the header). proxy3 appears as remote address of the request.

If this is the validation it must be wrong...
This validation is breaking the original X-Forwarded-For semantic...

The validation must be a new issue and not related to this PR... you cannot avoid the validation removing the original IP address.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kinncj As mentioned by @stof, the X-Forwarded-For header may be forged by client, so you should not trust its contents. But if you are sure you need this, you can always get the header contents by yourself. Or you could suggest adding something like Request::getUntrustedClientIp(), but you should not introduce security holes in the name of your convenience :)

Copy link
Author

Choose a reason for hiding this comment

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

Talking with blanco, we found an another sollution.
check: #7559

Copy link
Author

Choose a reason for hiding this comment

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

And FYI, anything can be forged by the client... even a X-Parmegiana-For... you cannot act as a firewall/proxy for this...
Anyway,
rails/rails#2490

Copy link
Author

Choose a reason for hiding this comment

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

And again, the InlineFragmentRenderer is not a real request, so it cannot overwrite the original REMOTE_ADDR to 127.0.0.1
RFC 2616, Sec 5.

$forwarded = $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_IP]);
$forwarded = explode(",",$forwarded);
$forwarded = trim($forwarded[0]);


return $forwarded;
}

//If it is not a forwarded IP, the Client IP should be in the trusted proxies
array_push($trustedProxies, $this->getClientIp());
$this->setTrustedProxies($trustedProxies);

$clientIps = array_diff($clientIps, $trustedProxies);

return array_pop($clientIps);
Expand Down
12 changes: 6 additions & 6 deletions src/Symfony/Component/HttpFoundation/Tests/RequestTest.php
View file
Open in desktop
Expand Up @@ -769,9 +769,9 @@ public function testGetClientIpProvider()
array('127.0.0.1', false, '127.0.0.1', '88.88.88.88', null),
array('88.88.88.88', true, '127.0.0.1', '88.88.88.88', null),
array('2620:0:1cfe:face:b00c::3', true, '::1', '2620:0:1cfe:face:b00c::3', null),
array('88.88.88.88', true, '123.45.67.89', '127.0.0.1, 87.65.43.21, 88.88.88.88', null),
array('87.65.43.21', true, '123.45.67.89', '127.0.0.1, 87.65.43.21, 88.88.88.88', array('123.45.67.89', '88.88.88.88')),
array('87.65.43.21', false, '123.45.67.89', '127.0.0.1, 87.65.43.21, 88.88.88.88', array('123.45.67.89', '88.88.88.88')),
array('127.0.0.1', true, '123.45.67.89', '127.0.0.1, 88.88.88.88, 87.65.43.21', null),
array('127.0.0.1', true, '123.45.67.89', '127.0.0.1, 87.65.43.21, 88.88.88.88', array('123.45.67.89', '88.88.88.88')),
array('127.0.0.1', false, '123.45.67.89', '127.0.0.1, 87.65.43.21, 88.88.88.88', array('123.45.67.89', '88.88.88.88')),
);
}

Expand Down Expand Up @@ -1281,7 +1281,7 @@ public function testTrustedProxies()
$request->headers->set('X_FORWARDED_HOST', 'foo.example.com, real.example.com:8080');
$request->headers->set('X_FORWARDED_PROTO', 'https');
$request->headers->set('X_FORWARDED_PORT', 443);
$request->headers->set('X_MY_FOR', '3.3.3.3, 4.4.4.4');
$request->headers->set('X_MY_FOR', '3.3.3.3, 4.4.4.4');//X-Forwarded-For: client[3.3.3.3], proxy1, proxy2
$request->headers->set('X_MY_HOST', 'my.example.com');
$request->headers->set('X_MY_PROTO', 'http');
$request->headers->set('X_MY_PORT', 81);
Expand All @@ -1306,12 +1306,12 @@ public function testTrustedProxies()
$this->assertEquals(443, $request->getPort());
$this->assertTrue($request->isSecure());

// custom header names
// custom header names : X-Forwarded-For: client[3.3.3.3], proxy1, proxy2
Request::setTrustedHeaderName(Request::HEADER_CLIENT_IP, 'X_MY_FOR');
Request::setTrustedHeaderName(Request::HEADER_CLIENT_HOST, 'X_MY_HOST');
Request::setTrustedHeaderName(Request::HEADER_CLIENT_PORT, 'X_MY_PORT');
Request::setTrustedHeaderName(Request::HEADER_CLIENT_PROTO, 'X_MY_PROTO');
$this->assertEquals('4.4.4.4', $request->getClientIp());
$this->assertEquals('3.3.3.3', $request->getClientIp());
$this->assertEquals('my.example.com', $request->getHost());
$this->assertEquals(81, $request->getPort());
$this->assertFalse($request->isSecure());
Expand Down
3 changes: 0 additions & 3 deletions src/Symfony/Component/HttpKernel/Fragment/InlineFragmentRenderer.php
View file
Open in desktop
Expand Up @@ -86,9 +86,6 @@ protected function createSubRequest($uri, Request $request)
$cookies = $request->cookies->all();
$server = $request->server->all();

// the sub-request is internal
$server['REMOTE_ADDR'] = '127.0.0.1';

$subRequest = $request::create($uri, 'get', array(), $cookies, array(), $server);
if ($session = $request->getSession()) {
$subRequest->setSession($session);
Expand Down