fix(security): resolve DNS once and reuse for SSRF validation to prevent rebinding by gabehamilton · Pull Request #518 · nearai/ironclaw

gabehamilton · 2026-03-04T03:21:43Z

Summary

Fixes DNS TOCTOU (Time-of-Check-Time-of-Use) vulnerability in SSRF protection for both http and web_fetch tools
DNS is now resolved once via tokio::net::lookup_host, validated against the SSRF blocklist, and pinned into a per-request reqwest Client via ClientBuilder::resolve() -- preventing reqwest from independently re-resolving to a potentially different (private) IP
WebFetchTool builds a fresh pinned client per redirect hop, ensuring DNS rebinding cannot occur at any point in a redirect chain

Problem

The previous SSRF protection in validate_url() resolved DNS to check resolved IPs against a blocklist (private ranges, loopback, link-local, cloud metadata endpoints). However, reqwest then independently re-resolved DNS when .send() was called. Between those two lookups, a DNS rebinding attacker could flip a hostname's DNS record from a public IP (passes validation) to a private IP like 169.254.169.254 (AWS metadata endpoint), bypassing the SSRF protection entirely.

Solution

Split URL validation into two distinct phases:

validate_url() -- Synchronous URL structure checks only (scheme, localhost, IP literals). No DNS resolution.
validate_and_resolve_url() -- Async DNS resolution via tokio::net::lookup_host, validates all resolved IPs against the blocklist, returns Vec<SocketAddr>.
build_pinned_client() -- Constructs a per-request reqwest::Client with resolve() pinning so reqwest connects directly to the pre-validated IPs without a second DNS lookup.

Files changed

src/tools/builtin/http.rs -- Refactored validate_url, added validate_and_resolve_url and build_pinned_client, removed stored Client from HttpTool, per-request pinned client construction, 3 new tests
src/tools/builtin/web_fetch.rs -- Removed stored Client from WebFetchTool, per-hop DNS pinning in redirect loop

Test plan

All 28 existing http::tests pass (including URL validation, approval, credential injection)
All 8 existing web_fetch::tests pass
New test: test_validate_and_resolve_rejects_loopback_hostname -- verifies loopback IPs are rejected at the DNS resolution layer
New test: test_validate_and_resolve_accepts_public_host -- verifies public hostnames (example.com) resolve and pass validation
New test: test_build_pinned_client_succeeds -- verifies pinned client construction works
cargo clippy --all --all-features -- zero warnings
No .unwrap() or .expect() in production code

Generated with Claude Code

…ent rebinding The previous SSRF protection resolved DNS in validate_url() to check IPs against a blocklist, but then reqwest independently re-resolved DNS when making the actual HTTP connection. Between validation and connection, a DNS rebinding attack could flip the record from a public IP (passes validation) to a private IP like 169.254.169.254 (AWS metadata endpoint). Fix: split URL validation into two phases: - validate_url(): synchronous URL structure checks (scheme, localhost, IP literals) -- no DNS resolution - validate_and_resolve_url(): async DNS resolution via tokio::net::lookup_host, validates all resolved IPs, returns SocketAddrs - build_pinned_client(): constructs a per-request reqwest Client with resolve() pinning so reqwest connects to the pre-validated IPs without a second DNS lookup Applied to both HttpTool and WebFetchTool. WebFetchTool builds a fresh pinned client per redirect hop, ensuring DNS rebinding cannot occur at any point in a redirect chain. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

gemini-code-assist · 2026-03-04T03:21:46Z

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

Copilot

Pull request overview

This PR hardens SSRF protection for the http and web_fetch tools by eliminating a DNS TOCTOU window: DNS is resolved once, validated against the SSRF blocklist, and then pinned into a per-request reqwest client so the subsequent request cannot be redirected to a different IP via DNS rebinding.

Changes:

Refactors URL validation into a structure-only phase (validate_url) plus an async DNS resolution + blocklist validation phase (validate_and_resolve_url).
Introduces per-request/per-hop reqwest client construction with DNS pinning (build_pinned_client) for both http and web_fetch.
Adds new tests covering DNS resolution validation and pinned client construction.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
`src/tools/builtin/http.rs`	Splits URL validation vs DNS resolution, adds DNS pinning helpers, and switches `HttpTool` to per-request pinned clients; adds tests.
`src/tools/builtin/web_fetch.rs`	Switches `WebFetchTool` to per-hop DNS resolution + pinned client construction during manual redirect handling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-04T03:25:22Z

