Skip to content

fix: harden REST tool outbound validation - #5925

Merged
Lang-Akshay merged 8 commits into
mainfrom
fix/rest-tool-outbound-validation
Jul 29, 2026
Merged

fix: harden REST tool outbound validation#5925
Lang-Akshay merged 8 commits into
mainfrom
fix/rest-tool-outbound-validation

Conversation

@gandhipratik203

@gandhipratik203 gandhipratik203 commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

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

Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
@prakhar-singh1928

Copy link
Copy Markdown
Collaborator

1 blocking issue
Blocking: intra-request DNS TOCTOU window in validate_url_for_connection_pinning

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>
@gandhipratik203

Copy link
Copy Markdown
Collaborator Author

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 Lang-Akshay left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@prakhar-singh1928

Copy link
Copy Markdown
Collaborator

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>

@prakhar-singh1928 prakhar-singh1928 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@Lang-Akshay Lang-Akshay left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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>
@gandhipratik203

Copy link
Copy Markdown
Collaborator Author

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.

Lang-Akshay
Lang-Akshay previously approved these changes Jul 29, 2026

@Lang-Akshay Lang-Akshay left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@prakhar-singh1928 prakhar-singh1928 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@Lang-Akshay Lang-Akshay left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@Lang-Akshay
Lang-Akshay added this pull request to the merge queue Jul 29, 2026
Merged via the queue into main with commit 75e9ccc Jul 29, 2026
36 checks passed
@Lang-Akshay
Lang-Akshay deleted the fix/rest-tool-outbound-validation branch July 29, 2026 15:10
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.

3 participants