From d034d5106f3c656c334d2a539bca37a0d4b022f6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 5 Jul 2026 15:33:06 +0000 Subject: [PATCH 1/2] Fix SSRF in canister discovery (CWE-918) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym --- src/discover.rs | 209 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 205 insertions(+), 4 deletions(-) diff --git a/src/discover.rs b/src/discover.rs index 623ed17..0290fee 100644 --- a/src/discover.rs +++ b/src/discover.rs @@ -14,7 +14,10 @@ //! result carries its provenance and the caller decides (and should confirm //! with `get_candid`). -use std::collections::BTreeMap; +use std::{ + collections::BTreeMap, + net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr, ToSocketAddrs}, +}; use candid::Principal; use regex::Regex; @@ -68,6 +71,145 @@ fn normalize(domain: &str) -> String { } } +// --------------------------------------------------------------------------- +// SSRF guard (CWE-918). `discover`'s `domain` is fully user-controlled, so every +// outbound request it drives must be constrained to PUBLIC https destinations: +// - the scheme is pinned to https (no http:// to metadata IPs / plaintext ports); +// - the host is resolved and refused if ANY address is loopback / private / +// link-local / CGNAT / documentation / otherwise-reserved; +// - the validated addresses are PINNED into the site client, so a later +// re-resolution can't rebind the connection to an internal address (DNS +// rebinding); and +// - each redirect hop is re-validated the same way (a 3xx can't bounce us to an +// internal host). +// --------------------------------------------------------------------------- + +/// Whether `ip` is a publicly-routable ("global") address. Hand-rolled because +/// `IpAddr::is_global` is still unstable; conservative — anything not clearly +/// public is treated as non-global and refused. +fn ip_is_global(ip: &IpAddr) -> bool { + match ip { + IpAddr::V4(v4) => ipv4_is_global(v4), + IpAddr::V6(v6) => ipv6_is_global(v6), + } +} + +fn ipv4_is_global(ip: &Ipv4Addr) -> bool { + let o = ip.octets(); + !(ip.is_unspecified() // 0.0.0.0 + || o[0] == 0 // 0.0.0.0/8 "this network" + || ip.is_loopback() // 127.0.0.0/8 + || ip.is_private() // 10/8, 172.16/12, 192.168/16 + || ip.is_link_local() // 169.254.0.0/16 + || ip.is_broadcast() // 255.255.255.255 + || ip.is_documentation() // 192.0.2/24, 198.51.100/24, 203.0.113/24 + || ip.is_multicast() // 224.0.0.0/4 + || (o[0] == 100 && (o[1] & 0xc0) == 64) // 100.64.0.0/10 CGNAT (shared) + || (o[0] == 192 && o[1] == 0 && o[2] == 0) // 192.0.0.0/24 IETF protocol + || (o[0] == 198 && (o[1] & 0xfe) == 18) // 198.18.0.0/15 benchmarking + || o[0] >= 240) // 240.0.0.0/4 reserved +} + +fn ipv6_is_global(ip: &Ipv6Addr) -> bool { + // An IPv4-mapped address (::ffff:0:0/96) is only as global as the v4 it embeds. + if let Some(v4) = ip.to_ipv4_mapped() { + return ipv4_is_global(&v4); + } + let seg = ip.segments(); + !(ip.is_unspecified() // :: + || ip.is_loopback() // ::1 + || ip.is_multicast() // ff00::/8 + || (seg[0] & 0xfe00) == 0xfc00 // fc00::/7 unique-local + || (seg[0] & 0xffc0) == 0xfe80 // fe80::/10 link-local unicast + || (seg[0] == 0x2001 && seg[1] == 0x0db8)) // 2001:db8::/32 documentation +} + +/// Validate a user-supplied discovery URL against SSRF and return the parsed URL +/// plus the socket addresses to PIN the client to. https only; every resolved +/// address must be global. Async DNS (no blocking of the executor). +async fn resolve_public_url(raw: &str) -> Result<(url::Url, Vec), String> { + let url = url::Url::parse(raw).map_err(|e| format!("invalid discovery URL {raw}: {e}"))?; + if url.scheme() != "https" { + return Err(format!( + "refusing to fetch {raw}: only https:// discovery targets are allowed (SSRF guard)" + )); + } + let port = url.port_or_known_default().unwrap_or(443); + let addrs: Vec = match url.host() { + Some(url::Host::Ipv4(v4)) => vec![SocketAddr::new(IpAddr::V4(v4), port)], + Some(url::Host::Ipv6(v6)) => vec![SocketAddr::new(IpAddr::V6(v6), port)], + Some(url::Host::Domain(host)) => tokio::net::lookup_host((host, port)) + .await + .map_err(|e| format!("could not resolve {host}: {e}"))? + .collect(), + None => return Err(format!("refusing to fetch {raw}: no host")), + }; + if addrs.is_empty() { + return Err(format!("refusing to fetch {raw}: host did not resolve")); + } + if let Some(bad) = addrs.iter().find(|a| !ip_is_global(&a.ip())) { + return Err(format!( + "refusing to fetch {raw}: it resolves to a non-public address ({}) — discovery is \ + restricted to public hosts (SSRF guard)", + bad.ip() + )); + } + Ok((url, addrs)) +} + +/// Whether a redirect target is an acceptable public https destination, used by +/// the client redirect policy so a 3xx can't bounce a request to an internal +/// host. Resolution here is blocking (reqwest's redirect callback is sync), but +/// only runs on the rare cross-host redirect. +fn redirect_target_ok(url: &url::Url) -> bool { + if url.scheme() != "https" { + return false; + } + let port = url.port_or_known_default().unwrap_or(443); + match url.host() { + Some(url::Host::Ipv4(v4)) => ip_is_global(&IpAddr::V4(v4)), + Some(url::Host::Ipv6(v6)) => ip_is_global(&IpAddr::V6(v6)), + Some(url::Host::Domain(host)) => match (host, port).to_socket_addrs() { + Ok(addrs) => { + let addrs: Vec = addrs.collect(); + !addrs.is_empty() && addrs.iter().all(|a| ip_is_global(&a.ip())) + } + Err(_) => false, + }, + None => false, + } +} + +/// Redirect policy shared by every discovery/dashboard client: follow only +/// public https hops, bounded, and stop (rather than follow) on a refused hop so +/// no request is ever issued to an internal host. +fn ssrf_redirect_policy() -> reqwest::redirect::Policy { + reqwest::redirect::Policy::custom(|attempt| { + if attempt.previous().len() >= 10 { + attempt.error("too many redirects") + } else if redirect_target_ok(attempt.url()) { + attempt.follow() + } else { + attempt.stop() + } + }) +} + +/// A client for fetching a user-supplied site: the SSRF redirect guard plus the +/// host PINNED to the pre-validated public addresses, so no re-resolution can +/// rebind the connection to an internal address between validation and connect. +/// (Pinning only overrides this host; requests to other hosts — e.g. the +/// dashboard — resolve normally, still under the redirect guard.) +fn site_client(host: &str, addrs: &[SocketAddr]) -> Result { + reqwest::Client::builder() + .user_agent("ic-mcp-discover/0.1") + .timeout(std::time::Duration::from_secs(15)) + .redirect(ssrf_redirect_policy()) + .resolve_to_addrs(host, addrs) + .build() + .map_err(|e| format!("http client: {e}")) +} + fn add(found: &mut BTreeMap, id: &str, label: Option, source: String) { // Drop false positives by validating as a real principal. if Principal::from_text(id).is_err() { @@ -90,7 +232,13 @@ fn add(found: &mut BTreeMap, id: &str, label: Option, sou pub async fn discover(domain: &str) -> Result, String> { let base = normalize(domain); - let client = http_client()?; + // SSRF guard (CWE-918): validate + resolve the fully user-controlled target + // to public addresses BEFORE any request, and pin them into the client so the + // connection can't rebind to an internal host. The pin only affects this site + // host, so the dashboard-enrichment calls below (different hosts) still work. + let (base_url, pinned) = resolve_public_url(&base).await?; + let host = base_url.host_str().unwrap_or_default().to_string(); + let client = site_client(&host, &pinned)?; let mut found: BTreeMap = BTreeMap::new(); @@ -231,12 +379,16 @@ fn resolve_base(configured: Option, default: &str) -> String { } } -/// Shared HTTP client for discovery + dashboard calls. Short-ish timeout since -/// these back interactive tools. +/// Shared HTTP client for dashboard/registry calls (fixed public hosts) and the +/// `lookup_canister` tool. Carries the SSRF redirect guard so a 3xx can never +/// bounce a request onto an internal host. Short-ish timeout since these back +/// interactive tools. (User-supplied *site* fetches use [`site_client`], which +/// additionally pins the resolved address.) pub fn http_client() -> Result { reqwest::Client::builder() .user_agent("ic-mcp-discover/0.1") .timeout(std::time::Duration::from_secs(15)) + .redirect(ssrf_redirect_policy()) .build() .map_err(|e| format!("http client: {e}")) } @@ -495,6 +647,55 @@ fn search_in(ledgers_json: &str, snses_json: &str, query: &str) -> Vec { mod tests { use super::*; + // SSRF guard (CWE-918): global vs. non-global IP classification (offline). + #[test] + fn ip_globality_classification() { + let g = |s: &str| ip_is_global(&s.parse::().unwrap()); + // Publicly-routable addresses. + assert!(g("8.8.8.8")); + assert!(g("1.1.1.1")); + assert!(g("2606:4700:4700::1111")); + // Loopback / private / link-local / CGNAT / reserved / doc / bench / mapped. + for bad in [ + "127.0.0.1", "10.0.0.1", "172.16.0.1", "192.168.1.1", "169.254.169.254", + "100.64.0.1", "0.0.0.0", "255.255.255.255", "192.0.2.1", "198.18.0.1", "240.0.0.1", + "::1", "::", "fc00::1", "fd12::1", "fe80::1", "2001:db8::1", "::ffff:127.0.0.1", + ] { + assert!(!g(bad), "{bad} must be classified non-global"); + } + } + + // The exact vectors from the finding, plus https-to-internal, are refused + // (all offline: scheme check and IP literals need no DNS). + #[tokio::test] + async fn resolve_public_url_rejects_ssrf_targets() { + for bad in [ + "http://169.254.169.254/latest/meta-data/", // http metadata + "http://127.0.0.1:6379/", // http plaintext port + "https://127.0.0.1:6379/", // https loopback + "https://10.0.0.1/", // private + "https://169.254.169.254/", // link-local metadata + "https://[::1]/", // loopback v6 + "ftp://example.com/", // non-https scheme + ] { + assert!(resolve_public_url(bad).await.is_err(), "{bad} must be refused"); + } + // A public IP literal is accepted and pinned to itself. + let (_, addrs) = resolve_public_url("https://8.8.8.8/").await.expect("public ip ok"); + assert_eq!(addrs, vec!["8.8.8.8:443".parse().unwrap()]); + } + + // Redirect targets: only public https hops are followable. + #[test] + fn redirect_target_rejects_internal_and_http() { + let ok = |s: &str| redirect_target_ok(&url::Url::parse(s).unwrap()); + assert!(ok("https://8.8.8.8/x")); + assert!(!ok("http://8.8.8.8/x")); // non-https + assert!(!ok("https://127.0.0.1/x")); + assert!(!ok("https://169.254.169.254/x")); + assert!(!ok("https://[::1]/x")); + } + // Live network test against a stable public IC app (OISY). #[tokio::test] async fn discovers_oisy_frontend_and_backend() { From af7988b96566ca6e1d133f66806d0708c68cd49c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 5 Jul 2026 15:42:58 +0000 Subject: [PATCH 2/2] SSRF guard: catch IPv4-compatible IPv6, harden redirect policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym --- src/discover.rs | 77 +++++++++++++++++++++++++++++-------------------- 1 file changed, 46 insertions(+), 31 deletions(-) diff --git a/src/discover.rs b/src/discover.rs index 0290fee..3167d00 100644 --- a/src/discover.rs +++ b/src/discover.rs @@ -16,7 +16,7 @@ use std::{ collections::BTreeMap, - net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr, ToSocketAddrs}, + net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}, }; use candid::Principal; @@ -111,8 +111,11 @@ fn ipv4_is_global(ip: &Ipv4Addr) -> bool { } fn ipv6_is_global(ip: &Ipv6Addr) -> bool { - // An IPv4-mapped address (::ffff:0:0/96) is only as global as the v4 it embeds. - if let Some(v4) = ip.to_ipv4_mapped() { + // An IPv4-mapped (`::ffff:0:0/96`) OR IPv4-compatible (`::/96`, e.g. + // `::127.0.0.1`) address is only as global as the v4 it embeds. `to_ipv4` + // (unlike `to_ipv4_mapped`) covers BOTH forms — plus `::`/`::1` — so a + // private/loopback v4 can't slip through via IPv6 embedding. + if let Some(v4) = ip.to_ipv4() { return ipv4_is_global(&v4); } let seg = ip.segments(); @@ -157,37 +160,40 @@ async fn resolve_public_url(raw: &str) -> Result<(url::Url, Vec), St Ok((url, addrs)) } -/// Whether a redirect target is an acceptable public https destination, used by -/// the client redirect policy so a 3xx can't bounce a request to an internal -/// host. Resolution here is blocking (reqwest's redirect callback is sync), but -/// only runs on the rare cross-host redirect. -fn redirect_target_ok(url: &url::Url) -> bool { - if url.scheme() != "https" { +/// Whether a redirect hop is safe to follow — decided WITHOUT a DNS lookup (the +/// reqwest redirect callback is synchronous, so resolving a fresh domain there +/// would both block a worker thread and reopen a rebind TOCTOU on the redirect +/// target). A hop is allowed only when it is https AND either (a) an IP literal +/// that is globally routable, or (b) a SAME-HOST redirect — the host was already +/// validated up front and, for the site client, pinned, so no new unvalidated +/// destination is introduced. A redirect to a *different* domain is refused; +/// discovery targets normally respond directly, so this trades that rare case for +/// safety (a cross-host redirect would need its own validate+pin). +fn redirect_hop_ok(next: &url::Url, prev_host: Option<&str>) -> bool { + if next.scheme() != "https" { return false; } - let port = url.port_or_known_default().unwrap_or(443); - match url.host() { + match next.host() { Some(url::Host::Ipv4(v4)) => ip_is_global(&IpAddr::V4(v4)), Some(url::Host::Ipv6(v6)) => ip_is_global(&IpAddr::V6(v6)), - Some(url::Host::Domain(host)) => match (host, port).to_socket_addrs() { - Ok(addrs) => { - let addrs: Vec = addrs.collect(); - !addrs.is_empty() && addrs.iter().all(|a| ip_is_global(&a.ip())) - } - Err(_) => false, - }, + Some(url::Host::Domain(h)) => prev_host == Some(h), None => false, } } -/// Redirect policy shared by every discovery/dashboard client: follow only -/// public https hops, bounded, and stop (rather than follow) on a refused hop so +/// Redirect policy shared by every discovery/dashboard client: follow only safe +/// public https hops (bounded), and stop — rather than follow — an unsafe hop, so /// no request is ever issued to an internal host. fn ssrf_redirect_policy() -> reqwest::redirect::Policy { reqwest::redirect::Policy::custom(|attempt| { + let prev_host = attempt + .previous() + .last() + .and_then(|u| u.host_str()) + .map(str::to_string); if attempt.previous().len() >= 10 { attempt.error("too many redirects") - } else if redirect_target_ok(attempt.url()) { + } else if redirect_hop_ok(attempt.url(), prev_host.as_deref()) { attempt.follow() } else { attempt.stop() @@ -655,11 +661,14 @@ mod tests { assert!(g("8.8.8.8")); assert!(g("1.1.1.1")); assert!(g("2606:4700:4700::1111")); - // Loopback / private / link-local / CGNAT / reserved / doc / bench / mapped. + // Loopback / private / link-local / CGNAT / reserved / doc / bench, plus + // IPv4 embedded in IPv6 as MAPPED (::ffff:…) and COMPATIBLE (::…) forms. for bad in [ "127.0.0.1", "10.0.0.1", "172.16.0.1", "192.168.1.1", "169.254.169.254", "100.64.0.1", "0.0.0.0", "255.255.255.255", "192.0.2.1", "198.18.0.1", "240.0.0.1", - "::1", "::", "fc00::1", "fd12::1", "fe80::1", "2001:db8::1", "::ffff:127.0.0.1", + "::1", "::", "fc00::1", "fd12::1", "fe80::1", "2001:db8::1", + "::ffff:127.0.0.1", "::ffff:10.0.0.1", // IPv4-mapped private/loopback + "::127.0.0.1", "::192.168.1.1", // IPv4-compatible private/loopback ] { assert!(!g(bad), "{bad} must be classified non-global"); } @@ -685,15 +694,21 @@ mod tests { assert_eq!(addrs, vec!["8.8.8.8:443".parse().unwrap()]); } - // Redirect targets: only public https hops are followable. + // Redirect hops: same-host or global-IP-literal https only; no DNS involved. #[test] - fn redirect_target_rejects_internal_and_http() { - let ok = |s: &str| redirect_target_ok(&url::Url::parse(s).unwrap()); - assert!(ok("https://8.8.8.8/x")); - assert!(!ok("http://8.8.8.8/x")); // non-https - assert!(!ok("https://127.0.0.1/x")); - assert!(!ok("https://169.254.169.254/x")); - assert!(!ok("https://[::1]/x")); + fn redirect_hop_policy() { + let u = |s: &str| url::Url::parse(s).unwrap(); + // Same-host https redirect is allowed (host already validated / pinned). + assert!(redirect_hop_ok(&u("https://oisy.com/app"), Some("oisy.com"))); + // A redirect to a DIFFERENT domain is refused (can't validate+pin here). + assert!(!redirect_hop_ok(&u("https://evil.example/x"), Some("oisy.com"))); + // Global IP-literal hop allowed; internal IP-literal hops refused. + assert!(redirect_hop_ok(&u("https://8.8.8.8/x"), Some("oisy.com"))); + assert!(!redirect_hop_ok(&u("https://127.0.0.1/x"), Some("oisy.com"))); + assert!(!redirect_hop_ok(&u("https://169.254.169.254/x"), Some("oisy.com"))); + assert!(!redirect_hop_ok(&u("https://[::1]/x"), Some("oisy.com"))); + // Non-https refused even on the same host. + assert!(!redirect_hop_ok(&u("http://oisy.com/x"), Some("oisy.com"))); } // Live network test against a stable public IC app (OISY).