+
+    let port = url.port_or_known_default().unwrap_or(443);
+
+    let addrs: Vec<SocketAddr> = tokio::net::lookup_host(format!("{}:{}", host, port))


validate_and_resolve_url builds the lookup target with format!("{}:{}", host, port). This breaks for IPv6 literals because host_str() returns e.g. 2001:db8::1 and the resulting string (2001:db8::1:443) is not a valid socket address. It also forces a DNS lookup path even when the host is already an IP literal.

Consider using tokio::net::lookup_host((host, port)) (as done elsewhere in the repo) and/or short-circuiting IP literals by returning SocketAddr::new(ip, port) after blocklist validation.

Suggested change

let addrs: Vec<SocketAddr> = tokio::net::lookup_host(format!("{}:{}", host, port))

// If the host is already an IP literal, validate it and short-circuit DNS.

if let Ok(ip) = host.parse::<IpAddr>() {

if is_disallowed_ip(&ip) {

return Err(ToolError::NotAuthorized(format!(

"hostname '{}' resolves to disallowed IP {}",

host, ip

)));

}

let addr = SocketAddr::new(ip, port);

return Ok(vec![addr]);

}

let addrs: Vec<SocketAddr> = tokio::net::lookup_host((host, port))

Copilot · 2026-03-04T03:25:22Z

+    for addr in resolved_addrs {
+        builder = builder.resolve(host, *addr);
+    }


build_pinned_client loops over resolved_addrs and calls ClientBuilder::resolve repeatedly for the same hostname. Reqwest also provides resolve_to_addrs, which avoids any ambiguity about whether repeated resolve calls append vs overwrite, and matches existing code patterns in the repo.

Suggested change

for addr in resolved_addrs {

builder = builder.resolve(host, *addr);

}

builder = builder.resolve_to_addrs(host, resolved_addrs.iter().copied());

Copilot · 2026-03-04T03:25:22Z

+    #[tokio::test]
+    async fn test_validate_and_resolve_accepts_public_host() {
+        // example.com resolves to public IPs.
+        let url = reqwest::Url::parse("https://example.com").unwrap();
+        let addrs = validate_and_resolve_url(&url).await.unwrap();
+        assert!(!addrs.is_empty(), "should resolve to at least one address");
+        for addr in &addrs {
+            assert!(
+                !is_disallowed_ip(&addr.ip()),
+                "example.com resolved to disallowed IP: {}",
+                addr.ip()
+            );
+        }
+    }


test_validate_and_resolve_accepts_public_host depends on real DNS resolution for example.com, which can be flaky or fail in sandboxed/offline CI environments. Consider making this test deterministic by using a public IP literal (no DNS), or by injecting/mocking the resolver for tests.

zmanian

Review

The approach is architecturally correct — resolving DNS once, validating all IPs, and pinning via reqwest::ClientBuilder::resolve() is the right way to eliminate the TOCTOU window. The three-phase split (validate_url → validate_and_resolve_url → build_pinned_client) is clean. Redirect handling in WebFetchTool correctly builds a fresh pinned client per hop.

Required before merge

1. resolve() loop bug drops all but last address (Critical)

for addr in resolved_addrs {
    builder = builder.resolve(host, *addr);
}

ClientBuilder::resolve() calls resolve_to_addrs(domain, &[addr]) which does HashMap::insert — each iteration overwrites the previous entry. Only the last address survives. For multi-A-record hosts (most major services), this silently discards validated addresses.

Fix — single call:

builder = builder.resolve_to_addrs(host, resolved_addrs);

2. Replace unwrap_or_default() on host_str() (Medium)

After validate_url() succeeds, host_str() is guaranteed non-None. But unwrap_or_default() returns empty string on violation, which causes build_pinned_client to pin an empty key — DNS override silently ignored, reqwest falls back to its own resolution. Use proper error propagation:

.ok_or_else(|| ToolError::InvalidParameters("URL missing host".into()))?

