Skip to content

Commit 677e740

Browse files
dgdg
committed
RequestFactory: fixes client spoofing in trusted-proxy forwarding (security)
Backport of the proxy-resolution hardening to the 3.3 branch, without the new configuration API. The "Forwarded" branch trusted the leftmost "for" element, which a client can forge by appending its own; it also merged the ',' and ';' separators with preg_split, losing the hop structure. Both branches could return a non-IP string from getRemoteAddress(). - Forwarded is now parsed per hop (RFC 7239): elements by ',', params by ';'. - The client is the rightmost untrusted hop after stripping trailing trusted proxies (shared findClientHop()), not the spoofable leftmost one. - host/proto are taken from the same hop as the client "for". - The resolved address is validated as an IP; an obfuscated/invalid value or a fully trusted chain now yields null instead of a non-IP string. BC: getRemoteAddress() returns null (not a non-IP string) when the innermost forwarded value is not a valid IP; a Forwarded header carrying only proto/host without a valid "for" no longer sets the scheme/host.
1 parent 159dbcd commit 677e740

3 files changed

Lines changed: 113 additions & 35 deletions

File tree

‎src/Http/RequestFactory.php‎

Lines changed: 62 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
use Nette;
1111
use Nette\Utils\Arrays;
1212
use Nette\Utils\Strings;
13-
use function array_filter, base64_encode, count, end, explode, file_get_contents, filter_input_array, filter_var, function_exists, get_debug_type, in_array, ini_get, is_array, is_string, key, min, preg_last_error, preg_match, preg_replace, preg_split, rtrim, sprintf, str_contains, strcasecmp, strlen, strncmp, strpos, strrpos, strtolower, strtr, substr, trim;
13+
use function array_filter, array_key_last, array_map, base64_encode, explode, file_get_contents, filter_input_array, filter_var, function_exists, get_debug_type, in_array, ini_get, is_array, is_string, min, preg_last_error, preg_match, preg_replace, rtrim, sprintf, str_contains, strcasecmp, strlen, strncmp, strpos, strrpos, strtolower, strtr, substr, trim;
1414
use const PHP_SAPI;
1515

1616

