Skip to content

Commit 4bf35cf

Browse files
SesquipedalianSesquipedalian
authored
Merge commit from fork
WebFetch: reject internal/loopback/link-local fetch targets (SSRF)
2 parents 57d30d5 + ce59add commit 4bf35cf

7 files changed

Lines changed: 141 additions & 24 deletions

File tree

‎Sources/IP.php‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,15 @@ public function __construct(self|string|null $ip)
100100
}
101101
// Is it in a valid IPv6 string?
102102
elseif (filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6) !== false) {
103-
// Pack and unpack to ensure it is in standard form.
104-
$this->ip = inet_ntop(inet_pton($ip));
103+
if (str_starts_with($ip, '64:ff9b::')) {
104+
// Workaround for a PHP bug where NAT64 addresses aren't handled
105+
// properly when the last 32 bits are in IPv6 notation instead
106+
// of IPv4 notation. See https://en.wikipedia.org/wiki/NAT64.
107+
$this->ip = '64:ff9b::' . inet_ntop(hex2bin(substr(bin2hex(inet_pton($ip)), -8)));
108+
} else {
109+
// Pack and unpack to ensure it is in standard form.
110+
$this->ip = inet_ntop(inet_pton($ip));
111+
}
105112
}
106113
// It's either in binary form or it's invalid.
107114
else {

‎Sources/ProxyServer.php‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -134,16 +134,13 @@ public function checkRequest(): bool
134134

135135
$request = new Url($_GET['request']);
136136

137-
// Basic sanity check.
138-
if (!$request->isValid()) {
139-
return false;
140-
}
141-
142-
// Just in case...
143137
if (
144-
filter_var($request->host, FILTER_VALIDATE_IP) !== false
145-
|| $request->host === 'localhost'
138+
// Basic sanity check.
139+
!$request->isValid()
140+
// Don't proxy our own resources.
146141
|| $request->host === Url::create(Config::$boardurl)->host
142+
// SSRF protection: don't proxy localhost, private or reserved IPs, etc.
143+
|| !WebFetchApi::isFetchSafe($request)
147144
) {
148145
return false;
149146
}

‎Sources/Url.php‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -494,9 +494,13 @@ public function proxied(): self
494494
|| $proxied->scheme === 'https'
495495
|| empty($proxied->host)
496496
|| empty($proxied->path)
497-
// Don't proxy localhost or IP addresses.
498-
|| $proxied->host === 'localhost'
499-
|| filter_var($proxied->host, FILTER_VALIDATE_IP) !== false
497+
// Don't proxy URLs with domains that aren't part of public DNS.
498+
|| preg_match('/\b(?' . '>example|local(?' . '>host)?|onion|test|alt|in(?' . '>ternal|valid))$/', $proxied->host)
499+
// Don't proxy URLs whose hosts are private or reserved IP addresses.
500+
|| (
501+
filter_var($proxied->host, FILTER_VALIDATE_IP) !== false
502+
&& filter_var($proxied->host, FILTER_VALIDATE_IP, FILTER_FLAG_GLOBAL_RANGE) === false
503+
)
500504
) {
501505
return $proxied;
502506
}

‎Sources/WebFetch/APIs/CurlFetcher.php‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -225,11 +225,12 @@ public function __construct(array $options = [], int $max_redirect = 3)
225225
*/
226226
public function request(string|Url $url, array|string $post_data = []): object
227227
{
228-
if (!$url instanceof Url) {
228+
if (!($url instanceof Url)) {
229229
$url = new Url($url, true);
230-
$url->toAscii();
231230
}
232231

232+
$url->toAscii();
233+
233234
// If we can't do it, bail out.
234235
if (!\function_exists('curl_init')) {
235236
$this->response[] = [
@@ -246,7 +247,7 @@ public function request(string|Url $url, array|string $post_data = []): object
246247
}
247248

248249
// Umm, this shouldn't happen?
249-
if (empty($url->scheme) || !\in_array($url->scheme, ['http', 'https'])) {
250+
if (!WebFetchApi::isFetchSafe($url, ['http', 'https'])) {
250251
trigger_error(Lang::getTxt('fetch_web_data_bad_url', [__METHOD__], file: 'Errors'), E_USER_NOTICE);
251252

252253
return $this;
@@ -434,6 +435,16 @@ private function setOptions(): void
434435
*/
435436
private function redirect(string $target_url, string $referrer_url): void
436437
{
438+
// SSRF guard: re-validate the redirect target before following it, so a
439+
// 302 -> http://127.0.0.1/ (or link-local cloud metadata) is refused.
440+
if (!WebFetchApi::isFetchSafe(Url::create($target_url, true))) {
441+
if (isset($this->response[$this->current_redirect - 1])) {
442+
$this->response[$this->current_redirect - 1]['success'] = false;
443+
}
444+
445+
return;
446+
}
447+
437448
// No, no, I last saw that over there... really, 301, 302, 307
438449
$this->setOptions();
439450
$this->options[CURLOPT_REFERER] = $referrer_url;

‎Sources/WebFetch/APIs/FtpFetcher.php‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,11 +108,12 @@ public function __construct(?string $user = null, ?string $email = null)
108108
*/
109109
public function request(string|Url $url, array|string $post_data = []): self
110110
{
111-
if (!$url instanceof Url) {
111+
if (!($url instanceof Url)) {
112112
$url = new Url($url, true);
113-
$url->toAscii();
114113
}
115114

115+
$url->toAscii();
116+
116117
// Umm, this shouldn't happen?
117118
if (empty($url->scheme) || !\in_array($url->scheme, ['ftp', 'ftps'])) {
118119
trigger_error(Lang::getTxt('fetch_web_data_bad_url', [__METHOD__], file: 'Errors'), E_USER_NOTICE);

‎Sources/WebFetch/APIs/SocketFetcher.php‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -164,13 +164,16 @@ public function __destruct()
164164
*/
165165
public function request(string|Url $url, array|string $post_data = []): object
166166
{
167-
if (!$url instanceof Url) {
167+
if (!($url instanceof Url)) {
168168
$url = new Url($url, true);
169-
$url->toAscii();
170169
}
171170

171+
$url->toAscii();
172+
172173
// Umm, this shouldn't happen?
173-
if (empty($url->scheme) || !\in_array($url->scheme, ['http', 'https'])) {
174+
if (!WebFetchApi::isFetchSafe($url, ['http', 'https'])) {
175+
$this->closeConnection();
176+
174177
trigger_error(Lang::getTxt('fetch_web_data_bad_url', [__METHOD__], file: 'Errors'), E_USER_NOTICE);
175178

176179
return $this;
@@ -260,8 +263,9 @@ public function request(string|Url $url, array|string $post_data = []): object
260263
return $this;
261264
}
262265

263-
// Close if it moved to a different host.
264-
if ($location->$host !== $url->host) {
266+
// Close if it moved to a different host. (The redirect target is
267+
// re-validated by the isFetchSafe() guard on the request() re-entry.)
268+
if ($location->host !== $url->host) {
265269
$this->closeConnection();
266270
}
267271

‎Sources/WebFetch/WebFetchApi.php‎

Lines changed: 94 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717

1818
use SMF\Lang;
1919
use SMF\Url;
20+
use SMF\IP;
2021

2122
/**
2223
* Class SearchApi
@@ -72,6 +73,13 @@ abstract class WebFetchApi implements WebFetchApiInterface
7273
*/
7374
private static array $still_alive = [];
7475

76+
/**
77+
* @var array
78+
*
79+
* Cache for the results of self::isFetchSafe()
80+
*/
81+
private static array $safe_hosts = [];
82+
7583
/****************
7684
* Public methods
7785
****************/
@@ -122,7 +130,17 @@ public function resultRaw(?int $response_number = null): array
122130
public static function fetch(Url|string $url, string|array $post_data = [], bool $keep_alive = false): string|false
123131
{
124132
if (!($url instanceof Url)) {
125-
$url = Url::create($url, true)->validate()->toAscii();
133+
$url = Url::create($url, true)->validate();
134+
}
135+
136+
$url->toAscii();
137+
138+
// SSRF guard: refuse loopback/private/link-local/reserved targets and
139+
// non-fetchable schemes before any connection is attempted.
140+
if (!self::isFetchSafe($url)) {
141+
trigger_error(Lang::getTxt('fetch_web_data_bad_url', [__METHOD__], file: 'Errors'), E_USER_NOTICE);
142+
143+
return false;
126144
}
127145

128146
// No scheme? No data for you!
@@ -174,6 +192,81 @@ public static function fetch(Url|string $url, string|array $post_data = [], bool
174192
return $fetcher->result('body');
175193
}
176194

195+
/**
196+
* Decides whether a URL is safe to fetch from the server.
197+
*
198+
* Rejects URLs whose scheme is not in the fetchable set, and URLs whose
199+
* host resolves (or is) a non-global IP address: loopback, private,
200+
* link-local (incl. 169.254.0.0/16 cloud metadata), or other reserved
201+
* ranges. This is the single chokepoint that prevents the avatar, proxy,
202+
* getMimeType, and task fetchers from being used as SSRF primitives. It is
203+
* also re-applied to each redirect target by the fetchers.
204+
*
205+
* @param \SMF\Url $url The URL to check.
206+
* @param array $allowed_schemes Optional list of allowed URL schemes.
207+
* If empty, all schemes that have handlers are allowed. Otherwise, only
208+
* URLs using the one of the specified schemes will be allowed.
209+
* Default: []
210+
* @return bool True if the URL is safe to fetch, false otherwise.
211+
*/
212+
public static function isFetchSafe(Url $url, array $allowed_schemes = []): bool
213+
{
214+
// Only known fetchable schemes.
215+
if (
216+
empty($url->scheme)
217+
|| !isset(self::$scheme_handlers[$url->scheme])
218+
|| (!empty($allowed_schemes) && !\in_array($url->scheme, $allowed_schemes))
219+
) {
220+
return false;
221+
}
222+
223+
if (
224+
// Must have a host.
225+
empty($url->host)
226+
// Reject reserved TLDs, since they are never in public DNS.
227+
|| preg_match('/\b(?' . '>example|local(?' . '>host)?|onion|test|alt|in(?' . '>ternal|valid))$/', $url->host)
228+
) {
229+
return false;
230+
}
231+
232+
// Avoid unnecessary repetition.
233+
if (isset(self::$safe_hosts[$url->host])) {
234+
return self::$safe_hosts[$url->host];
235+
}
236+
237+
// Resolve the host to its address(es). A literal IP resolves to itself.
238+
$ips = [];
239+
240+
if (filter_var($url->host, FILTER_VALIDATE_IP) !== false) {
241+
$ips[] = $url->host;
242+
} else {
243+
$records = @dns_get_record($url->host, DNS_A | DNS_AAAA);
244+
245+
foreach ((array) $records as $record) {
246+
if (!empty($record['ip'])) {
247+
$ips[] = $record['ip'];
248+
}
249+
250+
if (!empty($record['ipv6'])) {
251+
$ips[] = $record['ipv6'];
252+
}
253+
}
254+
}
255+
256+
if (
257+
// Couldn't resolve to anything: refuse rather than guess.
258+
$ips === []
259+
// EVERY resolved address must be a global, public, unicast IP.
260+
|| $ips !== array_filter($ips, fn($ip) => IP::create($ip)->isValid(FILTER_FLAG_GLOBAL_RANGE)),
261+
) {
262+
self::$safe_hosts[$url->host] = false;
263+
}
264+
265+
self::$safe_hosts[$url->host] ??= true;
266+
267+
return self::$safe_hosts[$url->host];
268+
}
269+
177270
/******************
178271
* Internal methods
179272
******************/

0 commit comments

Comments
 (0)