Validate CURLOPT_PROXY format in sanitizeCurlParams() (#9537)

* Validate CURLOPT_PROXY format to prevent SSRF allowlist bypass

sanitizeCurlParams() previously only whitelisted cURL option keys and
never validated the CURLOPT_PROXY value itself. httpGet() and the
SimplePie File constructor both strip a scheme from the proxy string
with a greedy preg_replace('#^.*://#', '', $proxy) before running the
host through the internal-host allowlist / DNS-pinning check, but the
*original*, unstripped value is what actually gets handed to cURL.

A proxy value containing a second "://", e.g.
http://127.0.0.1:8080/://8.8.8.8, is reduced by the greedy strip to
the trailing "decoy" host (8.8.8.8) for the allowlist check, while
cURL still connects using the full original string. Depending on how
the installed libcurl parses it, this can route the request to
127.0.0.1:8080 instead of the validated public address.

This adds isSafeProxyString(), a strict format check for
[scheme://][user[:pass]@]host[:port] with no extra path, query,
fragment, or embedded scheme separator, and rejects CURLOPT_PROXY in
sanitizeCurlParams() when it doesn't match. Since every per-feed and
OPML-imported curl_params array is routed through sanitizeCurlParams()
before use (Feed.php, SimplePieCustom.php, ImportService.php), this
single chokepoint covers both the httpGet() and SimplePie fetch paths.

Added regression tests in httpUtilTest.php covering the exploit
payload and a range of legitimate proxy strings (bare host:port,
scheme-prefixed, SOCKS5 with credentials, SOCKS5h host-only).

* Normalise CURLOPT_PROXY scheme once, centrally; drop redundant downstream strips

Per review feedback: now that sanitizeCurlParams() validates CURLOPT_PROXY
and guarantees at most one scheme separator, it also strips that (validated)
scheme once, so the value stored is always scheme-less. This removes the
need for httpGet() and the bundled SimplePie File::curlInit() to each
re-strip the scheme themselves -- both call sites are updated to rely on
the value already being normalised.

Updated the regression tests to assert the normalised value, not just
whether the key survives sanitization.

* Disallow a scheme in CURLOPT_PROXY entirely, per review

CURLOPT_PROXYTYPE is what tells cURL the proxy's scheme -- CURLOPT_PROXY
itself was never supposed to carry one. Rather than validating-then-
stripping a scheme, sanitizeCurlParams() now rejects any CURLOPT_PROXY
value that contains one at all, simplifying isSafeProxyString() and
removing the normalisation step (and its own small regex) entirely.

Trimmed the accompanying comment to the one load-bearing line, and
updated the stale 'strips the scheme' wording in httpGet() and the
SimplePie File::curlInit() comment to 'rejects a scheme-containing
value', matching the simpler behaviour. Updated the regression tests
to match: a scheme-containing value (even a well-formed one) is now
rejected outright instead of being normalised.

* Sync SimplePie
https://github.com/FreshRSS/simplepie/pull/95

* Simplify CURLOPT_PROXY validation to a single str_contains('/') check

Per review (AI-assisted, credited to Inverle): a regex was more than this
needs and had a regression -- it rejected valid IPv6 proxies like [::1]:8080
because their host part contains multiple colons. A proxy value never
legitimately contains a "/": that single check still rejects a scheme
(scheme:// always contains "/") and the original exploit payload, and
additionally blocks a SOCKS Unix-domain-socket path such as
localhost:9050/path/to.sock -- cURL treats that specially for SOCKS
proxies and ignores the port, so an allowlisted host:port could otherwise
be smuggled into connecting to an arbitrary local socket.

Also bumped the vendored SimplePie commit in lib/composer.json now that
the companion fix (FreshRSS/simplepie#95) is merged, and added test
cases for the IPv6 and Unix-socket scenarios.

* Reduce comments

---------

Co-authored-by: S31ZUR3 <191032395+S31ZUR3@users.noreply.github.com>
Co-authored-by: Alexandre Alapetite <alexandre@alapetite.fr>

app/Utils/httpUtil.php

@@ -125,6 +125,11 @@ final class FreshRSS_http_Util {
 				$curl_params[$k] = array_filter($curl_params[$k],
 					fn($header) => is_string($header) && !preg_match('/^(Remote[-_\s]*User|X[-_\s]*WebAuth[-_\s]*User)\\s*:/i', $header));
 			}
+			// Forbid '/' in CURLOPT_PROXY [user[:pass]@]host[:port] (the scheme comes from CURLOPT_PROXYTYPE),
+			// to reject e.g. 'http://example.net/://decoy' and 'localhost:9050/path/to.sock'
+			if ($k === CURLOPT_PROXY && (!is_string($curl_params[$k]) || str_contains($curl_params[$k], '/'))) {
+				unset($curl_params[$k], $curl_params[CURLOPT_PROXYTYPE]);
+			}
 		}
 		return $curl_params;
 	}
@@ -512,8 +517,8 @@ final class FreshRSS_http_Util {
 			}
 		}
 		$proxy_type = is_int($curl_options[CURLOPT_PROXYTYPE] ?? null) ? $curl_options[CURLOPT_PROXYTYPE] : $proxy_type;
+		// CURLOPT_PROXY is already scheme-less by the time it reaches here:
 		$proxy = is_string($curl_options[CURLOPT_PROXY] ?? null) ? $curl_options[CURLOPT_PROXY] : $proxy;
-		$proxy = preg_replace('#^.*://#', '', $proxy) ?? $proxy;	// Strip scheme, as it is provided by CURLOPT_PROXYTYPE
 
 		if (($retryAfter = FreshRSS_http_Util::getRetryAfter($url, $proxy)) > 0) {
 			Minz_Log::warning('For that domain, will first retry after ' . date('c', $retryAfter) . '. ' . \SimplePie\Misc::url_remove_credentials($url));

lib/composer.json

@@ -18,7 +18,7 @@
 		"marienfressinaud/lib_opml": "dev-main#f0e850b6394af90b898daf0e65fcc7363457b844",
 		"phpgt/cssxpath": "v1.6.0",
 		"phpmailer/phpmailer": "7.1.1",
-		"simplepie/simplepie": "dev-freshrss#042298d6550e74225cf0eedba1913cf68976d6bb"
+		"simplepie/simplepie": "dev-freshrss#760a0a3354a73c07d21c97e4400b85c04e1b57c7"
 	},
 	"config": {
 		"sort-packages": true,

lib/simplepie/simplepie/src/File.php

@@ -158,8 +158,10 @@ class File implements Response
                         $this->success = false;
                         return;
                     }
-                    $proxy = preg_replace('#^.*://#i', '', $proxy); // Strip any scheme already present in CURLOPT_PROXY
-                    $proxy_url = "$proxy_scheme://$proxy"; // CURLOPT_PROXY ($proxy) is formatted as user:pass@hostname:port, with the part before @ being optional
+                    // CURLOPT_PROXY ($proxy) is expected to already be scheme-less: the scheme comes from
+                    // CURLOPT_PROXYTYPE, and the caller (e.g. FreshRSS_http_Util::sanitizeCurlParams()) rejects
+                    // any scheme-containing value before it's ever stored or used.
+                    $proxy_url = "$proxy_scheme://$proxy"; // formatted as user:pass@hostname:port, with the part before @ being optional
                     $resolve = $this->get_curl_resolve_info($proxy_url);
                     if ($resolve === null) {
                         $this->error = 'Failed to fetch this URL, because the proxy’s IP is not in the allowlist [' .

tests/app/Utils/httpUtilTest.php

@@ -13,6 +13,38 @@ final class httpUtilTest extends \PHPUnit\Framework\TestCase {
 		self::assertEquals($expected, FreshRSS_http_Util::compareUrlIgnoringHttps($url1, $url2) === 0);
 	}
 
+	#[DataProvider('provideCurlProxyValues')]
+	public function test_sanitizeCurlParamsRejectsMalformedProxy(string $proxy, bool $shouldSurvive): void {
+		$sanitized = FreshRSS_http_Util::sanitizeCurlParams([CURLOPT_PROXY => $proxy, CURLOPT_PROXYTYPE => CURLPROXY_HTTP]);
+		if ($shouldSurvive) {
+			self::assertSame($proxy, $sanitized[CURLOPT_PROXY] ?? null);
+		} else {
+			self::assertArrayNotHasKey(CURLOPT_PROXY, $sanitized);
+		}
+	}
+
+	/** @return array<string,array{string,bool}> */
+	public static function provideCurlProxyValues(): array {
+		return [
+			// A second `://` would otherwise be reduced by a greedy scheme-strip to this
+			// trailing "decoy" host for the allowlist/DNS-pinning check, while cURL still
+			// received the original, unstripped value as CURLOPT_PROXY.
+			'rejects scheme-prefixed decoy host' => ['http://127.0.0.1:8080/://8.8.8.8', false],
+			'rejects scheme-less decoy host' => ['127.0.0.1:8080/://198.51.100.10', false],
+			// The scheme comes from CURLOPT_PROXYTYPE, not the value itself -- a scheme-containing
+			// value is rejected outright rather than normalised.
+			'rejects a value carrying its own scheme' => ['http://proxy.example.com:3128', false],
+			// cURL treats "host:port/path" as a Unix domain socket for SOCKS proxies and ignores the
+			// port, so an allowlisted host:port could otherwise be smuggled into connecting to an
+			// arbitrary local socket.
+			'rejects a SOCKS Unix-domain-socket path' => ['localhost:9050/path/to.sock', false],
+			'accepts bare host:port' => ['127.0.0.1:8080', true],
+			'accepts hostname:port' => ['proxy.example.com:3128', true],
+			'accepts host:port with credentials' => ['user:pass@203.0.113.5:1080', true],
+			'accepts an IPv6 proxy' => ['[::1]:8080', true],
+		];
+	}
+
 	#[\Override]
 	protected function tearDown(): void {
 		FreshRSS_Context::initSystem(reload: true);	// Restore the default internal host allowlist