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 4677f32

Browse filesBrowse files
nicolas-grekasnicolas-grekas
committed
bug #59013 [HttpClient] Fix checking for private IPs before connecting (nicolas-grekas)
This PR was merged into the 5.4 branch. Discussion ---------- [HttpClient] Fix checking for private IPs before connecting | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | - | License | MIT The patch for [CVE-2024-50342](https://symfony.com/blog/cve-2024-50342-internal-address-and-port-enumeration-allowed-by-noprivatenetworkhttpclient) isn't enough. As reported by `@cs278`: > A DNS name which presents multiple A records can be used to bypass the new fix due to a TOCTOU condition where the decorator is performing the DNS lookup but curl is then performing its own DNS query and getting a private IP address. In addition: > A DNS record which presents a public A record and a private AAAA record can be used to workaround the protection measures if certain conditions are present As a first reminder, NoPrivateNetworkHttpClient DOES prevent requests to unwanted servers, but it doesn't prevent connecting to them, allowing to use timings to scan private network topologies. As a second reminder, [according to our security policy](https://symfony.com/doc/current/contributing/code/security.html#reporting-a-security-issue), we shouldn't have considered this as a security issue in the first place, but as a security hardening that could be handled without a CVE. Yet we did so we're going to update the already published CVE/advisory/blog post after this PR. This patch rewrites `NoPrivateNetworkHttpClient` to make it responsible for resolving DNS and following redirections. I tried using `CURLOPT_RESOLVE` instead, but this setting is ignored during a curl transaction apparently so we have no other choices. In addition, the decorator will refuse to connect to IPv6/IPv4 hosts if the configured `$subnets` doesn't block any IPs for the corresponding version of the protocol. Because the DNS check is made earlier, the decorator now also rejects relative URLs when the base URI is unknown to it. Commits ------- 90e6b7e [HttpClient] Fix checking for private IPs before connecting
2 parents 9eea677 + 90e6b7e commit 4677f32
Copy full SHA for 4677f32

File tree

Expand file treeCollapse file tree

9 files changed

+246
-76
lines changed
Filter options
Expand file treeCollapse file tree

9 files changed

+246
-76
lines changed

‎src/Symfony/Component/HttpClient/NativeHttpClient.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpClient/NativeHttpClient.php
+1-10Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -141,22 +141,13 @@ public function request(string $method, string $url, array $options = []): Respo
141141
// Memoize the last progress to ease calling the callback periodically when no network transfer happens
142142
$lastProgress = [0, 0];
143143
$maxDuration = 0 < $options['max_duration'] ? $options['max_duration'] : \INF;
144-
$multi = $this->multi;
145-
$resolve = static function (string $host, ?string $ip = null) use ($multi): ?string {
146-
if (null !== $ip) {
147-
$multi->dnsCache[$host] = $ip;
148-
}
149-
150-
return $multi->dnsCache[$host] ?? null;
151-
};
152-
$onProgress = static function (...$progress) use ($onProgress, &$lastProgress, &$info, $maxDuration, $resolve) {
144+
$onProgress = static function (...$progress) use ($onProgress, &$lastProgress, &$info, $maxDuration) {
153145
if ($info['total_time'] >= $maxDuration) {
154146
throw new TransportException(sprintf('Max duration was reached for "%s".', implode('', $info['url'])));
155147
}
156148

157149
$progressInfo = $info;
158150
$progressInfo['url'] = implode('', $info['url']);
159-
$progressInfo['resolve'] = $resolve;
160151
unset($progressInfo['size_body']);
161152

162153
if ($progress && -1 === $progress[0]) {

‎src/Symfony/Component/HttpClient/NoPrivateNetworkHttpClient.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpClient/NoPrivateNetworkHttpClient.php
+154-31Lines changed: 154 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,11 @@
1313

1414
use Psr\Log\LoggerAwareInterface;
1515
use Psr\Log\LoggerInterface;
16-
use Symfony\Component\HttpClient\Exception\InvalidArgumentException;
1716
use Symfony\Component\HttpClient\Exception\TransportException;
17+
use Symfony\Component\HttpClient\Response\AsyncContext;
18+
use Symfony\Component\HttpClient\Response\AsyncResponse;
1819
use Symfony\Component\HttpFoundation\IpUtils;
20+
use Symfony\Contracts\HttpClient\ChunkInterface;
1921
use Symfony\Contracts\HttpClient\HttpClientInterface;
2022
use Symfony\Contracts\HttpClient\ResponseInterface;
2123
use Symfony\Contracts\HttpClient\ResponseStreamInterface;
@@ -25,10 +27,12 @@
2527
* Decorator that blocks requests to private networks by default.
2628
*
2729
* @author Hallison Boaventura <hallisonboaventura@gmail.com>
30+
* @author Nicolas Grekas <p@tchwork.com>
2831
*/
2932
final class NoPrivateNetworkHttpClient implements HttpClientInterface, LoggerAwareInterface, ResetInterface
3033
{
3134
use HttpClientTrait;
35+
use AsyncDecoratorTrait;
3236

3337
private const PRIVATE_SUBNETS = [
3438
'127.0.0.0/8',
@@ -45,11 +49,14 @@ final class NoPrivateNetworkHttpClient implements HttpClientInterface, LoggerAwa
4549
'::/128',
4650
];
4751

52+
private $defaultOptions = self::OPTIONS_DEFAULTS;
4853
private $client;
4954
private $subnets;
55+
private $ipFlags;
56+
private $dnsCache;
5057

5158
/**
52-
* @param string|array|null $subnets String or array of subnets using CIDR notation that will be used by IpUtils.
59+
* @param string|array|null $subnets String or array of subnets using CIDR notation that should be considered private.
5360
* If null is passed, the standard private subnets will be used.
5461
*/
5562
public function __construct(HttpClientInterface $client, $subnets = null)
@@ -62,56 +69,113 @@ public function __construct(HttpClientInterface $client, $subnets = null)
6269
throw new \LogicException(sprintf('You cannot use "%s" if the HttpFoundation component is not installed. Try running "composer require symfony/http-foundation".', __CLASS__));
6370
}
6471

72+
if (null === $subnets) {
73+
$ipFlags = \FILTER_FLAG_IPV4 | \FILTER_FLAG_IPV6;
74+
} else {
75+
$ipFlags = 0;
76+
foreach ((array) $subnets as $subnet) {
77+
$ipFlags |= str_contains($subnet, ':') ? \FILTER_FLAG_IPV6 : \FILTER_FLAG_IPV4;
78+
}
79+
}
80+
81+
if (!\defined('STREAM_PF_INET6')) {
82+
$ipFlags &= ~\FILTER_FLAG_IPV6;
83+
}
84+
6585
$this->client = $client;
66-
$this->subnets = $subnets;
86+
$this->subnets = null !== $subnets ? (array) $subnets : null;
87+
$this->ipFlags = $ipFlags;
88+
$this->dnsCache = new \ArrayObject();
6789
}
6890

6991
/**
7092
* {@inheritdoc}
7193
*/
7294
public function request(string $method, string $url, array $options = []): ResponseInterface
7395
{
74-
$onProgress = $options['on_progress'] ?? null;
75-
if (null !== $onProgress && !\is_callable($onProgress)) {
76-
throw new InvalidArgumentException(sprintf('Option "on_progress" must be callable, "%s" given.', get_debug_type($onProgress)));
77-
}
96+
[$url, $options] = self::prepareRequest($method, $url, $options, $this->defaultOptions, true);
7897

79-
$subnets = $this->subnets;
80-
$lastUrl = '';
81-
$lastPrimaryIp = '';
98+
$redirectHeaders = parse_url($url['authority']);
99+
$host = $redirectHeaders['host'];
100+
$url = implode('', $url);
101+
$dnsCache = $this->dnsCache;
82102

83-
$options['on_progress'] = function (int $dlNow, int $dlSize, array $info) use ($onProgress, $subnets, &$lastUrl, &$lastPrimaryIp): void {
84-
if ($info['url'] !== $lastUrl) {
85-
$host = parse_url($info['url'], PHP_URL_HOST) ?: '';
86-
$resolve = $info['resolve'] ?? static function () { return null; };
87-
88-
if (($ip = trim($host, '[]'))
89-
&& !filter_var($ip, \FILTER_VALIDATE_IP)
90-
&& !($ip = $resolve($host))
91-
&& $ip = @(gethostbynamel($host)[0] ?? dns_get_record($host, \DNS_AAAA)[0]['ipv6'] ?? null)
92-
) {
93-
$resolve($host, $ip);
94-
}
103+
$ip = self::dnsResolve($dnsCache, $host, $this->ipFlags, $options);
104+
self::ipCheck($ip, $this->subnets, $this->ipFlags, $host, $url);
95105

96-
if ($ip && IpUtils::checkIp($ip, $subnets ?? self::PRIVATE_SUBNETS)) {
97-
throw new TransportException(sprintf('Host "%s" is blocked for "%s".', $host, $info['url']));
98-
}
106+
if (0 < $maxRedirects = $options['max_redirects']) {
107+
$options['max_redirects'] = 0;
108+
$redirectHeaders['with_auth'] = $redirectHeaders['no_auth'] = $options['headers'];
99109

100-
$lastUrl = $info['url'];
110+
if (isset($options['normalized_headers']['host']) || isset($options['normalized_headers']['authorization']) || isset($options['normalized_headers']['cookie'])) {
111+
$redirectHeaders['no_auth'] = array_filter($redirectHeaders['no_auth'], static function ($h) {
112+
return 0 !== stripos($h, 'Host:') && 0 !== stripos($h, 'Authorization:') && 0 !== stripos($h, 'Cookie:');
113+
});
101114
}
115+
}
102116

103-
if ($info['primary_ip'] !== $lastPrimaryIp) {
104-
if ($info['primary_ip'] && IpUtils::checkIp($info['primary_ip'], $subnets ?? self::PRIVATE_SUBNETS)) {
105-
throw new TransportException(sprintf('IP "%s" is blocked for "%s".', $info['primary_ip'], $info['url']));
106-
}
117+
$onProgress = $options['on_progress'] ?? null;
118+
$subnets = $this->subnets;
119+
$ipFlags = $this->ipFlags;
120+
$lastPrimaryIp = '';
107121

122+
$options['on_progress'] = static function (int $dlNow, int $dlSize, array $info) use ($onProgress, $subnets, $ipFlags, &$lastPrimaryIp): void {
123+
if (($info['primary_ip'] ?? '') !== $lastPrimaryIp) {
124+
self::ipCheck($info['primary_ip'], $subnets, $ipFlags, null, $info['url']);
108125
$lastPrimaryIp = $info['primary_ip'];
109126
}
110127

111128
null !== $onProgress && $onProgress($dlNow, $dlSize, $info);
112129
};
113130

114-
return $this->client->request($method, $url, $options);
131+
return new AsyncResponse($this->client, $method, $url, $options, static function (ChunkInterface $chunk, AsyncContext $context) use (&$method, &$options, $maxRedirects, &$redirectHeaders, $subnets, $ipFlags, $dnsCache): \Generator {
132+
if (null !== $chunk->getError() || $chunk->isTimeout() || !$chunk->isFirst()) {
133+
yield $chunk;
134+
135+
return;
136+
}
137+
138+
$statusCode = $context->getStatusCode();
139+
140+
if ($statusCode < 300 || 400 <= $statusCode || null === $url = $context->getInfo('redirect_url')) {
141+
$context->passthru();
142+
143+
yield $chunk;
144+
145+
return;
146+
}
147+
148+
$host = parse_url($url, \PHP_URL_HOST);
149+
$ip = self::dnsResolve($dnsCache, $host, $ipFlags, $options);
150+
self::ipCheck($ip, $subnets, $ipFlags, $host, $url);
151+
152+
// Do like curl and browsers: turn POST to GET on 301, 302 and 303
153+
if (303 === $statusCode || 'POST' === $method && \in_array($statusCode, [301, 302], true)) {
154+
$method = 'HEAD' === $method ? 'HEAD' : 'GET';
155+
unset($options['body'], $options['json']);
156+
157+
if (isset($options['normalized_headers']['content-length']) || isset($options['normalized_headers']['content-type']) || isset($options['normalized_headers']['transfer-encoding'])) {
158+
$filterContentHeaders = static function ($h) {
159+
return 0 !== stripos($h, 'Content-Length:') && 0 !== stripos($h, 'Content-Type:') && 0 !== stripos($h, 'Transfer-Encoding:');
160+
};
161+
$options['header'] = array_filter($options['header'], $filterContentHeaders);
162+
$redirectHeaders['no_auth'] = array_filter($redirectHeaders['no_auth'], $filterContentHeaders);
163+
$redirectHeaders['with_auth'] = array_filter($redirectHeaders['with_auth'], $filterContentHeaders);
164+
}
165+
}
166+
167+
// Authorization and Cookie headers MUST NOT follow except for the initial host name
168+
$options['headers'] = $redirectHeaders['host'] === $host ? $redirectHeaders['with_auth'] : $redirectHeaders['no_auth'];
169+
170+
static $redirectCount = 0;
171+
$context->setInfo('redirect_count', ++$redirectCount);
172+
173+
$context->replaceRequest($method, $url, $options);
174+
175+
if ($redirectCount >= $maxRedirects) {
176+
$context->passthru();
177+
}
178+
});
115179
}
116180

117181
/**
@@ -139,14 +203,73 @@ public function withOptions(array $options): self
139203
{
140204
$clone = clone $this;
141205
$clone->client = $this->client->withOptions($options);
206+
$clone->defaultOptions = self::mergeDefaultOptions($options, $this->defaultOptions);
142207

143208
return $clone;
144209
}
145210

146211
public function reset()
147212
{
213+
$this->dnsCache->exchangeArray([]);
214+
148215
if ($this->client instanceof ResetInterface) {
149216
$this->client->reset();
150217
}
151218
}
219+
220+
private static function dnsResolve(\ArrayObject $dnsCache, string $host, int $ipFlags, array &$options): string
221+
{
222+
if ($ip = filter_var(trim($host, '[]'), \FILTER_VALIDATE_IP) ?: $options['resolve'][$host] ?? false) {
223+
return $ip;
224+
}
225+
226+
if ($dnsCache->offsetExists($host)) {
227+
return $dnsCache[$host];
228+
}
229+
230+
if ((\FILTER_FLAG_IPV4 & $ipFlags) && $ip = gethostbynamel($host)) {
231+
return $options['resolve'][$host] = $dnsCache[$host] = $ip[0];
232+
}
233+
234+
if (!(\FILTER_FLAG_IPV6 & $ipFlags)) {
235+
return $host;
236+
}
237+
238+
if ($ip = dns_get_record($host, \DNS_AAAA)) {
239+
$ip = $ip[0]['ipv6'];
240+
} elseif (extension_loaded('sockets')) {
241+
if (!$info = socket_addrinfo_lookup($host, 0, ['ai_socktype' => \SOCK_STREAM, 'ai_family' => \AF_INET6])) {
242+
return $host;
243+
}
244+
245+
$ip = socket_addrinfo_explain($info[0])['ai_addr']['sin6_addr'];
246+
} elseif ('localhost' === $host || 'localhost.' === $host) {
247+
$ip = '::1';
248+
} else {
249+
return $host;
250+
}
251+
252+
return $options['resolve'][$host] = $dnsCache[$host] = $ip;
253+
}
254+
255+
private static function ipCheck(string $ip, ?array $subnets, int $ipFlags, ?string $host, string $url): void
256+
{
257+
if (null === $subnets) {
258+
// Quick check, but not reliable enough, see https://github.com/php/php-src/issues/16944
259+
$ipFlags |= \FILTER_FLAG_NO_PRIV_RANGE | \FILTER_FLAG_NO_RES_RANGE;
260+
}
261+
262+
if (false !== filter_var($ip, \FILTER_VALIDATE_IP, $ipFlags) && !IpUtils::checkIp($ip, $subnets ?? self::PRIVATE_SUBNETS)) {
263+
return;
264+
}
265+
266+
if (null !== $host) {
267+
$type = 'Host';
268+
} else {
269+
$host = $ip;
270+
$type = 'IP';
271+
}
272+
273+
throw new TransportException($type.\sprintf(' "%s" is blocked for "%s".', $host, $url));
274+
}
152275
}

‎src/Symfony/Component/HttpClient/Response/AmpResponse.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpClient/Response/AmpResponse.php
+1-9Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -89,17 +89,9 @@ public function __construct(AmpClientState $multi, Request $request, array $opti
8989
$info['max_duration'] = $options['max_duration'];
9090
$info['debug'] = '';
9191

92-
$resolve = static function (string $host, ?string $ip = null) use ($multi): ?string {
93-
if (null !== $ip) {
94-
$multi->dnsCache[$host] = $ip;
95-
}
96-
97-
return $multi->dnsCache[$host] ?? null;
98-
};
9992
$onProgress = $options['on_progress'] ?? static function () {};
100-
$onProgress = $this->onProgress = static function () use (&$info, $onProgress, $resolve) {
93+
$onProgress = $this->onProgress = static function () use (&$info, $onProgress) {
10194
$info['total_time'] = microtime(true) - $info['start_time'];
102-
$info['resolve'] = $resolve;
10395
$onProgress((int) $info['size_download'], ((int) (1 + $info['download_content_length']) ?: 1) - 1, (array) $info);
10496
};
10597

‎src/Symfony/Component/HttpClient/Response/CurlResponse.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpClient/Response/CurlResponse.php
+2-9Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -115,20 +115,13 @@ public function __construct(CurlClientState $multi, $ch, ?array $options = null,
115115
curl_pause($ch, \CURLPAUSE_CONT);
116116

117117
if ($onProgress = $options['on_progress']) {
118-
$resolve = static function (string $host, ?string $ip = null) use ($multi): ?string {
119-
if (null !== $ip) {
120-
$multi->dnsCache->hostnames[$host] = $ip;
121-
}
122-
123-
return $multi->dnsCache->hostnames[$host] ?? null;
124-
};
125118
$url = isset($info['url']) ? ['url' => $info['url']] : [];
126119
curl_setopt($ch, \CURLOPT_NOPROGRESS, false);
127-
curl_setopt($ch, \CURLOPT_PROGRESSFUNCTION, static function ($ch, $dlSize, $dlNow) use ($onProgress, &$info, $url, $multi, $debugBuffer, $resolve) {
120+
curl_setopt($ch, \CURLOPT_PROGRESSFUNCTION, static function ($ch, $dlSize, $dlNow) use ($onProgress, &$info, $url, $multi, $debugBuffer) {
128121
try {
129122
rewind($debugBuffer);
130123
$debug = ['debug' => stream_get_contents($debugBuffer)];
131-
$onProgress($dlNow, $dlSize, $url + curl_getinfo($ch) + $info + $debug + ['resolve' => $resolve]);
124+
$onProgress($dlNow, $dlSize, $url + curl_getinfo($ch) + $info + $debug);
132125
} catch (\Throwable $e) {
133126
$multi->handlesActivity[(int) $ch][] = null;
134127
$multi->handlesActivity[(int) $ch][] = $e;

‎src/Symfony/Component/HttpClient/Tests/AmpHttpClientTest.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpClient/Tests/AmpHttpClientTest.php
+3Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,9 @@
1414
use Symfony\Component\HttpClient\AmpHttpClient;
1515
use Symfony\Contracts\HttpClient\HttpClientInterface;
1616

17+
/**
18+
* @group dns-sensitive
19+
*/
1720
class AmpHttpClientTest extends HttpClientTestCase
1821
{
1922
protected function getHttpClient(string $testCase): HttpClientInterface

‎src/Symfony/Component/HttpClient/Tests/CurlHttpClientTest.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpClient/Tests/CurlHttpClientTest.php
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717

1818
/**
1919
* @requires extension curl
20+
* @group dns-sensitive
2021
*/
2122
class CurlHttpClientTest extends HttpClientTestCase
2223
{

‎src/Symfony/Component/HttpClient/Tests/HttpClientTestCase.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpClient/Tests/HttpClientTestCase.php
+33Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
namespace Symfony\Component\HttpClient\Tests;
1313

1414
use PHPUnit\Framework\SkippedTestSuiteError;
15+
use Symfony\Bridge\PhpUnit\DnsMock;
1516
use Symfony\Component\HttpClient\Exception\ClientException;
1617
use Symfony\Component\HttpClient\Exception\InvalidArgumentException;
1718
use Symfony\Component\HttpClient\Exception\TransportException;
@@ -490,6 +491,38 @@ public function testNoPrivateNetworkWithResolve()
490491
$client->request('GET', 'http://symfony.com', ['resolve' => ['symfony.com' => '127.0.0.1']]);
491492
}
492493

494+
public function testNoPrivateNetworkWithResolveAndRedirect()
495+
{
496+
DnsMock::withMockedHosts([
497+
'localhost' => [
498+
[
499+
'host' => 'localhost',
500+
'class' => 'IN',
501+
'ttl' => 15,
502+
'type' => 'A',
503+
'ip' => '127.0.0.1',
504+
],
505+
],
506+
'symfony.com' => [
507+
[
508+
'host' => 'symfony.com',
509+
'class' => 'IN',
510+
'ttl' => 15,
511+
'type' => 'A',
512+
'ip' => '10.0.0.1',
513+
],
514+
],
515+
]);
516+
517+
$client = $this->getHttpClient(__FUNCTION__);
518+
$client = new NoPrivateNetworkHttpClient($client, '10.0.0.1/32');
519+
520+
$this->expectException(TransportException::class);
521+
$this->expectExceptionMessage('Host "symfony.com" is blocked');
522+
523+
$client->request('GET', 'http://localhost:8057/302?location=https://symfony.com/');
524+
}
525+
493526
public function testNoRedirectWithInvalidLocation()
494527
{
495528
$client = $this->getHttpClient(__FUNCTION__);

‎src/Symfony/Component/HttpClient/Tests/NativeHttpClientTest.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpClient/Tests/NativeHttpClientTest.php
+3Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,9 @@
1414
use Symfony\Component\HttpClient\NativeHttpClient;
1515
use Symfony\Contracts\HttpClient\HttpClientInterface;
1616

17+
/**
18+
* @group dns-sensitive
19+
*/
1720
class NativeHttpClientTest extends HttpClientTestCase
1821
{
1922
protected function getHttpClient(string $testCase): HttpClientInterface

0 commit comments

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