Fix DNS rebinding TOCTOU bypass in validate_restricted_url#21591
Fix DNS rebinding TOCTOU bypass in validate_restricted_url#21591devin-ai-integration[bot] wants to merge 2 commits intomainfrom
validate_restricted_url#21591Conversation
Prior SSRF protection resolved a hostname with socket.gethostbyname(), checked privateness of the returned IP, then passed the hostname to httpx which re-resolved DNS at connection time. An attacker-controlled DNS server could return a public IP for the validation lookup and a private IP for the actual connection, bypassing the check. - Hardens validate_restricted_url() to use socket.getaddrinfo() so every resolved address is validated (defends against mixed public/private A/AAAA responses). - Adds SSRFProtectedAsyncHTTPTransport and SSRFProtectedHTTPTransport, httpx transports that wrap the httpcore network backend; on each connect they resolve the hostname, reject private addresses, and pass the validated IP literal to the underlying backend so it cannot re-resolve DNS. - Uses the protected transports in Webhook and CustomWebhookNotificationBlock when allow_private_urls=False. Refs OSS-7874. Co-authored-by: Alexander Streed <alex.s@prefect.io>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
desertaxle
changed the title
validate_restricted_url
desertaxle
requested review from
chrisguidry,
cicdw,
desertaxle and
zzstoatzz
as code owners
![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: f2bad07afa
ℹ️ 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".
| resolved = _validate_resolved_hostname(host) | ||
| except _RestrictedHostError as exc: | ||
| raise httpcore.ConnectError(f"Refusing to connect to {host!r}: {exc}") from None | ||
| return resolved[0] |
There was a problem hiding this comment.
_resolve_and_validate_for_connect returns only resolved[0], so the protected transports attempt a single IP and give up even when later DNS answers are valid and reachable. This is a regression from the previous hostname-based connect path (which could fall back across A/AAAA answers), and it will cause false connection failures for dual-stack domains in single-stack environments (for example, first AAAA unreachable but A reachable). Consider retrying through all validated addresses instead of pinning only the first one.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in a080abd. _resolve_and_validate_for_connect now returns every validated IP, and the async/sync backends iterate them in order, retrying on ConnectError / ConnectTimeout / OSError and re-raising the last error only if all addresses fail. Added tests for the fallback-on-first-failure and all-fail-raises-last-error paths.
Previously the protected backends pinned a single resolved IP, which broke dual-stack hostnames in single-stack environments: if the first resolved address was unreachable (e.g. AAAA in an IPv4-only network) the connection would fail even when a later A/AAAA record was reachable. Now iterate every validated IP, retry on ConnectError / ConnectTimeout / OSError, and re-raise the last error only if all addresses fail. Addresses the Codex P1 review comment on #21591. Co-authored-by: Alexander Streed <alex.s@prefect.io> Co-Authored-By: alex.s <ajstreed1@gmail.com>
1 participant
Closes OSS-7874.
validate_restricted_url()previously resolved a hostname once viasocket.gethostbyname(), checked if the returned IP was private, then passed the original hostname to httpx. httpx re-resolves DNS at connection time, creating a TOCTOU window: an attacker-controlled DNS server can return a public IP during validation and a private IP during the actual connection, bypassing the SSRF check whenallow_private_urls=False.gethostbynamealso only considers the first A record, so a DNS response mixing public and private records could already bypass validation.This PR closes both windows:
validate_restricted_urlto usesocket.getaddrinfoso every resolved address is checked (defends against mixed public/private A/AAAA records).SSRFProtectedAsyncHTTPTransport/SSRFProtectedHTTPTransport— httpx transports that wrap the httpcore network backend. On each TCP connect they resolve the hostname, reject private addresses, and pass the validated IP literal to the underlying backend so it cannot re-resolve DNS. TLS SNI is preserved because httpcore passes the original hostname tostart_tlsindependently of the host used for the TCP connection.WebhookandCustomWebhookNotificationBlocknow use the protected transports wheneverallow_private_urls=False, in addition to the pre-flightvalidate_restricted_urlcheck.Verification
tests/utilities/test_urls.pyadds:TestValidateRestrictedUrlMultiRecord— mixed public/private addresses fromgetaddrinfoare now rejected.TestSSRFProtectedTransportTOCTOU— the transport rejects private IPs at connect time (simulates DNS rebinding), including the multi-record case, unresolvable hosts, and private IP literals; both async and sync transports are covered.TestSSRFProtectedBackendPinsIP— confirms the wrapper backend connects to the validated IP, not the original hostname, so the underlying backend cannot re-resolve DNS.tests/blocks/test_webhook.pyandtests/blocks/test_notifications.pycontinue to pass.uv run ruff checkanduv run ruff format --checkpass.Checklist
<link to issue>"mint.json.Link to Devin session: https://app.devin.ai/sessions/0b65f4db71f04387be5945d92cce8ed5
Requested by: @desertaxle