@@ -300,7 +300,8 @@ private function getClient(Url $url): array
300300
{
301301
$remoteAddr = !empty($_SERVER['REMOTE_ADDR']) ? $_SERVER['REMOTE_ADDR'] : null;
302302

303-
// use real client address and host if trusted proxy is used
303+
// trust forwarding headers only when the request comes through a trusted proxy;
304+
// the proxy in turn should strip any forwarding header it does not set itself
304305
$usingTrustedProxy = $remoteAddr && Arrays::some($this->proxies, fn(string $proxy): bool => Helpers::ipMatch($remoteAddr, $proxy));
305306
if ($usingTrustedProxy) {
306307
$remoteHost = null;
@@ -318,34 +319,42 @@ private function getClient(Url $url): array
318319

319320
private function useForwardedProxy(Url $url): ?string
320321
{
321-
$forwardParams = preg_split('/[,;]/', $_SERVER['HTTP_FORWARDED']);
322-
foreach ($forwardParams as $forwardParam) {
323-
[$key, $value] = explode('=', $forwardParam, 2) + [1 => ''];
324-
$proxyParams[strtolower(trim($key))][] = trim($value, " \t\"");
322+
// RFC 7239: split into hops (comma), each a set of params (semicolon)
323+
$hops = $addresses = [];
324+
foreach (explode(',', $_SERVER['HTTP_FORWARDED']) as $element) {
325+
$hop = [];
326+
foreach (explode(';', $element) as $pair) {
327+
[$key, $value] = explode('=', $pair, 2) + [1 => ''];
328+
$hop[strtolower(trim($key))] = trim($value, " \t\"");
329+
}
330+
331+
$for = $hop['for'] ?? '';
332+
$addresses[] = str_contains($for, '[')
333+
? substr($for, 1, strpos($for, ']') - 1) // IPv6 "[addr]:port"
334+
: explode(':', $for)[0]; // IPv4 "addr:port" or bare address
335+
$hops[] = $hop;
325336
}
326337

327-
if (isset($proxyParams['for'])) {
328-
$address = $proxyParams['for'][0];
329-
$remoteAddr = str_contains($address, '[')
330-
? substr($address, 1, strpos($address, ']') - 1) // IPv6
331-
: explode(':', $address)[0]; // IPv4
338+
$clientHop = $this->findClientHop($addresses);
339+
if ($clientHop === null) {
340+
return null;
332341
}
333342

334-
if (isset($proxyParams['proto']) && count($proxyParams['proto']) === 1) {
335-
$url->setScheme(strcasecmp($proxyParams['proto'][0], 'https') === 0 ? 'https' : 'http');
343+
// scheme and host from the client's own hop
344+
$hop = $hops[$clientHop[0]];
345+
if (isset($hop['proto'])) {
346+
$url->setScheme(strcasecmp($hop['proto'], 'https') === 0 ? 'https' : 'http');
336347
$url->setPort($url->getScheme() === 'https' ? 443 : 80);
337348
}
338349

339-
if (
340-
isset($proxyParams['host']) && count($proxyParams['host']) === 1
341-
&& ($pair = $this->parseHostAndPort($proxyParams['host'][0]))
342-
) {
350+
if (isset($hop['host']) && ($pair = $this->parseHostAndPort($hop['host']))) {
343351
$url->setHost($pair[0]);
344352
if (isset($pair[1])) {
345353
$url->setPort($pair[1]);
346354
}
347355
}
348-
return $remoteAddr ?? null;
356+
357+
return $clientHop[1];
349358
}
350359

351360

@@ -360,32 +369,50 @@ private function useNonstandardProxy(Url $url): ?string
360369
$url->setPort((int) $_SERVER['HTTP_X_FORWARDED_PORT']);
361370
}
362371

363-
if (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) {
364-
$xForwardedForWithoutProxies = array_filter(
365-
explode(',', $_SERVER['HTTP_X_FORWARDED_FOR']),
366-
fn(string $ip): bool => filter_var($ip = trim($ip), FILTER_VALIDATE_IP) === false
367-
|| !Arrays::some($this->proxies, fn(string $proxy): bool => Helpers::ipMatch($ip, $proxy)),
368-
);
369-
if ($xForwardedForWithoutProxies) {
370-
$remoteAddr = trim(end($xForwardedForWithoutProxies));
371-
$xForwardedForRealIpKey = key($xForwardedForWithoutProxies);
372-
}
372+
if (empty($_SERVER['HTTP_X_FORWARDED_FOR'])) {
373+
return null;
373374
}
374375

375-
if (isset($xForwardedForRealIpKey) && !empty($_SERVER['HTTP_X_FORWARDED_HOST'])) {
376-
$xForwardedHost = explode(',', $_SERVER['HTTP_X_FORWARDED_HOST']);
377-
if (
378-
isset($xForwardedHost[$xForwardedForRealIpKey])
379-
&& ($pair = $this->parseHostAndPort(trim($xForwardedHost[$xForwardedForRealIpKey])))
380-
) {
376+
$clientHop = $this->findClientHop(array_map(trim(...), explode(',', $_SERVER['HTTP_X_FORWARDED_FOR'])));
377+
if ($clientHop === null) {
378+
return null;
379+
}
380+
381+
if (!empty($_SERVER['HTTP_X_FORWARDED_HOST'])) {
382+
$hosts = explode(',', $_SERVER['HTTP_X_FORWARDED_HOST']);
383+
if (isset($hosts[$clientHop[0]]) && ($pair = $this->parseHostAndPort(trim($hosts[$clientHop[0]])))) {
381384
$url->setHost($pair[0]);
382385
if (isset($pair[1])) {
383386
$url->setPort($pair[1]);
384387
}
385388
}
386389
}
387390

388-
return $remoteAddr ?? null;
391+
return $clientHop[1];
392+
}
393+
394+
395+
/**
396+
* Returns [index, address] of the rightmost hop after stripping trailing trusted proxies,
397+
* or null when that hop is not a valid IP.
398+
* @param list<string> $addresses
399+
* @return array{int, string}|null
400+
*/
401+
private function findClientHop(array $addresses): ?array
402+
{
403+
$untrusted = array_filter(
404+
$addresses,
405+
fn(string $ip): bool => filter_var($ip, FILTER_VALIDATE_IP) === false
406+
|| !Arrays::some($this->proxies, fn(string $proxy): bool => Helpers::ipMatch($ip, $proxy)),
407+
);
408+
if (!$untrusted) {
409+
return null;
410+
}
411+
412+
$index = array_key_last($untrusted);
413+
return filter_var($untrusted[$index], FILTER_VALIDATE_IP) === false
414+
? null
415+
: [$index, $untrusted[$index]];
389416
}
390417

391418

‎tests/Http/RequestFactory.proxy.forwarded.phpt‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,3 +96,43 @@ test('forwarded protocol (HTTPS) handling', function () {
9696
$url = $factory->fromGlobals()->getUrl();
9797
Assert::same('https', $url->getScheme());
9898
});
99+
100+
101+
test('trusted proxies are stripped from the Forwarded chain (rightmost untrusted hop wins)', function () {
102+
$_SERVER = [
103+
'REMOTE_ADDR' => '10.0.0.2',
104+
'HTTP_FORWARDED' => 'for=23.75.45.200, for=172.16.0.1, for=10.0.0.1',
105+
];
106+
107+
$factory = new RequestFactory;
108+
$factory->setProxy('10.0.0.0/24');
109+
Assert::same('172.16.0.1', $factory->fromGlobals()->getRemoteAddress());
110+
});
111+
112+
113+
test('host and proto are resolved from the client hop, not from an injected element', function () {
114+
// the leftmost element is the one a client can forge by appending it
115+
$_SERVER = [
116+
'REMOTE_ADDR' => '10.0.0.2',
117+
'HTTP_FORWARDED' => 'for=6.6.6.6;host=evil.test;proto=http, for=172.16.0.1;host=real.test;proto=https, for=10.0.0.1',
118+
];
119+
120+
$factory = new RequestFactory;
121+
$factory->setProxy('10.0.0.0/24');
122+
$request = $factory->fromGlobals();
123+
Assert::same('172.16.0.1', $request->getRemoteAddress());
124+
Assert::same('real.test', $request->getUrl()->getHost());
125+
Assert::same('https', $request->getUrl()->getScheme());
126+
});
127+
128+
129+
test('obfuscated or invalid client identifier yields a null remote address', function () {
130+
$_SERVER = [
131+
'REMOTE_ADDR' => '10.0.0.1',
132+
'HTTP_FORWARDED' => 'for=unknown',
133+
];
134+
135+
$factory = new RequestFactory;
136+
$factory->setProxy('10.0.0.1');
137+
Assert::null($factory->fromGlobals()->getRemoteAddress());
138+
});

‎tests/Http/RequestFactory.proxy.x-forwarded.phpt‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,3 +106,14 @@ test('X-Forwarded-Host with multiple entries and port', function () {
106106
Assert::same('real', $url->getHost());
107107
Assert::same(8080, $url->getPort());
108108
});
109+
110+
test('X-Forwarded-For with a non-IP innermost value yields a null remote address', function () {
111+
$_SERVER = [
112+
'REMOTE_ADDR' => '10.0.0.1',
113+
'HTTP_X_FORWARDED_FOR' => 'not-an-ip',
114+
];
115+
116+
$factory = new RequestFactory;
117+
$factory->setProxy('10.0.0.1');
118+
Assert::null($factory->fromGlobals()->getRemoteAddress());
119+
});

0 commit comments

Comments
 (0)