🛡️ 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-10 - SSRF Vulnerability in Web Fetch
+**Vulnerability:** The `web_fetch` tool used `reqwest::blocking::get` without validating the resolved IP address, which allows an attacker to make requests to internal services or private IP addresses via Server-Side Request Forgery (SSRF).
+**Learning:** When mitigating SSRF with `reqwest`, replacing the URL's hostname with the validated IP address to prevent TOCTOU DNS Rebinding attacks breaks HTTPS connections due to SNI and certificate mismatch. The original hostname must be preserved. Furthermore, we must check all resolved IPs, block `0.0.0.0/8`, and properly handle IPv6 wrapper brackets when using `ToSocketAddrs`.
+**Prevention:** Always implement manual redirect tracking, sequentially validate IPs, disable automatic redirects, and use `ClientBuilder::resolve()` inside the loop to pin the validated IP, preventing TOCTOU DNS rebinding without breaking SNI.

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

@@ -292,6 +292,63 @@ async fn handle_bulk_update_dependencies(params: Value) -> PluginResult<Value> {
     }))
 }
 
+/// Helper to validate if an IP address is allowed (mitigates SSRF)
+fn is_allowed_ip(ip: &std::net::IpAddr) -> bool {
+    use std::net::IpAddr;
+    match ip {
+        IpAddr::V4(v4) => is_allowed_ipv4(v4),
+        IpAddr::V6(v6) => {
+            if let Some(v4) = v6.to_ipv4_mapped() {
+                is_allowed_ipv4(&v4)
+            } else {
+                let segments = v6.segments();
+                // Block loopback (::1), unspecified (::), multicast (ff00::/8)
+                if v6.is_loopback() || v6.is_unspecified() || v6.is_multicast() {
+                    return false;
+                }
+                // Block Unique Local (fc00::/7)
+                if (segments[0] & 0xfe00) == 0xfc00 {
+                    return false;
+                }
+                // Block Link-Local (fe80::/10)
+                if (segments[0] & 0xffc0) == 0xfe80 {
+                    return false;
+                }
+                true
+            }
+        }
+    }
+}
+
+fn is_allowed_ipv4(v4: &std::net::Ipv4Addr) -> bool {
+    let octets = v4.octets();
+    // Block 0.0.0.0/8, loopback (127.0.0.0/8), 10.0.0.0/8, link-local (169.254.0.0/16)
+    if octets[0] == 0
+        || octets[0] == 127
+        || octets[0] == 10
+        || (octets[0] == 169 && octets[1] == 254)
+    {
+        return false;
+    }
+    // Block 172.16.0.0/12
+    if octets[0] == 172 && (16..=31).contains(&octets[1]) {
+        return false;
+    }
+    // Block 192.168.0.0/16
+    if octets[0] == 192 && octets[1] == 168 {
+        return false;
+    }
+    // Block broadcast (255.255.255.255)
+    if octets[0] == 255 && octets[1] == 255 && octets[2] == 255 && octets[3] == 255 {
+        return false;
+    }
+    // Block multicast (224.0.0.0/4)
+    if (224..=239).contains(&octets[0]) {
+        return false;
+    }
+    true
+}
+
 /// Handle web_fetch tool
 fn handle_web_fetch(params: Value) -> PluginResult<Value> {
     #[derive(Debug, Deserialize)]
@@ -307,9 +364,110 @@ 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::Url::parse(&args.url).map_err(|e| PluginSystemError::IoError {
+        message: format!("Invalid URL: {}", e),
+    })?;
+
+    let mut redirect_count = 0;
+    let max_redirects = 10;
+    let mut response = None;
+
+    // Manual redirect tracking loop for IP pinning
+    while redirect_count < max_redirects {
+        let host = current_url
+            .host_str()
+            .ok_or_else(|| PluginSystemError::IoError {
+                message: "URL missing host".to_string(),
+            })?;
+
+        let port =
+            current_url
+                .port_or_known_default()
+                .ok_or_else(|| PluginSystemError::IoError {
+                    message: "Unknown port for URL scheme".to_string(),
+                })?;
+
+        // Format host for resolution, explicitly wrapping IPv6 in brackets if needed
+        let host_for_resolution = if host.contains(':') && !host.starts_with('[') {
+            format!("[{}]:{}", host, port)
+        } else {
+            format!("{}:{}", host, port)
+        };
+
+        use std::net::ToSocketAddrs;
+        let mut addrs = host_for_resolution
+            .to_socket_addrs()
+            .map_err(|e| PluginSystemError::IoError {
+                message: format!("Failed to resolve host {}: {}", host_for_resolution, e),
+            })?
+            .peekable();
+
+        if addrs.peek().is_none() {
+            return Err(PluginSystemError::IoError {
+                message: format!("Could not resolve host {}", host),
+            });
+        }
+
+        let mut validated_ip = None;
+        for addr in addrs {
+            let ip = addr.ip();
+            if !is_allowed_ip(&ip) {
+                return Err(PluginSystemError::IoError {
+                    message: format!("Access to IP {} is forbidden", ip),
+                });
+            }
+            // Use the first resolved IP for pinning
+            if validated_ip.is_none() {
+                validated_ip = Some(addr);
+            }
+        }
+
+        let pinned_addr = validated_ip.unwrap();
+
+        // Use ClientBuilder::resolve for IP pinning to mitigate TOCTOU DNS Rebinding attacks
+        // while preserving SNI and disabling automatic redirects to manually check the next hop.
+        let client = reqwest::blocking::Client::builder()
+            .redirect(reqwest::redirect::Policy::none())
+            .resolve(host, pinned_addr)
+            .build()
+            .map_err(|e| PluginSystemError::IoError {
+                message: format!("Failed to build client: {}", e),
+            })?;
+
+        let resp =
+            client
+                .get(current_url.clone())
+                .send()
+                .map_err(|e| PluginSystemError::IoError {
+                    message: format!("Failed to fetch URL: {}", e),
+                })?;
+
+        if resp.status().is_redirection() {
+            if let Some(loc) = resp.headers().get(reqwest::header::LOCATION) {
+                let loc_str = loc.to_str().map_err(|e| PluginSystemError::IoError {
+                    message: format!("Invalid location header: {}", e),
+                })?;
+                current_url =
+                    current_url
+                        .join(loc_str)
+                        .map_err(|e| PluginSystemError::IoError {
+                            message: format!("Invalid redirect URL: {}", e),
+                        })?;
+                redirect_count += 1;
+                continue;
+            } else {
+                return Err(PluginSystemError::IoError {
+                    message: "Redirect missing location header".to_string(),
+                });
+            }
+        }
+
+        response = Some(resp);
+        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 {

patch_script.py

@@ -0,0 +1,198 @@
+import re
+
+with open("crates/mill-plugin-system/src/system_tools_plugin.rs", "r") as f:
+    content = f.read()
+
+ssrf_code = """
+/// Helper to validate if an IP address is allowed (mitigates SSRF)
+fn is_allowed_ip(ip: &std::net::IpAddr) -> bool {
+    use std::net::IpAddr;
+    match ip {
+        IpAddr::V4(v4) => is_allowed_ipv4(v4),
+        IpAddr::V6(v6) => {
+            if let Some(v4) = v6.to_ipv4_mapped() {
+                is_allowed_ipv4(&v4)
+            } else {
+                let segments = v6.segments();
+                // Block loopback (::1), unspecified (::), multicast (ff00::/8)
+                if v6.is_loopback() || v6.is_unspecified() || v6.is_multicast() {
+                    return false;
+                }
+                // Block Unique Local (fc00::/7)
+                if (segments[0] & 0xfe00) == 0xfc00 {
+                    return false;
+                }
+                // Block Link-Local (fe80::/10)
+                if (segments[0] & 0xffc0) == 0xfe80 {
+                    return false;
+                }
+                true
+            }
+        }
+    }
+}
+
+fn is_allowed_ipv4(v4: &std::net::Ipv4Addr) -> bool {
+    let octets = v4.octets();
+    // Block 0.0.0.0/8, loopback (127.0.0.0/8), 10.0.0.0/8, link-local (169.254.0.0/16)
+    if octets[0] == 0 || octets[0] == 127 || octets[0] == 10 || (octets[0] == 169 && octets[1] == 254) {
+        return false;
+    }
+    // Block 172.16.0.0/12
+    if octets[0] == 172 && (16..=31).contains(&octets[1]) {
+        return false;
+    }
+    // Block 192.168.0.0/16
+    if octets[0] == 192 && octets[1] == 168 {
+        return false;
+    }
+    // Block broadcast (255.255.255.255)
+    if octets[0] == 255 && octets[1] == 255 && octets[2] == 255 && octets[3] == 255 {
+        return false;
+    }
+    // Block multicast (224.0.0.0/4)
+    if (224..=239).contains(&octets[0]) {
+        return false;
+    }
+    true
+}
+
+/// Handle web_fetch tool
+fn handle_web_fetch(params: Value) -> PluginResult<Value> {
+    #[derive(Debug, Deserialize)]
+    #[serde(rename_all = "snake_case")]
+    struct WebFetchArgs {
+        url: String,
+    }
+
+    let args: WebFetchArgs =
+        serde_json::from_value(params).map_err(|e| PluginSystemError::SerializationError {
+            message: format!("Invalid web_fetch args: {}", e),
+        })?;
+
+    debug!(url = %args.url, "Fetching URL content");
+
+    let mut current_url = url::Url::parse(&args.url).map_err(|e| PluginSystemError::IoError {
+        message: format!("Invalid URL: {}", e),
+    })?;
+
+    let mut redirect_count = 0;
+    let max_redirects = 10;
+    let mut response = None;
+
+    // Manual redirect tracking loop for IP pinning
+    while redirect_count < max_redirects {
+        let host = current_url.host_str().ok_or_else(|| PluginSystemError::IoError {
+            message: "URL missing host".to_string(),
+        })?;
+
+        let port = current_url.port_or_known_default().ok_or_else(|| PluginSystemError::IoError {
+            message: "Unknown port for URL scheme".to_string(),
+        })?;
+
+        // Format host for resolution, explicitly wrapping IPv6 in brackets if needed
+        let host_for_resolution = if host.contains(':') && !host.starts_with('[') {
+            format!("[{}]:{}", host, port)
+        } else {
+            format!("{}:{}", host, port)
+        };
+
+        use std::net::ToSocketAddrs;
+        let mut addrs = host_for_resolution.to_socket_addrs().map_err(|e| PluginSystemError::IoError {
+            message: format!("Failed to resolve host {}: {}", host_for_resolution, e),
+        })?.peekable();
+
+        if addrs.peek().is_none() {
+            return Err(PluginSystemError::IoError {
+                message: format!("Could not resolve host {}", host),
+            });
+        }
+
+        let mut validated_ip = None;
+        for addr in addrs {
+            let ip = addr.ip();
+            if !is_allowed_ip(&ip) {
+                return Err(PluginSystemError::IoError {
+                    message: format!("Access to IP {} is forbidden", ip),
+                });
+            }
+            // Use the first resolved IP for pinning
+            if validated_ip.is_none() {
+                validated_ip = Some(addr);
+            }
+        }
+
+        let pinned_addr = validated_ip.unwrap();
+
+        // Use ClientBuilder::resolve for IP pinning to mitigate TOCTOU DNS Rebinding attacks
+        // while preserving SNI and disabling automatic redirects to manually check the next hop.
+        let client = reqwest::blocking::Client::builder()
+            .redirect(reqwest::redirect::Policy::none())
+            .resolve(host, pinned_addr)
+            .build()
+            .map_err(|e| PluginSystemError::IoError {
+                message: format!("Failed to build client: {}", e),
+            })?;
+
+        let resp = client.get(current_url.clone()).send().map_err(|e| PluginSystemError::IoError {
+            message: format!("Failed to fetch URL: {}", e),
+        })?;
+
+        if resp.status().is_redirection() {
+            if let Some(loc) = resp.headers().get(reqwest::header::LOCATION) {
+                let loc_str = loc.to_str().map_err(|e| PluginSystemError::IoError {
+                    message: format!("Invalid location header: {}", e),
+                })?;
+                current_url = current_url.join(loc_str).map_err(|e| PluginSystemError::IoError {
+                    message: format!("Invalid redirect URL: {}", e),
+                })?;
+                redirect_count += 1;
+                continue;
+            } else {
+                return Err(PluginSystemError::IoError {
+                    message: "Redirect missing location header".to_string(),
+                });
+            }
+        }
+
+        response = Some(resp);
+        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 {
+        message: format!("Failed to read response text: {}", e),
+    })?;
+"""
+
+search = """/// Handle web_fetch tool
+fn handle_web_fetch(params: Value) -> PluginResult<Value> {
+    #[derive(Debug, Deserialize)]
+    #[serde(rename_all = "snake_case")]
+    struct WebFetchArgs {
+        url: String,
+    }
+
+    let args: WebFetchArgs =
+        serde_json::from_value(params).map_err(|e| PluginSystemError::SerializationError {
+            message: format!("Invalid web_fetch args: {}", e),
+        })?;
+
+    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 html_content = response.text().map_err(|e| PluginSystemError::IoError {
+        message: format!("Failed to read response text: {}", e),
+    })?;"""
+
+content = content.replace(search, ssrf_code)
+
+with open("crates/mill-plugin-system/src/system_tools_plugin.rs", "w") as f:
+    f.write(content)