diff --git a/Sources/PackageManager/FtpConnection.php b/Sources/PackageManager/FtpConnection.php index 995655d847..4c5fc577e9 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 4cd7caa5e3..212ce4e455 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 685226b581..1dd96e035a 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(array_keys(SMF\WebFetch\WebFetchApi::$scheme_handlers)); + } + + /** + * 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 985a3a8c18..9cba0ab872 100644 --- a/Sources/Url.php +++ b/Sources/Url.php @@ -162,6 +162,17 @@ class Url implements \Stringable */ protected $is_ascii; + /**************************** + * Internal static properties + ****************************/ + + /** + * @var array + * + * Cache for $this->getIPs() + */ + protected static array $ips; + /**************** * Public methods ****************/ @@ -526,26 +537,106 @@ public function proxied(): self */ public function getIPs(): array { - // Resolve the host to its address(es). A literal IP resolves to itself. - $ips = []; + $is_ascii = $this->is_ascii; + $this->toAscii(); + $ascii_host = $this->host; + + if (!isset(self::$ips[$ascii_host])) { + // Resolve the host to its address(es). A literal IP resolves to itself. + self::$ips[$ascii_host] = []; + + if (filter_var(trim($ascii_host, '[]'), FILTER_VALIDATE_IP)) { + self::$ips[$ascii_host][] = new IP(trim($ascii_host, '[]')); + } else { + $records = @dns_get_record($ascii_host, DNS_A | DNS_AAAA); + + foreach ((array) $records as $record) { + if (!empty($record['ip'])) { + self::$ips[$ascii_host][] = new IP($record['ip']); + } + + if (!empty($record['ipv6'])) { + self::$ips[$ascii_host][] = new IP($record['ipv6']); + } + } + } + } - if (filter_var(trim($this->host, '[]'), FILTER_VALIDATE_IP)) { - $ips[] = new IP(trim($this->host, '[]')); - } else { - $records = @dns_get_record($this->host, DNS_A | DNS_AAAA); + if (!$is_ascii) { + $this->toUtf8(); + } - foreach ((array) $records as $record) { - if (!empty($record['ip'])) { - $ips[] = new IP($record['ip']); - } + return self::$ips[$ascii_host]; + } - if (!empty($record['ipv6'])) { - $ips[] = new IP($record['ipv6']); - } + /** + * 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 IP addresses are the given one. + * + * @param \SMF\IP $ip The IP address to check. + * @return bool Whether this URL resolves to the given IP address. + */ + public function resolvesTo(IP $ip): bool + { + foreach ($this->getIPs() as $known_ip) { + if ((string) $ip === (string) $known_ip) { + return true; } } - return $ips; + 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. 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 + { + if (empty($allowed_schemes)) { + $allowed_schemes = array_keys(WebFetchApi::$scheme_handlers); + } + + $is_ascii = $this->is_ascii; + + $this->toAscii(); + + if ( + // Only known fetchable schemes. + empty($this->scheme) + || !\in_array($this->scheme, $allowed_schemes) + // Must have a 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))$/', $this->host) + ) { + $is_safe = false; + } else { + $ips = $this->getIPs(); + + $is_safe = $ips !== [] && $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 9d209ff7e4..40cfcd3e6e 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; @@ -229,7 +230,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')) { @@ -247,7 +248,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; @@ -266,7 +267,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; } @@ -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]; @@ -324,17 +331,36 @@ 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; + + // 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) { @@ -349,15 +375,37 @@ 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 + + // Double check that we connected to the expected IP. + 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); + + 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; // 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, @@ -367,7 +415,11 @@ private function sendRequest(string $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); @@ -377,24 +429,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 +484,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 (!$target_url->isFetchSafe(['http', 'https'])) { if (isset($this->response[$this->current_redirect - 1])) { $this->response[$this->current_redirect - 1]['success'] = false; } @@ -447,7 +498,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); } diff --git a/Sources/WebFetch/APIs/FtpFetcher.php b/Sources/WebFetch/APIs/FtpFetcher.php index 75bf22f639..490fa7bc36 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; @@ -112,10 +113,10 @@ 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'])) { + if (!$url->isFetchSafe(['ftp', 'ftps'])) { trigger_error(Lang::getTxt('fetch_web_data_bad_url', [__METHOD__], file: 'Errors'), E_USER_NOTICE); return $this; @@ -160,6 +161,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 71ef9d2b2f..ba17e4d06a 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; @@ -168,10 +169,10 @@ 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) { + if (!$url->isFetchSafe(['http', 'https'])) { $this->closeConnection(); trigger_error(Lang::getTxt('fetch_web_data_bad_url', [__METHOD__], file: 'Errors'), E_USER_NOTICE); @@ -213,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'); @@ -263,8 +280,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(); } @@ -375,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]; diff --git a/Sources/WebFetch/WebFetchApi.php b/Sources/WebFetch/WebFetchApi.php index 91197cf7f1..a829de09ab 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 ****************/ @@ -133,18 +125,11 @@ 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. - 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 ******************/