Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 0f7667d

Browse filesBrowse files
nicolas-grekasnicolas-grekas
committed
security #cve-2018-14774 [HttpKernel] fix trusted headers management in HttpCache and InlineFragmentRenderer (nicolas-grekas)
* commit '725dee4cd8': [HttpKernel] fix trusted headers management in HttpCache and InlineFragmentRenderer
2 parents 6604978 + 725dee4 commit 0f7667d
Copy full SHA for 0f7667d

8 files changed

+350-92Lines changed: 350 additions & 92 deletions

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎src/Symfony/Component/HttpFoundation/Request.php‎

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpFoundation/Request.php
+5Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1944,6 +1944,11 @@ private function getTrustedValues($type, $ip = null)
19441944
if (self::$trustedHeaders[self::HEADER_FORWARDED] && $this->headers->has(self::$trustedHeaders[self::HEADER_FORWARDED])) {
19451945
$forwardedValues = $this->headers->get(self::$trustedHeaders[self::HEADER_FORWARDED]);
19461946
$forwardedValues = preg_match_all(sprintf('{(?:%s)=(?:"?\[?)([a-zA-Z0-9\.:_\-/]*+)}', self::$forwardedParams[$type]), $forwardedValues, $matches) ? $matches[1] : array();
1947+
if (self::HEADER_CLIENT_PORT === $type) {
1948+
foreach ($forwardedValues as $k => $v) {
1949+
$forwardedValues[$k] = substr_replace($v, '0.0.0.0', 0, strrpos($v, ':'));
1950+
}
1951+
}
19471952
}
19481953

