fix(security): harden HTTP redirects by trust boundary#2634
Conversation
fl0rianr
left a comment
There was a problem hiding this comment.
Thanks for addressing this. I agree that external downloads should be restricted to HTTPS and that redirect chains should be bounded. However, the current change applies the HTTPS-only policy to the shared HttpClient::get() implementation, which is also used for communication with wrapped backends on http://127.0.0.1:<port>.
This is already causing real regressions rather than CI-only failures:
- Test .exe - llamacpp`fails with CURL error: Unsupported protocol
- Test .exe - audio-gen-acestep fails for the same reason
I do not think CI should be converted to HTTPS here. Production wrapped backends intentionally use loopback HTTP, so CI should continue testing that path.
I suggest separating the policies by trust boundary:
- External downloads / external requests: allow only https, follow redirects, restrict redirect protocols to https, and set
MAXREDIRS. - Lemonade-managed loopback backends: allow only
httpand disable redirects entirely. - User-configured HTTP mirrors, if supported: require an explicit opt-in such as
allow_insecure_http, disabled by default.
This could be implemented with a small HttpRequestOptions or HttpPolicy parameter and a shared helper that applies the corresponding curl options. The loopback callers in WrappedServer and ACE-Step should then explicitly select the trusted-loopback policy.
A few additional points:
-
Restricting protocols is useful hardening, but it does not fully prevent SSRF. HTTPS URLs can still target loopback/private addresses or resolve to them through DNS. The PR description should either narrow the claim to protocol and downgrade protection, or add resolved-address and redirect-target validation.
-
Lemonade requires libcurl >= 8.5.0, where redirects are already bounded by default.
MAXREDIRS=5is good defense in depth, but the description should not claim the existing behavior allows infinite redirect loops. -
Please check the return values of the security-related
curl_easy_setopt()calls so failure to apply a restriction fails closed. -
Please add focused tests for:
- trusted loopback HTTP GET without redirects,
- rejection of initial HTTP under the external policy,
- rejection of HTTPS-to-HTTP or non-HTTP(S) redirects,
- the configured redirect limit.
So I support the intended hardening, but the current global HTTPS-only change should not be merged as-is.
|
Thanks for the great rework. Two minor follow-ups: |
4f6195a to
cc11556
Compare
cc11556 to
27be686
Compare
fl0rianr
left a comment
There was a problem hiding this comment.
Thanks for rebasing and getting CI back on track. The original trust-boundary refactor still looks good, the title is now accurate, and the current workflows are green.
However, the latest loopback-provider fix introduces a new blocking issue.
The new is_loopback() logic selects TrustedLoopback based only on the parsed host. This also classifies a URL such as https://localhost:PORT/v1 as trusted loopback, but TrustedLoopback permits only the http protocol. A valid local HTTPS provider would therefore fail with Unsupported protocol.
The manual authority parsing is also not safe enough for a security-boundary decision:
const auto colon = host.find(':');
if (colon != std::string::npos) host = host.substr(0, colon);This does not correctly handle bracketed IPv6 and can confuse URL userinfo with the hostname. For example, http://localhost:80@evil.example/... is parsed here as localhost, while the actual destination host is evil.example.
I suggest removing this automatic loopback exception entirely:
- HTTPS provider URLs should use ExternalHttpsOnly.
- HTTP provider URLs should use AllowInsecureHttp only when
allow_insecure_http=true. - TrustedLoopback should remain limited to Lemonade-generated backend URLs whose destination is already known to be 127.0.0.1.
Local cloud proxies and their tests should explicitly enable the existing insecure-HTTP opt-in. That keeps the implementation aligned with the PR description and avoids a second ad-hoc URL parser.
There is also a problem with the new HTTPS-to-HTTP downgrade test. It connects with https:// to a plain httplib::Server, so the TLS handshake fails before the /http-redirect handler can return a redirect. The test then accepts any non-CURLE_OK result, meaning it passes on an unrelated TLS or connection failure and does not verify CURLOPT_REDIR_PROTOCOLS_STR.
Please either remove that test or run a real TLS test server and verify that:
- the HTTPS route was reached,
- it returned the redirect,
- the HTTP target was not reached, and
- curl returned the expected redirect-protocol error.
The main policy implementation is otherwise in good shape, but I would fix these two points before approval.
Fixes SWSPLAT-24200 by adding security restrictions to all CURLOPT_FOLLOWLOCATION usage in HttpClient to prevent SSRF, protocol downgrade attacks, and redirect-loop DoS. Changes: - Add CURLOPT_MAXREDIRS=5L to limit redirect chains - Add CURLOPT_PROTOCOLS_STR="https" to restrict initial protocol - Add CURLOPT_REDIR_PROTOCOLS_STR="https" to restrict redirect protocols This prevents: - HTTPS→HTTP downgrade attacks during backend binary downloads - FTP/FILE protocol SSRF to internal network endpoints - Infinite redirect loops causing DoS Applied to all three locations that use CURLOPT_FOLLOWLOCATION: 1. HttpClient::get() - line 375 2. HttpClient::download_attempt() - line 683 3. HttpClient::download_attempt() HEAD request - line 842 Co-Authored-By: Claude <noreply@anthropic.com>
Fixes SWSPLAT-24200 by splitting HttpClient security restrictions by trust boundary so loopback backend RPC keeps working over http. Policies (HttpSecurityPolicy): - External hosts (HF, GitHub, release CDNs): https-only for the initial request and every redirect hop, MAXREDIRS=5. - Lemonade-managed loopback backends: http only, redirects disabled entirely. - Opt-in plaintext endpoints (cloud allow_insecure_http): http+https, bounded redirects. A shared helper (apply_http_security_policy) applies the curl options and checks each setopt return value, so a restriction that fails to apply fails closed. Added test for HTTPS-to-HTTP protocol downgrade rejection under the external policy.
866b612 to
8ae5da6
Compare
Fixes SWSPLAT-24200 by applying scheme and redirect hardening to
HttpClient's redirect-following requests, split by trust boundary so loopback backend RPC keeps working over http.Policies (
HttpSecurityPolicy):MAXREDIRS=5.allow_insecure_http): http+https, bounded redirects.A shared helper applies the curl options and checks each
setoptreturn value, so a restriction that fails to apply fails closed.Scope of protection:
httpsURL can still resolve to loopback/private addresses; resolved-address validation is out of scope here.MAXREDIRS=5is defense-in-depth: libcurl already bounds redirects by default at our >= 8.5.0 floor, so the prior behavior did not permit infinite redirect loops.Tests:
test/cpp/test_http_client_security.cppcovers trusted-loopback GET without redirects, rejection of initial http under the external policy, the configured redirect limit, and rejection of non-http(s) redirects.