🛡️ Sentinel: [CRITICAL] Fix Server-Side Request Forgery in web_fetch tool#598
🛡️ Sentinel: [CRITICAL] Fix Server-Side Request Forgery in web_fetch tool#598mudcube wants to merge 1 commit into
Conversation
…tool This commit addresses a critical SSRF vulnerability in the `web_fetch` plugin system tool. Previously, arbitrary URLs provided by users were directly fetched using `reqwest`, potentially allowing scanning of internal network topologies and accessing localized cloud metadata services (e.g., AWS IMDSv2). The mitigation includes: - Manually resolving DNS records for the requested host. - Validating ALL resolved IP addresses against a strict blocklist (catching Private IPs, Loopback, Link-Local, Broadcast, and Multicast for both IPv4 and IPv6). - Pinning the validated IP address to the `reqwest` client using `resolve(host, pinned_addr)` to prevent Time-of-Check to Time-of-Use (TOCTOU) DNS Rebinding attacks without breaking SNI checks. - Disabling automatic redirects and manually tracking `Location` headers so that each redirect hop passes through the exact same DNS resolution and strict IP blocklist process. Co-authored-by: mudcube <101564+mudcube@users.noreply.github.com>
Deploying typemill with
|
| Latest commit: |
692b19e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e0dab6af.typemill.pages.dev |
| Branch Preview URL: | https://sentinel-fix-ssrf-web-fetch-d7q0.typemill.pages.dev |
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 692b19e807
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if octets[0] == 10 | ||
| || (octets[0] == 172 && (16..=31).contains(&octets[1])) | ||
| || (octets[0] == 192 && octets[1] == 168) | ||
| { |
There was a problem hiding this comment.
In environments that use RFC6598 shared address space (100.64.0.0/10) for internal services, this allowlist falls through to true, so web_fetch can still reach internal-only hosts such as http://100.64.0.1/ despite the SSRF mitigation. Since this code is denylisting selected ranges rather than requiring globally routable addresses, add the shared/non-global IPv4 ranges here (or use an is_global equivalent) before returning true.
Useful? React with 👍 / 👎.
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: Server-Side Request Forgery (SSRF) in the
web_fetchtool. The tool fetched any user-provided URL without validating its target IP, allowing potential internal network access, scanning, and metadata exfiltration.🎯 Impact: An attacker could use this tool to bypass firewalls and make HTTP requests to internal IP ranges (e.g.,
127.0.0.1,10.x,169.254.169.254for AWS metadata).🔧 Fix: Implemented an
is_allowed_iphelper that rigorously blocks local, private, and reserved IPs. Updatedhandle_web_fetchto manually resolve DNS and validate all IPs before request dispatch. We usereqwest::blocking::ClientBuilder::resolve()to securely pin the validated IP to the client, preventing TOCTOU DNS Rebinding attacks. Furthermore, redirects are followed manually so each new destination goes through the exact same IP checks.✅ Verification: Verified via
cargo testandcargo check. The implementation handles DNS rebinding safely while preserving TLS SNI. Added entry to.jules/sentinel.md.PR created automatically by Jules for task 2506383611434423128 started by @mudcube