…6, host_str Merge upstream/main (which unified http + web_fetch into a single tool) and fix all critical review findings: 1. resolve() loop drops all but last address: replaced per-address `builder.resolve(host, *addr)` (HashMap::insert semantics) with `builder.resolve_to_addrs(host, resolved_addrs)` which pins all resolved addresses at once. 2. unwrap_or_default on host_str(): after validate_url() succeeds host_str() is guaranteed non-None, but we now use proper error propagation via ok_or_else instead of silently defaulting. 3. IPv4-mapped IPv6 bypass: is_disallowed_ip() now extracts the inner v4 address from IPv4-mapped IPv6 (::ffff:x.x.x.x) and checks it against the IPv4 blocklist, closing the bypass vector. 4. Live DNS test marked #[ignore]: test_validate_and_resolve_accepts_public_host requires network access and is now opt-in via --ignored. 5. Simple GET redirect loop: now builds a per-hop pinned client with DNS resolution and SSRF validation on every redirect, matching the web_fetch approach that was merged into the unified http tool. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

gabehamilton

Review feedback addressed

All critical findings from the review have been fixed in commit 80bde5d:

1. resolve() loop drops all but last address

Before: Loop called builder.resolve(host, *addr) for each address — HashMap::insert semantics meant only the last address survived.
After: Single call to builder.resolve_to_addrs(host, resolved_addrs) pins all resolved addresses at once.

2. unwrap_or_default on host_str()

Before: url.host_str().unwrap_or_default() silently returned empty string on missing host.
After: .ok_or_else(|| ToolError::InvalidParameters("URL missing host".into()))? — proper error propagation.

3. IPv4-mapped IPv6 bypass in is_disallowed_ip

Before: ::ffff:169.254.169.254 (IPv4-mapped IPv6 cloud metadata) passed the IPv6 checks because it is not loopback/link-local/multicast.
After: Extracted inner v4 via v6.to_ipv4_mapped() and checked against is_disallowed_ipv4(). New test test_is_disallowed_ip_catches_ipv4_mapped_ipv6 covers loopback, metadata, private, and public mapped addresses.

4. Live DNS test marked #[ignore]

test_validate_and_resolve_accepts_public_host (resolves example.com) now has #[ignore] with comment: "Requires network access — run with: cargo test -- --ignored"

