🛡️ Sentinel: [CRITICAL] Fix Server-Side Request Forgery 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-05-24 - [SSRF Protection in web_fetch Tool]
+**Vulnerability:** The `web_fetch` tool in `system_tools_plugin` accepted arbitrary URLs and directly requested them, allowing Server-Side Request Forgery (SSRF). This enabled internal network scanning and access to local services like AWS metadata endpoints.
+**Learning:** SSRF mitigation with `reqwest` requires careful handling to prevent TOCTOU (DNS Rebinding) attacks. Resolving DNS first and then passing the IP to `reqwest` breaks HTTPS (SNI). Using `ClientBuilder::resolve()` pins the IP for a specific host safely. Additionally, automatic redirects must be disabled and manually followed so each redirect target's IP can be validated.
+**Prevention:** Always validate resolved IP addresses against a strict blocklist (private, loopback, link-local, etc.) for user-provided URLs. Use IP pinning (`resolve()`) and manual redirect tracking when using `reqwest` for external requests.

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

@@ -292,6 +292,66 @@ async fn handle_bulk_update_dependencies(params: Value) -> PluginResult<Value> {
     }))
 }
 
+/// Helper function to block private, loopback, link-local, multicast, and broadcast IPs
+fn is_allowed_ip(ip: &std::net::IpAddr) -> bool {
+    match ip {
+        std::net::IpAddr::V4(ipv4) => {
+            let octets = ipv4.octets();
+            // Block 0.0.0.0/8
+            if octets[0] == 0 {
+                return false;
+            }
+            // Block private network ranges
+            if octets[0] == 10
+                || (octets[0] == 172 && (16..=31).contains(&octets[1]))
+                || (octets[0] == 192 && octets[1] == 168)
+            {
+                return false;
+            }
+            // Block loopback (127.0.0.0/8)
+            if ipv4.is_loopback() {
+                return false;
+            }
+            // Block link-local (169.254.0.0/16)
+            if ipv4.is_link_local() {
+                return false;
+            }
+            // Block broadcast/multicast
+            if ipv4.is_broadcast() || ipv4.is_multicast() {
+                return false;
+            }
+            true
+        }
+        std::net::IpAddr::V6(ipv6) => {
+            // Unspecified
+            if ipv6.is_unspecified() {
+                return false;
+            }
+            // Loopback
+            if ipv6.is_loopback() {
+                return false;
+            }
+            // Multicast
+            if ipv6.is_multicast() {
+                return false;
+            }
+            // Unique Local (fc00::/7)
+            if (ipv6.segments()[0] & 0xfe00) == 0xfc00 {
+                return false;
+            }
+            // Link-Local (fe80::/10)
+            if (ipv6.segments()[0] & 0xffc0) == 0xfe80 {
+                return false;
+            }
+            // Check if it's an IPv4-mapped IPv6 address and validate the IPv4 part
+            if let Some(ipv4) = ipv6.to_ipv4_mapped() {
+                return is_allowed_ip(&std::net::IpAddr::V4(ipv4));
+            }
+            true
+        }
+    }
+}
+
 /// Handle web_fetch tool
 fn handle_web_fetch(params: Value) -> PluginResult<Value> {
     #[derive(Debug, Deserialize)]
@@ -307,9 +367,92 @@ 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),
+    // SSRF Protection: Parse URL, validate resolved IPs, and follow redirects manually
+    // to prevent DNS rebinding TOCTOU attacks while preserving SNI.
+    let mut current_url = url::Url::parse(&args.url).map_err(|e| PluginSystemError::IoError {
+        message: format!("Invalid URL: {}", e),
+    })?;
+
+    let mut response = None;
+    let max_redirects = 10;
+
+    for _ in 0..max_redirects {
+        let host = current_url
+            .host_str()
+            .ok_or_else(|| PluginSystemError::IoError {
+                message: "URL must have a host".to_string(),
+            })?;
+
+        let port =
+            current_url
+                .port_or_known_default()
+                .ok_or_else(|| PluginSystemError::IoError {
+                    message: "Unsupported URL scheme".to_string(),
+                })?;
+
+        // Resolve DNS
+        use std::net::ToSocketAddrs;
+        let addrs = format!("{}:{}", host, port)
+            .to_socket_addrs()
+            .map_err(|e| PluginSystemError::IoError {
+                message: format!("Failed to resolve DNS: {}", e),
+            })?;
+
+        // Validate ALL resolved IPs
+        let mut pinned_addr = None;
+        for addr in addrs {
+            if !is_allowed_ip(&addr.ip()) {
+                return Err(PluginSystemError::IoError {
+                    message: format!("Access to IP {} is forbidden (SSRF protection)", addr.ip()),
+                });
+            }
+            if pinned_addr.is_none() {
+                pinned_addr = Some(addr);
+            }
+        }
+
+        let pinned_addr = pinned_addr.ok_or_else(|| PluginSystemError::IoError {
+            message: "No IP addresses resolved".to_string(),
+        })?;
+
+        // Create a client that pins the IP for the hostname and doesn't auto-redirect
+        let client = reqwest::blocking::Client::builder()
+            .resolve(host, pinned_addr)
+            .redirect(reqwest::redirect::Policy::none())
+            .build()
+            .map_err(|e| PluginSystemError::IoError {
+                message: format!("Failed to create 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().map_err(|_| PluginSystemError::IoError {
+                    message: "Invalid Location header".to_string(),
+                })?;
+                current_url =
+                    current_url
+                        .join(loc_str)
+                        .map_err(|e| PluginSystemError::IoError {
+                            message: format!("Invalid redirect URL: {}", e),
+                        })?;
+                continue;
+            }
+        }
+
+        response = Some(res);
+        break;
+    }
+
+    let response = response.ok_or_else(|| PluginSystemError::IoError {
+        message: "Too many redirects".to_string(),
     })?;
 
     let html_content = response.text().map_err(|e| PluginSystemError::IoError {