🛡️ Sentinel: [CRITICAL] Fix SSRF vulnerability in web_fetch tool

.jules/sentinel.md

@@ -2,3 +2,8 @@
 **Vulnerability:** The `OperationQueue` worker in `mill-server` executed file operations (create, write, delete, rename) using raw paths from the operation object without validating they were within the project root.
 **Learning:** Background workers that process serialized operations are a common bypass for security checks enforced at the API layer. The API layer might validate the request, but if the worker is "dumb" and blindly executes the queued operation, an internal attacker or a buggy component can exploit it.
 **Prevention:** Validation must happen at the *execution point* (in the worker), not just at the ingestion point. We introduced `validate_path` in the worker loop to enforce project root containment using `canonicalize` (handling non-existent files correctly).
+
+## 2025-06-11 - SSRF Prevention via Manual IP Pinning
+**Vulnerability:** The `web_fetch` tool used `reqwest::blocking::get` directly, allowing an attacker to request internal network services via SSRF and bypassing DNS-based validation due to TOCTOU DNS Rebinding.
+**Learning:** Validating a URL's host IP before passing it to `reqwest` is insufficient because an attacker can return a benign IP during validation and a malicious internal IP during the actual fetch (DNS Rebinding). Furthermore, blindly replacing the host with the IP breaks SNI/HTTPS.
+**Prevention:** Mitigate SSRF by disabling automatic redirects, manually resolving the hostname, validating all returned IPs, and explicitly pinning the validated IP using `reqwest::blocking::ClientBuilder::resolve()`. Manually track HTTP redirects and repeat the validation loop. Ensure IP validation checks all restricted IPv4 and IPv6 ranges, including IPv4-mapped IPv6.

crates/mill-plugin-system/src/system_tools_plugin.rs

@@ -12,9 +12,11 @@ use ignore::WalkBuilder;
 use mill_plugin_api::language::detect_package_manager;
 use serde::Deserialize;
 use serde_json::{json, Value};
+use std::net::{IpAddr, Ipv4Addr, ToSocketAddrs};
 use std::path::Path;
 use std::sync::Arc;
 use tracing::{debug, warn};
+use url::Url;
 
 /// System tools plugin for non-LSP workspace operations
 pub struct SystemToolsPlugin {
@@ -292,6 +294,59 @@ async fn handle_bulk_update_dependencies(params: Value) -> PluginResult<Value> {
     }))
 }
 
