8000 security #cve-2018-14774 [HttpKernel] fix trusted headers management … · donquixote/symfony@9cfcaba · GitHub
[go: up one dir, main page]
Skip to content

Commit 9cfcaba

Browse files
nicolas-grekasnicolas-grekas
committed
security #cve-2018-14774 [HttpKernel] fix trusted headers management in HttpCache and InlineFragmentRenderer (nicolas-grekas)
* commit '08a32d44b6': [HttpKernel] fix trusted headers management in HttpCache and InlineFragmentRenderer
2 parents efcde3d + 08a32d4 commit 9cfcaba

File tree

7 files changed

+375
-72
lines changed

7 files changed

+375
-72
lines changed

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

Lines changed: 2 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
use Symfony\Component\HttpFoundation\Request;
1515
use Symfony\Component\HttpFoundation\Response;
16+
use Symfony\Component\HttpKernel\HttpCache\SubRequestHandler;
1617
use Symfony\Component\HttpKernel\HttpKernelInterface;
1718
use Symfony\Component\HttpKernel\Controller\ControllerReference;
1819
use Symfony\Component\HttpKernel\KernelEvents;
@@ -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,20 +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[ A93C 'HTTP_'.$trustedHeaderName] = ($currentXForwardedFor ? $currentXForwardedFor.', ' : '').$request->getClientIp();
120-
}
121-
} catch (\InvalidArgumentException $e) {
122-
// Do nothing
123-
}
124-
125-
$server['REMOTE_ADDR'] = '127.0.0.1';
126113
unset($server['HTTP_IF_MODIFIED_SINCE']);
127114
unset($server['HTTP_IF_NONE_MATCH']);
128115

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

Lines changed: 1 addition & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -464,27 +464,8 @@ protected function forward(Request $request, $catch = false, Response $entry = n
464464
$this->surrogate->addSurrogateCapability($request);
465465
}
466466

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

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

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

Lines 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+
}

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

Lines changed: 53 additions & 10 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('< 10000 span class=pl-s>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,16 +201,52 @@ 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' => '*'));
204209
$strategy->render('/', $request);
205210
}
206211

