fix: harden REST tool outbound validation - #5925
Conversation
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
|
1 blocking issue The new helper performs two independent DNS resolutions for the same hostname within a single invocation. Resolution #1 — inside the run_in_executor call at validators.py line 1562: cls.validate_url(value, field_name) calls _validate_ssrf(hostname) which calls socket.getaddrinfo(hostname). The SSRF policy decision is made against this result. Resolution #2 — at validators.py line 1585: _resolve_hostname_for_connection_pinning() calls socket.getaddrinfo(hostname) again. The IP returned here is what gets pinned into the outbound connection at tool_service.py line 5472. The SSRF check at lines 1587–1589 then calls _validate_ssrf(resolved_ip) with IPs from resolution #2. For standard loopback and RFC 1918 addresses this second check still catches the rebind. The gap opens when the policy allows a range (for example ssrf_allow_private_networks=True, or a cloud-internal VPC IP not classified as is_private by Python's ipaddress) — in that case resolution #1 passes on the public-looking IP, resolution #2 returns the internal IP, and the second _validate_ssrf pass allows it through because the policy permits that range. More fundamentally, the PR's stated goal is to close the TOCTOU gap, which requires that the IP used for the SSRF policy decision and the IP used for the connection are the same object from the same resolution. Currently they come from two separate getaddrinfo calls. Suggested fix: add a skip_ssrf parameter to validate_url (defaulting to False so all existing callers are unaffected), then pass skip_ssrf=True inside the validate_url_for_connection_pinning executor lambda. This makes validate_url run structural and XSS checks only, with zero DNS calls. All SSRF enforcement then comes from the single _resolve_hostname_for_connection_pinning call below it, whose result is both checked and pinned. One resolution, one policy check, one pin, no window |
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
|
Updated with a scoped follow-up. The REST tool path now performs structural URL validation without the extra resolver pass, preserves the hostname policy check, and uses the single resolved target for policy validation and connection pinning. Added focused regression coverage for this behavior. |
Lang-Akshay
left a comment
There was a problem hiding this comment.
Thanks for the PR @gandhipratik203
Please address this blocking Changes
| # | Area | File | Line | Blocking reason | Required change |
|---|---|---|---|---|---|
| 1 | Security / CWE-918, CWE-367 | mcpgateway/common/validators.py; mcpgateway/services/tool_service.py |
1618–1637; 5471–5475 | When ssrf_protection_enabled=true and ssrf_dns_fail_closed=false, _resolve_hostname_for_connection_pinning() returns [] on DNS timeout/error. tool_service.py then sees resolved_ip=None, skips _pin_url_to_resolved_ip, and issues the request with the original hostname. HTTPX re-resolves at I/O time — restoring the exact TOCTOU/DNS-rebinding window this PR claims to close. |
Fail closed for pinned REST calls: when SSRF protection is enabled and DNS resolution returns no addresses (or errors), raise immediately rather than proceeding unpinned. The ssrf_dns_fail_closed global is a compatibility knob that must not create an escape hatch on the security-sensitive REST call path. |
| 2 | Security / CWE-295, CWE-346, CWE-201 | mcpgateway/services/tool_service.py |
234–239, 1071, 5472–5475 | Replacing the URL authority with the resolved IP changes the HTTPX request origin to (scheme, IP, port). ToolService shares a single keep-alive AsyncClient across all REST tools. HTTPX/httpcore pools connections by URL origin; TLS SNI is applied only at handshake, not on reused connections. Same-IP different-hostname tools can therefore reuse a TLS connection authenticated for a different hostname. HTTPX's cookie jar also keys host-only cookies by the pinned IP, so cookies set by one logical host can be sent to another on the same IP. |
Restore logical-origin isolation. Options: (a) use a per-request transport with a custom resolver that resolves a pre-validated address but preserves the original hostname as the pool key; (b) maintain per-original-authority HTTPX clients; (c) disable connection reuse (limits=httpx.Limits(max_keepalive_connections=0)) for pinned requests. Do not rely solely on Host/sni_hostname overrides to recover origin isolation after an IP rewrite. |
|
The first blocking issue (DNS fail-open restoring the TOCTOU window) has been addressed — validate_url_for_connection_pinning now raises when resolved_ips is empty and ssrf_protection_enabled=true, regardless of ssrf_dns_fail_closed. The pinning block in tool_service.py will never be silently skipped with SSRF protection on. |
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Lang-Akshay
left a comment
There was a problem hiding this comment.
Blocking Changes
| # | Area | File | Line | Blocking reason | Required change |
|---|---|---|---|---|---|
| 1 | Security (CWE-918 SSRF) | mcpgateway/common/validators.py |
1509–1525 | High: _validate_ssrf() blocks is_private and loopback addresses but does not block RFC 6598 shared address space (100.64.0.0/10). Python's ipaddress does not classify 100.64.x.x as is_private, and it is absent from the default blocked-network set. A REST hostname that resolves there passes the validator and is then pinned and contacted by tool_service.py, achieving an SSRF to CGNAT/internal targets. The gateway-test validator has a separate CGNAT check; the REST path is inconsistent. |
Add 100.64.0.0/10 (and any other non-global/non-unicast blocks appropriate for your deployment) to the default blocked-network set or the _validate_ssrf() classifier. Apply a single shared outbound-IP classifier to both REST and gateway-test validation paths. |
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
|
Updated with a scoped follow-up. The outbound URL validation now treats RFC 6598 shared address space (100.64.0.0/10) as restricted. I added it to the default SSRF blocked networks and moved the CGNAT check into a shared classifier used by both REST connection-pinning validation and gateway-test validation. Added focused regression coverage for direct SSRF validation, gateway-test validation, and REST connection-pinning DNS results resolving into that range. |
Summary
Tightens REST tool outbound request handling and adds regression coverage for runtime URL handling.
Internal tracking: https://github.ibm.com/contextforge-org/internal_issues/issues/451