[3.0] Fixes broken SNI resolution without breaking the fetch guards

Sources/PackageManager/FtpConnection.php

@@ -15,8 +15,7 @@
 
 namespace SMF\PackageManager;
 
-use SMF\Url;
-use SMF\WebFetch\WebFetchApi;
+use SMF\IP;
 
 /**
  * Class FtpConnection
@@ -303,15 +302,20 @@ 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)) {
+		$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;
 	}

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;
 		}

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,56 @@ 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 is_fetch_safe($url)
+	{
+		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\WebFetch\WebFetchApi::makeSafe($url);
+		return SMF\Url::create($url)->isFetchSafe() ? $url : null;
+	}
+
+	/**
+	 * 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));
 	}
 
 	/**

Sources/Url.php

@@ -162,6 +162,24 @@ class Url implements \Stringable
 	 */
 	protected $is_ascii;
 
+	/**
+	 * @var array
+	 *
+	 * Cache for $this->getIPs()
+	 */
+	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
 	 ****************/
@@ -422,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});
@@ -526,26 +548,129 @@ public function proxied(): self
 	 */
 	public function getIPs(): array
 	{
+		if (isset($this->ips)) {
+			return $this->ips;
+		}
+
+		$is_ascii = $this->is_ascii;
+
+		$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.
-		$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;
+		// 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();
+		}
+
+		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 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 ((string) $ip === (string) $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. 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
+	{
+		$allowed_schemes = $allowed_schemes === [] ? array_keys(WebFetchApi::$scheme_handlers) : $allowed_schemes;
+
+		$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();
+
+			// 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),
+			);
+		}
+
+		if (!$is_ascii) {
+			$this->toUtf8();
+		}
+
+		return $is_safe;
 	}
 
 	/**