5. Merge conflict with upstream/main (PR #578)

Upstream merged web_fetch.rs into the unified http.rs tool. Resolved by:

Accepting web_fetch.rs deletion
Adapting DNS pinning into the unified tool's simple-GET redirect loop (per-hop pinned clients)
Adding USER_AGENT constant from upstream

Verification

cargo clippy --all --benches --tests --examples --all-features — zero warnings
cargo test — 1996 passed, 0 failed, 2 ignored
No .unwrap() or .expect() in production code

Preserves DNS TOCTOU fix (validate_and_resolve_url, build_pinned_client, resolve_to_addrs, IPv4-mapped IPv6 protection, per-hop pinned clients) while incorporating upstream changes: - save_to file download support with path traversal validation - MAX_SAVE_TO_SIZE constant for larger file download limits - requires_approval multi-thread safety test - redirect policy SSRF checks (subsumed by our manual redirect loop) [skip-regression-check] Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-09T17:42:15Z

+    let port = url.port_or_known_default().unwrap_or(443);
+
+    let addrs: Vec<SocketAddr> = tokio::net::lookup_host(format!("{}:{}", host, port))
+        .await
+        .map_err(|e| {
+            ToolError::ExternalService(format!("DNS resolution failed for '{}': {}", host, e))
+        })?


validate_and_resolve_url builds the lookup target with format!("{}:{}", host, port). This breaks for IPv6 literals because the host contains : and needs brackets (e.g. [::1]:443), so DNS resolution will fail for otherwise-valid https://[v6]/... URLs. Consider calling tokio::net::lookup_host((host, port)) (tuple form) or otherwise ensuring IPv6 literals are correctly formatted before resolution.

Copilot · 2026-03-09T17:42:15Z

 /// Tool for making HTTP requests.
+///
+/// Each request builds a per-request [`Client`] with DNS pinning to prevent
+/// TOCTOU DNS rebinding attacks.  The hostname is resolved once, validated
+/// against the SSRF blocklist, and then pinned via
+/// [`reqwest::ClientBuilder::resolve_to_addrs`] so that reqwest connects
+/// directly to the pre-validated IPs without a second DNS lookup.


The PR description says SSRF DNS pinning was updated for both http and web_fetch, but this repository snapshot doesn't contain src/tools/builtin/web_fetch.rs (and there are no other web_fetch tool implementation files found). Either the description needs updating, or the intended web_fetch changes are missing from this PR.

Copilot · 2026-03-09T17:42:16Z

+        // Resolve DNS once, validate against SSRF blocklist, then pin the
+        // resolved addresses into the reqwest client so it cannot re-resolve
+        // to a different (potentially private) IP.
+        let resolved_addrs = validate_and_resolve_url(&parsed_url).await?;
+        let host = parsed_url
+            .host_str()
+            .ok_or_else(|| ToolError::InvalidParameters("URL missing host".into()))?
+            .to_string();
+        let client = build_pinned_client(
+            &host,
+            &resolved_addrs,
+            Duration::from_secs(30),
+            reqwest::redirect::Policy::none(),
+        )?;


In execute, DNS is resolved and a pinned Client is built unconditionally, but the is_simple_get redirect-following path does its own per-hop validate_and_resolve_url + build_pinned_client and never uses this initial client. This causes an extra DNS lookup and client build on the most common GET path; consider deferring the initial resolution/client creation until after is_simple_get is determined, or sharing the first hop’s resolution/client with the redirect loop.

Copilot · 2026-03-09T17:42:16Z

+                let status = resp.status().as_u16();
+                if (300..400).contains(&status) {
+                    if redirects_remaining == 0 {
+                        return Err(ToolError::ExecutionFailed(format!(
+                            "too many redirects (max {})",
+                            MAX_REDIRECTS
+                        )));
+                    }
+
+                    let location = resp
+                        .headers()
+                        .get(reqwest::header::LOCATION)
+                        .and_then(|v| v.to_str().ok())
+                        .ok_or_else(|| {
+                            ToolError::ExecutionFailed(format!(
+                                "redirect (HTTP {}) has no Location header",
+                                status
+                            ))
+                        })?;


Redirect detection currently treats any HTTP 3xx as a redirect ((300..400).contains(&status)) and then requires a Location header. This will incorrectly fail on non-redirect 3xx responses like 304 Not Modified (typically no Location). Consider restricting to the redirect status codes that actually require Location (301/302/303/307/308), or otherwise handling 304/300/etc. explicitly.

Copilot · 2026-03-09T17:42:16Z

+                    // SSRF re-validation on every hop (URL structure checks).
+                    // DNS resolution + IP validation happens at the top of the
+                    // next loop iteration via validate_and_resolve_url.
+                    parsed_url = validate_url(&next_url_str)?;
+                    let hop_detector = LeakDetector::new();


In the redirect-following branch, parsed_url is mutated to the next hop (parsed_url = validate_url(&next_url_str)?;). However the HTTP interceptor request descriptor used for recording/replay is built earlier (before redirects) and isn’t updated, so recordings can end up attributing the final response to the original URL. Consider updating/rebuilding the interceptor request metadata once the final URL is known (or recording the redirect chain explicitly).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zmanian

Re-review: fix(security): resolve DNS once for SSRF validation

Verdict: APPROVE

All four items from the original review have been correctly addressed:

resolve() loop bug -- Now uses tokio::net::lookup_host() collecting all addresses into a Vec, validating every one. No more silent address dropping.
host_str() error propagation -- All call sites use .ok_or_else() with proper error messages instead of unwrap_or_default().
IPv4-mapped IPv6 bypass -- is_disallowed_ip() now checks v6.to_ipv4_mapped() and validates against is_disallowed_ipv4(). Tests cover ::ffff:127.0.0.1, ::ffff:169.254.169.254, ::ffff:10.0.0.1.
Network-dependent test -- test_validate_and_resolve_accepts_public_host properly marked #[ignore].

DNS pinning via resolve_to_addrs() and per-redirect-hop revalidation are solid. Ready to merge.

…ent rebinding (nearai#518) * fix(security): resolve DNS once and reuse for SSRF validation to prevent rebinding The previous SSRF protection resolved DNS in validate_url() to check IPs against a blocklist, but then reqwest independently re-resolved DNS when making the actual HTTP connection. Between validation and connection, a DNS rebinding attack could flip the record from a public IP (passes validation) to a private IP like 169.254.169.254 (AWS metadata endpoint). Fix: split URL validation into two phases: - validate_url(): synchronous URL structure checks (scheme, localhost, IP literals) -- no DNS resolution - validate_and_resolve_url(): async DNS resolution via tokio::net::lookup_host, validates all resolved IPs, returns SocketAddrs - build_pinned_client(): constructs a per-request reqwest Client with resolve() pinning so reqwest connects to the pre-validated IPs without a second DNS lookup Applied to both HttpTool and WebFetchTool. WebFetchTool builds a fresh pinned client per redirect hop, ensuring DNS rebinding cannot occur at any point in a redirect chain. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * style: run cargo fmt Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings March 4, 2026 03:21

github-actions Bot added scope: tool/builtin Built-in tools size: L 200-499 changed lines risk: medium Business logic, config, or moderate-risk modules contributor: new First-time contributor labels Mar 4, 2026

Copilot started reviewing on behalf of gabehamilton March 4, 2026 03:22

View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

zmanian requested changes Mar 4, 2026

View reviewed changes

github-actions Bot added contributor: regular 2-5 merged PRs and removed contributor: new First-time contributor labels Mar 6, 2026

gabehamilton commented Mar 6, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings March 9, 2026 17:37

Copilot started reviewing on behalf of gabehamilton March 9, 2026 17:38

View session

Copilot AI reviewed Mar 9, 2026

View reviewed changes

style: run cargo fmt

d2dc3e6

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

henrypark133 changed the base branch from main to staging March 10, 2026 02:31

zmanian approved these changes Mar 11, 2026

View reviewed changes

zmanian merged commit bb06565 into nearai:staging Mar 11, 2026
22 checks passed

gabehamilton deleted the fix/m6-dns-toctou-ssrf branch March 12, 2026 18:11

henrypark133 mentioned this pull request Mar 12, 2026

chore: promote staging to main (2026-03-12 11:12 UTC) #1032

Merged

ironclaw-ci Bot mentioned this pull request Mar 12, 2026

chore: release v0.19.0 #973

Merged

theredspoon mentioned this pull request Aug 2, 2026

[QA] Ambient proxy env vars bypass DNS-rebinding protection in ReqwestNetworkTransport #7016

Open

3 tasks done


		let port = url.port_or_known_default().unwrap_or(443);

		let addrs: Vec<SocketAddr> = tokio::net::lookup_host(format!("{}:{}", host, port))

-    let addrs: Vec<SocketAddr> = tokio::net::lookup_host(format!("{}:{}", host, port))
+    // If the host is already an IP literal, validate it and short-circuit DNS.
+    if let Ok(ip) = host.parse::<IpAddr>() {
+        if is_disallowed_ip(&ip) {
+            return Err(ToolError::NotAuthorized(format!(
+                "hostname '{}' resolves to disallowed IP {}",
+                host, ip
+            )));
+        }
+        let addr = SocketAddr::new(ip, port);
+        return Ok(vec![addr]);
+    }
+    let addrs: Vec<SocketAddr> = tokio::net::lookup_host((host, port))

Conversation

gabehamilton commented Mar 4, 2026

Summary

Problem

Solution

Files changed

Test plan

Uh oh!

gemini-code-assist Bot commented Mar 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

zmanian left a comment

Choose a reason for hiding this comment

Review

Required before merge

Recommended

Uh oh!

gabehamilton left a comment

Choose a reason for hiding this comment

Review feedback addressed

1. resolve() loop drops all but last address

2. unwrap_or_default on host_str()

3. IPv4-mapped IPv6 bypass in is_disallowed_ip

4. Live DNS test marked #[ignore]

5. Merge conflict with upstream/main (PR #578)

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

zmanian left a comment

Choose a reason for hiding this comment

Re-review: fix(security): resolve DNS once for SSRF validation

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants