From f1a9485032839a2441b307f426f66134bb22135a Mon Sep 17 00:00:00 2001 From: Jon Stovell Date: Tue, 18 Aug 2026 18:01:31 -0600 Subject: [PATCH 1/8] Always normalizes URLs in WebFetch APIs Signed-off-by: Jon Stovell --- Sources/WebFetch/APIs/CurlFetcher.php | 2 +- Sources/WebFetch/APIs/FtpFetcher.php | 2 +- Sources/WebFetch/APIs/SocketFetcher.php | 2 +- Sources/WebFetch/WebFetchApi.php | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Sources/WebFetch/APIs/CurlFetcher.php b/Sources/WebFetch/APIs/CurlFetcher.php index 9d209ff7e45..5436aab6781 100644 --- a/Sources/WebFetch/APIs/CurlFetcher.php +++ b/Sources/WebFetch/APIs/CurlFetcher.php @@ -229,7 +229,7 @@ public function request(string|Url $url, array|string $post_data = []): object $url = new Url($url, true); } - $url->toAscii(); + $url->normalize()->toAscii(); // If we can't do it, bail out. if (!\function_exists('curl_init')) { diff --git a/Sources/WebFetch/APIs/FtpFetcher.php b/Sources/WebFetch/APIs/FtpFetcher.php index 75bf22f639a..97df597957b 100644 --- a/Sources/WebFetch/APIs/FtpFetcher.php +++ b/Sources/WebFetch/APIs/FtpFetcher.php @@ -112,7 +112,7 @@ public function request(string|Url $url, array|string $post_data = []): self $url = new Url($url, true); } - $url->toAscii(); + $url->normalize()->toAscii(); // Umm, this shouldn't happen? if (empty($url->scheme) || !\in_array($url->scheme, ['ftp', 'ftps'])) { diff --git a/Sources/WebFetch/APIs/SocketFetcher.php b/Sources/WebFetch/APIs/SocketFetcher.php index 71ef9d2b2f5..6f6427f934b 100644 --- a/Sources/WebFetch/APIs/SocketFetcher.php +++ b/Sources/WebFetch/APIs/SocketFetcher.php @@ -168,7 +168,7 @@ public function request(string|Url $url, array|string $post_data = []): object $url = new Url($url, true); } - $url->toAscii(); + $url->normalize()->toAscii(); // Umm, this shouldn't happen? if (($url = WebFetchApi::makeSafe($url, ['http', 'https'])) === null) { diff --git a/Sources/WebFetch/WebFetchApi.php b/Sources/WebFetch/WebFetchApi.php index 91197cf7f14..e29d4740470 100644 --- a/Sources/WebFetch/WebFetchApi.php +++ b/Sources/WebFetch/WebFetchApi.php @@ -133,7 +133,7 @@ public static function fetch(Url|string $url, string|array $post_data = [], bool $url = Url::create($url, true)->validate(); } - $url->toAscii(); + $url->normalize()->toAscii(); // SSRF guard: refuse loopback/private/link-local/reserved targets and // non-fetchable schemes before any connection is attempted. From 637bbb36f36a8103ccbce7f8c0fe250a74476041 Mon Sep 17 00:00:00 2001 From: Jon Stovell Date: Tue, 18 Aug 2026 18:03:46 -0600 Subject: [PATCH 2/8] Uses SMF\Url rather than strings in SMF\WebFetch\APIs\CurlFetcher Signed-off-by: Jon Stovell --- Sources/WebFetch/APIs/CurlFetcher.php | 37 +++++++++++++-------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/Sources/WebFetch/APIs/CurlFetcher.php b/Sources/WebFetch/APIs/CurlFetcher.php index 5436aab6781..003eb1835f5 100644 --- a/Sources/WebFetch/APIs/CurlFetcher.php +++ b/Sources/WebFetch/APIs/CurlFetcher.php @@ -266,7 +266,7 @@ public function request(string|Url $url, array|string $post_data = []): object } $this->setOptions(); - $this->sendRequest(str_replace(' ', '%20', \strval($url))); + $this->sendRequest($url); return $this; } @@ -324,17 +324,17 @@ public function resultRaw(?int $response_number = null): array * - Detects 301, 302, 307 codes and will redirect to the given response * header location. * - * @param string $url The site to fetch. + * @param Url $url The site to fetch. * @param bool $redirect Whether or not this was a redirect request. */ - private function sendRequest(string $url, bool $redirect = false): void + private function sendRequest(Url $url, bool $redirect = false): void { // We do have a url, I hope. - if ($url == '') { + if ((string) $url == '') { return; } - $this->options[CURLOPT_URL] = $url; + $this->options[CURLOPT_URL] = (string) $url; // If we have not already been redirected, set it up so we can if needed. if (!$redirect) { @@ -350,14 +350,14 @@ private function sendRequest(string $url, bool $redirect = false): void // Get what was returned. $curl_info = curl_getinfo($cr); $curl_content = curl_multi_getcontent($cr); - $url = $curl_info['url']; // Last effective URL + $url = new Url($curl_info['url']); // Last effective URL $http_code = (string) $curl_info['http_code']; // Last HTTP code $body = (!curl_error($cr)) ? substr($curl_content, $curl_info['header_size']) : false; $error = (curl_error($cr)) ? curl_error($cr) : false; // Store this loop's data, someone may want all of these. :O $this->response[] = [ - 'url' => $url, + 'url' => (string) $url, 'success' => $error === false && $body !== false, 'code' => $http_code, 'error' => $error, @@ -377,24 +377,23 @@ private function sendRequest(string $url, bool $redirect = false): void /** * Used if being redirected to ensure we have a fully qualified address. * - * @param string $last_url The URL we went to. + * @param Url $last_url The URL we went to. * @param string $new_url The URL we were redirected to. - * @return string The new URL that was in the HTTP header. + * @return Url The new URL that was in the HTTP header. */ - private function getRedirectUrl(string $last_url = '', string $new_url = ''): string + private function getRedirectUrl(Url $last_url, string $new_url): Url { - // Get the elements for these urls. - $last_url_parse = parse_url($last_url); + // Get the elements for the new URL. $new_url_parse = parse_url($new_url); // Redirect headers are often incomplete or relative so we need to make sure they are fully qualified. - $new_url_parse['scheme'] = $new_url_parse['scheme'] ?? $last_url_parse['scheme']; - $new_url_parse['host'] = $new_url_parse['host'] ?? $last_url_parse['host']; - $new_url_parse['path'] = $new_url_parse['path'] ?? $last_url_parse['path']; + $new_url_parse['scheme'] = $new_url_parse['scheme'] ?? $last_url->scheme; + $new_url_parse['host'] = $new_url_parse['host'] ?? $last_url->host; + $new_url_parse['path'] = $new_url_parse['path'] ?? $last_url->path; $new_url_parse['query'] = $new_url_parse['query'] ?? ''; // Build the new URL that was in the http header. - return $new_url_parse['scheme'] . '://' . $new_url_parse['host'] . $new_url_parse['path'] . (!empty($new_url_parse['query']) ? '?' . $new_url_parse['query'] : ''); + return new URL($new_url_parse['scheme'] . '://' . $new_url_parse['host'] . $new_url_parse['path'] . (!empty($new_url_parse['query']) ? '?' . $new_url_parse['query'] : '')); } /** @@ -433,11 +432,11 @@ private function setOptions(): void * @param string $target_url The URL we want to redirect to. * @param string $referrer_url The URL that we're redirecting from. */ - private function redirect(string $target_url, string $referrer_url): void + private function redirect(Url $target_url, Url $referrer_url): void { // SSRF guard: re-validate the redirect target before following it, so a // 302 -> http://127.0.0.1/ (or link-local cloud metadata) is refused. - if (WebFetchApi::makeSafe(Url::create($target_url, true)) === null) { + if (WebFetchApi::makeSafe($target_url) === null) { if (isset($this->response[$this->current_redirect - 1])) { $this->response[$this->current_redirect - 1]['success'] = false; } @@ -447,7 +446,7 @@ private function redirect(string $target_url, string $referrer_url): void // No, no, I last saw that over there... really, 301, 302, 307 $this->setOptions(); - $this->options[CURLOPT_REFERER] = $referrer_url; + $this->options[CURLOPT_REFERER] = (string) $referrer_url; $this->sendRequest($target_url, true); } From 8288285c8a287a222d24233e2c07d0e6d542011c Mon Sep 17 00:00:00 2001 From: Jon Stovell Date: Tue, 18 Aug 2026 18:06:48 -0600 Subject: [PATCH 3/8] Implements SMF\Url::isFetchSafe() and SMF\Url::resolvesTo() Signed-off-by: Jon Stovell --- Sources/PackageManager/FtpConnection.php | 10 --- Sources/ProxyServer.php | 2 +- Sources/Subs-Compat.php | 37 ++++++++-- Sources/Url.php | 93 ++++++++++++++++++++++-- Sources/WebFetch/APIs/CurlFetcher.php | 2 +- Sources/WebFetch/APIs/FtpFetcher.php | 2 +- Sources/WebFetch/APIs/SocketFetcher.php | 6 +- Sources/WebFetch/WebFetchApi.php | 92 +---------------------- 8 files changed, 125 insertions(+), 119 deletions(-) diff --git a/Sources/PackageManager/FtpConnection.php b/Sources/PackageManager/FtpConnection.php index 995655d8473..4c5fc577e92 100644 --- a/Sources/PackageManager/FtpConnection.php +++ b/Sources/PackageManager/FtpConnection.php @@ -15,9 +15,6 @@ namespace SMF\PackageManager; -use SMF\Url; -use SMF\WebFetch\WebFetchApi; - /** * Class FtpConnection * Simple FTP protocol implementation. @@ -303,13 +300,6 @@ public function passive(): bool return false; } - // Let's just double check that... - if (!(WebFetchApi::makeSafe(new Url('ftp://' . $match[1] . '.' . $match[2] . '.' . $match[3] . '.' . $match[4] . ':' . ($match[5] * 256 + $match[6]))) instanceof Url)) { - $this->error = 'bad_server'; - - return false; - } - // This is pretty simple - store it for later use ;). $this->pasv = ['ip' => $match[1] . '.' . $match[2] . '.' . $match[3] . '.' . $match[4], 'port' => $match[5] * 256 + $match[6]]; diff --git a/Sources/ProxyServer.php b/Sources/ProxyServer.php index 4cd7caa5e30..e09c13f9d4f 100644 --- a/Sources/ProxyServer.php +++ b/Sources/ProxyServer.php @@ -140,7 +140,7 @@ public function checkRequest(): bool // Don't proxy our own resources. || $request->host === Url::create(Config::$boardurl)->host // SSRF protection: don't proxy localhost, private or reserved IPs, etc. - || ($request = WebFetchApi::makeSafe($request)) === null + || $request->isFetchSafe(['http', 'https']) ) { return false; } diff --git a/Sources/Subs-Compat.php b/Sources/Subs-Compat.php index 685226b581c..68a95736343 100644 --- a/Sources/Subs-Compat.php +++ b/Sources/Subs-Compat.php @@ -4005,9 +4005,7 @@ function iri_to_url(string $iri): string|bool } /** - * Checks whether a URL is safe to fetch from the server, and then returns - * either a version of the URL where the host has been resolved to a literal - * IP address, or else null if the URL was unsafe to fetch. + * Checks whether a URL is safe to fetch from the server. * * Rejects URLs whose scheme is not in the fetchable set, and URLs whose * host resolves (or is) a non-global IP address: loopback, private, @@ -4017,12 +4015,37 @@ function iri_to_url(string $iri): string|bool * is also re-applied to each redirect target by the fetchers. * * @param string $url The URL to check. - * @return string|null A version of $url where the host has been resolved to - * a literal IP address, or else null if the URL was unsafe to fetch. + * @return bool Whether the URL is safe to fetch. */ - function make_fetch_safe($url) + function is_fetch_safe($url) { - return SMF\WebFetch\WebFetchApi::makeSafe($url); + return SMF\Url::create($url)->isFetchSafe([]); + } + + /** + * Looks up the IP address(es) that the given URL's host resolves to. + * + * @param string $url The URL + * @return array The IP address(es). + */ + function get_ips_for_url($url) + { + return SMF\Url::create($url)->getIPs(); + } + + /** + * Checks whether the given URL resolves to the given IP address. + * + * If the URL resolves to multiple IP addresses, this function returns true + * if any of those IP addresses are the given one. + * + * @param string $url The URL + * @param string $ip The IP address. + * @return bool Whether this URL resolves to the given IP address. + */ + function url_resolves_to($url, $ip) + { + return SMF\Url::create($url)->resolvesTo(SMF\IP::create($ip)); } /** diff --git a/Sources/Url.php b/Sources/Url.php index 985a3a8c18a..c72416819b2 100644 --- a/Sources/Url.php +++ b/Sources/Url.php @@ -162,6 +162,13 @@ class Url implements \Stringable */ protected $is_ascii; + /** + * @var array + * + * Cache for $this->getIPs() + */ + protected array $ips; + /**************** * Public methods ****************/ @@ -526,26 +533,102 @@ public function proxied(): self */ public function getIPs(): array { + if (isset($this->ips)) { + return $this->ips; + } + + $is_ascii = $this->is_ascii; + + $this->toAscii(); + // Resolve the host to its address(es). A literal IP resolves to itself. - $ips = []; + $this->ips = []; if (filter_var(trim($this->host, '[]'), FILTER_VALIDATE_IP)) { - $ips[] = new IP(trim($this->host, '[]')); + $this->ips[] = new IP(trim($this->host, '[]')); } else { $records = @dns_get_record($this->host, DNS_A | DNS_AAAA); foreach ((array) $records as $record) { if (!empty($record['ip'])) { - $ips[] = new IP($record['ip']); + $this->ips[] = new IP($record['ip']); } if (!empty($record['ipv6'])) { - $ips[] = new IP($record['ipv6']); + $this->ips[] = new IP($record['ipv6']); } } } - return $ips; + if (!$is_ascii) { + $this->toUtf8(); + } + + return $this->ips; + } + + /** + * Checks whether this URL resolves to the given IP address. + * + * If this URL resolves to multiple IP addresses, this method returns true + * if any of those I{ addresses are the given one. + * + * @param \SMF\IP $ip The IP address to check + * @return bool + */ + public function resolvesTo(IP $ip): bool + { + foreach ($this->getIPs() as $known_ip) { + if ($ip == $known_ip) { + return true; + } + } + + return false; + } + + /** + * Checks whether it is safe for the server to fetch this URL. + * + * Rejects URLs whose scheme is not in the fetchable set, and URLs whose + * host resolves to (or is) a non-global IP address: loopback, private, + * link-local (incl. 169.254.0.0/16 cloud metadata), or other reserved + * ranges. + * + * @param array $allowed_schemes The URL schemes that the WebFetchApi is + * willing to use when fetching the content of this URL. + * @return bool Whether this URL is safe to fetch. + */ + public function isFetchSafe(array $allowed_schemes): bool + { + $is_ascii = $this->is_ascii; + + $this->toAscii(); + + if ( + // Only known fetchable schemes. + empty($url->scheme) + || !\in_array($url->scheme, $allowed_schemes) + // Must have a host. + || empty($url->host) + // Reject reserved TLDs, since they are never in public DNS. + || preg_match('/\b(?' . '>example|local(?' . '>host)?|onion|test|alt|in(?' . '>ternal|valid))$/', $url->host) + ) { + $is_safe = false; + } else { + $ips = $this->getIPs(); + + $is_safe = $ips === array_filter( + $ips, + fn($ip) => $ip->isValid(FILTER_FLAG_GLOBAL_RANGE), + ); + } + + if (!$is_ascii) { + $this->toUtf8(); + } + + return $is_safe; } /** diff --git a/Sources/WebFetch/APIs/CurlFetcher.php b/Sources/WebFetch/APIs/CurlFetcher.php index 003eb1835f5..e63c1a34cb4 100644 --- a/Sources/WebFetch/APIs/CurlFetcher.php +++ b/Sources/WebFetch/APIs/CurlFetcher.php @@ -247,7 +247,7 @@ public function request(string|Url $url, array|string $post_data = []): object } // Umm, this shouldn't happen? - if (($url = WebFetchApi::makeSafe($url, ['http', 'https'])) === null) { + if (!$url->isFetchSafe(['http', 'https'])) { trigger_error(Lang::getTxt('fetch_web_data_bad_url', [__METHOD__], file: 'Errors'), E_USER_NOTICE); return $this; diff --git a/Sources/WebFetch/APIs/FtpFetcher.php b/Sources/WebFetch/APIs/FtpFetcher.php index 97df597957b..5a665658fcd 100644 --- a/Sources/WebFetch/APIs/FtpFetcher.php +++ b/Sources/WebFetch/APIs/FtpFetcher.php @@ -115,7 +115,7 @@ public function request(string|Url $url, array|string $post_data = []): self $url->normalize()->toAscii(); // Umm, this shouldn't happen? - if (empty($url->scheme) || !\in_array($url->scheme, ['ftp', 'ftps'])) { + if (!$url->isFetchSafe(['ftp', 'ftps'])) { trigger_error(Lang::getTxt('fetch_web_data_bad_url', [__METHOD__], file: 'Errors'), E_USER_NOTICE); return $this; diff --git a/Sources/WebFetch/APIs/SocketFetcher.php b/Sources/WebFetch/APIs/SocketFetcher.php index 6f6427f934b..5f7078de3e1 100644 --- a/Sources/WebFetch/APIs/SocketFetcher.php +++ b/Sources/WebFetch/APIs/SocketFetcher.php @@ -15,6 +15,7 @@ namespace SMF\WebFetch\APIs; +use SMF\IP; use SMF\Lang; use SMF\Url; use SMF\WebFetch\WebFetchApi; @@ -171,7 +172,7 @@ public function request(string|Url $url, array|string $post_data = []): object $url->normalize()->toAscii(); // Umm, this shouldn't happen? - if (($url = WebFetchApi::makeSafe($url, ['http', 'https'])) === null) { + if (!$url->isFetchSafe(['http', 'https'])) { $this->closeConnection(); trigger_error(Lang::getTxt('fetch_web_data_bad_url', [__METHOD__], file: 'Errors'), E_USER_NOTICE); @@ -263,8 +264,7 @@ public function request(string|Url $url, array|string $post_data = []): object return $this; } - // Close if it moved to a different host. (The redirect target is - // re-validated by the makeSafe() guard on the request() re-entry.) + // Close if it moved to a different host. if ($location->host !== $url->host) { $this->closeConnection(); } diff --git a/Sources/WebFetch/WebFetchApi.php b/Sources/WebFetch/WebFetchApi.php index e29d4740470..a829de09ab5 100644 --- a/Sources/WebFetch/WebFetchApi.php +++ b/Sources/WebFetch/WebFetchApi.php @@ -15,7 +15,6 @@ namespace SMF\WebFetch; -use SMF\IP; use SMF\Lang; use SMF\Url; @@ -73,13 +72,6 @@ abstract class WebFetchApi implements WebFetchApiInterface */ private static array $still_alive = []; - /** - * @var array - * - * Cache for the results of self::makeSafe() - */ - private static array $resolved_hosts = []; - /**************** * Public methods ****************/ @@ -137,14 +129,7 @@ public static function fetch(Url|string $url, string|array $post_data = [], bool // SSRF guard: refuse loopback/private/link-local/reserved targets and // non-fetchable schemes before any connection is attempted. - if (($url = WebFetchApi::makeSafe($url)) === null) { - trigger_error(Lang::getTxt('fetch_web_data_bad_url', [__METHOD__], file: 'Errors'), E_USER_NOTICE); - - return false; - } - - // No scheme? No data for you! - if (empty($url->scheme) || !isset(self::$scheme_handlers[$url->scheme])) { + if (!$url->isFetchSafe(array_keys(self::$scheme_handlers))) { trigger_error(Lang::getTxt('fetch_web_data_bad_url', [__METHOD__], file: 'Errors'), E_USER_NOTICE); return false; @@ -193,81 +178,6 @@ public static function fetch(Url|string $url, string|array $post_data = [], bool return $fetcher->result('body'); } - /** - * Checks whether a URL is safe to fetch from the server, and then returns - * either a version of the URL where the host has been resolved to a literal - * IP address, or else null if the URL was unsafe to fetch. - * - * Rejects URLs whose scheme is not in the fetchable set, and URLs whose - * host resolves (or is) a non-global IP address: loopback, private, - * link-local (incl. 169.254.0.0/16 cloud metadata), or other reserved - * ranges. This is the single chokepoint that prevents the avatar, proxy, - * getMimeType, and task fetchers from being used as SSRF primitives. It is - * also re-applied to each redirect target by the fetchers. - * - * @param \SMF\Url $url The URL to check. - * @param array $allowed_schemes Optional list of allowed URL schemes. - * If empty, all schemes that have handlers are allowed. Otherwise, only - * URLs using the one of the specified schemes will be allowed. - * Default: [] - * @return ?Url A version of $url where the host has been resolved to a - * literal IP address, or else null if the URL was unsafe to fetch. - */ - public static function makeSafe(Url $url, array $allowed_schemes = []): ?Url - { - $url->toAscii(); - - if ( - // Only known fetchable schemes. - empty($url->scheme) - || !isset(self::$scheme_handlers[$url->scheme]) - || (!empty($allowed_schemes) && !\in_array($url->scheme, $allowed_schemes)) - // Must have a host. - || empty($url->host) - // Reject reserved TLDs, since they are never in public DNS. - || preg_match('/\b(?' . '>example|local(?' . '>host)?|onion|test|alt|in(?' . '>ternal|valid))$/', $url->host) - ) { - return null; - } - - // Avoid unnecessary repetition. - if (isset(self::$resolved_hosts[$url->host])) { - if (empty(self::$resolved_hosts[$url->host])) { - return null; - } - - return new Url( - preg_replace( - '/' . preg_quote($url->host) . '/', - self::$resolved_hosts[$url->host][0], - (string) $url, - 1, - ), - ); - } - - self::$resolved_hosts[$url->host] = array_values(array_map( - fn($ip) => $ip->isValid(FILTER_FLAG_IPV6) ? '[' . (string) $ip . ']' : (string) $ip, - array_filter( - $url->getIPs(), - fn($ip) => $ip->isValid(FILTER_FLAG_GLOBAL_RANGE), - ), - )); - - if (empty(self::$resolved_hosts[$url->host])) { - return null; - } - - return new Url( - preg_replace( - '/' . preg_quote($url->host) . '/', - self::$resolved_hosts[$url->host][0], - (string) $url, - 1, - ), - ); - } - /****************** * Internal methods ******************/ From c5ebb08fee43e570f2e7b3bc5bd6c1e5832ee25d Mon Sep 17 00:00:00 2001 From: Jon Stovell Date: Tue, 18 Aug 2026 18:40:10 -0600 Subject: [PATCH 4/8] Double checks connected IP address in WebFetch APIs Signed-off-by: Jon Stovell --- Sources/WebFetch/APIs/CurlFetcher.php | 19 +++++++++++++++++-- Sources/WebFetch/APIs/FtpFetcher.php | 17 +++++++++++++++++ Sources/WebFetch/APIs/SocketFetcher.php | 16 ++++++++++++++++ 3 files changed, 50 insertions(+), 2 deletions(-) diff --git a/Sources/WebFetch/APIs/CurlFetcher.php b/Sources/WebFetch/APIs/CurlFetcher.php index e63c1a34cb4..51fa96e47ee 100644 --- a/Sources/WebFetch/APIs/CurlFetcher.php +++ b/Sources/WebFetch/APIs/CurlFetcher.php @@ -349,9 +349,20 @@ private function sendRequest(Url $url, bool $redirect = false): void // Get what was returned. $curl_info = curl_getinfo($cr); - $curl_content = curl_multi_getcontent($cr); + + // Double check that we connected to the expected IP. + if (!$url->resolvesTo(new IP(trim($curl_info['primary_ip'], '[]')))) { + $this->response[$this->current_redirect]['success'] = false; + + trigger_error(Lang::getTxt('fetch_web_data_bad_url', [__METHOD__], file: 'Errors'), E_USER_NOTICE); + + return; + } + $url = new Url($curl_info['url']); // Last effective URL $http_code = (string) $curl_info['http_code']; // Last HTTP code + + $curl_content = curl_multi_getcontent($cr); $body = (!curl_error($cr)) ? substr($curl_content, $curl_info['header_size']) : false; $error = (curl_error($cr)) ? curl_error($cr) : false; @@ -367,7 +378,11 @@ private function sendRequest(Url $url, bool $redirect = false): void ]; // If this a redirect with a location header and we have not given up, then do it again. - if (preg_match('~30[127]~i', $http_code) === 1 && $this->headers['location'] != '' && $this->current_redirect <= $this->max_redirect) { + if ( + preg_match('~30[127]~i', $http_code) + && $this->headers['location'] != '' + && $this->current_redirect <= $this->max_redirect + ) { $this->current_redirect++; $header_location = $this->getRedirectUrl($url, $this->headers['location']); $this->redirect($header_location, $url); diff --git a/Sources/WebFetch/APIs/FtpFetcher.php b/Sources/WebFetch/APIs/FtpFetcher.php index 5a665658fcd..33ebdc2a33c 100644 --- a/Sources/WebFetch/APIs/FtpFetcher.php +++ b/Sources/WebFetch/APIs/FtpFetcher.php @@ -160,6 +160,23 @@ public function request(string|Url $url, array|string $post_data = []): self return $this; } + // Double check that we connected to the expected IP and port. + // If the connection was successful, name will be ":" + $socket_name = @stream_socket_get_name($fp, true); + + if ( + !\is_string($socket_name) + || !str_ends_with($socket_name, ':' . $ftp->pasv['port']) + || $ftp->pasv['ip'] != substr($socket_name, 0, -\strlen(':' . $ftp->pasv['port'])) + || !$url->resolvesTo(new IP($ftp->pasv['ip'])) + ) { + fclose($fp); + + trigger_error(Lang::getTxt('fetch_web_data_bad_url', [__METHOD__], file: 'Errors'), E_USER_NOTICE); + + return $this; + } + // The server should now say something in acknowledgement. $ftp->check_response(150); $this->response[0]['code'] = substr($ftp->last_message, 0, 3); diff --git a/Sources/WebFetch/APIs/SocketFetcher.php b/Sources/WebFetch/APIs/SocketFetcher.php index 5f7078de3e1..6404e7a5a5e 100644 --- a/Sources/WebFetch/APIs/SocketFetcher.php +++ b/Sources/WebFetch/APIs/SocketFetcher.php @@ -214,6 +214,22 @@ public function request(string|Url $url, array|string $post_data = []): object return $this; } + // Double check that we connected to the expected IP and port. + // If the connection was successful, name will be ":" + $socket_name = @stream_socket_get_name($this->fp, true); + + if ( + !\is_string($socket_name) + || !str_ends_with($socket_name, ':' . $this->port) + || !$url->resolvesTo(new IP(trim(substr($socket_name, 0, -\strlen(':' . $this->port)), '[]'))) + ) { + $this->closeConnection(); + + trigger_error(Lang::getTxt('fetch_web_data_bad_url', [__METHOD__], file: 'Errors'), E_USER_NOTICE); + + return $this; + } + // I want this, from there, and I may or may not bother you for more later. if (empty($post_data)) { fwrite($this->fp, 'GET ' . $path_and_query . ' HTTP/1.1' . $this->line_break) || throw new \Exception('Failed to write to socket'); From 725b6980e1aa02c1b9e09f3f032e36f176dfc1aa Mon Sep 17 00:00:00 2001 From: Jon Stovell Date: Tue, 18 Aug 2026 20:37:36 -0600 Subject: [PATCH 5/8] Simplifies checks in Url::proxied() Signed-off-by: Jon Stovell --- Sources/Url.php | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/Sources/Url.php b/Sources/Url.php index c72416819b2..02b821ce9af 100644 --- a/Sources/Url.php +++ b/Sources/Url.php @@ -495,12 +495,7 @@ public function proxied(): self || empty($proxied->host) || empty($proxied->path) // Don't proxy URLs with domains that aren't part of public DNS. - || preg_match('/\b(?' . '>example|local(?' . '>host)?|onion|test|alt|in(?' . '>ternal|valid))$/', $proxied->host) - // Don't proxy URLs whose hosts are private or reserved IP addresses. - || ( - filter_var(trim($proxied->host, '[]'), FILTER_VALIDATE_IP) !== false - && filter_var(trim($proxied->host, '[]'), FILTER_VALIDATE_IP, FILTER_FLAG_GLOBAL_RANGE) === false - ) + || !$proxied->isFetchSafe() ) { return $proxied; } From 7152421b9599aebb85b6a0dfcf76743ae7d2da9f Mon Sep 17 00:00:00 2001 From: albertlast Date: Wed, 19 Aug 2026 08:10:49 +0200 Subject: [PATCH 6/8] Fixes SMF\Url::isFetchSafe() so that it looks at this URL The method read $url throughout, which is never assigned, so every check ran against null and the method always returned false. That is why nothing could be fetched at all: the guard rejected the URL before any connection was attempted. While in here: - An empty $allowed_schemes now means "any scheme the WebFetchApi has a handler for", which is what WebFetchApi::makeSafe() meant by it, and what the callers that pass nothing are expecting. Without this, the parameter had no sensible value for Url and Subs-Compat to pass. - A host that resolves to nothing is no longer treated as safe. An empty array trivially equals its own array_filter(), and dns_get_record() never sees names that only exist in the system's hosts file, so http://someinternalbox/ was passing the guard. - getIPs() remembers what a host resolved to for the rest of the request. WebFetchApi::$resolved_hosts used to do this. It matters for more than lookup counts now: the post-connection checks in the fetchers are only meaningful if they compare against what we resolved *before* connecting, otherwise a rebinding attack simply answers twice. - parse() drops those addresses, since the host may be about to change. - resolvesTo() compares the addresses rather than the objects. A loose comparison also weighs IP::$host, which either side may have filled in with a reverse lookup. - proxied() goes back to the cheap test it had before. It runs for every image in every post, and it only decides whether to route through proxy.php, which does its own checking. Making it depend on the resolver means a DNS hiccup silently stops proxying. Co-Authored-By: Claude Opus 5 Signed-off-by: albertlast --- Sources/Url.php | 67 +++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 57 insertions(+), 10 deletions(-) diff --git a/Sources/Url.php b/Sources/Url.php index 02b821ce9af..d087fb227e1 100644 --- a/Sources/Url.php +++ b/Sources/Url.php @@ -169,6 +169,17 @@ class Url implements \Stringable */ protected array $ips; + /**************************** + * Internal static properties + ****************************/ + + /** + * @var array + * + * Cache of the IP addresses that hosts resolved to during this request. + */ + protected static array $resolved_hosts = []; + /**************** * Public methods ****************/ @@ -429,6 +440,10 @@ function ($matches) { $parsed = parse_url($url); + // The host may be about to change, so the addresses we resolved for the + // old one no longer describe this URL. + unset($this->ips); + foreach (['scheme', 'host', 'port', 'user', 'pass', 'path', 'query', 'fragment'] as $prop) { // Clear out any old value. unset($this->{$prop}); @@ -495,7 +510,12 @@ public function proxied(): self || empty($proxied->host) || empty($proxied->path) // Don't proxy URLs with domains that aren't part of public DNS. - || !$proxied->isFetchSafe() + || preg_match('/\b(?' . '>example|local(?' . '>host)?|onion|test|alt|in(?' . '>ternal|valid))$/', $proxied->host) + // Don't proxy URLs whose hosts are private or reserved IP addresses. + || ( + filter_var(trim($proxied->host, '[]'), FILTER_VALIDATE_IP) !== false + && filter_var(trim($proxied->host, '[]'), FILTER_VALIDATE_IP, FILTER_FLAG_GLOBAL_RANGE) === false + ) ) { return $proxied; } @@ -536,6 +556,17 @@ public function getIPs(): array $this->toAscii(); + // Someone else already asked about this host during this request. + if (isset(self::$resolved_hosts[$this->host])) { + $this->ips = self::$resolved_hosts[$this->host]; + + if (!$is_ascii) { + $this->toUtf8(); + } + + return $this->ips; + } + // Resolve the host to its address(es). A literal IP resolves to itself. $this->ips = []; @@ -555,6 +586,12 @@ public function getIPs(): array } } + // Remember it. Besides saving lookups, this is what makes the + // post-connection checks in the WebFetch APIs meaningful: they compare + // against what the host resolved to *before* we connected, so a DNS + // rebinding attack can't answer differently the second time around. + self::$resolved_hosts[$this->host] = $this->ips; + if (!$is_ascii) { $this->toUtf8(); } @@ -566,15 +603,18 @@ public function getIPs(): array * Checks whether this URL resolves to the given IP address. * * If this URL resolves to multiple IP addresses, this method returns true - * if any of those I{ addresses are the given one. + * if any of those IP addresses are the given one. * * @param \SMF\IP $ip The IP address to check * @return bool */ public function resolvesTo(IP $ip): bool { + // Compare the addresses themselves. A loose comparison of the objects + // would also weigh IP::$host, which either side may have populated + // with a reverse lookup. foreach ($this->getIPs() as $known_ip) { - if ($ip == $known_ip) { + if ((string) $ip === (string) $known_ip) { return true; } } @@ -591,29 +631,36 @@ public function resolvesTo(IP $ip): bool * ranges. * * @param array $allowed_schemes The URL schemes that the WebFetchApi is - * willing to use when fetching the content of this URL. + * willing to use when fetching the content of this URL. If empty, any + * scheme that the WebFetchApi has a handler for is allowed. + * Default: [] * @return bool Whether this URL is safe to fetch. */ - public function isFetchSafe(array $allowed_schemes): bool + public function isFetchSafe(array $allowed_schemes = []): bool { + $allowed_schemes = $allowed_schemes === [] ? array_keys(WebFetchApi::$scheme_handlers) : $allowed_schemes; + $is_ascii = $this->is_ascii; $this->toAscii(); if ( // Only known fetchable schemes. - empty($url->scheme) - || !\in_array($url->scheme, $allowed_schemes) + empty($this->scheme) + || !\in_array($this->scheme, $allowed_schemes) // Must have a host. - || empty($url->host) + || empty($this->host) // Reject reserved TLDs, since they are never in public DNS. - || preg_match('/\b(?' . '>example|local(?' . '>host)?|onion|test|alt|in(?' . '>ternal|valid))$/', $url->host) + || preg_match('/\b(?' . '>example|local(?' . '>host)?|onion|test|alt|in(?' . '>ternal|valid))$/', $this->host) ) { $is_safe = false; } else { $ips = $this->getIPs(); - $is_safe = $ips === array_filter( + // A host that resolves to nothing is not safe. We cannot know what + // the connection would actually reach, and dns_get_record() never + // sees names that only exist in the system's hosts file. + $is_safe = $ips !== [] && $ips === array_filter( $ips, fn($ip) => $ip->isValid(FILTER_FLAG_GLOBAL_RANGE), ); From 8a1de3381a91bf753b24f41c16eef4a79aff5b3c Mon Sep 17 00:00:00 2001 From: albertlast Date: Wed, 19 Aug 2026 08:11:05 +0200 Subject: [PATCH 7/8] Restores the fetch guards, and pins curl to an address we vetted Four of them had stopped guarding anything: - ProxyServer::checkRequest() had the test the wrong way round, so proxy.php refused every public image and happily fetched the private addresses the check exists to keep it away from. - CurlFetcher::redirect() still called WebFetchApi::makeSafe(), which is gone, so following any 301, 302 or 307 was a fatal error rather than a re-validated hop. - CurlFetcher and FtpFetcher never imported SMF\IP, so the new post-connection checks resolved SMF\WebFetch\APIs\IP and fatalled on every request that got as far as them. - FtpConnection::passive() no longer looked at the address the server asked us to connect to next, which is the whole point of the check: an FTP bounce points it at the local network. FtpFetcher does check, but the package manager and the maintenance tools use FtpConnection directly and never go near FtpFetcher. Then curl gets to do this properly. Comparing curl_getinfo()['primary_ip'] after the fact only tells us where we already went. CURLOPT_RESOLVE tells curl up front which address it is allowed to use for this host, so the URL keeps its name and SNI, the Host header and certificate validation all still see the real one. The check afterwards stays as a second opinion for builds that ignore the option, but it no longer writes its failure into a response slot that does not exist yet. Lastly, result() returns null instead of reading index -1 when a request was refused before anything was recorded. WebFetchApi::fetch() asks for result('success') on that path, so every refusal emitted a warning. Co-Authored-By: Claude Opus 5 Signed-off-by: albertlast --- Sources/PackageManager/FtpConnection.php | 16 +++++++- Sources/ProxyServer.php | 2 +- Sources/WebFetch/APIs/CurlFetcher.php | 47 ++++++++++++++++++++++-- Sources/WebFetch/APIs/FtpFetcher.php | 1 + Sources/WebFetch/APIs/SocketFetcher.php | 6 +++ 5 files changed, 66 insertions(+), 6 deletions(-) diff --git a/Sources/PackageManager/FtpConnection.php b/Sources/PackageManager/FtpConnection.php index 4c5fc577e92..16bf355d134 100644 --- a/Sources/PackageManager/FtpConnection.php +++ b/Sources/PackageManager/FtpConnection.php @@ -15,6 +15,8 @@ namespace SMF\PackageManager; +use SMF\IP; + /** * Class FtpConnection * Simple FTP protocol implementation. @@ -300,8 +302,20 @@ public function passive(): bool return false; } + $pasv_ip = $match[1] . '.' . $match[2] . '.' . $match[3] . '.' . $match[4]; + + // The server told us where to connect next, so don't take its word for + // it. An FTP bounce points that at something on the local network. + // FtpFetcher checks this too, but the package manager and the + // maintenance tools use this class directly. + if (!(new IP($pasv_ip))->isValid(FILTER_FLAG_GLOBAL_RANGE)) { + $this->error = 'bad_server'; + + return false; + } + // This is pretty simple - store it for later use ;). - $this->pasv = ['ip' => $match[1] . '.' . $match[2] . '.' . $match[3] . '.' . $match[4], 'port' => $match[5] * 256 + $match[6]]; + $this->pasv = ['ip' => $pasv_ip, 'port' => $match[5] * 256 + $match[6]]; return true; } diff --git a/Sources/ProxyServer.php b/Sources/ProxyServer.php index e09c13f9d4f..212ce4e4556 100644 --- a/Sources/ProxyServer.php +++ b/Sources/ProxyServer.php @@ -140,7 +140,7 @@ public function checkRequest(): bool // Don't proxy our own resources. || $request->host === Url::create(Config::$boardurl)->host // SSRF protection: don't proxy localhost, private or reserved IPs, etc. - || $request->isFetchSafe(['http', 'https']) + || !$request->isFetchSafe(['http', 'https']) ) { return false; } diff --git a/Sources/WebFetch/APIs/CurlFetcher.php b/Sources/WebFetch/APIs/CurlFetcher.php index 51fa96e47ee..555ea908085 100644 --- a/Sources/WebFetch/APIs/CurlFetcher.php +++ b/Sources/WebFetch/APIs/CurlFetcher.php @@ -15,6 +15,7 @@ namespace SMF\WebFetch\APIs; +use SMF\IP; use SMF\Lang; use SMF\Url; use SMF\WebFetch\WebFetchApi; @@ -284,6 +285,12 @@ public function result(?string $area = null): mixed { $max_result = \count($this->response) - 1; + // Nothing was recorded, because the request was refused before we ever + // got as far as making it. + if ($max_result < 0) { + return null; + } + // Just return a specified area or the entire result? if (empty($area)) { return $this->response[$max_result]; @@ -336,6 +343,25 @@ private function sendRequest(Url $url, bool $redirect = false): void $this->options[CURLOPT_URL] = (string) $url; + // Pin the connection to an address that we already vetted. The URL + // keeps its host name, so SNI, the Host header and the certificate + // check all still see the real name; curl just isn't allowed to ask + // the resolver a second time and get a different answer. + if (filter_var(trim($url->host, '[]'), FILTER_VALIDATE_IP) === false) { + $ips = array_map(fn($ip) => (string) $ip, $url->getIPs()); + + // Listing several addresses in one entry needs curl 7.59.0. + if (version_compare(curl_version()['version'], '7.59.0', '<')) { + $ips = \array_slice($ips, 0, 1); + } + + if ($ips !== []) { + $port = !empty($url->port) ? $url->port : ($url->scheme === 'https' ? 443 : 80); + + $this->options[CURLOPT_RESOLVE] = [$url->host . ':' . $port . ':' . implode(',', $ips)]; + } + } + // If we have not already been redirected, set it up so we can if needed. if (!$redirect) { $this->current_redirect = 1; @@ -350,9 +376,22 @@ private function sendRequest(Url $url, bool $redirect = false): void // Get what was returned. $curl_info = curl_getinfo($cr); - // Double check that we connected to the expected IP. - if (!$url->resolvesTo(new IP(trim($curl_info['primary_ip'], '[]')))) { - $this->response[$this->current_redirect]['success'] = false; + // Double check the address we actually connected to, in case this build + // of curl ignored CURLOPT_RESOLVE. An empty value means we never got a + // connection at all, which the normal error handling below reports. + if ( + $curl_info['primary_ip'] !== '' + && !$url->resolvesTo(new IP(trim($curl_info['primary_ip'], '[]'))) + ) { + $this->response[] = [ + 'url' => (string) $url, + 'success' => false, + 'code' => null, + 'error' => null, + 'headers' => [], + 'body' => null, + 'size' => 0, + ]; trigger_error(Lang::getTxt('fetch_web_data_bad_url', [__METHOD__], file: 'Errors'), E_USER_NOTICE); @@ -451,7 +490,7 @@ private function redirect(Url $target_url, Url $referrer_url): void { // SSRF guard: re-validate the redirect target before following it, so a // 302 -> http://127.0.0.1/ (or link-local cloud metadata) is refused. - if (WebFetchApi::makeSafe($target_url) === null) { + if (!$target_url->isFetchSafe(['http', 'https'])) { if (isset($this->response[$this->current_redirect - 1])) { $this->response[$this->current_redirect - 1]['success'] = false; } diff --git a/Sources/WebFetch/APIs/FtpFetcher.php b/Sources/WebFetch/APIs/FtpFetcher.php index 33ebdc2a33c..490fa7bc366 100644 --- a/Sources/WebFetch/APIs/FtpFetcher.php +++ b/Sources/WebFetch/APIs/FtpFetcher.php @@ -16,6 +16,7 @@ namespace SMF\WebFetch\APIs; use SMF\Config; +use SMF\IP; use SMF\Lang; use SMF\PackageManager\FtpConnection; use SMF\Url; diff --git a/Sources/WebFetch/APIs/SocketFetcher.php b/Sources/WebFetch/APIs/SocketFetcher.php index 6404e7a5a5e..ba17e4d06a5 100644 --- a/Sources/WebFetch/APIs/SocketFetcher.php +++ b/Sources/WebFetch/APIs/SocketFetcher.php @@ -391,6 +391,12 @@ public function result(?string $area = null): mixed { $max_result = \count($this->response) - 1; + // Nothing was recorded, because the request was refused before we ever + // got as far as making it. + if ($max_result < 0) { + return null; + } + // Just return a specified area or the entire result? if (\is_null($area)) { return $this->response[$max_result]; From cba08460d5507cdb9fb6233ea146743245c63e10 Mon Sep 17 00:00:00 2001 From: albertlast Date: Wed, 19 Aug 2026 08:11:14 +0200 Subject: [PATCH 8/8] Keeps make_fetch_safe() around for 2.1 mods make_fetch_safe() is a 2.1 function, and Subs-Compat.php exists to keep 2.1 names callable, so removing it outright means any mod that calls it fatals. It comes back, deprecated, returning the URL unchanged when it is safe and null when it is not. Callers written as $url = make_fetch_safe($url); if ($url === null) keep working, and what they get back is now a URL that can actually be fetched, rather than one with the host replaced by an IP address. is_fetch_safe() was passing an empty array through as the allowed scheme list, which matched nothing and made it always return false. It leaves the argument off now and takes the default. Co-Authored-By: Claude Opus 5 Signed-off-by: albertlast --- Sources/Subs-Compat.php | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/Sources/Subs-Compat.php b/Sources/Subs-Compat.php index 68a95736343..2640c1b2b9f 100644 --- a/Sources/Subs-Compat.php +++ b/Sources/Subs-Compat.php @@ -4019,7 +4019,26 @@ function iri_to_url(string $iri): string|bool */ function is_fetch_safe($url) { - return SMF\Url::create($url)->isFetchSafe([]); + return SMF\Url::create($url)->isFetchSafe(); + } + + /** + * Checks whether a URL is safe to fetch from the server. + * + * In SMF 2.1 this returned a version of the URL where the host had been + * replaced with a literal IP address. It no longer does that, because it + * broke every host that needs SNI or name-based virtual hosting, and it + * defeated certificate validation as well. The URL now comes back exactly + * as it went in, and the fetchers pin the connection to a vetted address + * themselves. Use is_fetch_safe() instead. + * + * @deprecated 3.0 + * @param string $url The URL to check. + * @return ?string $url if it is safe to fetch, or null if it is not. + */ + function make_fetch_safe($url) + { + return SMF\Url::create($url)->isFetchSafe() ? $url : null; } /**