Skip to content

fix(fetch): harden URL fetching against SSRF - #4773

Open
Ethanz11-creat wants to merge 1 commit into
modelcontextprotocol:mainfrom
Ethanz11-creat:fix/fetch-ssrf-hardening
Open

fix(fetch): harden URL fetching against SSRF#4773
Ethanz11-creat wants to merge 1 commit into
modelcontextprotocol:mainfrom
Ethanz11-creat:fix/fetch-ssrf-hardening

Conversation

@Ethanz11-creat

Copy link
Copy Markdown
Contributor

Summary

The fetch server currently accepts an arbitrary URL and follows HTTP redirects with no address/scheme checks, so an agent could read loopback, RFC1918/ULA, or cloud metadata endpoints (e.g. 169.254.169.254, IMDS) when the MCP server runs on a cloud VM. This implements the SSRF hardening described in #3741.

Changed (src/fetch/src/mcp_server_fetch/server.py):

  • Restrict allowed schemes to http/https (blocks file://, data://, ftp://, ... protocol-abuse vectors).
  • Block non-public address ranges for both IPv4 and IPv6: loopback, RFC1918, IPv6 unique-local, link-local (incl. cloud metadata 169.254.0.0/16), CGNAT, multicast, reserved, and IPv4-mapped IPv6 (::ffff:127.0.0.1).
  • Hostnames are resolved (all A/AAAA records) and rejected fail-closed if any address lands on a blocked network.
  • Redirects are now followed manually, with a hop cap, re-validating every intermediate target — previously follow_redirects=True let an open-redirect re-point the request at an internal address after the first URL passed.

Validation is a standalone pure function split so it is unit-testable without network.

Tests

Added TestIsBlockedIp, TestValidateUrl, and TestRedirectValidation covering private/loopback/metadata IPv4+IPv6 literals, IPv4-mapped IPv6, scheme ablation, hostnames resolving to a blocked address, and redirects into private networks. Existing tests pin hostname resolution to a public IP so the suite stays hermetic.

Green on this branch:

pytest -q            55 passed  (21 pre-existing + 34 new)
ruff check           0 errors
ruff format --check  0 diffs
pyright              0 errors

Fixes #3741

The fetch server currently accepts an arbitrary URL and follows redirects
unconditionally, so an agent can read loopback, private-network, or cloud
metadata endpoints (e.g. 169.254.169.254). This mirrors modelcontextprotocol#3741.

Add scheme + address validation applied on every redirect hop:
- restrict to http/https (blocks file://, data://, ftp:// abuse);
- block loopback, RFC1918/ULA, link-local (incl. cloud metadata), CGNAT,
  multicast and reserved ranges for both IPv4 and IPv6, including IPv4-mapped
  IPv6 addresses;
- resolve hostnames and fail-closed if any A/AAAA record lands on a blocked
  network, and re-validate each redirect target instead of only the first URL.

Redirects are now followed manually with a hop cap so an open-redirect can no
longer re-point the request at an internal address after validation.

Fixes modelcontextprotocol#3741
@tiagovilasboas

Copy link
Copy Markdown

AppSec review — fetch SSRF harden (#4773)

Strong direction on #3741: scheme allowlist, private/link-local denylist, fail-closed on any blocked A/AAAA at check time, and manual redirect revalidation (incl. robots.txt) are the right threat model. IPv4-mapped coverage looks good.

Verdict: Request changes (comment form — no formal review permission on this repo).

Blocking

  1. DNS TOCTOU / rebinding: _validate_url uses getaddrinfo, then httpx resolves again on connect. A name that flips public→IMDS between check and dial still bypasses. Prefer connect-by-validated-IP (keep Host/SNI/TLS for the original name), or document as accepted residual risk — I would not call the harden “done” while rebinding still hits metadata.

  2. Azure special IP: 168.63.129.16 is not in _BLOCKED_NETWORKS and ipaddress treats it as global — literal fetch would pass today. Also consider NAT64 64:ff9b::/96 and non-mapped ::127.0.0.1 forms (or move toward not ip.is_global with explicit exceptions).

  3. Empty Location: Location: "" / whitespace via urljoin keeps current and burns the hop budget; missing Location on 3xx returns the redirect body. Please fail closed on empty/missing Location when is_redirect (feat(fetch): add opt-in host allowlisting (--allowed-hosts) #4770 already tests this).

Non-blocking

  • Decimal/octal/short IPv4: blocked via glibc getaddrinfo here but untested / libc-dependent — add cases.
  • Hop cap 5 vs feat(fetch): add opt-in host allowlisting (--allowed-hosts) #4770’s 20: prefer one shared redirect helper that runs IP denylist + optional --allowed-hosts per hop (mechanical merge conflict otherwise).
  • Error code INTERNAL_ERROR vs INVALID_PARAMS for blocked targets — minor.

Happy to re-review quickly once the three blockers are addressed or explicitly deferred with docs.

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.

Security Hardening Recommendations for Fetch Server (SSRF Prevention)

2 participants