+// 🛡️ Sentinel: Helper to check if an IPv4 address is allowed (mitigates SSRF)
+fn is_allowed_ipv4(ip: &Ipv4Addr) -> bool {
+    let octets = ip.octets();
+
+    // Explicitly block 0.0.0.0/8
+    if octets[0] == 0 {
+        return false;
+    }
+    if ip.is_loopback() || ip.is_broadcast() || ip.is_multicast() || ip.is_unspecified() {
+        return false;
+    }
+    // Private (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16)
+    if octets[0] == 10 {
+        return false;
+    }
+    if octets[0] == 172 && (16..=31).contains(&octets[1]) {
+        return false;
+    }
+    if octets[0] == 192 && octets[1] == 168 {
+        return false;
+    }
+    // Link-local (169.254.0.0/16)
+    if octets[0] == 169 && octets[1] == 254 {
+        return false;
+    }
+    true
+}
+
+// 🛡️ Sentinel: Helper to check if an IP address is allowed (mitigates SSRF)
+fn is_allowed_ip(ip: &IpAddr) -> bool {
+    match ip {
+        IpAddr::V4(ipv4) => is_allowed_ipv4(ipv4),
+        IpAddr::V6(ipv6) => {
+            if let Some(ipv4) = ipv6.to_ipv4_mapped() {
+                return is_allowed_ipv4(&ipv4);
+            }
+            if ipv6.is_loopback() || ipv6.is_unspecified() || ipv6.is_multicast() {
+                return false;
+            }
+            let segments = ipv6.segments();
+            // Link-local (fe80::/10)
+            if segments[0] & 0xffc0 == 0xfe80 {
+                return false;
+            }
+            // Unique-local (fc00::/7)
+            if segments[0] & 0xfe00 == 0xfc00 {
+                return false;
+            }
+            true
+        }
+    }
+}
+
 /// Handle web_fetch tool
 fn handle_web_fetch(params: Value) -> PluginResult<Value> {
     #[derive(Debug, Deserialize)]
@@ -307,11 +362,90 @@ fn handle_web_fetch(params: Value) -> PluginResult<Value> {
 
     debug!(url = %args.url, "Fetching URL content");
 
-    // Use reqwest to fetch the URL content
-    let response = reqwest::blocking::get(&args.url).map_err(|e| PluginSystemError::IoError {
-        message: format!("Failed to fetch URL: {}", e),
+    let mut current_url = Url::parse(&args.url).map_err(|e| PluginSystemError::IoError {
+        message: format!("Invalid URL: {}", e),
     })?;
 
+    let mut redirects = 0;
+    const MAX_REDIRECTS: u32 = 10;
+
+    let response = loop {
+        if redirects >= MAX_REDIRECTS {
+            return Err(PluginSystemError::IoError {
+                message: "Too many redirects".to_string(),
+            });
+        }
+
+        let host = current_url
+            .host_str()
+            .ok_or_else(|| PluginSystemError::IoError {
+                message: "URL must have a host".to_string(),
+            })?
+            .to_string();
+
+        let port = current_url.port_or_known_default().unwrap_or(80);
+
+        // Resolve host to IPs
+        let addr_str = format!("{}:{}", host, port);
+        let addrs = addr_str
+            .to_socket_addrs()
+            .map_err(|e| PluginSystemError::IoError {
+                message: format!("Failed to resolve host: {}", e),
+            })?;
+
+        let mut allowed_addr = None;
+        for addr in addrs {
+            // 🛡️ Sentinel: Ensure ALL resolved IPs are allowed, otherwise an attacker
+            // could return a mix of allowed/disallowed IPs to bypass the check.
+            if !is_allowed_ip(&addr.ip()) {
+                return Err(PluginSystemError::IoError {
+                    message: format!("Access to IP {} is denied", addr.ip()),
+                });
+            }
+            if allowed_addr.is_none() {
+                allowed_addr = Some(addr);
+            }
+        }
+
+        let addr = allowed_addr.ok_or_else(|| PluginSystemError::IoError {
+            message: "No resolved IP addresses found".to_string(),
+        })?;
+
+        // 🛡️ Sentinel: Explicitly pin the validated IP while preserving original hostname for SNI.
+        // Disable automatic redirects to prevent TOCTOU DNS Rebinding across redirects.
+        let client = reqwest::blocking::Client::builder()
+            .redirect(reqwest::redirect::Policy::none())
+            .resolve(&host, addr)
+            .build()
+            .map_err(|e| PluginSystemError::IoError {
+                message: format!("Failed to build HTTP client: {}", e),
+            })?;
+
+        let res =
+            client
+                .get(current_url.clone())
+                .send()
+                .map_err(|e| PluginSystemError::IoError {
+                    message: format!("Failed to fetch URL: {}", e),
+                })?;
+
+        if res.status().is_redirection() {
+            if let Some(loc) = res.headers().get(reqwest::header::LOCATION) {
+                let loc_str = loc.to_str().unwrap_or("");
+                current_url =
+                    current_url
+                        .join(loc_str)
+                        .map_err(|e| PluginSystemError::IoError {
+                            message: format!("Invalid redirect URL: {}", e),
+                        })?;
+                redirects += 1;
+                continue;
+            }
+        }
+
+        break res;
+    };
+
     let html_content = response.text().map_err(|e| PluginSystemError::IoError {
         message: format!("Failed to read response text: {}", e),
     })?;