19491954
if (null !== $ip) {
Collapse file

‎src/Symfony/Component/HttpKernel/Fragment/InlineFragmentRenderer.php‎

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpKernel/Fragment/InlineFragmentRenderer.php
+2-27Lines changed: 2 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use Symfony\Component\HttpFoundation\Response;
1717
use Symfony\Component\HttpKernel\Controller\ControllerReference;
1818
use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent;
19+
use Symfony\Component\HttpKernel\HttpCache\SubRequestHandler;
1920
use Symfony\Component\HttpKernel\HttpKernelInterface;
2021
use Symfony\Component\HttpKernel\KernelEvents;
2122

@@ -76,7 +77,7 @@ public function render($uri, Request $request, array $options = array())
7677

7778
$level = ob_get_level();
7879
try {
79-
return $this->kernel->handle($subRequest, HttpKernelInterface::SUB_REQUEST, false);
80+
return SubRequestHandler::handle($this->kernel, $subRequest, HttpKernelInterface::SUB_REQUEST, false);
8081
} catch (\Exception $e) {
8182
// we dispatch the exception event to trigger the logging
8283
// the response that comes back is simply ignored
@@ -109,21 +110,6 @@ protected function createSubRequest($uri, Request $request)
109110
$cookies = $request->cookies->all();
110111
$server = $request->server->all();
111112

112-
// Override the arguments to emulate a sub-request.
113-
// Sub-request object will point to localhost as client ip and real client ip
114-
// will be included into trusted header for client ip
115-
try {
116-
if ($trustedHeaderName = Request::getTrustedHeaderName(Request::HEADER_CLIENT_IP)) {
117-
$currentXForwardedFor = $request->headers->get($trustedHeaderName, '');
118-
119-
$server['HTTP_'.$trustedHeaderName] = ($currentXForwardedFor ? $currentXForwardedFor.', ' : '').$request->getClientIp();
120-
}
121-
} catch (\InvalidArgumentException $e) {
122-
// Do nothing
123-
}
124-
125-
$server['REMOTE_ADDR'] = $this->resolveTrustedProxy();
126-
127113
unset($server['HTTP_IF_MODIFIED_SINCE']);
128114
unset($server['HTTP_IF_NONE_MATCH']);
129115

@@ -139,17 +125,6 @@ protected function createSubRequest($uri, Request $request)
139125
return $subRequest;
140126
}
141127

142-
private function resolveTrustedProxy()
143-
{
144-
if (!$trustedProxies = Request::getTrustedProxies()) {
145-
return '127.0.0.1';
146-
}
147-
148-
$firstTrustedProxy = reset($trustedProxies);
149-
150-
return false !== ($i = strpos($firstTrustedProxy, '/')) ? substr($firstTrustedProxy, 0, $i) : $firstTrustedProxy;
151-
}
152-
153128
/**
154129
* {@inheritdoc}
155130
*/
Collapse file

‎src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php‎

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php
+1-20Lines changed: 1 addition & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -468,27 +468,8 @@ protected function forward(Request $request, $catch = false, Response $entry = n
468468
$this->surrogate->addSurrogateCapability($request);
469469
}
470470

471-
// modify the X-Forwarded-For header if needed
472-
$forwardedFor = $request->headers->get('X-Forwarded-For');
473-
if ($forwardedFor) {
474-
$request->headers->set('X-Forwarded-For', $forwardedFor.', '.$request->server->get('REMOTE_ADDR'));
475-
} else {
476-
$request->headers->set('X-Forwarded-For', $request->server->get('REMOTE_ADDR'));
477-
}
478-
479-
// fix the client IP address by setting it to 127.0.0.1 as HttpCache
480-
// is always called from the same process as the backend.
481-
$request->server->set('REMOTE_ADDR', '127.0.0.1');
482-
483-
// make sure HttpCache is a trusted proxy
484-
if (!\in_array('127.0.0.1', $trustedProxies = Request::getTrustedProxies())) {
485-
$trustedProxies[] = '127.0.0.1';
486-
Request::setTrustedProxies($trustedProxies);
487-
}
488-
489471
// always a "master" request (as the real master request can be in cache)
490-
$response = $this->kernel->handle($request, HttpKernelInterface::MASTER_REQUEST, $catch);
491-
// FIXME: we probably need to also catch exceptions if raw === true
472+
$response = SubRequestHandler::handle($this->kernel, $request, HttpKernelInterface::MASTER_REQUEST, $catch);
492473

493474
// we don't implement the stale-if-error on Requests, which is nonetheless part of the RFC
494475
if (null !== $entry && \in_array($response->getStatusCode(), array(500, 502, 503, 504))) {
Collapse file

‎src/Symfony/Component/HttpKernel/HttpCache/SubRequestHandler.php‎

Copy file name to clipboard
+100Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Component\HttpKernel\HttpCache;
13+
14+
use Symfony\Component\HttpFoundation\IpUtils;
15+
use Symfony\Component\HttpFoundation\Request;
16+
use Symfony\Component\HttpFoundation\Response;
17+
use Symfony\Component\HttpKernel\HttpKernelInterface;
18+
19+
/**
20+
* @author Nicolas Grekas <p@tchwork.com>
21+
*
22+
* @internal
23+
*/
24+
class SubRequestHandler
25+
{
26+
/**
27+
* @return Response
28+
*/
29+
public static function handle(HttpKernelInterface $kernel, Request $request, $type, $catch)
30+
{
31+
// save global state related to trusted headers and proxies
32+
$trustedProxies = Request::getTrustedProxies();
33+
$trustedHeaders = array(
34+
Request::HEADER_FORWARDED => Request::getTrustedHeaderName(Request::HEADER_FORWARDED),
35+
Request::HEADER_CLIENT_IP => Request::getTrustedHeaderName(Request::HEADER_CLIENT_IP),
36+
Request::HEADER_CLIENT_HOST => Request::getTrustedHeaderName(Request::HEADER_CLIENT_HOST),
37+
Request::HEADER_CLIENT_PROTO => Request::getTrustedHeaderName(Request::HEADER_CLIENT_PROTO),
38+
Request::HEADER_CLIENT_PORT => Request::getTrustedHeaderName(Request::HEADER_CLIENT_PORT),
39+
);
40+
41+
// remove untrusted values
42+
$remoteAddr = $request->server->get('REMOTE_ADDR');
43+
if (!IpUtils::checkIp($remoteAddr, $trustedProxies)) {
44+
foreach (array_filter($trustedHeaders) as $name) {
45+
$request->headers->remove($name);
46+
}
47+
}
48+
49+
// compute trusted values, taking any trusted proxies into account
50+
$trustedIps = array();
51+
$trustedValues = array();
52+
foreach (array_reverse($request->getClientIps()) as $ip) {
53+
$trustedIps[] = $ip;
54+
$trustedValues[] = sprintf('for="%s"', $ip);
55+
}
56+
if ($ip !== $remoteAddr) {
57+
$trustedIps[] = $remoteAddr;
58+
$trustedValues[] = sprintf('for="%s"', $remoteAddr);
59+
}
60+
61+
// set trusted values, reusing as much as possible the global trusted settings
62+
if ($name = $trustedHeaders[Request::HEADER_FORWARDED]) {
63+
$trustedValues[0] .= sprintf(';host="%s";proto=%s', $request->getHttpHost(), $request->getScheme());
64+
$request->headers->set($name, implode(', ', $trustedValues));
65+
}
66+
if ($name = $trustedHeaders[Request::HEADER_CLIENT_IP]) {
67+
$request->headers->set($name, implode(', ', $trustedIps));
68+
}
69+
if (!$name && !$trustedHeaders[Request::HEADER_FORWARDED]) {
70+
$request->headers->set('X-Forwarded-For', implode(', ', $trustedIps));
71+
Request::setTrustedHeaderName(Request::HEADER_CLIENT_IP, 'X_FORWARDED_FOR');
72+
}
73+
74+
// fix the client IP address by setting it to 127.0.0.1,
75+
// which is the core responsibility of this method
76+
$request->server->set('REMOTE_ADDR', '127.0.0.1');
77+
78+
// ensure 127.0.0.1 is set as trusted proxy
79+
if (!IpUtils::checkIp('127.0.0.1', $trustedProxies)) {
80+
Request::setTrustedProxies(array_merge($trustedProxies, array('127.0.0.1')));
81+
}
82+
83+
try {
84+
$e = null;
85+
$response = $kernel->handle($request, $type, $catch);
86+
} catch (\Throwable $e) {
87+
} catch (\Exception $e) {
88+
}
89+
90+
// restore global state
91+
Request::setTrustedHeaderName(Request::HEADER_CLIENT_IP, $trustedHeaders[Request::HEADER_CLIENT_IP]);
92+
Request::setTrustedProxies($trustedProxies);
93+
94+
if (null !== $e) {
95+
throw $e;
96+
}
97+
98+
return $response;
99+
}
100+
}
Collapse file

‎src/Symfony/Component/HttpKernel/Tests/Fragment/InlineFragmentRendererTest.php‎

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpKernel/Tests/Fragment/InlineFragmentRendererTest.php
+20-18Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,16 @@ class InlineFragmentRendererTest extends TestCase
2626

2727
protected function setUp()
2828
{
29-
$this->originalTrustedHeaderName = Request::getTrustedHeaderName(Request::HEADER_CLIENT_IP);
29+
$this->originalTrustedHeaderNames = array(
30+
Request::getTrustedHeaderName(Request::HEADER_CLIENT_IP),
31+
Request::getTrustedHeaderName(Request::HEADER_FORWARDED),
32+
);
3033
}
3134

3235
protected function tearDown()
3336
{
34-
Request::setTrustedHeaderName(Request::HEADER_CLIENT_IP, $this->originalTrustedHeaderName);
37+
Request::setTrustedHeaderName(Request::HEADER_CLIENT_IP, $this->originalTrustedHeaderNames[0]);
38+
Request::setTrustedHeaderName(Request::HEADER_FORWARDED, $this->originalTrustedHeaderNames[1]);
3539
}
3640

3741
public function testRender()
@@ -55,7 +59,7 @@ public function testRenderWithObjectsAsAttributes()
5559
$subRequest = Request::create('/_fragment?_path=_format%3Dhtml%26_locale%3Den%26_controller%3Dmain_controller');
5660
$subRequest->attributes->replace(array('object' => $object, '_format' => 'html', '_controller' => 'main_controller', '_locale' => 'en'));
5761
$subRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
58-
$subRequest->server->set('HTTP_X_FORWARDED_FOR', '127.0.0.1');
62+
$subRequest->headers->set('forwarded', array('for="127.0.0.1";host="localhost";proto=http'));
5963

6064
$strategy = new InlineFragmentRenderer($this->getKernelExpectingRequest($subRequest));
6165

@@ -83,8 +87,12 @@ public function testRenderWithObjectsAsAttributesPassedAsObjectsInTheController(
8387
public function testRenderWithTrustedHeaderDisabled()
8488
{
8589
Request::setTrustedHeaderName(Request::HEADER_CLIENT_IP, '');
90+
Request::setTrustedHeaderName(Request::HEADER_FORWARDED, '');
8691

87-
$strategy = new InlineFragmentRenderer($this->getKernelExpectingRequest(Request::create('/')));
92+
$expectedSubRequest = Request::create('/');
93+
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
94+
95+
$strategy = new InlineFragmentRenderer($this->getKernelExpectingRequest($expectedSubRequest));
8896
$this->assertSame('foo', $strategy->render('/', Request::create('/'))->getContent());
8997
}
9098

@@ -168,11 +176,10 @@ public function testESIHeaderIsKeptInSubrequest()
168176
{
169177
$expectedSubRequest = Request::create('/');
170178
$expectedSubRequest->headers->set('Surrogate-Capability', 'abc="ESI/1.0"');
171-
172179
if (Request::getTrustedHeaderName(Request::HEADER_CLIENT_IP)) {
173180
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
174-
$expectedSubRequest->server->set('HTTP_X_FORWARDED_FOR', '127.0.0.1');
175181
}
182+
$expectedSubRequest->headers->set('forwarded', array('for="127.0.0.1";host="localhost";proto=http'));
176183

177184
$strategy = new InlineFragmentRenderer($this->getKernelExpectingRequest($expectedSubRequest));
178185

@@ -194,10 +201,8 @@ public function testESIHeaderIsKeptInSubrequestWithTrustedHeaderDisabled()
194201
public function testHeadersPossiblyResultingIn304AreNotAssignedToSubrequest()
195202
{
196203
$expectedSubRequest = Request::create('/');
197-
if (Request::getTrustedHeaderName(Request::HEADER_CLIENT_IP)) {
198-
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
199-
$expectedSubRequest->server->set('HTTP_X_FORWARDED_FOR', '127.0.0.1');
200-
}
204+
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
205+
$expectedSubRequest->headers->set('forwarded', array('for="127.0.0.1";host="localhost";proto=http'));
201206

202207
$strategy = new InlineFragmentRenderer($this->getKernelExpectingRequest($expectedSubRequest));
203208
$request = Request::create('/', 'GET', array(), array(), array(), array('HTTP_IF_MODIFIED_SINCE' => 'Fri, 01 Jan 2016 00:00:00 GMT', 'HTTP_IF_NONE_MATCH' => '*'));
@@ -208,12 +213,9 @@ public function testFirstTrustedProxyIsSetAsRemote()
208213
{
209214
$expectedSubRequest = Request::create('/');
210215
$expectedSubRequest->headers->set('Surrogate-Capability', 'abc="ESI/1.0"');
211-
$expectedSubRequest->server->set('REMOTE_ADDR', '1.1.1.1');
212-
213-
if (Request::getTrustedHeaderName(Request::HEADER_CLIENT_IP)) {
214-
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
215-
$expectedSubRequest->server->set('HTTP_X_FORWARDED_FOR', '127.0.0.1');
216-
}
216+
$expectedSubRequest->server->set('REMOTE_ADDR', '127.0.0.1');
217+
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
218+
$expectedSubRequest->headers->set('forwarded', array('for="127.0.0.1";host="localhost";proto=http'));
217219

218220
Request::setTrustedProxies(array('1.1.1.1'));
219221

@@ -230,9 +232,9 @@ public function testIpAddressOfRangedTrustedProxyIsSetAsRemote()
230232
{
231233
$expectedSubRequest = Request::create('/');
232234
$expectedSubRequest->headers->set('Surrogate-Capability', 'abc="ESI/1.0"');
233-
$expectedSubRequest->server->set('REMOTE_ADDR', '1.1.1.1');
235+
$expectedSubRequest->server->set('REMOTE_ADDR', '127.0.0.1');
234236
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
235-
$expectedSubRequest->server->set('HTTP_X_FORWARDED_FOR', '127.0.0.1');
237+
$expectedSubRequest->headers->set('forwarded', array('for="127.0.0.1";host="localhost";proto=http'));
236238

237239
Request::setTrustedProxies(array('1.1.1.1/24'));
238240

Collapse file

‎src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php‎

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php
+30-24Lines changed: 30 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1303,66 +1303,72 @@ public function testClientIpIsAlwaysLocalhostForForwardedRequests()
13031303
$this->setNextResponse();
13041304
$this->request('GET', '/', array('REMOTE_ADDR' => '10.0.0.1'));
13051305

1306-
$this->assertEquals('127.0.0.1', $this->kernel->getBackendRequest()->server->get('REMOTE_ADDR'));
1306+
$that = $this;
1307+
$this->kernel->assert(function ($backendRequest) use ($that) {
1308+
$that->assertSame('127.0.0.1', $backendRequest->server->get('REMOTE_ADDR'));
1309+
});
13071310
}
13081311

13091312
/**
13101313
* @dataProvider getTrustedProxyData
13111314
*/
1312-
public function testHttpCacheIsSetAsATrustedProxy(array $existing, array $expected)
1315+
public function testHttpCacheIsSetAsATrustedProxy(array $existing)
13131316
{
13141317
Request::setTrustedProxies($existing);
13151318

13161319
$this->setNextResponse();
13171320
$this->request('GET', '/', array('REMOTE_ADDR' => '10.0.0.1'));
1321+
$this->assertSame($existing, Request::getTrustedProxies());
13181322

1319-
$this->assertEquals($expected, Request::getTrustedProxies());
1323+
$that = $this;
1324+
$existing = array_unique(array_merge($existing, array('127.0.0.1')));
1325+
$this->kernel->assert(function ($backendRequest) use ($existing, $that) {
1326+
$that->assertSame($existing, Request::getTrustedProxies());
1327+
$that->assertsame('10.0.0.1', $backendRequest->getClientIp());
1328+
});
13201329

13211330
Request::setTrustedProxies(array());
13221331
}
13231332

13241333
public function getTrustedProxyData()
13251334
{
13261335
return array(
1327-
array(array(), array('127.0.0.1')),
1328-
array(array('10.0.0.2'), array('10.0.0.2', '127.0.0.1')),
1329-
array(array('10.0.0.2', '127.0.0.1'), array('10.0.0.2', '127.0.0.1')),
1336+
array(array()),
1337+
array(array('10.0.0.2')),
1338+
array(array('10.0.0.2', '127.0.0.1')),
13301339
);
13311340
}
13321341

13331342
/**
1334-
* @dataProvider getXForwardedForData
1343+
* @dataProvider getForwardedData
13351344
*/
1336-
public function testXForwarderForHeaderForForwardedRequests($xForwardedFor, $expected)
1345+
public function testForwarderHeaderForForwardedRequests($forwarded, $expected)
13371346
{
13381347
$this->setNextResponse();
13391348
$server = array('REMOTE_ADDR' => '10.0.0.1');
1340-
if (false !== $xForwardedFor) {
1341-
$server['HTTP_X_FORWARDED_FOR'] = $xForwardedFor;
1349+
if (null !== $forwarded) {
1350+
Request::setTrustedProxies($server);
1351+
$server['HTTP_FORWARDED'] = $forwarded;
13421352
}
13431353
$this->request('GET', '/', $server);
13441354

1345-
$this->assertEquals($expected, $this->kernel->getBackendRequest()->headers->get('X-Forwarded-For'));
1355+
$that = $this;
1356+
$this->kernel->assert(function ($backendRequest) use ($expected, $that) {
1357+
$that->assertSame($expected, $backendRequest->headers->get('Forwarded'));
1358+
});
1359+
1360+
Request::setTrustedProxies(array());
13461361
}
13471362

1348-
public function getXForwardedForData()
1363+
public function getForwardedData()
13491364
{
13501365
return array(
1351-
array(false, '10.0.0.1'),
1352-
array('10.0.0.2', '10.0.0.2, 10.0.0.1'),
1353-
array('10.0.0.2, 10.0.0.3', '10.0.0.2, 10.0.0.3, 10.0.0.1'),
1366+
array(null, 'for="10.0.0.1";host="localhost";proto=http'),
1367+
array('for=10.0.0.2', 'for="10.0.0.2";host="localhost";proto=http, for="10.0.0.1"'),
1368+
array('for=10.0.0.2, for=10.0.0.3', 'for="10.0.0.2";host="localhost";proto=http, for="10.0.0.3", for="10.0.0.1"'),
13541369
);
13551370
}
13561371

1357-
public function testXForwarderForHeaderForPassRequests()
1358-
{
1359-
$this->setNextResponse();
1360-
$server = array('REMOTE_ADDR' => '10.0.0.1');
1361-
$this->request('POST', '/', $server);
1362-
1363-
$this->assertEquals('10.0.0.1', $this->kernel->getBackendRequest()->headers->get('X-Forwarded-For'));
1364-
}
1365-
13661372
public function testEsiCacheRemoveValidationHeadersIfEmbeddedResponses()
13671373
{
13681374
$time = \DateTime::createFromFormat('U', time());

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.