diff --git a/crates/kingfisher-scanner/src/validation/http_validation.rs b/crates/kingfisher-scanner/src/validation/http_validation.rs index fb32b28..4658092 100644 --- a/crates/kingfisher-scanner/src/validation/http_validation.rs +++ b/crates/kingfisher-scanner/src/validation/http_validation.rs @@ -395,7 +395,7 @@ pub fn validate_response( /// Returns `true` if the IP address is safe for outbound validation requests /// (i.e., it is a publicly routable address, not internal/reserved). /// -/// Covers all IANA special-purpose ranges from RFC 6890 and RFC 8190. +/// Blocks common IANA special-purpose ranges from RFC 6890 and RFC 8190. pub fn is_ssrf_safe_ip(ip: &IpAddr) -> bool { if ip.is_loopback() || ip.is_unspecified() || ip.is_multicast() { return false; diff --git a/crates/kingfisher-scanner/src/validation/jwt.rs b/crates/kingfisher-scanner/src/validation/jwt.rs index 5d5d35b..33828f4 100644 --- a/crates/kingfisher-scanner/src/validation/jwt.rs +++ b/crates/kingfisher-scanner/src/validation/jwt.rs @@ -1,3 +1,4 @@ +use super::http_validation::check_url_resolvable; use anyhow::{anyhow, Result}; use base64::{engine::general_purpose::URL_SAFE_NO_PAD, Engine as _}; use chrono::Utc; @@ -7,9 +8,6 @@ use jsonwebtoken::{ use once_cell::sync::Lazy; use reqwest::{redirect::Policy, Client, Url}; use serde::Deserialize; -use tokio::net::lookup_host; - -use super::http_validation::{check_url_resolvable, is_ssrf_safe_ip}; /// Global redirect-free client with strict TLS validation. static STRICT_CLIENT: Lazy = Lazy::new(|| { @@ -198,18 +196,17 @@ pub async fn validate_jwt_with( )); } - if !allow_internal_ips { - for addr in lookup_host((jwks_host.as_str(), 443)).await? { - if !is_ssrf_safe_ip(&addr.ip()) { + match check_url_resolvable(&url, allow_internal_ips).await { + Ok(()) => {} + Err(e) => { + let msg = e.to_string(); + if msg.contains("SSRF protection") { return Ok((false, "jwks_uri resolves to non-public or reserved IP".to_string())); } + return Err(anyhow!("jwks uri unresolvable: {e}")); } } - check_url_resolvable(&url, allow_internal_ips) - .await - .map_err(|e| anyhow!("jwks uri unresolvable: {e}"))?; - let jwks_resp = client.get(url).send().await.map_err(|e| anyhow!("jwks fetch failed: {e}"))?; if !jwks_resp.status().is_success() { return Ok((false, format!("jwks fetch failed: {}", jwks_resp.status()))); diff --git a/src/validation.rs b/src/validation.rs index bda079c..edf38a3 100644 --- a/src/validation.rs +++ b/src/validation.rs @@ -127,8 +127,13 @@ pub struct ValidationClients { /// Each redirect hop is checked: IP-literal targets are validated directly via /// `is_ssrf_safe_ip`, and hostname targets are resolved synchronously via /// `std::net::ToSocketAddrs` so that all resolved IPs can be checked. This -/// closes the hostname-redirect SSRF gap (e.g., a public URL that 302s to an -/// attacker-controlled hostname resolving to `169.254.169.254`). +/// significantly reduces the hostname-redirect SSRF risk (e.g., a public URL +/// that 302s to an attacker-controlled hostname resolving to `169.254.169.254`). +/// This is a best-effort check: reqwest performs its own DNS resolution when +/// connecting, so a malicious DNS server could return different IPs between +/// this check and the actual request (DNS rebinding / TOCTOU). A future +/// hardening step would be a pinned/custom resolver so that validated IPs are +/// exactly those used for the outbound connection. /// /// **Note:** reqwest runs redirect callbacks on Tokio worker threads, so the /// blocking DNS lookup here can briefly stall other async tasks on that thread. @@ -156,7 +161,7 @@ pub(crate) fn ssrf_safe_redirect_policy() -> reqwest::redirect::Policy { } else { // Hostname: resolve synchronously and check all resolved IPs. let port = url.port().unwrap_or(if url.scheme() == "https" { 443 } else { 80 }); - match std::net::ToSocketAddrs::to_socket_addrs(&(host, port as u16)) { + match std::net::ToSocketAddrs::to_socket_addrs(&(host, port)) { Ok(addrs) => { for addr in addrs { if !kingfisher_scanner::validation::is_ssrf_safe_ip(&addr.ip()) {