212+
public function testFirstTrustedProxyIsSetAsRemote()
213+
{
214+
$expectedSubRequest = Request::create('/');
215+
$expectedSubRequest->headers->set('Surrogate-Capability', 'abc="ESI/1.0"');
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'));
219+
220+
Request::setTrustedProxies(array('1.1.1.1'));
221+
222+
$strategy = new InlineFragmentRenderer($this->getKernelExpectingRequest($expectedSubRequest));
223+
224+
$request = Request::create('/');
225+
$request->headers->set('Surrogate-Capability', 'abc="ESI/1.0"');
226+
$strategy->render('/', $request);
227+
228+
Request::setTrustedProxies(array());
229+
}
230+
231+
public function testIpAddressOfRangedTrustedProxyIsSetAsRemote()
232+
{
233+
$expectedSubRequest = Request::create('/');
234+
$expectedSubRequest->headers->set('Surrogate-Capability', 'abc="ESI/1.0"');
235+
$expectedSubRequest->server->set('REMOTE_ADDR', '127.0.0.1');
236+
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
237+
$expectedSubRequest->headers->set('forwarded', array('for="127.0.0.1";host="localhost";proto=http'));
238+
239+
Request::setTrustedProxies(array('1.1.1.1/24'));
240+
241+
$strategy = new InlineFragmentRenderer($this->getKernelExpectingRequest($expectedSubRequest));
242+
243+
$request = Request::create('/');
244+
$request->headers->set('Surrogate-Capability', 'abc="ESI/1.0"');
245+
$strategy->render('/', $request);
246+
247+
Request::setTrustedProxies(array());
248+
}
249+
207250
/**
208251
* Creates a Kernel expecting a request equals to $request
209252
* Allows delta in comparison in case REQUEST_TIME changed by 1 second.

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

Lines changed: 32 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1301,64 +1301,72 @@ public function testClientIpIsAlwaysLocalhostForForwardedRequests()
13011301
$this->setNextResponse();
13021302
$this->request('GET', '/', array('REMOTE_ADDR' => '10.0.0.1'));
13031303

1304-
$this->assertEquals('127.0.0.1', $this->kernel->getBackendRequest()->server->get('REMOTE_ADDR'));
1304+
$that = $this;
1305+
$this->kernel->assert(function ($backendRequest) use ($that) {
1306+
$that->assertSame('127.0.0.1', $backendRequest->server->get('REMOTE_ADDR'));
1307+
});
13051308
}
13061309

13071310
/**
13081311
* @dataProvider getTrustedProxyData
13091312
*/
1310-
public function testHttpCacheIsSetAsATrustedProxy(array $existing, array $expected)
1313+
public function testHttpCacheIsSetAsATrustedProxy(array $existing)
13111314
{
13121315
Request::setTrustedProxies($existing);
13131316

13141317
$this->setNextResponse();
13151318
$this->request('GET', '/', array('REMOTE_ADDR' => '10.0.0.1'));
1319+
$this->assertSame($existing, Request::getTrustedProxies());
13161320

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

13201331
public function getTrustedProxyData()
13211332
{
13221333
return array(
1323-
array(array(), array('127.0.0.1')),
1324-
array(array('10.0.0.2'), array('10.0.0.2', '127.0.0.1')),
1325-
array(array('10.0.0.2', '127.0.0.1'), array('10.0.0.2', '127.0.0.1')),
1334+
array(array()),
1335+
array(array('10.0.0.2')),
1336+
array(array('10.0.0.2', '127.0.0.1')),
13261337
);
13271338
}
13281339

13291340
/**
1330-
* @dataProvider getXForwardedForData
1341+
* @dataProvider getForwardedData
13311342
*/
1332-
public function testXForwarderForHeaderForForwardedRequests($xForwardedFor, $expected)
1343+
public function testForwarderHeaderForForwardedRequests($forwarded, $expected)
13331344
{
13341345
$this->setNextResponse();
13351346
$server = array('REMOTE_ADDR' => '10.0.0.1');
1336-
if (false !== $xForwardedFor) {
1337-
$server['HTTP_X_FORWARDED_FOR'] = $xForwardedFor;
1347+
if (null !== $forwarded) {
1348+
Request::setTrustedProxies($server);
1349+
$server['HTTP_FORWARDED'] = $forwarded;
13381350
}
13391351
$this->request('GET', '/', $server);
13401352

1341-
$this->assertEquals($expected, $this->kernel->getBackendRequest()->headers->get('X-Forwarded-For'));
1353+
$that = $this;
1354+
$this->kernel->assert(function ($backendRequest) use ($expected, $that) {
1355+
$that->assertSame($expected, $backendRequest->headers->get('Forwarded'));
1356+
});
1357+
1358+
Request::setTrustedProxies(array());
13421359
}
13431360

1344-
public function getXForwardedForData()
1361+
public function getForwardedData()
13451362
{
13461363
return array(
1347-
array(false, '10.0.0.1'),
1348-
array('10.0.0.2', '10.0.0.2, 10.0.0.1'),
1349-
array('10.0.0.2, 10.0.0.3', '10.0.0.2, 10.0.0.3, 10.0.0.1'),
1364+
array(null, 'for="10.0.0.1";host="localhost";proto=http'),
1365+
array('for=10.0.0.2', 'for="10.0.0.2";host="localhost";proto=http, for="10.0.0.1"'),
1366+
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"'),
13501367
);
13511368
}
13521369

1353-
public function testXForwarderForHeaderForPassRequests()
1354-
{
1355-
$this->setNextResponse();
1356-
$server = array('REMOTE_ADDR' => '10.0.0.1');
1357-
$this->request('POST', '/', $server);
1358-
1359-
$this->assertEquals('10.0.0.1', $this->kernel->getBackendRequest()->headers->get('X-Forwarded-For'));
1360-
}
1361-
13621370
public function testEsiCacheRemoveValidationHeadersIfEmbeddedResponses()
13631371
{
13641372
$time = \DateTime::createFromFormat('U', time());

0 commit comments

Comments
 (0)
0