Skip to content

fix(llm-access): block DNS-rebinding SSRF in kiro remote-media fetch#21

Merged
acking-you merged 2 commits into
masterfrom
fix/kiro-remote-media-ssrf
Jun 1, 2026
Merged

fix(llm-access): block DNS-rebinding SSRF in kiro remote-media fetch#21
acking-you merged 2 commits into
masterfrom
fix/kiro-remote-media-ssrf

Conversation

@acking-you
Copy link
Copy Markdown
Owner

What

Fixes the TOCTOU DNS-rebinding SSRF in the Kiro remote-media fetcher, confirmed during PR #20's review (gemini). Dedicated follow-up, same pattern as the duckdb write-race (#18) found during #16's review.

The bug

ReqwestKiroRemoteMediaFetcher::fetch validated the host with its own tokio::net::lookup_host(...) and rejected private/local IPs — then self.client.get(url).send() let reqwest perform an independent DNS resolution at connect time. A hostile authoritative DNS server answers the pre-flight check with a public IP and the connect-time resolution with 127.0.0.1 / 169.254.169.254 (cloud metadata) / RFC1918 → the private-IP guard is bypassed.

Reachable when a key has kiro_remote_media_resolution_enabled set, since the source URLs come straight from the request payload.

The fix

A custom reqwest::dns::ResolvePrivateFilteringDnsResolver — installed on KIRO_REMOTE_MEDIA_CLIENT via ClientBuilder::dns_resolver. It resolves the host and filters out private/local addresses, returning only the public ones (erroring if none remain). Because reqwest dials exactly the addresses the resolver returns, the IP that is vetted is the IP that is connected to — closing the check/use gap that a separate pre-flight lookup fundamentally cannot.

  • IP-literal URLs are never sent to a resolver, so they stay covered by the existing literal-IP gate in validate_kiro_remote_media_url.
  • The strict pre-flight validate_kiro_remote_media_resolved_addresses is kept for early, clear errors (defense in depth) — the resolver is now the actual security boundary.
  • Only KIRO_REMOTE_MEDIA_CLIENT gets the filtering resolver (it's the only client fetching user-supplied URLs); the codex/kiro upstream clients dial fixed trusted endpoints.

Tests

  • filter_public_kiro_remote_media_addrs: keeps public / drops private / rejects all-private / rejects empty.
  • resolver_rejects_loopback_hostname: localhost (RFC 6761 loopback) is rejected end-to-end through the resolver.

Verification

  • cargo clippy -p llm-access --all-targets -- -D warnings → clean
  • cargo test -p llm-access provider:: → 115 passed (111 prior + 4 new)
  • rustfmt on the 2 changed files only; deps/lance/deps/lancedb untouched

🤖 Generated with Claude Code

The kiro remote-media fetcher validated resolved IPs with its own
`tokio::net::lookup_host` (rejecting private/local addresses), then let
reqwest perform an INDEPENDENT DNS resolution at connect time. A hostile
authoritative DNS server could answer the pre-flight check with a public IP
and the connect-time resolution with 127.0.0.1 / 169.254.169.254 / RFC1918,
bypassing the guard — a TOCTOU DNS-rebinding SSRF (reachable when a key has
kiro_remote_media_resolution_enabled, since the source URLs come from the
request payload). Found in PR #20 review (gemini).

Fix: a custom `reqwest::dns::Resolve` (PrivateFilteringDnsResolver) installed
on KIRO_REMOTE_MEDIA_CLIENT via `dns_resolver`. It resolves the host and
filters out private/local addresses, returning only public ones (erroring if
none remain). Because reqwest dials exactly the addresses the resolver
returns, the IP that is vetted IS the IP connected to — closing the
check/use gap. IP-literal URLs (never sent to a resolver) remain covered by
the existing literal-IP gate in validate_kiro_remote_media_url; the strict
pre-flight check is kept for early, clear errors (defense in depth).

Tests: pure `filter_public_kiro_remote_media_addrs` cases (keeps public /
drops private / rejects all-private / rejects empty) + a resolver test that
`localhost` (RFC 6761 loopback) is rejected end-to-end.

Verification:
- cargo clippy -p llm-access --all-targets -- -D warnings -> clean
- cargo test -p llm-access provider:: -> 115 passed (111 prior + 4 new)
- rustfmt on the 2 changed files only; deps/lance, deps/lancedb untouched

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a custom DNS resolver, PrivateFilteringDnsResolver, for the reqwest::Client used in kiro_media. This resolver filters out private and local IP addresses during DNS resolution to prevent DNS rebinding and Server-Side Request Forgery (SSRF) attacks. It also includes unit tests to verify that public addresses are kept, private addresses are rejected, and loopback hostnames like localhost are correctly blocked. All review comments have been filtered out as they merely explain or validate the implemented changes, so there is no additional feedback to provide.

@acking-you
Copy link
Copy Markdown
Owner Author

/gemini review

@acking-you
Copy link
Copy Markdown
Owner Author

@codex review

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a custom DNS resolver, PrivateFilteringDnsResolver, to mitigate SSRF DNS rebinding attacks by filtering out private and local IP addresses during host resolution for the remote media client. While this is a solid security improvement, the current filtering logic is vulnerable to bypasses via IPv4-mapped IPv6 addresses (such as ::ffff:127.0.0.1). The feedback recommends extracting and validating the mapped IPv4 address when handling IPv6 addresses and adding corresponding unit tests to ensure these bypasses are correctly blocked.

Comment on lines +538 to +541
let public = addrs
.into_iter()
.filter(|addr| !is_private_kiro_remote_media_ip(addr.ip()))
.collect::<Vec<_>>();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

Security Vulnerability: SSRF Bypass via IPv4-Mapped IPv6 Addresses

The current implementation of filter_public_kiro_remote_media_addrs filters addresses using is_private_kiro_remote_media_ip(addr.ip()). However, if a hostname resolves to an IPv4-mapped IPv6 address (e.g., ::ffff:127.0.0.1 or ::ffff:10.1.2.3), is_private_kiro_remote_media_ipv6 will not recognize it as a private or loopback address because it only checks standard IPv6 loopback (::1) and unique local/link-local prefixes.

Since dual-stack sockets automatically translate IPv4-mapped IPv6 addresses to IPv4 connections at the OS layer, an attacker can bypass the SSRF protection entirely by returning IPv4-mapped IPv6 addresses pointing to private IPv4 targets.

Recommendation

Extract the mapped IPv4 address using .to_ipv4() if the address is IPv6, and validate it against is_private_kiro_remote_media_ipv4.

Additionally, to prevent DNS Rebinding SSRF (TOCTOU vulnerabilities), ensure this filtering is performed within a custom connection-level resolver on the HTTP client (or by resolving once and pinning the IP directly) rather than performing DNS resolution once for validation and a second time for the actual HTTP request.

    let public = addrs
        .into_iter()
        .filter(|addr| {
            let ip = addr.ip();
            if is_private_kiro_remote_media_ip(ip) {
                return false;
            }
            if let IpAddr::V6(ipv6) = ip {
                if let Some(ipv4) = ipv6.to_ipv4() {
                    if is_private_kiro_remote_media_ipv4(ipv4) {
                        return false;
                    }
                }
            }
            true
        })
        .collect::<Vec<_>>();
References
  1. To prevent DNS Rebinding SSRF (TOCTOU vulnerabilities) when validating and fetching external URLs, do not perform DNS resolution once for validation and a second time for the actual HTTP request. Instead, configure a custom connection-level resolver on the HTTP client (e.g., reqwest::Client) to validate and filter out private/local IP addresses at socket connection time, or resolve the IP once and request the pinned IP directly while preserving the Host header and SNI for TLS.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e182cc4. is_private_kiro_remote_media_ipv6 now re-checks the embedded IPv4 via Ipv6Addr::to_ipv4() against the v4 private rules, so ::ffff:127.0.0.1 / ::ffff:10.x / ::ffff:169.254.169.254 are rejected by both the literal gate (validate_kiro_remote_media_url) and the resolving filter (filter_public_kiro_remote_media_addrs). Tests: filter_public_addrs_rejects_ipv4_mapped_private + filter_public_addrs_keeps_ipv4_mapped_public.

Comment on lines +597 to +606
#[test]
fn filter_public_addrs_rejects_all_private() {
let err = filter_public_kiro_remote_media_addrs("rebind.test", vec![
sa("127.0.0.1"),
sa("169.254.169.254"),
sa("10.1.2.3"),
])
.expect_err("an all-private resolution must be rejected");
assert!(err.contains("private or local"), "unexpected message: {err}");
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Add a unit test to verify that IPv4-mapped IPv6 addresses pointing to private IPv4 addresses are correctly rejected by the filtering resolver.

    #[test]
    fn filter_public_addrs_rejects_all_private() {
        let err = filter_public_kiro_remote_media_addrs("rebind.test", vec![
            sa("127.0.0.1"),
            sa("169.254.169.254"),
            sa("10.1.2.3"),
        ])
        .expect_err("an all-private resolution must be rejected");
        assert!(err.contains("private or local"), "unexpected message: {err}");
    }

    #[test]
    fn filter_public_addrs_rejects_ipv4_mapped_ipv6() {
        let err = filter_public_kiro_remote_media_addrs("rebind.test", vec![
            sa("[::ffff:127.0.0.1]"),
            sa("[::ffff:10.1.2.3]"),
        ])
        .expect_err("IPv4-mapped private addresses must be rejected");
        assert!(err.contains("private or local"), "unexpected message: {err}");
    }

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in e182cc4. filter_public_addrs_rejects_ipv4_mapped_private rejects ::ffff:127.0.0.1, ::ffff:10.1.2.3, ::ffff:169.254.169.254; filter_public_addrs_keeps_ipv4_mapped_public confirms ::ffff:8.8.8.8 survives while ::ffff:10.0.0.1 is dropped. Also added validate_url_rejects_literal_loopback_variants (covers [::1], [::ffff:127.0.0.1], 127.0.0.1, decimal-form 2130706433) and validate_url_allows_public_literal_ipv6.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1d4a6b304a

ℹ️ 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".

reqwest::Client::builder()
.timeout(KIRO_REMOTE_MEDIA_TIMEOUT)
.redirect(reqwest::redirect::Policy::none())
.dns_resolver(Arc::new(kiro_media::PrivateFilteringDnsResolver))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Disable inherited proxies for the guarded fetcher

When llm-access runs with HTTP_PROXY/HTTPS_PROXY set, reqwest will still use the automatic system proxy here because this builder never calls no_proxy(). In that configuration this resolver is applied to the proxy host, not the user-supplied media host, so an internal proxy hostname can be rejected outright; and if the proxy is usable, the target hostname is resolved by the proxy/CONNECT path while the old preflight lookup remains the only SSRF check. For the remote-media security boundary to hold, this client should force direct connections (or otherwise validate the exact target address the proxy will dial).

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentionally declining the forced no_proxy(). If an operator sets HTTP(S)_PROXY the fetcher must honor it — forcing direct connections would break egress-proxy deployments. And in CONNECT mode the client never learns the IP the proxy dials, so connect-time target pinning is fundamentally impossible there: the proxy is the egress boundary by design. Protections that still hold in proxy mode: the static literal-IP gate in validate_kiro_remote_media_url (rejects literal private/loopback targets with no resolution at all) plus the preflight resolution check. The DNS-rebinding resolver fix targets the default direct-connection path, which is this service's actual egress mode.

) -> Result<Vec<SocketAddr>, String> {
let public = addrs
.into_iter()
.filter(|addr| !is_private_kiro_remote_media_ip(addr.ip()))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reject IPv4-mapped private IPv6 addresses

A hostile DNS server can still pass the new resolver by returning an IPv4-mapped IPv6 address such as ::ffff:127.0.0.1 or ::ffff:169.254.169.254 on the connect-time lookup. This filter treats those as ordinary IPv6 addresses, but Rust does not classify ::ffff:127.0.0.1 itself as loopback unless it is first converted to its mapped IPv4 address, so the resolver can hand reqwest a private endpoint after the preflight lookup answered with a public IP. Convert IPv4-mapped/compatible IPv6 addresses to IPv4 before applying the private/local checks.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e182cc4 — the embedded IPv4 of mapped/compatible IPv6 (::ffff:a.b.c.d / ::a.b.c.d) is now re-checked against the v4 private rules in is_private_kiro_remote_media_ipv6 (Ipv6Addr::to_ipv4()), so a hostile resolver returning ::ffff:127.0.0.1 is dropped. Covered by filter_public_addrs_rejects_ipv4_mapped_private.

CR on PR #21 surfaced two residual bypasses of the kiro remote-media
private-address gate:

- IPv4-mapped (`::ffff:a.b.c.d`) / IPv4-compatible (`::a.b.c.d`) IPv6
  addresses are dialed as their embedded IPv4 by dual-stack sockets, so
  e.g. `::ffff:127.0.0.1` slipped past the native-IPv6 checks. Re-check
  the embedded IPv4 (via `Ipv6Addr::to_ipv4`) against the v4 rules in
  both the literal gate and the resolving filter.
- `url.host_str()` brackets IPv6 literals (`[::1]`), so the
  `parse::<IpAddr>()` literal gate never matched them and a literal-IP
  URL skipped the private check entirely (reqwest dials the literal
  without ever consulting the filtering resolver). Match the parsed
  `url.host()` enum (Domain/Ipv4/Ipv6) instead, in both the pre-flight
  validate and the resolve-addresses pre-check.

Tests: IPv4-mapped private rejection + mapped-public retention; literal
loopback / mapped-private / url-normalized decimal-IPv4 URL rejection;
public literal IPv6 allowed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@acking-you acking-you merged commit a4766dc into master Jun 1, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant