fix(server): restrict outbound fetches of caller-supplied URLs - #378
fix(server): restrict outbound fetches of caller-supplied URLs#378yuvrajjsingh0 wants to merge 1 commit into
Conversation
`download_and_calculate_filesize` built a default reqwest client and fetched whatever URL it was handed, with no scheme restriction, no address restriction, no redirect policy, no size cap, and no timeouts. The URL comes from the caller: `create_file` and `bulk_create_files` take it from the request body, and the build path takes it from a row those endpoints wrote. That let an authorised caller borrow the server's network position. The server would connect to anything it could reach — loopback services, the cloud metadata endpoint, hosts on the internal network — and hand back the size and checksum of whatever answered, which is enough to probe for what exists and to fingerprint it. Fetches now go through a guarded client: - only http and https - non-public addresses refused, both when written into the URL and when returned by DNS - at most 5 redirects, with scheme and address re-checked per hop - response size capped, checked against Content-Length and again against the bytes that actually arrive - 10s connect timeout, 300s total The address check exists in two places on purpose. A guarded DNS resolver covers names and, because it is what the client resolves through, leaves no gap between checking and connecting. But hyper connects straight to an IP literal without consulting the resolver, so a literal address is checked explicitly as well; without that, `http://169.254.169.254/` would pass straight through. The host in PUBLIC_ENDPOINT is exempt automatically, since the server fetches its own release config and its own uploaded assets through it and that usually resolves privately. SSRF_ALLOWED_HOSTS names any further internal host a deployment must reach, and MAX_DOWNLOAD_BYTES tunes the size cap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Review changes with Changed Files
|
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| outbound::check_url(url)?; | ||
|
|
||
| let mut request = outbound::client() | ||
| .get(url) |
| if let Some(host) = host_of(public_endpoint) { | ||
| exempt_hosts.insert(host); | ||
| } else if !public_endpoint.is_empty() { | ||
| warn!( |
2 participants
Problem
download_and_calculate_filesizefetched whatever URL it was handed:No scheme restriction, no address restriction, no redirect policy, no size cap, no timeouts. The URL is caller-controlled —
create_fileandbulk_create_filesread it from the request body, andbuild.rsreads it from a row those endpoints wrote.An authorised caller could therefore borrow the server's network position. The server would connect to anything reachable from where it runs — loopback services,
169.254.169.254, hosts on the internal network — and return the size and checksum of whatever answered, which is enough to probe for what exists and to fingerprint it.Audit reference: S8. Note this also covers two call sites in
build.rs(:345,:396) that the original report did not list.Change
Fetches now go through a guarded client in
utils::outbound:http,httpsonlyContent-Lengthand actual bytesThe part worth reviewing carefully
The address check exists in two places, and both are necessary:
A guarded DNS resolver (
reqwest::dns::Resolve) covers hostnames. Filtering at resolution rather than in a pre-flight check is what closes DNS rebinding — the client can only connect to what the resolver returns, so a name that answers publicly during a check and privately at connect time still cannot be reached.An explicit check on the URL covers literal addresses. I initially had only the resolver, and while verifying I found this in hyper-util's connector:
A literal IP never reaches the resolver.
http://169.254.169.254/— the single most common SSRF payload — would have gone straight through a resolver-only guard. Tests for this are inurls_naming_a_non_public_address_directly_are_refused.Two other bugs my tests caught while writing this, both now fixed and covered:
::1was classified as public, because it also satisfies the deprecated IPv4-compatible form and was being read as0.0.0.1before the IPv6 checks ran. The IPv6-native ranges are now checked first.Url::host_str()keeps the brackets on an IPv6 literal, so[::1]never matched an exemption entry.Not breaking existing deployments
The host in
PUBLIC_ENDPOINTis exempt automatically. This is load-bearing:build.rs:396fetches{public_url}/release/{org}/{app}(the server calling itself), and uploaded files are stored withurl = "{public_url}/{s3_path}"— in local dev that ishttp://localhost:3000, and in most self-hosted deployments it is private. Without the exemption,make runand every build would break.SSRF_ALLOWED_HOSTScovers anything else internal (an in-cluster MinIO, an internal mirror). Exemptions are exact and per-host — they do not extend to subdomains, whichexemptions_do_not_leak_to_other_hostsasserts.If the policy is somehow never initialised, it falls back to no exemptions — failing closed.
Testing
29 tests pass. The security-relevant ones:
a_loopback_url_is_refused_without_connectingis the end-to-end one: it stands up a real loopback HTTP server, points the actual download function at it, and asserts both that the call is refused and that the server recorded zero connections — i.e. the guard stopped it before a socket was opened.Verified non-vacuous. With the guard disabled, that test fails on the connection count, confirming it detects the real pre-fix behaviour.
That exercise also caught a weak test of my own:
the_cloud_metadata_address_is_refusedoriginally passed even with the guard off, because the address simply timed out after 75s. It now asserts the error is a policyBadRequestand that it returns in under 5s, so it cannot pass by network accident.cargo fmt --checkandcargo clippy --all-targets -- -Dwarningspass.Docs
airborne_docs/docs/server/configuration.md— new Outbound fetches section documentingSSRF_ALLOWED_HOSTSandMAX_DOWNLOAD_BYTES, what is enforced, thatPUBLIC_ENDPOINTis auto-exempt, and a caution for self-hosted internal storage.airborne_server/.env.example— commented examples.npm run buildpasses (exit 0; the blog sidebar line is the pre-existing llms-txt noise).Rollout note
A deployment that registers files from an internal host at a private address will now get a 400 naming that host. Add it to
SSRF_ALLOWED_HOSTS. Worth checking existingfiles.urlvalues for private hosts before rolling out.🤖 Generated with Claude Code