diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 798ec7de6..aa2f699ae 100644 --- a/.jules/sentinel.md +++ b/.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. diff --git a/crates/mill-plugin-system/src/system_tools_plugin.rs b/crates/mill-plugin-system/src/system_tools_plugin.rs index e20b7d1ae..04d61ef1b 100644 --- a/crates/mill-plugin-system/src/system_tools_plugin.rs +++ b/crates/mill-plugin-system/src/system_tools_plugin.rs @@ -292,6 +292,63 @@ async fn handle_bulk_update_dependencies(params: Value) -> PluginResult { })) } +/// 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 { #[derive(Debug, Deserialize)] @@ -307,9 +364,110 @@ fn handle_web_fetch(params: Value) -> PluginResult { 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 { diff --git a/patch_script.py b/patch_script.py new file mode 100644 index 000000000..4a1f98340 --- /dev/null +++ b/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 { + #[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 { + #[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) diff --git a/plan.md b/plan.md new file mode 100644 index 000000000..fe91b0dea --- /dev/null +++ b/plan.md @@ -0,0 +1,15 @@ +1. **Analyze Security Issue**: I have identified a Server-Side Request Forgery (SSRF) vulnerability in the `web_fetch` tool inside `crates/mill-plugin-system/src/system_tools_plugin.rs`. The code uses `reqwest::blocking::get(&args.url)` without validating the resolved IP address, which allows an attacker to make requests to internal services or private IP addresses. + +2. **Develop SSRF Mitigation**: Based on the memory instructions: + * To mitigate SSRF with `reqwest::blocking` (v0.12+), `ClientBuilder::resolve(host, socket_addr)` IS supported for IP pinning. + * Implement manual redirect tracking, sequentially validate IPs, disable automatic redirects, and use `.resolve()` inside the loop to pin the validated IP, preventing TOCTOU DNS rebinding without breaking SNI. + * The `is_allowed_ip` helper function must manually filter loopback, private, unspecified, multicast, broadcast, and link-local IPs. + * Explicitly apply filtering to both native IPv4 addresses and IPv4-mapped IPv6 addresses (`to_ipv4_mapped()`). + +3. **Implement the Fix**: Modify `handle_web_fetch` in `crates/mill-plugin-system/src/system_tools_plugin.rs` to include the manual redirect tracking loop, DNS resolution, and IP filtering logic. + +4. **Add Security Journal Entry**: Based on the memory instructions, document the SSRF vulnerability and the fix in `.jules/sentinel.md`. + +5. **Pre-commit Steps**: Ensure proper testing, verification, review, and reflection are done. + +6. **Submit PR**: Create a PR with the required title and description for a Sentinel task.