Repository navigation
Fix SSRF in canister discovery (CWE-918): restrict outbound fetches to public https hosts - #35
Merged
Merged
Conversation
discover_canisters passes a fully user-controlled `domain` to the HTTP client with no restriction on the destination, so an authenticated user could make the server GET arbitrary internal hosts/ports (http://169.254.169.254/…, http://127.0.0.1:6379/, …) — a blind/semi-blind SSRF and internal port scanner, with reflected connection errors acting as a reachability oracle. reqwest also followed redirects by default, so a public host could 3xx the request onto an internal one. Restrict all discovery to public https destinations: - Only https:// targets are accepted (kills the http metadata / plaintext -port vectors). - The target host is resolved up front and REFUSED if any address is loopback / private / link-local / CGNAT / documentation / benchmarking / reserved / IPv4-mapped-to-such (hand-rolled is_global since the std one is unstable). Internal targets are rejected before any connection, so the error-string reachability oracle is closed too. - The validated addresses are PINNED into the site client (resolve_to_addrs), so a re-resolution can't rebind the connection to an internal address (DNS rebinding). The pin only overrides the site host, so dashboard-enrichment calls to other hosts still work. - A custom redirect policy re-validates every hop and stops (never follows) a non-public / non-https redirect; applied to the dashboard/lookup client too as defense in depth. Mirrors the existing SSRF guard in skills.rs (which pins to a fixed registry origin); discovery needs the IP-range check because its host is arbitrary. Adds offline tests for IP classification, URL rejection (incl. the finding's exact vectors), and redirect validation; the live oisy discovery test still passes through the pinned client. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym
There was a problem hiding this comment.
Pull request overview
This PR hardens discover against SSRF by restricting outbound discovery fetches to public HTTPS destinations, validating resolved IPs against non-public ranges, pinning validated addresses into the reqwest client to mitigate DNS rebinding, and applying a redirect policy intended to prevent redirects to internal/non-HTTPS targets.
Changes:
- Added IP “global/public” classification helpers and URL resolution/validation for user-supplied discovery targets.
- Updated
discover()to pre-resolve/validate the target and pin the resolved addresses into a dedicated reqwest client. - Added a shared redirect policy (and tests) to constrain redirect hops to safe destinations.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aterga
marked this pull request as ready for review
July 5, 2026 15:38
Addresses the Copilot review on #35: - ipv6_is_global used to_ipv4_mapped(), which only catches ::ffff:… ; an IPv4-COMPATIBLE literal like ::127.0.0.1 fell through and was classified global. Switch to to_ipv4(), which covers both mapped and compatible (and ::/::1), so a private/loopback v4 can't be reached via IPv6 embedding. - The redirect policy resolved domain hops with a blocking to_socket_addrs() inside the sync redirect callback (blocks a worker thread) and, having no pin, left a DNS-rebinding TOCTOU on the redirect target. Replace it with a DNS-free decision: follow a hop only if it is https AND either a globally- routable IP literal or a same-host redirect (host already validated, and pinned for the site client); refuse cross-host domain redirects. - Add IPv4-mapped and IPv4-compatible embedded-v4 cases to the classification test and a redirect-hop policy test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym
aterga
pushed a commit
that referenced
this pull request
Jul 5, 2026
Resolve the conflict in src/main.rs: PRs #35/#37/#38 landed on main while this branch was open. The only content conflict was the Candid encode/decode layer — #37 added CWE-674 hardening (guard_candid_text + MAX_CANDID_DEPTH/size caps) to the very functions this branch had moved into the new src/calls.rs module. Resolution: keep this branch's structure (the functions live in calls.rs) and port #37's hardening into calls.rs: - guard_candid_text / MAX_CANDID_TEXT_BYTES / MAX_CANDID_DEPTH / depth_err now live in calls.rs (pub(crate)), used by encode_args + decode_bytes_with_did. - management.rs's install-arg guard now calls crate::calls::guard_candid_text. - the CWE-674 guard test moved to calls::tests alongside the function. discover.rs (SSRF #35 + discovery buffering #37), management.rs, and the monitoring/* changes auto-merged. Build clean, all 53 tests pass, no new clippy warnings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hv4cGJfD97SGxHM2myTAbA
5 of 7 tasks
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.
What & why
Addresses the reported SSRF (CWE-918, MEDIUM) in
discover_canisters. Itsdomainargument is fully user-controlled and was passed to the shared HTTP client with no destination restriction, so any authenticated user (anyone can self-register an II anchor) could make the server issue GETs from inside the deployment's network — e.g.http://169.254.169.254/latest/meta-data/orhttp://127.0.0.1:6379/— a blind/semi-blind SSRF and internal port scanner. Reflected connection errors (could not reach {base}: {e}) acted as a reachability oracle, and reqwest followed redirects by default, so a public host could3xxthe request onto an internal one.The sibling
skills.rsfetch already guards against this by pinning to a fixed registry origin; discovery can't pin to a fixed host (its target is arbitrary), so it needs the IP-range approach the finding recommends.Fix (all outbound discovery constrained to public https destinations)
httpstargets are refused, killing thehttp://metadata / plaintext-port vectors outright.100.64/10) / documentation / benchmarking / reserved / IPv4-mapped-to-such. (is_globalis hand-rolled sinceIpAddr::is_globalis still unstable.) Internal targets are rejected before any connection, which also closes the error-string reachability oracle (no connect ⇒ no refused/timeout/TLS distinction leaks).resolve_to_addrs, so a re-resolution can't rebind the connection to an internal address (DNS rebinding TOCTOU). The pin only overrides the site host, so the dashboard-enrichment calls (other hosts) still resolve normally.lookup_canisterclient too as defense-in-depth.Testing
cargo buildclean;cargo test— 47/47.ip_globality_classification(public vs. loopback/private/link-local/CGNAT/reserved/doc/bench/IPv4-mapped, v4 + v6),resolve_public_url_rejects_ssrf_targets(the finding's exact vectors + https-to-internal-literal + non-https scheme; and a public IP literal is accepted & pinned), andredirect_target_rejects_internal_and_http.discovers_oisy_frontend_and_backendtest still passes — real public discovery works end-to-end through the new pinned client.Notes
discover_canistersnow refuses non-httpsinputs (previouslyhttp://was passed through) and any host that resolves only to non-public addresses. Legitimate public IC sites are unaffected.main(after Add MCP tool annotations so read-only tools aren't shown as destructive #34 merged); the diff is onlysrc/discover.rs.🤖 Generated with Claude Code
https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym
Generated by Claude Code