Repository navigation
Remove redundant CURLOPT_PROXY scheme-strip in File::curlInit() - #95
Merged
Merged
Conversation
CURLOPT_PROXYTYPE is what tells cURL the proxy's scheme; CURLOPT_PROXY
itself was never supposed to carry one. The greedy preg_replace('#^.*://#i', '', $proxy)
used to strip a scheme before building $proxy_url for the allowlist/DNS-pinning
check -- but if the value contained a second "://", the greedy strip reduced it
to the text after the LAST occurrence (an attacker-chosen "decoy" host) for that
check, while the original, unstripped value was still what ultimately reached cURL
via $curl_options[CURLOPT_PROXY], letting the checked host and the contacted host
differ.
Companion fix: the FreshRSS caller (FreshRSS_http_Util::sanitizeCurlParams())
now validates CURLOPT_PROXY and rejects any value containing a scheme outright,
so this file no longer needs to strip one -- see
FreshRSS/FreshRSS#9537
Inverle
approved these changes
Nov 8, 2026
Alkarex
added a commit
to S31ZUR3/FreshRSS
that referenced
this pull request
Nov 8, 2026
S31ZUR3
added a commit
to S31ZUR3/FreshRSS
that referenced
this pull request
Nov 9, 2026
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.
Alkarex
added a commit
to FreshRSS/FreshRSS
that referenced
this pull request
Nov 9, 2026
* 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
FreshRSS/simplepie#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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Companion fix to FreshRSS/FreshRSS#9537.
File::curlInit()stripped a scheme fromCURLOPT_PROXYwith a greedypreg_replace('#^.*://#i', '', $proxy)before building$proxy_urlfor the allowlist/DNS-pinning check.CURLOPT_PROXYTYPEis what actually tells cURL the proxy's scheme --CURLOPT_PROXYwas never supposed to carry one. If the value contained a second://, the greedy strip reduced it to the text after the last occurrence for that check, while the original, unstripped value was still what ultimately reached cURL via$curl_options[CURLOPT_PROXY]-- letting the checked host and the contacted host differ.The FreshRSS caller (
FreshRSS_http_Util::sanitizeCurlParams()) now validatesCURLOPT_PROXYand rejects any value containing a scheme outright, so this file no longer needs to strip one.See GHSA-xfvx-g9wc-f8wc on FreshRSS/FreshRSS for the reported details.
AI disclosure
This fix (and the underlying report, filed against FreshRSS/FreshRSS) was prepared with AI assistance (Claude). I reviewed and tested the change myself before opening this PR.