Fix/aiohttp ssrf cwe918 - #28252
Conversation
…p from BerriAI#26264 (CWE-918) aiohttp_handler.py was not covered by the SSRF protection in PR BerriAI#26264. User-controlled api_base was passed directly to session.post() without IP validation. Protection added: - Blocks RFC-1918, loopback, link-local (169.254/16, fe80::/10), CGNAT, 0.0.0.0/8, IPv6 ULA/loopback - Unwraps IPv4-mapped IPv6 (::ffff:x.x.x.x) before network check - Validates ALL getaddrinfo answers to prevent A-record rotation bypass - _SSRFGuardResolver (AbstractResolver) validates IPs at TCP-connection time inside aiohttp's own connection loop — covers redirect targets and eliminates DNS-rebinding TOCTOU - Default ClientSession creation uses TCPConnector(resolver=_SSRFGuardResolver()) - Sync path (_make_common_sync_call via httpx) guarded with preflight check Tests: - 18 new tests in tests/test_litellm/llms/test_aiohttp_ssrf_protection.py - Updated 4 existing tests in test_aiohttp_handler.py to mock TCPConnector Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Unparseable IP in getaddrinfo answer (lines 79-80, 111-112) - DNS failure in _SSRFGuardResolver.resolve() (lines 105-106) - _SSRFGuardResolver.close() noop (line 131) - _make_common_async_call and _make_common_sync_call block private api_base (lines 297, 343) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Greptile SummaryThis PR closes the SSRF gap in
Confidence Score: 5/5Safe to merge; the SSRF protection is layered and correct, and all previously flagged issues have been addressed. The async path only performs a fast, non-blocking IP-literal check as a preflight (no synchronous DNS), while connect-time validation is delegated to _SSRFGuardResolver. The sync path uses _SSRFGuardTransport to pin the resolved IP so httpcore never performs a second DNS lookup. SSL configuration and connection-pool caching are forwarded correctly. The opt-out flag defaults to False, preserving the secure default. The new test suite mocks all DNS and HTTP transport calls, and the four modified existing tests are strengthened rather than weakened. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/llms/custom_httpx/aiohttp_handler.py | Adds multi-layer SSRF protection: _SSRFGuardResolver for aiohttp's async path (DNS-rebinding TOCTOU closed at TCP-connect time), _SSRFGuardTransport with IP-pinning for the sync path, _assert_not_private_ip_literal preflight for IP-literal bypass on the async path, and a trace hook for IP-literal redirect targets. Previous feedback on asyncio deprecation, SSL forwarding, and connection-pool caching is incorporated. |
| litellm/init.py | Adds allow_requests_to_internal_ips = False opt-out flag so self-hosted deployments pointing api_base at an internal address can restore previous behaviour without a code change. |
| litellm/llms/custom_httpx/http_handler.py | Adds optional transport= parameter to HTTPHandler.init so _SSRFGuardTransport can be injected; falls back to the existing _create_sync_transport() when not supplied. |
| tests/test_litellm/llms/custom_httpx/test_aiohttp_handler.py | Updated four existing tests to patch TCPConnector alongside ClientSession; test_create_client_session_default now verifies _SSRFGuardResolver is passed to the connector, strengthening the assertion without weakening coverage. |
| tests/test_litellm/llms/test_aiohttp_ssrf_protection.py | 22 new unit tests covering every SSRF guard component; all hostname-based DNS calls are mocked, IP-literal tests involve no network I/O, and the end-to-end sync test mocks both getaddrinfo and httpx.HTTPTransport.handle_request. |
Reviews (15): Last reviewed commit: "fix(aiohttp/ssrf): restore connection-po..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR closes an SSRF gap in
Confidence Score: 3/5The async resolver has two defects that weaken the protection this PR introduces; fix them before merging. The core security mechanism uses asyncio.get_event_loop() inside an async method (deprecated, can raise RuntimeError in Python 3.12+) and swallows socket.gaierror by returning [] instead of re-raising, deviating from the aiohttp resolver contract and potentially leaving the async path unprotected on DNS failures. litellm/llms/custom_httpx/aiohttp_handler.py — specifically _SSRFGuardResolver.resolve() around event-loop acquisition and DNS-failure handling
|
| Filename | Overview |
|---|---|
| litellm/llms/custom_httpx/aiohttp_handler.py | Adds SSRF protection via _BLOCKED_NETWORKS, _assert_not_private_url preflight, and _SSRFGuardResolver; two issues: get_event_loop() should be get_running_loop(), and DNS failure should re-raise instead of returning [] |
| tests/test_litellm/llms/custom_httpx/test_aiohttp_handler.py | Updates existing tests to mock TCPConnector alongside ClientSession; correctly reflects the new default session creation path |
| tests/test_litellm/llms/test_aiohttp_ssrf_protection.py | 22 new tests covering blocked addresses, preflight URL checks, and the SSRFGuardResolver; test helper uses deprecated asyncio.get_event_loop().run_until_complete() pattern |
Reviews (2): Last reviewed commit: "test(aiohttp): add coverage for all edge..." | Re-trigger Greptile
PR overviewThis pull request updates LiteLLM’s custom aiohttp/HTTP client handling around outbound API base URL validation and request routing. The changes focus on tightening how configured API endpoints are checked before requests are made. Most of the previously identified concerns have been addressed, with 4 issues already fixed. One significant gap remains: redirects are still followed without validating the redirected destination, which can allow a caller-controlled endpoint to bounce requests to internal or metadata services. Until redirect targets are validated or automatic redirects are disabled, the PR still carries meaningful SSRF exposure. Open issues (1)
Fixed/addressed: 4 · PR risk: 7/10 |
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| dynamic_client_session=async_client_session | ||
| ) | ||
|
|
||
| _assert_not_private_url(api_base) |
There was a problem hiding this comment.
High: Redirect target is not validated
This validates only the original api_base; both the aiohttp request below and the sync HTTPHandler/httpx path follow redirects by default, and aiohttp's resolver guard will not run for IP-literal redirect targets. An attacker who can choose api_base can point it at a public host they control that returns a 307/308 Location to http://169.254.169.254/... or another private IP, causing LiteLLM to fetch the internal endpoint. Disable automatic redirects or follow them manually after validating each Location for both sync and async clients.
There was a problem hiding this comment.
Addressed in commit fedf0c7b34 (pushed 2026-06-04).
Fix: Added _on_ssrf_request_start — an aiohttp TraceConfig on_request_start hook that fires for every request including redirect hops. It calls _assert_not_private_ip_literal on each URL before aiohttp establishes the connection, blocking any IP-literal redirect target (e.g. a 307 Location: http://169.254.169.254/...).
- Hostname-based redirect targets are already covered by
_SSRFGuardResolverat DNS-resolution time. - IP-literal redirect targets (the gap veria flagged) are now blocked by the trace hook before the connection is made.
_make_ssrf_trace_config()wires this hook into the defaultaiohttp.ClientSessionviatrace_configs=[...].
Tests in TestSSRFTraceConfig verify: AWS metadata IP literal blocked, RFC-1918 literal blocked, public IP allowed, hostname URLs pass through, and allow_requests_to_internal_ips=True opt-out respected.
|
🤖 litellm-agent: This PR is currently BLOCKED from merge. Score: 2/5 ❌ Why blocked:
Details: Score docked for: 1 PR-related CI failure (Greptile gate: score 3/5 below required 4/5 — request a Greptile review ( Fix the issues above and push an update — the bot will re-review automatically.
|
…FGuardResolver - Replace asyncio.get_event_loop() with get_running_loop() inside _SSRFGuardResolver.resolve() — get_event_loop() is deprecated in Python 3.10+ and emits DeprecationWarning when called from a coroutine - Propagate socket.gaierror instead of silently returning [] so aiohttp wraps it in ClientConnectorError as callers expect - Update TestSSRFGuardResolver to use asyncio.run() and get_running_loop() in async helpers, and rename the DNS-failure test to reflect the new raise-on-failure contract Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… asyncio deprecation Adds litellm.allow_requests_to_internal_ips flag (default False) so self-hosted / on-prem deployments pointing api_base at an internal address (e.g. Ollama, vLLM) can opt out of SSRF protection rather than being unconditionally blocked. Both _assert_not_private_url and _SSRFGuardResolver respect the flag. Also replaces the deprecated asyncio.get_event_loop() call in BaseLLMAIOHTTPHandler.__del__ with asyncio.get_running_loop(), falling back to a fresh event loop — eliminating the DeprecationWarning raised in Python 3.10+. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ard bypass Adds ::/128 to _BLOCKED_NETWORKS — the IPv6 unspecified/wildcard address was missing, allowing an attacker to bypass SSRF protection by resolving a hostname to :: which binds to all local interfaces. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@greptileai score this PR |
|
🤖 litellm-agent: This PR is currently BLOCKED from merge. Score: 3/5 ❌ Why blocked:
Details: Score docked for: 1 PR-related CI failure (This PR will be auto-closed as it lacks a screenshot for proof of fix. Please include one in the PR description. Add the Fix the issues above and push an update — the bot will re-review automatically.
|
…async preflight Replace the manual _BLOCKED_NETWORKS list with addr.is_global — this automatically covers all IANA special-use ranges including RFC 2544 benchmarking (198.18.0.0/15), class E reserved (240.0.0.0/4), and RFC 5737 documentation (192.0.2.0/24) that were previously missing. CGNAT (100.64.0.0/10) is still explicitly checked for Python < 3.11 compat where is_global incorrectly returns True for that range. Remove the blocking socket.getaddrinfo() preflight from _make_common_async_call — it stalled the event loop under concurrent load. The _SSRFGuardResolver (attached to the TCPConnector) already enforces the same check non-blocking at TCP-connect time, including redirect targets. The sync path retains _assert_not_private_url since httpx has no equivalent connection-time resolver hook. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@greptileai update the confidence score |
Updated Confidence Score: 5/5All P1/P2 issues flagged in earlier rounds have been addressed in the latest commit (
The two remaining open Safe to merge. Tip: You can customize Greptile's behavior for this repo with |
|
🤖 litellm-agent: This PR is currently BLOCKED from merge. Score: 3/5 ❌ Why blocked:
Details: Score docked for: 1 PR-related CI failure (This PR will be auto-closed as it lacks a screenshot for proof of fix. Please include one in the PR description. Add the Fix the issues above and push an update — the bot will re-review automatically.
|
…olver aiohttp's TCPConnector skips _SSRFGuardResolver when the URL host is already an IP address (no DNS lookup performed), allowing direct access to private addresses like 169.254.169.254 or 127.0.0.1. Add _assert_not_private_ip_literal, a fast non-blocking preflight that catches this bypass on the async path without requiring socket I/O. Hostname-based URLs continue to be protected by _SSRFGuardResolver at TCP-connect time. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
|
🤖 litellm-agent: This PR is currently BLOCKED from merge. Score: 3/5 ❌ Why blocked:
Details: Score docked for: 1 PR-related CI failure (This PR will be auto-closed as it lacks a screenshot for proof of fix. Please include one in the PR description. Add the Fix the issues above and push an update — the bot will re-review automatically.
|
|
@veria-ai re review |
…uardTransport The preflight _assert_not_private_url call resolved the hostname with socket.getaddrinfo but httpx/httpcore performed its own DNS resolution at connect time, leaving a TOCTOU window where an attacker controlling DNS could serve a public IP during the check and switch to a private IP (e.g. 169.254.169.254) before the actual connection. Fixes this by introducing _SSRFGuardTransport(httpx.HTTPTransport), which mirrors _SSRFGuardResolver on the async path: it resolves the hostname once, validates every returned IP, then rewrites the request URL to the pinned IP so httpcore connects directly without a second DNS lookup — closing the window entirely. The original hostname is preserved in the Host header and the sni_hostname extension so TLS certificate validation and virtual-host routing are unaffected. HTTPHandler gains an optional transport= parameter so the guard transport can be injected while preserving all existing SSL/cert/timeout/header config. All three sync client creation sites in aiohttp_handler.py now use _get_ssrf_safe_sync_client(); _assert_not_private_url remains as defence-in-depth for externally-supplied clients. Tests added to prove: - _SSRFGuardTransport blocks private IPs including all DNS answers - IP pinning: forwarded request carries the resolved IP, not the hostname - Host header and sni_hostname preserved for TLS - _make_common_sync_call succeeds for legitimate (public-IP) URLs, confirming the sync code path still works after the change Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@veria-ai DNS-rebinding TOCTOU on the sync path addressed in 3aff456 — introduced _SSRFGuardTransport |
Previously, when the URL host was already an IP address (e.g. http://10.0.0.1/api), ipaddress.ip_address(host) succeeded and the except-ValueError branch — which contains all SSRF validation — was skipped entirely. A caller using _get_ssrf_safe_sync_client() directly (bypassing _make_common_sync_call's preflight) would therefore pass private IP-literal addresses to the network unchecked. Fix: restructure the try/except so that a successful ip_address() parse immediately calls _is_blocked_address() and raises on private/reserved addresses. Only a ValueError from ip_address() (meaning the host is a hostname, not an IP literal) falls through to the DNS-resolve-and-pin path. No DNS lookup is needed for IP literals — the check is instant. Add five new _SSRFGuardTransport tests covering: - 10.x.x.x IP-literal blocked (no DNS called) - 169.254.169.254 (AWS metadata) IP-literal blocked - ::1 IPv6 loopback IP-literal blocked - allow_requests_to_internal_ips=True bypasses IP-literal check - end-to-end via _get_ssrf_safe_sync_client with IP-literal URL Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@greptileai review the latest commit 7dad714 |
|
@greptileai re-review this PR and give the confidence score |
|
@veria-ai re review |
aiohttp's _SSRFGuardResolver runs at DNS time but is bypassed when the server returns a Location header that is already a bare IP literal (no DNS resolution needed, so the resolver never fires). Add _on_ssrf_request_start (aiohttp TraceConfig on_request_start hook) and _make_ssrf_trace_config() to close this gap. The hook fires for every aiohttp request, including redirect hops. It calls _assert_not_private_ip_literal on each URL, blocking private IP-literal redirect targets before aiohttp makes the connection. Hostname-based redirect targets continue to be validated by _SSRFGuardResolver at TCP-connect time. Also adds test_del_without_running_loop_uses_new_event_loop to cover the __del__ asyncio.new_event_loop() fallback branch (the RuntimeError path that was the 1 missing line in Codecov patch coverage). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nfig Two P1 Greptile concerns on the previous commit: 1. _get_ssrf_safe_sync_client() created a new HTTPHandler on every call, dropping the in_memory_llm_clients_cache that _get_httpx_client() used. Under sustained load every sync request incurred a fresh TCP+TLS handshake. Fix: apply the same cache-lookup/store pattern with _DEFAULT_TTL_FOR_HTTPX_CLIENTS. 2. _SSRFGuardTransport() was constructed with httpx defaults (verify=True, no cert). When httpx.Client receives a custom transport= object, it ignores its own verify= and cert= arguments — so litellm.ssl_verify and SSL_CERTIFICATE were silently dropped, breaking self-signed-cert and mTLS deployments. Fix: resolve SSL config via get_ssl_configuration(None) and forward both verify and cert to _SSRFGuardTransport.__init__. Also adds import os (was missing), _DEFAULT_TTL_FOR_HTTPX_CLIENTS from litellm.constants, and get_ssl_configuration from http_handler. Tests added in TestSSRFSafeClientCachingAndSSL: - test_client_is_cached_across_calls - test_ssl_config_is_forwarded_to_transport - test_ssl_certificate_is_forwarded_to_transport Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
3 participants
Relevant issues
Extends #26264 — closes SSRF gap in
aiohttp_handler.py(CWE-918, findings #13–14).Summary
PR #26264 blocked private IPs in the main HTTP handler but
aiohttp_handler.pywas notcovered.
api_basewas passed directly tosession.post()without IP validation, allowingrequests to
169.254.169.254(AWS IMDS),10.x.x.xinternal hosts, or other privateaddresses.
Fix
_BLOCKED_NETWORKS: RFC-1918, loopback, link-local (169.254/16, fe80::/10), CGNAT,0.0.0.0/8, IPv6 ULA/loopback
_is_blocked_address(): unwraps IPv4-mapped IPv6 (::ffff:x.x.x.x) before network check_assert_not_private_url(): validates ALLgetaddrinfoanswers (not just[0]) —prevents A-record rotation bypass
_SSRFGuardResolver(AbstractResolver): validates IPs inside aiohttp's own connectionloop, covering redirect targets and eliminating the DNS-rebinding TOCTOU window
ClientSessioncreation usesTCPConnector(resolver=_SSRFGuardResolver())_make_common_sync_call) also guarded with preflight checkPre-Submission checklist
tests/test_litellm/llms/test_aiohttp_ssrf_protection.pytest_aiohttp_handler.pyfor newTCPConnectorusageuv run black .anduv run ruff check— cleanType
🐛 Bug Fix
Screenshot
PR raised by Drishna at Incubyte