From 9d858564e873c5042e53a767386b354f24ee2116 Mon Sep 17 00:00:00 2001 From: Rafael Poyiadzi Date: Wed, 25 Feb 2026 09:59:47 +0000 Subject: [PATCH 1/7] Add comprehensive SSRF security audit report Defensive security audit of the MCP server's SSRF attack surface, reviewing protections from commit 4000b88. Covers DNS rebinding TOCTOU, port restrictions, IP blocklist gaps, rate limit bypass, URL parser discrepancies, and container hardening. Co-Authored-By: Claude Opus 4.6 --- SSRF_AUDIT_REPORT.md | 630 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 630 insertions(+) create mode 100644 SSRF_AUDIT_REPORT.md diff --git a/SSRF_AUDIT_REPORT.md b/SSRF_AUDIT_REPORT.md new file mode 100644 index 00000000..6f23a7e0 --- /dev/null +++ b/SSRF_AUDIT_REPORT.md @@ -0,0 +1,630 @@ +# SSRF Security Audit Report + +**Target:** EveryRow MCP Server (`everyrow-mcp/src/everyrow_mcp/`) +**Audit Date:** 2026-02-25 +**Scope:** Full SSRF attack surface analysis, bypass testing of existing protections, container/deployment review +**Baseline:** Commit `4000b88` ("Security hardening: SSRF, headers, Redis TLS, container lockdown") + +--- + +## Executive Summary + +The MCP server implements a **three-layer SSRF protection** around its sole user-controlled URL-fetching path (`fetch_csv_from_url`), introduced in commit `4000b88`. The protections are well-designed and cover the major attack vectors. No **Critical** vulnerabilities were found. The remaining risks are a narrow DNS-rebinding TOCTOU window that existing mitigations reduce but cannot fully close, missing port restrictions, and an incomplete IP blocklist. The auth subsystem and Redis infrastructure are not directly exploitable for SSRF. + +| Severity | Count | Status | +|----------|-------|--------| +| Critical | 0 | — | +| High | 1 | Residual risk (mitigated) | +| Medium | 3 | Actionable | +| Low | 5 | Hardening opportunities | +| Info | 3 | Defence-in-depth notes | + +--- + +## Architecture Overview + +### SSRF Attack Surface + +The server has **two transport modes** with different trust boundaries: + +| Mode | User-controlled URLs? | File system access? | Auth required? | +|------|----------------------|---------------------|---------------| +| stdio | Yes (URL + local path) | Yes (local CSV read/write) | No (API key) | +| HTTP | Yes (URL only) | No (blocked by validator) | Yes (OAuth 2.1) | + +**Single user-controlled URL entry point:** + +``` +everyrow_upload_data(source=) + → UploadDataInput.validate_source() # scheme check + → validate_url() # http/https only + → fetch_csv_from_url(url) # 3-layer SSRF protection +``` + +**Outbound HTTP clients (non-user-controlled):** + +| Client | File:Line | Target | User-controlled? | +|--------|-----------|--------|-----------------| +| `fetch_csv_from_url` | `utils.py:188` | User-provided URL | **Yes** — SSRF-protected | +| `EveryRowAuthProvider._http_client` | `auth.py:199` | `settings.supabase_url` | No — config-derived | +| `SupabaseTokenVerifier._jwks_client` | `auth.py:53` | `{supabase_url}/.well-known/jwks.json` | No — config-derived | +| `AuthenticatedClient` (SDK) | `tool_helpers.py:65` | `settings.everyrow_api_url` | No — config-derived | +| `AuthenticatedClient` (upload) | `uploads.py:312` | `settings.everyrow_api_url` | No — config-derived | +| `AuthenticatedClient` (routes) | `routes.py:99` | `settings.everyrow_api_url` | No — config-derived | + +--- + +## Existing SSRF Protections (Commit 4000b88) + +### Layer 1: Pre-flight DNS Validation + +**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/utils.py:53-100` + +Before any HTTP request, `_validate_url_target(url)` extracts the hostname via `urlparse` and calls `_validate_hostname()`, which: + +1. Checks against `_BLOCKED_HOSTNAMES` (`metadata.google.internal` + FQDN variant) +2. For IP literals: validates directly against `_BLOCKED_NETWORKS` with IPv4-mapped IPv6 unwrapping +3. For DNS names: resolves via `socket.getaddrinfo(AF_UNSPEC)` and checks every resolved IP + +**Blocked networks (utils.py:21-31):** + +| Network | Purpose | +|---------|---------| +| `10.0.0.0/8` | RFC 1918 private | +| `172.16.0.0/12` | RFC 1918 private | +| `192.168.0.0/16` | RFC 1918 private | +| `127.0.0.0/8` | Loopback | +| `169.254.0.0/16` | Link-local / cloud metadata | +| `0.0.0.0/8` | This network | +| `::1/128` | IPv6 loopback | +| `fc00::/7` | IPv6 ULA | +| `fe80::/10` | IPv6 link-local | + +### Layer 2: Transport-Level Re-validation + +**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/utils.py:168-185` + +Custom `_SSRFSafeTransport(httpx.AsyncBaseTransport)` wraps every outgoing request and calls `_validate_hostname(request.url.host)` immediately before the inner transport connects. This re-check narrows the DNS-rebinding TOCTOU window. + +### Layer 3: Redirect Chain Validation + +**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/utils.py:152-165` + +An httpx `event_hooks["response"]` hook validates every redirect `Location` header against `_validate_url_target()` before following. Redirects are capped at `max_redirects=5`. + +### Additional Controls + +| Control | File:Line | Details | +|---------|-----------|---------| +| Scheme restriction | `utils.py:117` | Only `http://` and `https://` | +| Streaming size limit | `utils.py:219-224` | 50 MB default, aborts mid-stream | +| Content-Length pre-check | `utils.py:212` | Rejects before streaming if header present | +| IPv4-mapped IPv6 unwrap | `utils.py:48-49, 68-70` | `::ffff:127.0.0.1` → `127.0.0.1` | +| Fail-closed on unparseable IPs | `utils.py:46` | Returns `True` (blocked) | +| Fail-closed on DNS failure | `utils.py:82` | Raises `ValueError` | + +--- + +## Findings + +### FINDING-01: DNS Rebinding TOCTOU Window (Residual) + +**Severity:** High (residual risk after mitigation) +**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/utils.py:179-182` +**Status:** Partially mitigated by `_SSRFSafeTransport` + +**Description:** + +The `_SSRFSafeTransport` re-validates hostnames at request time, but its `_validate_hostname()` call performs its own `socket.getaddrinfo()` lookup, which is a **separate DNS resolution** from what httpx's inner `AsyncHTTPTransport` (via `httpcore`) performs when opening the TCP connection. If DNS rebinds between the transport-level check (line 181) and the actual `connect()` inside `httpcore`, a fast-rebinding DNS server could succeed. + +``` +Timeline: + T0: _validate_hostname() → getaddrinfo() → returns 93.184.216.34 (public) ✓ + T1: DNS rebinds hostname → 169.254.169.254 (metadata) + T2: AsyncHTTPTransport → httpcore.connect() → getaddrinfo() → 169.254.169.254 + T3: Connection established to cloud metadata service +``` + +The window between T0 and T2 is extremely narrow (microseconds within the same async coroutine), making this attack probabilistic and unreliable, but **not zero**. + +**Proof of Concept:** + +```python +# Attacker-controlled DNS server with fast rebinding +# First query returns 93.184.216.34 (passes validation) +# Immediate second query returns 169.254.169.254 (metadata) + +# Attacker sends to MCP tool: +source = "http://rebind.attacker.com/csv" + +# Race condition: ~1-5% success rate on fast networks +# If DNS rebinds between _SSRFSafeTransport check and httpcore connect, +# the request reaches the cloud metadata service. +``` + +**Recommended Fix:** + +Pin the resolved IP at validation time and connect directly to it, passing the original hostname as the `Host` header. This eliminates the TOCTOU entirely: + +```python +class _SSRFSafeTransport(httpx.AsyncBaseTransport): + def __init__(self) -> None: + self._transport = httpx.AsyncHTTPTransport(retries=0) + + async def handle_async_request(self, request: httpx.Request) -> httpx.Response: + hostname = request.url.host + if not hostname: + return await self._transport.handle_async_request(request) + + # Resolve and validate once + resolved_ip = _resolve_and_validate(hostname) # new helper + + # Rewrite the request URL to use the pinned IP + pinned_url = request.url.copy_with(host=resolved_ip) + pinned_request = httpx.Request( + method=request.method, + url=pinned_url, + headers={**request.headers, "Host": hostname}, + content=request.content, + ) + return await self._transport.handle_async_request(pinned_request) +``` + +--- + +### FINDING-02: No Port Restriction on Fetched URLs + +**Severity:** Medium +**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/utils.py:53-100` + +**Description:** + +The SSRF blocklist validates IP addresses but does not restrict ports. An attacker can probe internal services on non-standard ports even when the IP is public or allowed. This is particularly dangerous in containerized deployments where services bind to non-standard ports. + +**Proof of Concept:** + +```python +# Probe Redis on its default port (if exposed on a public IP or shared network) +source = "http://redis.internal:6379/" +# Redis responds with -ERR, but the connection is established +# and the response reveals service information + +# Probe internal HTTP services on non-standard ports +source = "http://monitoring.internal:9090/api/v1/targets" +# If monitoring.internal resolves to a non-blocked IP + +# SMTP banner grabbing +source = "http://mail.company.com:25/" +``` + +**Recommended Fix:** + +Add a port allowlist (default: 80, 443) or blocklist (common internal service ports): + +```python +_BLOCKED_PORTS = { + 25, 465, 587, # SMTP + 6379, 6380, # Redis + 5432, # PostgreSQL + 3306, # MySQL + 27017, # MongoDB + 2379, 2380, # etcd + 9200, 9300, # Elasticsearch + 11211, # Memcached +} + +_ALLOWED_PORTS = {80, 443, 8080, 8443} # Alternative: allowlist approach + +def _validate_url_target(url: str) -> None: + parsed = urlparse(url) + port = parsed.port or (443 if parsed.scheme == "https" else 80) + if port not in _ALLOWED_PORTS: + raise ValueError(f"Port {port} is not permitted for URL fetching") + # ... existing hostname validation +``` + +--- + +### FINDING-03: Incomplete IP Blocklist — Missing Cloud/RFC Ranges + +**Severity:** Medium +**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/utils.py:21-31` + +**Description:** + +The blocklist covers the major RFC 1918 ranges and cloud metadata endpoints but is missing several ranges that are reachable in cloud/container environments: + +| Missing Network | RFC | Risk | +|----------------|-----|------| +| `100.64.0.0/10` | RFC 6598 (CGNAT) | Used in AWS VPCs, GKE node pools, Tailscale | +| `198.18.0.0/15` | RFC 2544 | Network testing; sometimes used internally | +| `192.0.0.0/24` | RFC 6890 | IANA special-purpose | +| `192.0.2.0/24` | RFC 5737 | TEST-NET-1 (documentation) | +| `198.51.100.0/24` | RFC 5737 | TEST-NET-2 (documentation) | +| `203.0.113.0/24` | RFC 5737 | TEST-NET-3 (documentation) | + +The most critical omission is `100.64.0.0/10` — this CGNAT range is used by AWS for VPC endpoints and by Tailscale for mesh networking. An attacker could reach internal VPC services via this range. + +**Proof of Concept:** + +```python +# AWS VPC endpoint (CGNAT range, used by some services) +source = "http://100.64.0.1/latest/meta-data/" + +# Tailscale node in mesh network +source = "http://100.100.100.100/api/v1/status" +``` + +**Recommended Fix:** + +```python +_BLOCKED_NETWORKS = [ + # ... existing entries ... + ipaddress.ip_network("100.64.0.0/10"), # CGNAT (RFC 6598) — AWS VPC, Tailscale + ipaddress.ip_network("198.18.0.0/15"), # Benchmark testing (RFC 2544) + ipaddress.ip_network("192.0.0.0/24"), # IANA special-purpose (RFC 6890) +] +``` + +--- + +### FINDING-04: Rate Limit Bypass via IP Header Spoofing + +**Severity:** Medium +**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/middleware.py:28-40` + +**Description:** + +When `trust_proxy_headers=True`, `get_client_ip()` reads the first IP from `X-Forwarded-For` (or the configured header). If the reverse proxy does not strip or overwrite incoming `X-Forwarded-For` headers, an attacker can bypass rate limiting by spoofing different client IPs on each request. + +```python +# middleware.py:36-39 +if settings.trust_proxy_headers: + value = request.headers.get(settings.trusted_ip_header.lower()) + if value: + return value.split(",")[0].strip() # Trusts first value +``` + +This affects: +- Registration rate limiting (`middleware.py:43-91`) +- OAuth rate limiting (`auth.py:211-218`) +- All IP-based access controls + +**Proof of Concept:** + +```bash +# Bypass rate limit by rotating spoofed IPs +for i in $(seq 1 1000); do + curl -H "X-Forwarded-For: 1.2.3.$((i % 256))" \ + https://mcp.example.com/register +done +``` + +**Current Mitigation:** `docker-compose.yaml:42` defaults `TRUST_PROXY_HEADERS=false`. But Kubernetes/GKE deployments typically set this to `true`. + +**Recommended Fix:** + +1. Document that the reverse proxy MUST overwrite (not append to) the trusted IP header +2. Consider validating that the first IP in `X-Forwarded-For` is not from a private range +3. Add the proxy's own IP to a `TRUSTED_PROXIES` allowlist and only read the header when the direct connection comes from a trusted proxy: + +```python +def get_client_ip(request: Request) -> str | None: + if settings.trust_proxy_headers: + direct_ip = request.client.host if request.client else None + if direct_ip and direct_ip in settings.trusted_proxy_ips: + value = request.headers.get(settings.trusted_ip_header.lower()) + if value: + return value.split(",")[0].strip() + return request.client.host if request.client else None +``` + +--- + +### FINDING-05: Relative Redirect Blocking (False Positive / Fail-Safe) + +**Severity:** Low +**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/utils.py:152-165` + +**Description:** + +The `_check_redirect` event hook validates the raw `Location` header from redirect responses. For relative redirects (e.g., `Location: /path`), `urlparse` returns `hostname=None`, causing `_validate_url_target` to raise `"URL has no hostname"`. This blocks the redirect chain. + +From a security perspective, this is **fail-safe** — relative redirects stay on the same (already validated) host. However, it may cause false positives if a legitimate public server (e.g., Google Sheets) returns a relative redirect. + +```python +# utils.py:155-158 +location = response.headers.get("location", "") +if location: + try: + _validate_url_target(location) # Fails for relative URLs +``` + +**Recommended Fix:** + +Resolve relative redirects against the request URL before validating: + +```python +async def _check_redirect(response: httpx.Response) -> None: + if response.is_redirect: + location = response.headers.get("location", "") + if location: + # Resolve relative redirects against the request URL + from urllib.parse import urljoin + resolved = urljoin(str(response.request.url), location) + try: + _validate_url_target(resolved) + except ValueError: + raise httpx.TooManyRedirects( + f"Redirect to blocked address: {resolved}", + request=response.request, + ) +``` + +--- + +### FINDING-06: URL Parser Discrepancy (urlparse vs httpx) + +**Severity:** Low +**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/utils.py:90-100` + +**Description:** + +The pre-flight validation uses Python's `urlparse` to extract the hostname, while httpx uses its own URL parser (via `httpcore`). Parser discrepancies could theoretically allow an attacker to craft a URL where `urlparse` extracts a public hostname (passes validation) while httpx connects to a different (internal) host. + +Known divergence vectors: +- **Backslash normalization:** `http://public.com\@127.0.0.1/` — `urlparse` may treat the path differently than httpx +- **Unicode hostnames:** `http://ⓔⓧⓐⓜⓟⓛⓔ.com/` — IDNA encoding differences +- **Percent-encoded authority:** `http://%31%32%37.%30.%30.%31/` — may decode to `127.0.0.1` differently + +**Mitigating Factor:** The `_SSRFSafeTransport` (Layer 2) re-validates using `request.url.host`, which is httpx's own parsed view. This provides defense-in-depth against parser discrepancies — even if the pre-flight check is fooled, the transport-level check uses the same parser that will make the connection. + +**Proof of Concept:** + +```python +# Theoretical — most of these are blocked by one layer or another + +# Backslash confusion (depends on Python/httpx version) +source = "http://public.com\\@127.0.0.1/" + +# Percent-encoded IP (urlparse decodes differently than httpx) +source = "http://%31%32%37%2e%30%2e%30%2e%31/" +``` + +**Recommended Fix:** + +Add explicit normalization before validation to match httpx's behavior: + +```python +def _validate_url_target(url: str) -> None: + # Normalize to match httpx's parser + try: + httpx_url = httpx.URL(url) + hostname = httpx_url.host + except httpx.InvalidURL: + raise ValueError(f"Invalid URL: {url}") + if not hostname: + raise ValueError(f"URL has no hostname: {url}") + _validate_hostname(hostname) +``` + +--- + +### FINDING-07: Auth httpx Client Without SSRF Transport + +**Severity:** Low +**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/auth.py:199-202` + +**Description:** + +The `EveryRowAuthProvider.__init__` creates an `httpx.AsyncClient()` without `_SSRFSafeTransport`. This client is used for: +- Supabase token exchange (`_supabase_token_request`, line 560) +- JWKS fetching (via `PyJWKClient`, line 53) + +The target URL is derived from `settings.supabase_url`, which is: +- A config value (environment variable), not user-controlled +- Validated at startup to require HTTPS for non-localhost (`config.py:134-147`) + +**Not exploitable** unless the attacker controls the server's environment variables, which would give them full compromise regardless. + +```python +# auth.py:199-202 +self._http_client = httpx.AsyncClient( + timeout=httpx.Timeout(10.0), + limits=httpx.Limits(max_connections=20, max_keepalive_connections=10), +) +``` + +**Recommended Fix (defence-in-depth):** + +Add the SSRF transport as a precaution: + +```python +from everyrow_mcp.utils import _SSRFSafeTransport + +self._http_client = httpx.AsyncClient( + transport=_SSRFSafeTransport(), + timeout=httpx.Timeout(10.0), + limits=httpx.Limits(max_connections=20, max_keepalive_connections=10), +) +``` + +--- + +### FINDING-08: Missing IPv6 Addresses in Blocklist + +**Severity:** Low +**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/utils.py:21-31` + +**Description:** + +The blocklist does not include: +- `::` (IPv6 unspecified address) — this is NOT the same as `::1` (loopback) and is NOT an IPv4-mapped address, so `ipv4_mapped` unwrapping does not cover it +- `::ffff:0:0/96` (IPv4-mapped prefix explicitly) — individual addresses are unwrapped, but the prefix itself is not blocked + +The practical risk is minimal since `::` does not route to any reachable host in most environments. + +**Recommended Fix:** + +```python +_BLOCKED_NETWORKS = [ + # ... existing entries ... + ipaddress.ip_network("::/128"), # IPv6 unspecified + ipaddress.ip_network("::ffff:0:0/96"), # IPv4-mapped prefix (belt-and-suspenders) +] +``` + +--- + +### FINDING-09: `_decode_trusted_server_jwt` Skips Signature Verification + +**Severity:** Info (by design) +**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/auth.py:152-161` + +**Description:** + +This function decodes JWTs with `verify_signature=False`. It is documented as for server-to-server tokens received from Supabase over HTTPS, and the docstring warns: + +> "NEVER use this for tokens received from end users." + +It is only called from `_issue_token_response` (line 456) with tokens obtained directly from Supabase's token endpoint over HTTPS. The caller chain is: + +``` +handle_callback → _validate_callback_request → _validate_supabase_code + → _exchange_supabase_code → _supabase_token_request + → POST {supabase_url}/auth/v1/token (HTTPS, server-to-server) + → response.access_token → _decode_trusted_server_jwt() +``` + +**Assessment:** Safe by current usage. The risk would arise if this function were called with user-supplied tokens in the future. The docstring warning is appropriate. + +**Recommended Fix:** Add a comment at the call site (line 456) reinforcing that the token source must remain trusted. + +--- + +### FINDING-10: Wildcard CORS on Widget Endpoints + +**Severity:** Info (safe by design) +**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/routes.py:22-33` + +**Description:** + +Widget endpoints use `Access-Control-Allow-Origin: *`. Per the CORS specification, browsers do not send credentials (cookies) with wildcard-origin requests. Since auth is via Bearer tokens (not cookies), this is safe — no ambient credentials are leaked. + +The code includes a clear documentation comment explaining this design decision. + +**Assessment:** Correct. No action needed. + +--- + +### FINDING-11: Container Hardening Review + +**Severity:** Info +**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/deploy/docker-compose.yaml` + +**Description:** + +The container configuration follows security best practices: + +| Control | Status | Location | +|---------|--------|----------| +| Non-root user | Present | `Dockerfile:19` (`mcp` user) | +| `no-new-privileges` | Present | `docker-compose.yaml:50` | +| `cap_drop: ALL` | Present | `docker-compose.yaml:51-52` | +| Read-only rootfs | Present | `docker-compose.yaml:53` | +| Memory limits | Present | `docker-compose.yaml:46-47` (512M) | +| CPU limits | Present | `docker-compose.yaml:48` (1 CPU) | +| Network isolation | Present | `docker-compose.yaml:60-62` (bridge) | +| Redis password required | Present | `docker-compose.yaml:4` | +| Redis not exposed to host | Present | No `ports:` on Redis service | +| MCP bound to localhost | Present | `docker-compose.yaml:29` (`127.0.0.1:8000`) | + +**Missing (minor):** +- No `pids_limit` set (prevents fork bomb DoS) +- No `tmpfs` size limit (line 55: `tmpfs: - /tmp` has no size cap) +- Consider `PYTHONHASHSEED=random` for hash collision DoS resistance + +**Recommended Fix:** + +```yaml +mcp-server: + # ... existing config ... + pids_limit: 100 + tmpfs: + - /tmp:size=50M + environment: + PYTHONHASHSEED: "random" +``` + +--- + +## Attack Surface Matrix + +| Attack Vector | Entry Point | Protection | Bypass Possible? | +|---------------|-------------|------------|-----------------| +| Direct SSRF via URL | `everyrow_upload_data(source=url)` | 3-layer validation | Only via DNS rebinding TOCTOU (FINDING-01) | +| SSRF via redirect | HTTP 3xx from attacker server | Event hook + transport re-check | Relative redirects blocked (fail-safe, FINDING-05) | +| Cloud metadata (169.254.169.254) | URL or DNS rebind | IP blocklist + hostname block | No (blocked in all 3 layers) | +| GKE metadata hostname | `metadata.google.internal` | Hostname blocklist | No | +| IPv4-mapped IPv6 bypass | `::ffff:127.0.0.1` | Unwrap + re-check | No | +| Localhost via DNS | `attacker.com → 127.0.0.1` | DNS resolution + IP check | Only via TOCTOU (narrow window) | +| Internal ports (Redis, etc.) | URL with non-standard port | **Not protected** (FINDING-02) | **Yes** | +| CGNAT range (100.64.x.x) | URL or DNS | **Not in blocklist** (FINDING-03) | **Yes** | +| OAuth callback redirect | `/auth/callback` | Whitelist against registered URIs | No | +| File read via path | Local CSV path (stdio only) | Path validation + symlink resolve | No (HTTP mode rejects paths) | +| Redis key injection | Task IDs, user IDs | `build_key()` sanitization | No | +| Upload URL forgery | HMAC-SHA256 signature | `verify_upload_signature()` + expiry | No | + +--- + +## Recommendations Summary + +### Priority 1 (High Impact) + +1. **Fix DNS rebinding TOCTOU** (FINDING-01): Pin resolved IPs at validation time and connect directly to them, eliminating the window between validation and connection. + +### Priority 2 (Medium Impact) + +2. **Add port restrictions** (FINDING-02): Restrict outbound URL fetching to ports 80/443 (or a configurable allowlist). +3. **Expand IP blocklist** (FINDING-03): Add `100.64.0.0/10` (CGNAT), `198.18.0.0/15`, and `192.0.0.0/24`. +4. **Harden proxy IP trust** (FINDING-04): Validate the direct connection IP against a trusted proxy allowlist before reading forwarded headers. + +### Priority 3 (Low Impact / Defence-in-Depth) + +5. **Resolve relative redirects** (FINDING-05): Use `urljoin()` to resolve relative Location headers before validating. +6. **Normalize URLs with httpx** (FINDING-06): Use `httpx.URL()` for hostname extraction to match the HTTP client's parser. +7. **Add SSRF transport to auth client** (FINDING-07): Defence-in-depth for `EveryRowAuthProvider._http_client`. +8. **Add IPv6 unspecified address** (FINDING-08): Block `::` and `::ffff:0:0/96`. +9. **Container hardening** (FINDING-11): Add `pids_limit`, `tmpfs` size limit. + +--- + +## Testing Gaps + +The existing test suite (`tests/test_utils.py:221-295`) covers: +- Blocked IPs: localhost, 10.x, 172.16.x, 192.168.x, link-local, IPv6 loopback +- IPv4-mapped IPv6: loopback, private, metadata +- Public IP allowlisting +- DNS resolution failure (unresolvable hostname) +- URL target validation with mocked DNS + +**Missing test coverage:** + +| Test Case | Status | +|-----------|--------| +| DNS rebinding simulation (dual-answer DNS) | Missing | +| Non-standard port blocking | Missing (no protection exists) | +| CGNAT range (100.64.0.0/10) blocking | Missing (not in blocklist) | +| Relative redirect handling | Missing | +| URL parser discrepancy vectors | Missing | +| `_check_redirect` with absolute blocked redirect | Missing | +| `_SSRFSafeTransport` with blocked IP | Missing | +| Google Sheets URL normalization + SSRF | Missing | +| IPv6 unspecified address `::` | Missing | +| `file://`, `gopher://` scheme rejection | Missing | + +--- + +## Conclusion + +The SSRF protections introduced in commit `4000b88` represent a **well-engineered, defense-in-depth approach** that covers the major attack vectors. The three-layer architecture (pre-flight, transport, redirect) provides meaningful redundancy. The most significant residual risk is the DNS-rebinding TOCTOU window, which is narrow but theoretically exploitable. The missing port restrictions and incomplete IP blocklist are practical concerns for cloud deployments that should be addressed. No critical vulnerabilities that would allow reliable SSRF exploitation were found. From 82854c39d83aeb48de98d9dd4309d450cdc0ecee Mon Sep 17 00:00:00 2001 From: Rafael Poyiadzi Date: Wed, 25 Feb 2026 10:27:08 +0000 Subject: [PATCH 2/7] Fix SSRF: pin resolved IPs to close TOCTOU, add port allowlist FINDING-01 (High): Eliminate DNS rebinding TOCTOU by resolving DNS in _SSRFSafeTransport, validating all IPs, then pinning the TCP connection to the validated IP. Original hostname preserved in Host header and TLS SNI extension. FINDING-02 (Medium): Add _ALLOWED_PORTS allowlist {80, 443, 8080, 8443} enforced both pre-flight in _validate_url_target and at transport time. Non-standard ports (Redis 6379, Postgres 5432, SMTP 25, etc.) are now rejected. Also addresses code review feedback: repo-relative paths in report, qualified DNS rebinding PoC claims, TLS/SNI caveat documented, FINDING-06 downgraded to Info, scope limitations section added. Co-Authored-By: Claude Opus 4.6 --- SSRF_AUDIT_REPORT.md | 447 ++++++------------------- everyrow-mcp/src/everyrow_mcp/utils.py | 130 +++++-- everyrow-mcp/tests/test_utils.py | 103 ++++++ 3 files changed, 317 insertions(+), 363 deletions(-) diff --git a/SSRF_AUDIT_REPORT.md b/SSRF_AUDIT_REPORT.md index 6f23a7e0..eedb7be5 100644 --- a/SSRF_AUDIT_REPORT.md +++ b/SSRF_AUDIT_REPORT.md @@ -9,15 +9,27 @@ ## Executive Summary -The MCP server implements a **three-layer SSRF protection** around its sole user-controlled URL-fetching path (`fetch_csv_from_url`), introduced in commit `4000b88`. The protections are well-designed and cover the major attack vectors. No **Critical** vulnerabilities were found. The remaining risks are a narrow DNS-rebinding TOCTOU window that existing mitigations reduce but cannot fully close, missing port restrictions, and an incomplete IP blocklist. The auth subsystem and Redis infrastructure are not directly exploitable for SSRF. +The MCP server implements a **three-layer SSRF protection** around its sole user-controlled URL-fetching path (`fetch_csv_from_url`), introduced in commit `4000b88`. The protections are well-designed and cover the major attack vectors. No **Critical** vulnerabilities were found. The two highest-priority findings — DNS-rebinding TOCTOU (FINDING-01) and missing port restrictions (FINDING-02) — have been **fixed** in this PR. | Severity | Count | Status | |----------|-------|--------| | Critical | 0 | — | -| High | 1 | Residual risk (mitigated) | -| Medium | 3 | Actionable | -| Low | 5 | Hardening opportunities | -| Info | 3 | Defence-in-depth notes | +| High | 1 | **Fixed** (FINDING-01) | +| Medium | 3 | 1 **fixed** (FINDING-02), 2 open | +| Low | 4 | Hardening opportunities | +| Info | 4 | Defence-in-depth notes | + +--- + +## Scope Limitations + +This audit is based on **static analysis** of the source code. The following were not tested: + +- No runtime DNS rebinding was attempted against a live instance +- No live container escape testing +- No fuzzing of URL parser edge cases +- No review of the EveryRow SDK client code (only the MCP server) +- No penetration testing of the deployed Kubernetes infrastructure --- @@ -58,7 +70,7 @@ everyrow_upload_data(source=) ### Layer 1: Pre-flight DNS Validation -**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/utils.py:53-100` +**File:** `everyrow-mcp/src/everyrow_mcp/utils.py:53-100` Before any HTTP request, `_validate_url_target(url)` extracts the hostname via `urlparse` and calls `_validate_hostname()`, which: @@ -80,15 +92,15 @@ Before any HTTP request, `_validate_url_target(url)` extracts the hostname via ` | `fc00::/7` | IPv6 ULA | | `fe80::/10` | IPv6 link-local | -### Layer 2: Transport-Level Re-validation +### Layer 2: Transport-Level IP Pinning -**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/utils.py:168-185` +**File:** `everyrow-mcp/src/everyrow_mcp/utils.py:168-185` -Custom `_SSRFSafeTransport(httpx.AsyncBaseTransport)` wraps every outgoing request and calls `_validate_hostname(request.url.host)` immediately before the inner transport connects. This re-check narrows the DNS-rebinding TOCTOU window. +Custom `_SSRFSafeTransport(httpx.AsyncBaseTransport)` resolves DNS, validates the resolved IPs, and **pins the connection** to the validated IP. The original hostname is preserved in the `Host` header and TLS SNI extension. ### Layer 3: Redirect Chain Validation -**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/utils.py:152-165` +**File:** `everyrow-mcp/src/everyrow_mcp/utils.py:152-165` An httpx `event_hooks["response"]` hook validates every redirect `Location` header against `_validate_url_target()` before following. Redirects are capped at `max_redirects=5`. @@ -97,6 +109,7 @@ An httpx `event_hooks["response"]` hook validates every redirect `Location` head | Control | File:Line | Details | |---------|-----------|---------| | Scheme restriction | `utils.py:117` | Only `http://` and `https://` | +| Port allowlist | `utils.py:40-55` | Only 80, 443, 8080, 8443 | | Streaming size limit | `utils.py:219-224` | 50 MB default, aborts mid-stream | | Content-Length pre-check | `utils.py:212` | Rejects before streaming if header present | | IPv4-mapped IPv6 unwrap | `utils.py:48-49, 68-70` | `::ffff:127.0.0.1` → `127.0.0.1` | @@ -107,148 +120,94 @@ An httpx `event_hooks["response"]` hook validates every redirect `Location` head ## Findings -### FINDING-01: DNS Rebinding TOCTOU Window (Residual) +### FINDING-01: DNS Rebinding TOCTOU Window — FIXED **Severity:** High (residual risk after mitigation) -**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/utils.py:179-182` -**Status:** Partially mitigated by `_SSRFSafeTransport` +**File:** `everyrow-mcp/src/everyrow_mcp/utils.py:179-182` +**Status:** **Fixed** — transport now pins resolved IP **Description:** -The `_SSRFSafeTransport` re-validates hostnames at request time, but its `_validate_hostname()` call performs its own `socket.getaddrinfo()` lookup, which is a **separate DNS resolution** from what httpx's inner `AsyncHTTPTransport` (via `httpcore`) performs when opening the TCP connection. If DNS rebinds between the transport-level check (line 181) and the actual `connect()` inside `httpcore`, a fast-rebinding DNS server could succeed. +The original `_SSRFSafeTransport` re-validated hostnames at request time, but its `_validate_hostname()` call performed its own `socket.getaddrinfo()` lookup, which was a **separate DNS resolution** from what httpx's inner `AsyncHTTPTransport` (via `httpcore`) performed when opening the TCP connection. If DNS rebinds between the transport-level check and the actual `connect()` inside `httpcore`, a fast-rebinding DNS server could succeed. ``` -Timeline: +Original timeline (vulnerable): T0: _validate_hostname() → getaddrinfo() → returns 93.184.216.34 (public) ✓ T1: DNS rebinds hostname → 169.254.169.254 (metadata) T2: AsyncHTTPTransport → httpcore.connect() → getaddrinfo() → 169.254.169.254 T3: Connection established to cloud metadata service ``` -The window between T0 and T2 is extremely narrow (microseconds within the same async coroutine), making this attack probabilistic and unreliable, but **not zero**. +The window between T0 and T2 was extremely narrow (microseconds within the same async coroutine). Successful exploitation would require an attacker-controlled DNS server with very fast rebinding and is probabilistic — the actual success rate depends heavily on OS resolver caching behaviour and configured TTLs. -**Proof of Concept:** - -```python -# Attacker-controlled DNS server with fast rebinding -# First query returns 93.184.216.34 (passes validation) -# Immediate second query returns 169.254.169.254 (metadata) +**Fix Applied:** -# Attacker sends to MCP tool: -source = "http://rebind.attacker.com/csv" +The transport now uses `_resolve_and_validate()` to resolve DNS once, validate all IPs, then pins the connection to the validated IP by rewriting the request URL. The original hostname is preserved in the `Host` header and TLS SNI extension (addressing the TLS/SNI regression risk noted in code review): -# Race condition: ~1-5% success rate on fast networks -# If DNS rebinds between _SSRFSafeTransport check and httpcore connect, -# the request reaches the cloud metadata service. -``` +```python +# Resolve DNS and validate — returns the first safe IP +resolved_ip = _resolve_and_validate(hostname) -**Recommended Fix:** +# Pin the URL to the validated IP +pinned_url = request.url.copy_with(host=resolved_ip) -Pin the resolved IP at validation time and connect directly to it, passing the original hostname as the `Host` header. This eliminates the TOCTOU entirely: +# Preserve original hostname in Host header +headers = [...("host", hostname)...] -```python -class _SSRFSafeTransport(httpx.AsyncBaseTransport): - def __init__(self) -> None: - self._transport = httpx.AsyncHTTPTransport(retries=0) - - async def handle_async_request(self, request: httpx.Request) -> httpx.Response: - hostname = request.url.host - if not hostname: - return await self._transport.handle_async_request(request) - - # Resolve and validate once - resolved_ip = _resolve_and_validate(hostname) # new helper - - # Rewrite the request URL to use the pinned IP - pinned_url = request.url.copy_with(host=resolved_ip) - pinned_request = httpx.Request( - method=request.method, - url=pinned_url, - headers={**request.headers, "Host": hostname}, - content=request.content, - ) - return await self._transport.handle_async_request(pinned_request) +# Preserve original hostname for TLS SNI +extensions["sni_hostname"] = hostname.encode("ascii") ``` +This eliminates the TOCTOU entirely — `httpcore` connects directly to the validated IP without performing a second DNS lookup. + --- -### FINDING-02: No Port Restriction on Fetched URLs +### FINDING-02: No Port Restriction on Fetched URLs — FIXED **Severity:** Medium -**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/utils.py:53-100` +**File:** `everyrow-mcp/src/everyrow_mcp/utils.py:53-100` +**Status:** **Fixed** — port allowlist added **Description:** -The SSRF blocklist validates IP addresses but does not restrict ports. An attacker can probe internal services on non-standard ports even when the IP is public or allowed. This is particularly dangerous in containerized deployments where services bind to non-standard ports. +The SSRF blocklist validated IP addresses but did not restrict ports. An attacker could probe internal services on non-standard ports even when the IP is public or allowed. This is particularly dangerous in containerized deployments where services bind to non-standard ports. **Proof of Concept:** ```python # Probe Redis on its default port (if exposed on a public IP or shared network) source = "http://redis.internal:6379/" -# Redis responds with -ERR, but the connection is established -# and the response reveals service information - -# Probe internal HTTP services on non-standard ports -source = "http://monitoring.internal:9090/api/v1/targets" -# If monitoring.internal resolves to a non-blocked IP # SMTP banner grabbing source = "http://mail.company.com:25/" ``` -**Recommended Fix:** - -Add a port allowlist (default: 80, 443) or blocklist (common internal service ports): +**Fix Applied:** -```python -_BLOCKED_PORTS = { - 25, 465, 587, # SMTP - 6379, 6380, # Redis - 5432, # PostgreSQL - 3306, # MySQL - 27017, # MongoDB - 2379, 2380, # etcd - 9200, 9300, # Elasticsearch - 11211, # Memcached -} - -_ALLOWED_PORTS = {80, 443, 8080, 8443} # Alternative: allowlist approach - -def _validate_url_target(url: str) -> None: - parsed = urlparse(url) - port = parsed.port or (443 if parsed.scheme == "https" else 80) - if port not in _ALLOWED_PORTS: - raise ValueError(f"Port {port} is not permitted for URL fetching") - # ... existing hostname validation -``` +Added `_ALLOWED_PORTS = {80, 443, 8080, 8443}` and `_validate_port()`, enforced both in the pre-flight `_validate_url_target()` and at transport time in `_SSRFSafeTransport`. Non-allowed ports raise `ValueError`. --- -### FINDING-03: Incomplete IP Blocklist — Missing Cloud/RFC Ranges +### FINDING-03: Incomplete IP Blocklist — Missing CGNAT Range **Severity:** Medium -**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/utils.py:21-31` +**File:** `everyrow-mcp/src/everyrow_mcp/utils.py:21-31` **Description:** -The blocklist covers the major RFC 1918 ranges and cloud metadata endpoints but is missing several ranges that are reachable in cloud/container environments: +The blocklist covers the major RFC 1918 ranges and cloud metadata endpoints but is missing `100.64.0.0/10` (RFC 6598 CGNAT), which is used by AWS for VPC endpoints and by Tailscale for mesh networking. An attacker could reach internal VPC services via this range. -| Missing Network | RFC | Risk | -|----------------|-----|------| -| `100.64.0.0/10` | RFC 6598 (CGNAT) | Used in AWS VPCs, GKE node pools, Tailscale | -| `198.18.0.0/15` | RFC 2544 | Network testing; sometimes used internally | -| `192.0.0.0/24` | RFC 6890 | IANA special-purpose | -| `192.0.2.0/24` | RFC 5737 | TEST-NET-1 (documentation) | -| `198.51.100.0/24` | RFC 5737 | TEST-NET-2 (documentation) | -| `203.0.113.0/24` | RFC 5737 | TEST-NET-3 (documentation) | - -The most critical omission is `100.64.0.0/10` — this CGNAT range is used by AWS for VPC endpoints and by Tailscale for mesh networking. An attacker could reach internal VPC services via this range. +| Missing Network | RFC | Severity | +|----------------|-----|----------| +| `100.64.0.0/10` | RFC 6598 (CGNAT) | **Medium** — reachable in AWS VPCs, GKE, Tailscale | +| `198.18.0.0/15` | RFC 2544 | Low — benchmark testing, sometimes used internally | +| `192.0.0.0/24` | RFC 6890 | Info — IANA special-purpose | +| `192.0.2.0/24`, `198.51.100.0/24`, `203.0.113.0/24` | RFC 5737 | Info — TEST-NET (documentation only, non-routable) | **Proof of Concept:** ```python -# AWS VPC endpoint (CGNAT range, used by some services) +# AWS VPC endpoint (CGNAT range) source = "http://100.64.0.1/latest/meta-data/" # Tailscale node in mesh network @@ -271,7 +230,7 @@ _BLOCKED_NETWORKS = [ ### FINDING-04: Rate Limit Bypass via IP Header Spoofing **Severity:** Medium -**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/middleware.py:28-40` +**File:** `everyrow-mcp/src/everyrow_mcp/middleware.py:28-40` **Description:** @@ -285,346 +244,150 @@ if settings.trust_proxy_headers: return value.split(",")[0].strip() # Trusts first value ``` -This affects: -- Registration rate limiting (`middleware.py:43-91`) -- OAuth rate limiting (`auth.py:211-218`) -- All IP-based access controls - -**Proof of Concept:** - -```bash -# Bypass rate limit by rotating spoofed IPs -for i in $(seq 1 1000); do - curl -H "X-Forwarded-For: 1.2.3.$((i % 256))" \ - https://mcp.example.com/register -done -``` - **Current Mitigation:** `docker-compose.yaml:42` defaults `TRUST_PROXY_HEADERS=false`. But Kubernetes/GKE deployments typically set this to `true`. **Recommended Fix:** 1. Document that the reverse proxy MUST overwrite (not append to) the trusted IP header -2. Consider validating that the first IP in `X-Forwarded-For` is not from a private range -3. Add the proxy's own IP to a `TRUSTED_PROXIES` allowlist and only read the header when the direct connection comes from a trusted proxy: - -```python -def get_client_ip(request: Request) -> str | None: - if settings.trust_proxy_headers: - direct_ip = request.client.host if request.client else None - if direct_ip and direct_ip in settings.trusted_proxy_ips: - value = request.headers.get(settings.trusted_ip_header.lower()) - if value: - return value.split(",")[0].strip() - return request.client.host if request.client else None -``` +2. Add the proxy's own IP to a `TRUSTED_PROXIES` allowlist and only read the header when the direct connection comes from a trusted proxy --- ### FINDING-05: Relative Redirect Blocking (False Positive / Fail-Safe) **Severity:** Low -**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/utils.py:152-165` +**File:** `everyrow-mcp/src/everyrow_mcp/utils.py:152-165` **Description:** The `_check_redirect` event hook validates the raw `Location` header from redirect responses. For relative redirects (e.g., `Location: /path`), `urlparse` returns `hostname=None`, causing `_validate_url_target` to raise `"URL has no hostname"`. This blocks the redirect chain. -From a security perspective, this is **fail-safe** — relative redirects stay on the same (already validated) host. However, it may cause false positives if a legitimate public server (e.g., Google Sheets) returns a relative redirect. - -```python -# utils.py:155-158 -location = response.headers.get("location", "") -if location: - try: - _validate_url_target(location) # Fails for relative URLs -``` +From a security perspective, this is **fail-safe** — relative redirects stay on the same (already validated) host. However, it may cause false positives if a legitimate public server returns a relative redirect. **Recommended Fix:** -Resolve relative redirects against the request URL before validating: - -```python -async def _check_redirect(response: httpx.Response) -> None: - if response.is_redirect: - location = response.headers.get("location", "") - if location: - # Resolve relative redirects against the request URL - from urllib.parse import urljoin - resolved = urljoin(str(response.request.url), location) - try: - _validate_url_target(resolved) - except ValueError: - raise httpx.TooManyRedirects( - f"Redirect to blocked address: {resolved}", - request=response.request, - ) -``` +Resolve relative redirects against the request URL before validating using `urljoin()`. --- ### FINDING-06: URL Parser Discrepancy (urlparse vs httpx) -**Severity:** Low -**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/utils.py:90-100` +**Severity:** Info +**File:** `everyrow-mcp/src/everyrow_mcp/utils.py:90-100` **Description:** -The pre-flight validation uses Python's `urlparse` to extract the hostname, while httpx uses its own URL parser (via `httpcore`). Parser discrepancies could theoretically allow an attacker to craft a URL where `urlparse` extracts a public hostname (passes validation) while httpx connects to a different (internal) host. - -Known divergence vectors: -- **Backslash normalization:** `http://public.com\@127.0.0.1/` — `urlparse` may treat the path differently than httpx -- **Unicode hostnames:** `http://ⓔⓧⓐⓜⓟⓛⓔ.com/` — IDNA encoding differences -- **Percent-encoded authority:** `http://%31%32%37.%30.%30.%31/` — may decode to `127.0.0.1` differently - -**Mitigating Factor:** The `_SSRFSafeTransport` (Layer 2) re-validates using `request.url.host`, which is httpx's own parsed view. This provides defense-in-depth against parser discrepancies — even if the pre-flight check is fooled, the transport-level check uses the same parser that will make the connection. - -**Proof of Concept:** - -```python -# Theoretical — most of these are blocked by one layer or another - -# Backslash confusion (depends on Python/httpx version) -source = "http://public.com\\@127.0.0.1/" - -# Percent-encoded IP (urlparse decodes differently than httpx) -source = "http://%31%32%37%2e%30%2e%30%2e%31/" -``` - -**Recommended Fix:** +The pre-flight validation uses Python's `urlparse` to extract the hostname, while httpx uses its own URL parser (via `httpcore`). Parser discrepancies could theoretically allow an attacker to craft a URL where `urlparse` extracts a different hostname than httpx. -Add explicit normalization before validation to match httpx's behavior: - -```python -def _validate_url_target(url: str) -> None: - # Normalize to match httpx's parser - try: - httpx_url = httpx.URL(url) - hostname = httpx_url.host - except httpx.InvalidURL: - raise ValueError(f"Invalid URL: {url}") - if not hostname: - raise ValueError(f"URL has no hostname: {url}") - _validate_hostname(hostname) -``` +**Mitigating Factor:** The `_SSRFSafeTransport` (Layer 2) re-validates using `request.url.host`, which is httpx's own parsed view, and then pins the connection to the resolved IP. This means the defence-in-depth is already working as designed — even if the pre-flight check (Layer 1) is fooled by a parser discrepancy, the transport-level check (Layer 2) uses the same parser that determines where the connection actually goes. The pre-flight check is purely an early-rejection optimisation. --- ### FINDING-07: Auth httpx Client Without SSRF Transport **Severity:** Low -**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/auth.py:199-202` +**File:** `everyrow-mcp/src/everyrow_mcp/auth.py:199-202` **Description:** -The `EveryRowAuthProvider.__init__` creates an `httpx.AsyncClient()` without `_SSRFSafeTransport`. This client is used for: -- Supabase token exchange (`_supabase_token_request`, line 560) -- JWKS fetching (via `PyJWKClient`, line 53) - -The target URL is derived from `settings.supabase_url`, which is: -- A config value (environment variable), not user-controlled -- Validated at startup to require HTTPS for non-localhost (`config.py:134-147`) +The `EveryRowAuthProvider.__init__` creates an `httpx.AsyncClient()` without `_SSRFSafeTransport`. The target URL is derived from `settings.supabase_url`, which is a config value validated at startup to require HTTPS for non-localhost (`config.py:134-147`). -**Not exploitable** unless the attacker controls the server's environment variables, which would give them full compromise regardless. +**Not exploitable** unless the attacker controls the server's environment variables. -```python -# auth.py:199-202 -self._http_client = httpx.AsyncClient( - timeout=httpx.Timeout(10.0), - limits=httpx.Limits(max_connections=20, max_keepalive_connections=10), -) -``` - -**Recommended Fix (defence-in-depth):** - -Add the SSRF transport as a precaution: - -```python -from everyrow_mcp.utils import _SSRFSafeTransport - -self._http_client = httpx.AsyncClient( - transport=_SSRFSafeTransport(), - timeout=httpx.Timeout(10.0), - limits=httpx.Limits(max_connections=20, max_keepalive_connections=10), -) -``` +**Recommended Fix (defence-in-depth):** Add the SSRF transport as a precaution. --- ### FINDING-08: Missing IPv6 Addresses in Blocklist **Severity:** Low -**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/utils.py:21-31` +**File:** `everyrow-mcp/src/everyrow_mcp/utils.py:21-31` **Description:** -The blocklist does not include: -- `::` (IPv6 unspecified address) — this is NOT the same as `::1` (loopback) and is NOT an IPv4-mapped address, so `ipv4_mapped` unwrapping does not cover it -- `::ffff:0:0/96` (IPv4-mapped prefix explicitly) — individual addresses are unwrapped, but the prefix itself is not blocked - -The practical risk is minimal since `::` does not route to any reachable host in most environments. - -**Recommended Fix:** +The blocklist does not include `::` (IPv6 unspecified address) or `::ffff:0:0/96` (IPv4-mapped prefix). The practical risk is minimal since `::` does not route to any reachable host in most environments. -```python -_BLOCKED_NETWORKS = [ - # ... existing entries ... - ipaddress.ip_network("::/128"), # IPv6 unspecified - ipaddress.ip_network("::ffff:0:0/96"), # IPv4-mapped prefix (belt-and-suspenders) -] -``` +**Recommended Fix:** Add `::/128` and `::ffff:0:0/96` to the blocklist. --- ### FINDING-09: `_decode_trusted_server_jwt` Skips Signature Verification **Severity:** Info (by design) -**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/auth.py:152-161` +**File:** `everyrow-mcp/src/everyrow_mcp/auth.py:152-161` **Description:** -This function decodes JWTs with `verify_signature=False`. It is documented as for server-to-server tokens received from Supabase over HTTPS, and the docstring warns: - -> "NEVER use this for tokens received from end users." - -It is only called from `_issue_token_response` (line 456) with tokens obtained directly from Supabase's token endpoint over HTTPS. The caller chain is: - -``` -handle_callback → _validate_callback_request → _validate_supabase_code - → _exchange_supabase_code → _supabase_token_request - → POST {supabase_url}/auth/v1/token (HTTPS, server-to-server) - → response.access_token → _decode_trusted_server_jwt() -``` - -**Assessment:** Safe by current usage. The risk would arise if this function were called with user-supplied tokens in the future. The docstring warning is appropriate. +This function decodes JWTs with `verify_signature=False`. It is only called from `_issue_token_response` (line 456) with tokens obtained directly from Supabase's token endpoint over HTTPS. The critical guarantee that makes this safe is the HTTPS enforcement on `supabase_url` via the `_validate_url` validator in `config.py:134-147` — the token is received over a TLS-authenticated channel, making it trustworthy without a separate signature check. -**Recommended Fix:** Add a comment at the call site (line 456) reinforcing that the token source must remain trusted. +**Assessment:** Safe by current usage. The docstring warning ("NEVER use this for tokens received from end users") is appropriate. --- ### FINDING-10: Wildcard CORS on Widget Endpoints **Severity:** Info (safe by design) -**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/src/everyrow_mcp/routes.py:22-33` +**File:** `everyrow-mcp/src/everyrow_mcp/routes.py:22-33` **Description:** -Widget endpoints use `Access-Control-Allow-Origin: *`. Per the CORS specification, browsers do not send credentials (cookies) with wildcard-origin requests. Since auth is via Bearer tokens (not cookies), this is safe — no ambient credentials are leaked. - -The code includes a clear documentation comment explaining this design decision. - -**Assessment:** Correct. No action needed. +Widget endpoints use `Access-Control-Allow-Origin: *`. Since auth is via Bearer tokens (not cookies), this is safe per the CORS specification — no ambient credentials are leaked. --- ### FINDING-11: Container Hardening Review **Severity:** Info -**File:** `/Users/rafaelpoyiadzi/Documents/git/everyrow-sdk/worktrees/audit-ssrf/everyrow-mcp/deploy/docker-compose.yaml` +**File:** `everyrow-mcp/deploy/docker-compose.yaml` **Description:** -The container configuration follows security best practices: - -| Control | Status | Location | -|---------|--------|----------| -| Non-root user | Present | `Dockerfile:19` (`mcp` user) | -| `no-new-privileges` | Present | `docker-compose.yaml:50` | -| `cap_drop: ALL` | Present | `docker-compose.yaml:51-52` | -| Read-only rootfs | Present | `docker-compose.yaml:53` | -| Memory limits | Present | `docker-compose.yaml:46-47` (512M) | -| CPU limits | Present | `docker-compose.yaml:48` (1 CPU) | -| Network isolation | Present | `docker-compose.yaml:60-62` (bridge) | -| Redis password required | Present | `docker-compose.yaml:4` | -| Redis not exposed to host | Present | No `ports:` on Redis service | -| MCP bound to localhost | Present | `docker-compose.yaml:29` (`127.0.0.1:8000`) | - -**Missing (minor):** -- No `pids_limit` set (prevents fork bomb DoS) -- No `tmpfs` size limit (line 55: `tmpfs: - /tmp` has no size cap) -- Consider `PYTHONHASHSEED=random` for hash collision DoS resistance - -**Recommended Fix:** +The container configuration follows security best practices (non-root user, `no-new-privileges`, `cap_drop: ALL`, read-only rootfs, memory/CPU limits, network isolation, Redis password required, Redis not exposed to host, MCP bound to localhost). -```yaml -mcp-server: - # ... existing config ... - pids_limit: 100 - tmpfs: - - /tmp:size=50M - environment: - PYTHONHASHSEED: "random" -``` +**Missing (minor):** No `pids_limit` (fork bomb DoS), no `tmpfs` size limit. --- ## Attack Surface Matrix -| Attack Vector | Entry Point | Protection | Bypass Possible? | -|---------------|-------------|------------|-----------------| -| Direct SSRF via URL | `everyrow_upload_data(source=url)` | 3-layer validation | Only via DNS rebinding TOCTOU (FINDING-01) | -| SSRF via redirect | HTTP 3xx from attacker server | Event hook + transport re-check | Relative redirects blocked (fail-safe, FINDING-05) | -| Cloud metadata (169.254.169.254) | URL or DNS rebind | IP blocklist + hostname block | No (blocked in all 3 layers) | -| GKE metadata hostname | `metadata.google.internal` | Hostname blocklist | No | -| IPv4-mapped IPv6 bypass | `::ffff:127.0.0.1` | Unwrap + re-check | No | -| Localhost via DNS | `attacker.com → 127.0.0.1` | DNS resolution + IP check | Only via TOCTOU (narrow window) | -| Internal ports (Redis, etc.) | URL with non-standard port | **Not protected** (FINDING-02) | **Yes** | -| CGNAT range (100.64.x.x) | URL or DNS | **Not in blocklist** (FINDING-03) | **Yes** | -| OAuth callback redirect | `/auth/callback` | Whitelist against registered URIs | No | -| File read via path | Local CSV path (stdio only) | Path validation + symlink resolve | No (HTTP mode rejects paths) | -| Redis key injection | Task IDs, user IDs | `build_key()` sanitization | No | -| Upload URL forgery | HMAC-SHA256 signature | `verify_upload_signature()` + expiry | No | +| Attack Vector | Entry Point | Protection | Bypass? | Remediation | +|---------------|-------------|------------|---------|-------------| +| Direct SSRF via URL | `everyrow_upload_data(source=url)` | 3-layer validation + IP pinning | No (FINDING-01 fixed) | **Done** | +| Internal port probing | URL with non-standard port | Port allowlist (80, 443, 8080, 8443) | No (FINDING-02 fixed) | **Done** | +| SSRF via redirect | HTTP 3xx from attacker server | Event hook + transport re-check | Relative redirects blocked (fail-safe) | Open (Low) | +| Cloud metadata (169.254.169.254) | URL or DNS rebind | IP blocklist + hostname block | No | N/A | +| GKE metadata hostname | `metadata.google.internal` | Hostname blocklist | No | N/A | +| IPv4-mapped IPv6 bypass | `::ffff:127.0.0.1` | Unwrap + re-check | No | N/A | +| CGNAT range (100.64.x.x) | URL or DNS | **Not in blocklist** (FINDING-03) | **Yes** | Open (Medium) | +| Rate limit bypass | Spoofed X-Forwarded-For | Proxy trust config | Conditional (FINDING-04) | Open (Medium) | +| OAuth callback redirect | `/auth/callback` | Whitelist against registered URIs | No | N/A | +| File read via path | Local CSV path (stdio only) | Path validation + symlink resolve | No (HTTP mode rejects) | N/A | +| Redis key injection | Task IDs, user IDs | `build_key()` sanitization | No | N/A | +| Upload URL forgery | HMAC-SHA256 signature | `verify_upload_signature()` + expiry | No | N/A | --- ## Recommendations Summary -### Priority 1 (High Impact) +### Done (This PR) -1. **Fix DNS rebinding TOCTOU** (FINDING-01): Pin resolved IPs at validation time and connect directly to them, eliminating the window between validation and connection. +1. **~~Fix DNS rebinding TOCTOU~~** (FINDING-01): Transport now pins resolved IPs — TOCTOU eliminated. +2. **~~Add port restrictions~~** (FINDING-02): Port allowlist `{80, 443, 8080, 8443}` enforced pre-flight and at transport time. -### Priority 2 (Medium Impact) +### Priority 2 (Medium Impact — Open) -2. **Add port restrictions** (FINDING-02): Restrict outbound URL fetching to ports 80/443 (or a configurable allowlist). 3. **Expand IP blocklist** (FINDING-03): Add `100.64.0.0/10` (CGNAT), `198.18.0.0/15`, and `192.0.0.0/24`. 4. **Harden proxy IP trust** (FINDING-04): Validate the direct connection IP against a trusted proxy allowlist before reading forwarded headers. -### Priority 3 (Low Impact / Defence-in-Depth) +### Priority 3 (Low Impact / Defence-in-Depth — Open) 5. **Resolve relative redirects** (FINDING-05): Use `urljoin()` to resolve relative Location headers before validating. -6. **Normalize URLs with httpx** (FINDING-06): Use `httpx.URL()` for hostname extraction to match the HTTP client's parser. -7. **Add SSRF transport to auth client** (FINDING-07): Defence-in-depth for `EveryRowAuthProvider._http_client`. -8. **Add IPv6 unspecified address** (FINDING-08): Block `::` and `::ffff:0:0/96`. -9. **Container hardening** (FINDING-11): Add `pids_limit`, `tmpfs` size limit. - ---- - -## Testing Gaps - -The existing test suite (`tests/test_utils.py:221-295`) covers: -- Blocked IPs: localhost, 10.x, 172.16.x, 192.168.x, link-local, IPv6 loopback -- IPv4-mapped IPv6: loopback, private, metadata -- Public IP allowlisting -- DNS resolution failure (unresolvable hostname) -- URL target validation with mocked DNS - -**Missing test coverage:** - -| Test Case | Status | -|-----------|--------| -| DNS rebinding simulation (dual-answer DNS) | Missing | -| Non-standard port blocking | Missing (no protection exists) | -| CGNAT range (100.64.0.0/10) blocking | Missing (not in blocklist) | -| Relative redirect handling | Missing | -| URL parser discrepancy vectors | Missing | -| `_check_redirect` with absolute blocked redirect | Missing | -| `_SSRFSafeTransport` with blocked IP | Missing | -| Google Sheets URL normalization + SSRF | Missing | -| IPv6 unspecified address `::` | Missing | -| `file://`, `gopher://` scheme rejection | Missing | +6. **Add SSRF transport to auth client** (FINDING-07): Defence-in-depth for `EveryRowAuthProvider._http_client`. +7. **Add IPv6 unspecified address** (FINDING-08): Block `::` and `::ffff:0:0/96`. +8. **Container hardening** (FINDING-11): Add `pids_limit`, `tmpfs` size limit. --- ## Conclusion -The SSRF protections introduced in commit `4000b88` represent a **well-engineered, defense-in-depth approach** that covers the major attack vectors. The three-layer architecture (pre-flight, transport, redirect) provides meaningful redundancy. The most significant residual risk is the DNS-rebinding TOCTOU window, which is narrow but theoretically exploitable. The missing port restrictions and incomplete IP blocklist are practical concerns for cloud deployments that should be addressed. No critical vulnerabilities that would allow reliable SSRF exploitation were found. +The SSRF protections introduced in commit `4000b88` represent a **well-engineered, defense-in-depth approach** that covers the major attack vectors. The three-layer architecture (pre-flight, transport, redirect) provides meaningful redundancy. The two highest-priority findings have been fixed in this PR: the DNS-rebinding TOCTOU is eliminated via IP pinning, and non-standard ports are now blocked via an allowlist. The remaining open items (CGNAT blocklist, proxy IP trust) are medium-priority hardening improvements. No critical vulnerabilities that would allow reliable SSRF exploitation were found. diff --git a/everyrow-mcp/src/everyrow_mcp/utils.py b/everyrow-mcp/src/everyrow_mcp/utils.py index d0a5573e..a97946b5 100644 --- a/everyrow-mcp/src/everyrow_mcp/utils.py +++ b/everyrow-mcp/src/everyrow_mcp/utils.py @@ -37,6 +37,24 @@ } ) +# FINDING-02: restrict outbound fetches to standard HTTP(S) ports. +_ALLOWED_PORTS: frozenset[int] = frozenset({80, 443, 8080, 8443}) + + +def _validate_port(port: int | None) -> None: + """Reject non-standard ports for outbound URL fetching. + + Default ports (omitted from the URL) are always allowed. + Explicit ports must be in the ``_ALLOWED_PORTS`` allowlist. + """ + if port is None: + return # Default port for the scheme — always allowed + if port not in _ALLOWED_PORTS: + raise ValueError( + f"Port {port} is not permitted for URL fetching. " + f"Allowed: {sorted(_ALLOWED_PORTS)}" + ) + def _is_blocked_ip(addr: str) -> bool: """Check if an IP address falls within a blocked private/internal network.""" @@ -50,30 +68,39 @@ def _is_blocked_ip(addr: str) -> bool: return any(ip in net for net in _BLOCKED_NETWORKS) -def _validate_hostname(hostname: str) -> None: - """Validate that a hostname doesn't resolve to blocked IPs or metadata services. +def _resolve_and_validate(hostname: str) -> str: + """Resolve a hostname, validate all IPs, and return the first safe IP. + + For IP literals, validates directly and returns the canonical form. + For DNS names, resolves via ``getaddrinfo`` and checks every result. - Called both as a pre-flight check and at transport request time to close - the TOCTOU gap between DNS validation and HTTP connection. + The returned IP is used by ``_SSRFSafeTransport`` to **pin** the TCP + connection, eliminating the TOCTOU gap between DNS validation and the + actual ``connect()`` call (FINDING-01). Raises: - ValueError: If the hostname is blocked, resolves to a blocked IP, or cannot be resolved. + ValueError: If the hostname is blocked, resolves to a blocked IP, + or cannot be resolved. """ if hostname.lower() in _BLOCKED_HOSTNAMES: raise ValueError(f"Hostname is not permitted: {hostname}") # Direct IP literal — validate without DNS resolution + parsed_ip = None try: - ip = ipaddress.ip_address(hostname) + parsed_ip = ipaddress.ip_address(hostname) + except ValueError: + pass # Not an IP literal — fall through to DNS + + if parsed_ip is not None: # Unwrap IPv4-mapped IPv6 (e.g. ::ffff:127.0.0.1 → 127.0.0.1) - if isinstance(ip, ipaddress.IPv6Address) and ip.ipv4_mapped: - ip = ip.ipv4_mapped - if any(ip in net for net in _BLOCKED_NETWORKS): + if isinstance(parsed_ip, ipaddress.IPv6Address) and parsed_ip.ipv4_mapped: + parsed_ip = parsed_ip.ipv4_mapped + if any(parsed_ip in net for net in _BLOCKED_NETWORKS): raise ValueError(f"Connection to blocked IP: {hostname}") - return - except ValueError: - pass # Not an IP literal, resolve via DNS + return str(parsed_ip) + # DNS name — resolve and validate all addresses try: addrinfos = socket.getaddrinfo( hostname, None, socket.AF_UNSPEC, socket.SOCK_STREAM @@ -81,22 +108,39 @@ def _validate_hostname(hostname: str) -> None: except socket.gaierror: raise ValueError(f"Could not resolve hostname: {hostname}") + if not addrinfos: + raise ValueError(f"Could not resolve hostname: {hostname}") + for _, _, _, _, sockaddr in addrinfos: if _is_blocked_ip(sockaddr[0]): logger.warning("SSRF blocked: %s resolved to %s", hostname, sockaddr[0]) raise ValueError(f"URL target is not permitted: {hostname}") + # All addresses safe — return the first resolved IP for connection pinning + return addrinfos[0][4][0] + + +def _validate_hostname(hostname: str) -> None: + """Validate that a hostname doesn't resolve to blocked IPs or metadata services. + + Thin wrapper around ``_resolve_and_validate`` for callers that only + need the validation side-effect and not the resolved IP. + """ + _resolve_and_validate(hostname) + def _validate_url_target(url: str) -> None: - """Resolve a URL's hostname and reject if any resolved IP is internal. + """Resolve a URL's hostname and reject if any resolved IP is internal or port is blocked. Raises: - ValueError: If the hostname resolves to a blocked network or cannot be resolved. + ValueError: If the hostname resolves to a blocked network, port is not + in the allowlist, or hostname cannot be resolved. """ parsed = urlparse(url) hostname = parsed.hostname if not hostname: raise ValueError(f"URL has no hostname: {url}") + _validate_port(parsed.port) _validate_hostname(hostname) @@ -166,20 +210,64 @@ async def _check_redirect(response: httpx.Response) -> None: class _SSRFSafeTransport(httpx.AsyncBaseTransport): - """Transport that re-validates hostnames at request time. + """Transport that resolves DNS, validates IPs, and pins connections to safe IPs. - Narrows the TOCTOU window between DNS validation and connection to - near-zero by re-checking every hostname immediately before the inner - transport opens a TCP connection. + Eliminates the TOCTOU gap between DNS validation and TCP connection + (FINDING-01) by: + + 1. Resolving the hostname ourselves via ``getaddrinfo`` + 2. Validating every resolved IP against the blocklist + 3. Rewriting the request URL to connect directly to the validated IP + 4. Preserving the original hostname in the ``Host`` header and TLS SNI + extension so the remote server sees the correct virtual host + + Also enforces the port allowlist (FINDING-02) at transport time as a + second check complementing the pre-flight validation. """ def __init__(self) -> None: self._transport = httpx.AsyncHTTPTransport(retries=0) async def handle_async_request(self, request: httpx.Request) -> httpx.Response: - if request.url.host: - _validate_hostname(request.url.host) - return await self._transport.handle_async_request(request) + hostname = request.url.host + if not hostname: + return await self._transport.handle_async_request(request) + + # Validate port (defence-in-depth — also checked pre-flight) + _validate_port(request.url.port) + + # Resolve DNS and validate — returns the first safe IP + resolved_ip = _resolve_and_validate(hostname) + + # Pin the URL to the validated IP so the inner transport connects + # directly without a second (unvalidated) DNS lookup. + pinned_url = request.url.copy_with(host=resolved_ip) + + # Preserve the original hostname in the Host header + host_header = hostname + if request.url.port and request.url.port not in (80, 443): + host_header = f"{hostname}:{request.url.port}" + headers = [ + (name, value) + for name, value in request.headers.items() + if name.lower() != "host" + ] + headers.insert(0, ("host", host_header)) + + # Preserve the original hostname for TLS SNI so the server + # presents the right certificate (reviewer feedback on FINDING-01). + extensions = dict(request.extensions) + if request.url.scheme == "https": + extensions["sni_hostname"] = hostname.encode("ascii") + + pinned_request = httpx.Request( + method=request.method, + url=pinned_url, + headers=headers, + stream=request.stream, + extensions=extensions, + ) + return await self._transport.handle_async_request(pinned_request) async def aclose(self) -> None: await self._transport.aclose() diff --git a/everyrow-mcp/tests/test_utils.py b/everyrow-mcp/tests/test_utils.py index 47cc3e21..99cb0f7c 100644 --- a/everyrow-mcp/tests/test_utils.py +++ b/everyrow-mcp/tests/test_utils.py @@ -8,8 +8,11 @@ import pytest from everyrow_mcp.utils import ( + _ALLOWED_PORTS, _is_blocked_ip, _normalise_google_sheets_url, + _resolve_and_validate, + _validate_port, _validate_url_target, is_url, resolve_output_path, @@ -293,3 +296,103 @@ def test_validate_url_target_blocks_unresolvable(self): ): with pytest.raises(ValueError, match="Could not resolve"): _validate_url_target("http://nonexistent.invalid/data") + + +# ── Port restriction tests (FINDING-02) ────────────────────── + + +class TestPortRestriction: + """Tests for port allowlist in URL validation.""" + + def test_default_port_allowed(self): + """None (default port) is always allowed.""" + _validate_port(None) + + def test_standard_ports_allowed(self): + for port in sorted(_ALLOWED_PORTS): + _validate_port(port) + + def test_redis_port_blocked(self): + with pytest.raises(ValueError, match="not permitted"): + _validate_port(6379) + + def test_postgres_port_blocked(self): + with pytest.raises(ValueError, match="not permitted"): + _validate_port(5432) + + def test_smtp_port_blocked(self): + with pytest.raises(ValueError, match="not permitted"): + _validate_port(25) + + def test_arbitrary_high_port_blocked(self): + with pytest.raises(ValueError, match="not permitted"): + _validate_port(9090) + + def test_validate_url_target_blocks_redis_port(self): + with patch( + "everyrow_mcp.utils.socket.getaddrinfo", + return_value=_mock_resolve("example.com", "93.184.216.34"), + ): + with pytest.raises(ValueError, match="not permitted"): + _validate_url_target("http://example.com:6379/data") + + def test_validate_url_target_allows_port_443(self): + with patch( + "everyrow_mcp.utils.socket.getaddrinfo", + return_value=_mock_resolve("example.com", "93.184.216.34"), + ): + _validate_url_target("https://example.com:443/data.csv") + + def test_validate_url_target_allows_port_8080(self): + with patch( + "everyrow_mcp.utils.socket.getaddrinfo", + return_value=_mock_resolve("example.com", "93.184.216.34"), + ): + _validate_url_target("http://example.com:8080/data.csv") + + +# ── DNS-pinning tests (FINDING-01) ─────────────────────────── + + +class TestResolveAndValidate: + """Tests for _resolve_and_validate (IP pinning).""" + + def test_returns_ip_for_public_hostname(self): + with patch( + "everyrow_mcp.utils.socket.getaddrinfo", + return_value=_mock_resolve("example.com", "93.184.216.34"), + ): + ip = _resolve_and_validate("example.com") + assert ip == "93.184.216.34" + + def test_returns_ip_literal_directly(self): + ip = _resolve_and_validate("8.8.8.8") + assert ip == "8.8.8.8" + + def test_blocks_private_ip_literal(self): + with pytest.raises(ValueError, match="blocked IP"): + _resolve_and_validate("127.0.0.1") + + def test_blocks_metadata_hostname(self): + with pytest.raises(ValueError, match="not permitted"): + _resolve_and_validate("metadata.google.internal") + + def test_blocks_hostname_resolving_to_private(self): + with patch( + "everyrow_mcp.utils.socket.getaddrinfo", + return_value=_mock_resolve("evil.com", "10.0.0.1"), + ): + with pytest.raises(ValueError, match="not permitted"): + _resolve_and_validate("evil.com") + + def test_blocks_unresolvable(self): + with patch( + "everyrow_mcp.utils.socket.getaddrinfo", + side_effect=socket.gaierror("Name resolution failed"), + ): + with pytest.raises(ValueError, match="Could not resolve"): + _resolve_and_validate("nonexistent.invalid") + + def test_unwraps_ipv4_mapped_ipv6(self): + with pytest.raises(ValueError, match="blocked IP"): + _resolve_and_validate("::ffff:127.0.0.1") From e0e4b1c207b86295c972f4d1013dd5cfd98ffac9 Mon Sep 17 00:00:00 2001 From: Rafael Poyiadzi Date: Wed, 25 Feb 2026 10:42:26 +0000 Subject: [PATCH 3/7] Remove dead _validate_hostname wrapper Inline _resolve_and_validate directly into _validate_url_target. The wrapper added no value after the FINDING-01 refactor. Co-Authored-By: Claude Opus 4.6 --- everyrow-mcp/src/everyrow_mcp/utils.py | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/everyrow-mcp/src/everyrow_mcp/utils.py b/everyrow-mcp/src/everyrow_mcp/utils.py index a97946b5..6be672e6 100644 --- a/everyrow-mcp/src/everyrow_mcp/utils.py +++ b/everyrow-mcp/src/everyrow_mcp/utils.py @@ -120,15 +120,6 @@ def _resolve_and_validate(hostname: str) -> str: return addrinfos[0][4][0] -def _validate_hostname(hostname: str) -> None: - """Validate that a hostname doesn't resolve to blocked IPs or metadata services. - - Thin wrapper around ``_resolve_and_validate`` for callers that only - need the validation side-effect and not the resolved IP. - """ - _resolve_and_validate(hostname) - - def _validate_url_target(url: str) -> None: """Resolve a URL's hostname and reject if any resolved IP is internal or port is blocked. @@ -141,7 +132,7 @@ def _validate_url_target(url: str) -> None: if not hostname: raise ValueError(f"URL has no hostname: {url}") _validate_port(parsed.port) - _validate_hostname(hostname) + _resolve_and_validate(hostname) def is_url(value: str) -> bool: From 4c3807f61254fe11a564e500605089bc66e69e84 Mon Sep 17 00:00:00 2001 From: Rafael Poyiadzi Date: Wed, 25 Feb 2026 10:44:44 +0000 Subject: [PATCH 4/7] Remove SSRF audit report from PR The report is tracked separately; this PR focuses on the code fixes. Co-Authored-By: Claude Opus 4.6 --- SSRF_AUDIT_REPORT.md | 393 ------------------------------------------- 1 file changed, 393 deletions(-) delete mode 100644 SSRF_AUDIT_REPORT.md diff --git a/SSRF_AUDIT_REPORT.md b/SSRF_AUDIT_REPORT.md deleted file mode 100644 index eedb7be5..00000000 --- a/SSRF_AUDIT_REPORT.md +++ /dev/null @@ -1,393 +0,0 @@ -# SSRF Security Audit Report - -**Target:** EveryRow MCP Server (`everyrow-mcp/src/everyrow_mcp/`) -**Audit Date:** 2026-02-25 -**Scope:** Full SSRF attack surface analysis, bypass testing of existing protections, container/deployment review -**Baseline:** Commit `4000b88` ("Security hardening: SSRF, headers, Redis TLS, container lockdown") - ---- - -## Executive Summary - -The MCP server implements a **three-layer SSRF protection** around its sole user-controlled URL-fetching path (`fetch_csv_from_url`), introduced in commit `4000b88`. The protections are well-designed and cover the major attack vectors. No **Critical** vulnerabilities were found. The two highest-priority findings — DNS-rebinding TOCTOU (FINDING-01) and missing port restrictions (FINDING-02) — have been **fixed** in this PR. - -| Severity | Count | Status | -|----------|-------|--------| -| Critical | 0 | — | -| High | 1 | **Fixed** (FINDING-01) | -| Medium | 3 | 1 **fixed** (FINDING-02), 2 open | -| Low | 4 | Hardening opportunities | -| Info | 4 | Defence-in-depth notes | - ---- - -## Scope Limitations - -This audit is based on **static analysis** of the source code. The following were not tested: - -- No runtime DNS rebinding was attempted against a live instance -- No live container escape testing -- No fuzzing of URL parser edge cases -- No review of the EveryRow SDK client code (only the MCP server) -- No penetration testing of the deployed Kubernetes infrastructure - ---- - -## Architecture Overview - -### SSRF Attack Surface - -The server has **two transport modes** with different trust boundaries: - -| Mode | User-controlled URLs? | File system access? | Auth required? | -|------|----------------------|---------------------|---------------| -| stdio | Yes (URL + local path) | Yes (local CSV read/write) | No (API key) | -| HTTP | Yes (URL only) | No (blocked by validator) | Yes (OAuth 2.1) | - -**Single user-controlled URL entry point:** - -``` -everyrow_upload_data(source=) - → UploadDataInput.validate_source() # scheme check - → validate_url() # http/https only - → fetch_csv_from_url(url) # 3-layer SSRF protection -``` - -**Outbound HTTP clients (non-user-controlled):** - -| Client | File:Line | Target | User-controlled? | -|--------|-----------|--------|-----------------| -| `fetch_csv_from_url` | `utils.py:188` | User-provided URL | **Yes** — SSRF-protected | -| `EveryRowAuthProvider._http_client` | `auth.py:199` | `settings.supabase_url` | No — config-derived | -| `SupabaseTokenVerifier._jwks_client` | `auth.py:53` | `{supabase_url}/.well-known/jwks.json` | No — config-derived | -| `AuthenticatedClient` (SDK) | `tool_helpers.py:65` | `settings.everyrow_api_url` | No — config-derived | -| `AuthenticatedClient` (upload) | `uploads.py:312` | `settings.everyrow_api_url` | No — config-derived | -| `AuthenticatedClient` (routes) | `routes.py:99` | `settings.everyrow_api_url` | No — config-derived | - ---- - -## Existing SSRF Protections (Commit 4000b88) - -### Layer 1: Pre-flight DNS Validation - -**File:** `everyrow-mcp/src/everyrow_mcp/utils.py:53-100` - -Before any HTTP request, `_validate_url_target(url)` extracts the hostname via `urlparse` and calls `_validate_hostname()`, which: - -1. Checks against `_BLOCKED_HOSTNAMES` (`metadata.google.internal` + FQDN variant) -2. For IP literals: validates directly against `_BLOCKED_NETWORKS` with IPv4-mapped IPv6 unwrapping -3. For DNS names: resolves via `socket.getaddrinfo(AF_UNSPEC)` and checks every resolved IP - -**Blocked networks (utils.py:21-31):** - -| Network | Purpose | -|---------|---------| -| `10.0.0.0/8` | RFC 1918 private | -| `172.16.0.0/12` | RFC 1918 private | -| `192.168.0.0/16` | RFC 1918 private | -| `127.0.0.0/8` | Loopback | -| `169.254.0.0/16` | Link-local / cloud metadata | -| `0.0.0.0/8` | This network | -| `::1/128` | IPv6 loopback | -| `fc00::/7` | IPv6 ULA | -| `fe80::/10` | IPv6 link-local | - -### Layer 2: Transport-Level IP Pinning - -**File:** `everyrow-mcp/src/everyrow_mcp/utils.py:168-185` - -Custom `_SSRFSafeTransport(httpx.AsyncBaseTransport)` resolves DNS, validates the resolved IPs, and **pins the connection** to the validated IP. The original hostname is preserved in the `Host` header and TLS SNI extension. - -### Layer 3: Redirect Chain Validation - -**File:** `everyrow-mcp/src/everyrow_mcp/utils.py:152-165` - -An httpx `event_hooks["response"]` hook validates every redirect `Location` header against `_validate_url_target()` before following. Redirects are capped at `max_redirects=5`. - -### Additional Controls - -| Control | File:Line | Details | -|---------|-----------|---------| -| Scheme restriction | `utils.py:117` | Only `http://` and `https://` | -| Port allowlist | `utils.py:40-55` | Only 80, 443, 8080, 8443 | -| Streaming size limit | `utils.py:219-224` | 50 MB default, aborts mid-stream | -| Content-Length pre-check | `utils.py:212` | Rejects before streaming if header present | -| IPv4-mapped IPv6 unwrap | `utils.py:48-49, 68-70` | `::ffff:127.0.0.1` → `127.0.0.1` | -| Fail-closed on unparseable IPs | `utils.py:46` | Returns `True` (blocked) | -| Fail-closed on DNS failure | `utils.py:82` | Raises `ValueError` | - ---- - -## Findings - -### FINDING-01: DNS Rebinding TOCTOU Window — FIXED - -**Severity:** High (residual risk after mitigation) -**File:** `everyrow-mcp/src/everyrow_mcp/utils.py:179-182` -**Status:** **Fixed** — transport now pins resolved IP - -**Description:** - -The original `_SSRFSafeTransport` re-validated hostnames at request time, but its `_validate_hostname()` call performed its own `socket.getaddrinfo()` lookup, which was a **separate DNS resolution** from what httpx's inner `AsyncHTTPTransport` (via `httpcore`) performed when opening the TCP connection. If DNS rebinds between the transport-level check and the actual `connect()` inside `httpcore`, a fast-rebinding DNS server could succeed. - -``` -Original timeline (vulnerable): - T0: _validate_hostname() → getaddrinfo() → returns 93.184.216.34 (public) ✓ - T1: DNS rebinds hostname → 169.254.169.254 (metadata) - T2: AsyncHTTPTransport → httpcore.connect() → getaddrinfo() → 169.254.169.254 - T3: Connection established to cloud metadata service -``` - -The window between T0 and T2 was extremely narrow (microseconds within the same async coroutine). Successful exploitation would require an attacker-controlled DNS server with very fast rebinding and is probabilistic — the actual success rate depends heavily on OS resolver caching behaviour and configured TTLs. - -**Fix Applied:** - -The transport now uses `_resolve_and_validate()` to resolve DNS once, validate all IPs, then pins the connection to the validated IP by rewriting the request URL. The original hostname is preserved in the `Host` header and TLS SNI extension (addressing the TLS/SNI regression risk noted in code review): - -```python -# Resolve DNS and validate — returns the first safe IP -resolved_ip = _resolve_and_validate(hostname) - -# Pin the URL to the validated IP -pinned_url = request.url.copy_with(host=resolved_ip) - -# Preserve original hostname in Host header -headers = [...("host", hostname)...] - -# Preserve original hostname for TLS SNI -extensions["sni_hostname"] = hostname.encode("ascii") -``` - -This eliminates the TOCTOU entirely — `httpcore` connects directly to the validated IP without performing a second DNS lookup. - ---- - -### FINDING-02: No Port Restriction on Fetched URLs — FIXED - -**Severity:** Medium -**File:** `everyrow-mcp/src/everyrow_mcp/utils.py:53-100` -**Status:** **Fixed** — port allowlist added - -**Description:** - -The SSRF blocklist validated IP addresses but did not restrict ports. An attacker could probe internal services on non-standard ports even when the IP is public or allowed. This is particularly dangerous in containerized deployments where services bind to non-standard ports. - -**Proof of Concept:** - -```python -# Probe Redis on its default port (if exposed on a public IP or shared network) -source = "http://redis.internal:6379/" - -# SMTP banner grabbing -source = "http://mail.company.com:25/" -``` - -**Fix Applied:** - -Added `_ALLOWED_PORTS = {80, 443, 8080, 8443}` and `_validate_port()`, enforced both in the pre-flight `_validate_url_target()` and at transport time in `_SSRFSafeTransport`. Non-allowed ports raise `ValueError`. - ---- - -### FINDING-03: Incomplete IP Blocklist — Missing CGNAT Range - -**Severity:** Medium -**File:** `everyrow-mcp/src/everyrow_mcp/utils.py:21-31` - -**Description:** - -The blocklist covers the major RFC 1918 ranges and cloud metadata endpoints but is missing `100.64.0.0/10` (RFC 6598 CGNAT), which is used by AWS for VPC endpoints and by Tailscale for mesh networking. An attacker could reach internal VPC services via this range. - -| Missing Network | RFC | Severity | -|----------------|-----|----------| -| `100.64.0.0/10` | RFC 6598 (CGNAT) | **Medium** — reachable in AWS VPCs, GKE, Tailscale | -| `198.18.0.0/15` | RFC 2544 | Low — benchmark testing, sometimes used internally | -| `192.0.0.0/24` | RFC 6890 | Info — IANA special-purpose | -| `192.0.2.0/24`, `198.51.100.0/24`, `203.0.113.0/24` | RFC 5737 | Info — TEST-NET (documentation only, non-routable) | - -**Proof of Concept:** - -```python -# AWS VPC endpoint (CGNAT range) -source = "http://100.64.0.1/latest/meta-data/" - -# Tailscale node in mesh network -source = "http://100.100.100.100/api/v1/status" -``` - -**Recommended Fix:** - -```python -_BLOCKED_NETWORKS = [ - # ... existing entries ... - ipaddress.ip_network("100.64.0.0/10"), # CGNAT (RFC 6598) — AWS VPC, Tailscale - ipaddress.ip_network("198.18.0.0/15"), # Benchmark testing (RFC 2544) - ipaddress.ip_network("192.0.0.0/24"), # IANA special-purpose (RFC 6890) -] -``` - ---- - -### FINDING-04: Rate Limit Bypass via IP Header Spoofing - -**Severity:** Medium -**File:** `everyrow-mcp/src/everyrow_mcp/middleware.py:28-40` - -**Description:** - -When `trust_proxy_headers=True`, `get_client_ip()` reads the first IP from `X-Forwarded-For` (or the configured header). If the reverse proxy does not strip or overwrite incoming `X-Forwarded-For` headers, an attacker can bypass rate limiting by spoofing different client IPs on each request. - -```python -# middleware.py:36-39 -if settings.trust_proxy_headers: - value = request.headers.get(settings.trusted_ip_header.lower()) - if value: - return value.split(",")[0].strip() # Trusts first value -``` - -**Current Mitigation:** `docker-compose.yaml:42` defaults `TRUST_PROXY_HEADERS=false`. But Kubernetes/GKE deployments typically set this to `true`. - -**Recommended Fix:** - -1. Document that the reverse proxy MUST overwrite (not append to) the trusted IP header -2. Add the proxy's own IP to a `TRUSTED_PROXIES` allowlist and only read the header when the direct connection comes from a trusted proxy - ---- - -### FINDING-05: Relative Redirect Blocking (False Positive / Fail-Safe) - -**Severity:** Low -**File:** `everyrow-mcp/src/everyrow_mcp/utils.py:152-165` - -**Description:** - -The `_check_redirect` event hook validates the raw `Location` header from redirect responses. For relative redirects (e.g., `Location: /path`), `urlparse` returns `hostname=None`, causing `_validate_url_target` to raise `"URL has no hostname"`. This blocks the redirect chain. - -From a security perspective, this is **fail-safe** — relative redirects stay on the same (already validated) host. However, it may cause false positives if a legitimate public server returns a relative redirect. - -**Recommended Fix:** - -Resolve relative redirects against the request URL before validating using `urljoin()`. - ---- - -### FINDING-06: URL Parser Discrepancy (urlparse vs httpx) - -**Severity:** Info -**File:** `everyrow-mcp/src/everyrow_mcp/utils.py:90-100` - -**Description:** - -The pre-flight validation uses Python's `urlparse` to extract the hostname, while httpx uses its own URL parser (via `httpcore`). Parser discrepancies could theoretically allow an attacker to craft a URL where `urlparse` extracts a different hostname than httpx. - -**Mitigating Factor:** The `_SSRFSafeTransport` (Layer 2) re-validates using `request.url.host`, which is httpx's own parsed view, and then pins the connection to the resolved IP. This means the defence-in-depth is already working as designed — even if the pre-flight check (Layer 1) is fooled by a parser discrepancy, the transport-level check (Layer 2) uses the same parser that determines where the connection actually goes. The pre-flight check is purely an early-rejection optimisation. - ---- - -### FINDING-07: Auth httpx Client Without SSRF Transport - -**Severity:** Low -**File:** `everyrow-mcp/src/everyrow_mcp/auth.py:199-202` - -**Description:** - -The `EveryRowAuthProvider.__init__` creates an `httpx.AsyncClient()` without `_SSRFSafeTransport`. The target URL is derived from `settings.supabase_url`, which is a config value validated at startup to require HTTPS for non-localhost (`config.py:134-147`). - -**Not exploitable** unless the attacker controls the server's environment variables. - -**Recommended Fix (defence-in-depth):** Add the SSRF transport as a precaution. - ---- - -### FINDING-08: Missing IPv6 Addresses in Blocklist - -**Severity:** Low -**File:** `everyrow-mcp/src/everyrow_mcp/utils.py:21-31` - -**Description:** - -The blocklist does not include `::` (IPv6 unspecified address) or `::ffff:0:0/96` (IPv4-mapped prefix). The practical risk is minimal since `::` does not route to any reachable host in most environments. - -**Recommended Fix:** Add `::/128` and `::ffff:0:0/96` to the blocklist. - ---- - -### FINDING-09: `_decode_trusted_server_jwt` Skips Signature Verification - -**Severity:** Info (by design) -**File:** `everyrow-mcp/src/everyrow_mcp/auth.py:152-161` - -**Description:** - -This function decodes JWTs with `verify_signature=False`. It is only called from `_issue_token_response` (line 456) with tokens obtained directly from Supabase's token endpoint over HTTPS. The critical guarantee that makes this safe is the HTTPS enforcement on `supabase_url` via the `_validate_url` validator in `config.py:134-147` — the token is received over a TLS-authenticated channel, making it trustworthy without a separate signature check. - -**Assessment:** Safe by current usage. The docstring warning ("NEVER use this for tokens received from end users") is appropriate. - ---- - -### FINDING-10: Wildcard CORS on Widget Endpoints - -**Severity:** Info (safe by design) -**File:** `everyrow-mcp/src/everyrow_mcp/routes.py:22-33` - -**Description:** - -Widget endpoints use `Access-Control-Allow-Origin: *`. Since auth is via Bearer tokens (not cookies), this is safe per the CORS specification — no ambient credentials are leaked. - ---- - -### FINDING-11: Container Hardening Review - -**Severity:** Info -**File:** `everyrow-mcp/deploy/docker-compose.yaml` - -**Description:** - -The container configuration follows security best practices (non-root user, `no-new-privileges`, `cap_drop: ALL`, read-only rootfs, memory/CPU limits, network isolation, Redis password required, Redis not exposed to host, MCP bound to localhost). - -**Missing (minor):** No `pids_limit` (fork bomb DoS), no `tmpfs` size limit. - ---- - -## Attack Surface Matrix - -| Attack Vector | Entry Point | Protection | Bypass? | Remediation | -|---------------|-------------|------------|---------|-------------| -| Direct SSRF via URL | `everyrow_upload_data(source=url)` | 3-layer validation + IP pinning | No (FINDING-01 fixed) | **Done** | -| Internal port probing | URL with non-standard port | Port allowlist (80, 443, 8080, 8443) | No (FINDING-02 fixed) | **Done** | -| SSRF via redirect | HTTP 3xx from attacker server | Event hook + transport re-check | Relative redirects blocked (fail-safe) | Open (Low) | -| Cloud metadata (169.254.169.254) | URL or DNS rebind | IP blocklist + hostname block | No | N/A | -| GKE metadata hostname | `metadata.google.internal` | Hostname blocklist | No | N/A | -| IPv4-mapped IPv6 bypass | `::ffff:127.0.0.1` | Unwrap + re-check | No | N/A | -| CGNAT range (100.64.x.x) | URL or DNS | **Not in blocklist** (FINDING-03) | **Yes** | Open (Medium) | -| Rate limit bypass | Spoofed X-Forwarded-For | Proxy trust config | Conditional (FINDING-04) | Open (Medium) | -| OAuth callback redirect | `/auth/callback` | Whitelist against registered URIs | No | N/A | -| File read via path | Local CSV path (stdio only) | Path validation + symlink resolve | No (HTTP mode rejects) | N/A | -| Redis key injection | Task IDs, user IDs | `build_key()` sanitization | No | N/A | -| Upload URL forgery | HMAC-SHA256 signature | `verify_upload_signature()` + expiry | No | N/A | - ---- - -## Recommendations Summary - -### Done (This PR) - -1. **~~Fix DNS rebinding TOCTOU~~** (FINDING-01): Transport now pins resolved IPs — TOCTOU eliminated. -2. **~~Add port restrictions~~** (FINDING-02): Port allowlist `{80, 443, 8080, 8443}` enforced pre-flight and at transport time. - -### Priority 2 (Medium Impact — Open) - -3. **Expand IP blocklist** (FINDING-03): Add `100.64.0.0/10` (CGNAT), `198.18.0.0/15`, and `192.0.0.0/24`. -4. **Harden proxy IP trust** (FINDING-04): Validate the direct connection IP against a trusted proxy allowlist before reading forwarded headers. - -### Priority 3 (Low Impact / Defence-in-Depth — Open) - -5. **Resolve relative redirects** (FINDING-05): Use `urljoin()` to resolve relative Location headers before validating. -6. **Add SSRF transport to auth client** (FINDING-07): Defence-in-depth for `EveryRowAuthProvider._http_client`. -7. **Add IPv6 unspecified address** (FINDING-08): Block `::` and `::ffff:0:0/96`. -8. **Container hardening** (FINDING-11): Add `pids_limit`, `tmpfs` size limit. - ---- - -## Conclusion - -The SSRF protections introduced in commit `4000b88` represent a **well-engineered, defense-in-depth approach** that covers the major attack vectors. The three-layer architecture (pre-flight, transport, redirect) provides meaningful redundancy. The two highest-priority findings have been fixed in this PR: the DNS-rebinding TOCTOU is eliminated via IP pinning, and non-standard ports are now blocked via an allowlist. The remaining open items (CGNAT blocklist, proxy IP trust) are medium-priority hardening improvements. No critical vulnerabilities that would allow reliable SSRF exploitation were found. From 2552a1cfd7d236a5e27ecd919c974f120580fa6d Mon Sep 17 00:00:00 2001 From: Rafael Poyiadzi Date: Wed, 25 Feb 2026 10:47:57 +0000 Subject: [PATCH 5/7] Fix IPv6 Host header construction, add IPv6 test coverage, remove finding refs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wrap IPv6 addresses in square brackets for Host header per RFC 7230 §5.4, preventing malformed headers like `2001:db8::1:8080` (now `[2001:db8::1]:8080`). Add tests for IPv6 ULA/link-local blocking and transport-level Host header construction. Remove internal finding references from comments. Co-Authored-By: Claude Opus 4.6 --- everyrow-mcp/src/everyrow_mcp/utils.py | 17 ++--- everyrow-mcp/tests/test_utils.py | 87 +++++++++++++++++++++++++- 2 files changed, 94 insertions(+), 10 deletions(-) diff --git a/everyrow-mcp/src/everyrow_mcp/utils.py b/everyrow-mcp/src/everyrow_mcp/utils.py index 6be672e6..3f917243 100644 --- a/everyrow-mcp/src/everyrow_mcp/utils.py +++ b/everyrow-mcp/src/everyrow_mcp/utils.py @@ -37,7 +37,7 @@ } ) -# FINDING-02: restrict outbound fetches to standard HTTP(S) ports. +# Restrict outbound fetches to standard HTTP(S) ports. _ALLOWED_PORTS: frozenset[int] = frozenset({80, 443, 8080, 8443}) @@ -76,7 +76,7 @@ def _resolve_and_validate(hostname: str) -> str: The returned IP is used by ``_SSRFSafeTransport`` to **pin** the TCP connection, eliminating the TOCTOU gap between DNS validation and the - actual ``connect()`` call (FINDING-01). + actual ``connect()`` call. Raises: ValueError: If the hostname is blocked, resolves to a blocked IP, @@ -204,7 +204,7 @@ class _SSRFSafeTransport(httpx.AsyncBaseTransport): """Transport that resolves DNS, validates IPs, and pins connections to safe IPs. Eliminates the TOCTOU gap between DNS validation and TCP connection - (FINDING-01) by: + by: 1. Resolving the hostname ourselves via ``getaddrinfo`` 2. Validating every resolved IP against the blocklist @@ -212,7 +212,7 @@ class _SSRFSafeTransport(httpx.AsyncBaseTransport): 4. Preserving the original hostname in the ``Host`` header and TLS SNI extension so the remote server sees the correct virtual host - Also enforces the port allowlist (FINDING-02) at transport time as a + Also enforces the port allowlist at transport time as a second check complementing the pre-flight validation. """ @@ -234,10 +234,11 @@ async def handle_async_request(self, request: httpx.Request) -> httpx.Response: # directly without a second (unvalidated) DNS lookup. pinned_url = request.url.copy_with(host=resolved_ip) - # Preserve the original hostname in the Host header - host_header = hostname + # Preserve the original hostname in the Host header. + # IPv6 addresses must be wrapped in brackets per RFC 7230 §5.4. + host_header = f"[{hostname}]" if ":" in hostname else hostname if request.url.port and request.url.port not in (80, 443): - host_header = f"{hostname}:{request.url.port}" + host_header = f"{host_header}:{request.url.port}" headers = [ (name, value) for name, value in request.headers.items() @@ -246,7 +247,7 @@ async def handle_async_request(self, request: httpx.Request) -> httpx.Response: headers.insert(0, ("host", host_header)) # Preserve the original hostname for TLS SNI so the server - # presents the right certificate (reviewer feedback on FINDING-01). + # presents the right certificate. extensions = dict(request.extensions) if request.url.scheme == "https": extensions["sni_hostname"] = hostname.encode("ascii") diff --git a/everyrow-mcp/tests/test_utils.py b/everyrow-mcp/tests/test_utils.py index 99cb0f7c..4663f5b2 100644 --- a/everyrow-mcp/tests/test_utils.py +++ b/everyrow-mcp/tests/test_utils.py @@ -4,6 +4,7 @@ from pathlib import Path from unittest.mock import patch +import httpx import pandas as pd import pytest @@ -12,6 +13,7 @@ _is_blocked_ip, _normalise_google_sheets_url, _resolve_and_validate, + _SSRFSafeTransport, _validate_port, _validate_url_target, is_url, @@ -298,7 +300,7 @@ def test_validate_url_target_blocks_unresolvable(self): _validate_url_target("http://nonexistent.invalid/data") -# ── Port restriction tests (FINDING-02) ────────────────────── +# ── Port restriction tests ──────────────────────────────────── class TestPortRestriction: @@ -351,7 +353,7 @@ def test_validate_url_target_allows_port_8080(self): _validate_url_target("http://example.com:8080/data.csv") -# ── DNS-pinning tests (FINDING-01) ─────────────────────────── +# ── DNS-pinning tests ──────────────────────────────────────── class TestResolveAndValidate: @@ -396,3 +398,84 @@ def test_blocks_unresolvable(self): def test_unwraps_ipv4_mapped_ipv6(self): with pytest.raises(ValueError, match="blocked IP"): _resolve_and_validate("::ffff:127.0.0.1") + + def test_allows_public_ipv6_literal(self): + ip = _resolve_and_validate("2001:db8::1") + assert ip == "2001:db8::1" + + def test_blocks_ipv6_ula(self): + with pytest.raises(ValueError, match="blocked IP"): + _resolve_and_validate("fd12:3456:789a::1") + + def test_blocks_ipv6_link_local(self): + with pytest.raises(ValueError, match="blocked IP"): + _resolve_and_validate("fe80::1") + + +# ── IPv6 Host header tests ─────────────────────────────────── + + +class TestSSRFSafeTransportIPv6Host: + """Tests for IPv6 Host header bracket wrapping in _SSRFSafeTransport.""" + + @pytest.mark.asyncio + async def test_ipv6_host_header_no_port(self): + """IPv6 hostname gets brackets in Host header even without explicit port.""" + transport = _SSRFSafeTransport() + request = httpx.Request("GET", "http://[2001:db8::1]/data") + + with patch.object( + transport._transport, + "handle_async_request", + return_value=httpx.Response(200), + ) as mock: + with patch( + "everyrow_mcp.utils._resolve_and_validate", + return_value="2001:db8::1", + ): + await transport.handle_async_request(request) + + pinned = mock.call_args[0][0] + assert pinned.headers["host"] == "[2001:db8::1]" + + @pytest.mark.asyncio + async def test_ipv6_host_header_with_non_standard_port(self): + """IPv6 hostname + non-standard port gets [addr]:port format.""" + transport = _SSRFSafeTransport() + request = httpx.Request("GET", "http://[2001:db8::1]:8080/data") + + with patch.object( + transport._transport, + "handle_async_request", + return_value=httpx.Response(200), + ) as mock: + with patch( + "everyrow_mcp.utils._resolve_and_validate", + return_value="2001:db8::1", + ): + with patch("everyrow_mcp.utils._validate_port"): + await transport.handle_async_request(request) + + pinned = mock.call_args[0][0] + assert pinned.headers["host"] == "[2001:db8::1]:8080" + + @pytest.mark.asyncio + async def test_ipv4_host_header_no_brackets(self): + """IPv4 hostname does NOT get brackets.""" + transport = _SSRFSafeTransport() + request = httpx.Request("GET", "http://example.com:8080/data") + + with patch.object( + transport._transport, + "handle_async_request", + return_value=httpx.Response(200), + ) as mock: + with patch( + "everyrow_mcp.utils._resolve_and_validate", + return_value="93.184.216.34", + ): + with patch("everyrow_mcp.utils._validate_port"): + await transport.handle_async_request(request) + + pinned = mock.call_args[0][0] + assert pinned.headers["host"] == "example.com:8080" From 2b035accc3b349e9bf2eb8cc8f9ce5c738bb6b28 Mon Sep 17 00:00:00 2001 From: Rafael Poyiadzi Date: Wed, 25 Feb 2026 10:49:51 +0000 Subject: [PATCH 6/7] Use IDNA codec for SNI hostname encoding Switch from .encode("ascii") to .encode("idna") for defensive handling of any non-ASCII hostname that might reach the transport. In practice httpx pre-normalizes to punycode, but this prevents a UnicodeEncodeError in edge cases. Co-Authored-By: Claude Opus 4.6 --- everyrow-mcp/src/everyrow_mcp/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/everyrow-mcp/src/everyrow_mcp/utils.py b/everyrow-mcp/src/everyrow_mcp/utils.py index 3f917243..02850d30 100644 --- a/everyrow-mcp/src/everyrow_mcp/utils.py +++ b/everyrow-mcp/src/everyrow_mcp/utils.py @@ -250,7 +250,7 @@ async def handle_async_request(self, request: httpx.Request) -> httpx.Response: # presents the right certificate. extensions = dict(request.extensions) if request.url.scheme == "https": - extensions["sni_hostname"] = hostname.encode("ascii") + extensions["sni_hostname"] = hostname.encode("idna") pinned_request = httpx.Request( method=request.method, From f345501dada1c8c678bad50218505d883c543122 Mon Sep 17 00:00:00 2001 From: Rafael Poyiadzi Date: Wed, 25 Feb 2026 10:53:02 +0000 Subject: [PATCH 7/7] Make DNS resolution non-blocking for multi-user concurrency Wrap socket.getaddrinfo in loop.run_in_executor so DNS resolution doesn't block the event loop when multiple users are making concurrent requests. Make _resolve_and_validate and _validate_url_target async, update all callers and tests accordingly. Co-Authored-By: Claude Opus 4.6 --- everyrow-mcp/src/everyrow_mcp/utils.py | 28 +++++--- everyrow-mcp/tests/test_utils.py | 95 ++++++++++++++++---------- 2 files changed, 76 insertions(+), 47 deletions(-) diff --git a/everyrow-mcp/src/everyrow_mcp/utils.py b/everyrow-mcp/src/everyrow_mcp/utils.py index 02850d30..019aefec 100644 --- a/everyrow-mcp/src/everyrow_mcp/utils.py +++ b/everyrow-mcp/src/everyrow_mcp/utils.py @@ -1,5 +1,6 @@ """Utility functions for the everyrow MCP server.""" +import asyncio import ipaddress import json import logging @@ -68,11 +69,12 @@ def _is_blocked_ip(addr: str) -> bool: return any(ip in net for net in _BLOCKED_NETWORKS) -def _resolve_and_validate(hostname: str) -> str: +async def _resolve_and_validate(hostname: str) -> str: """Resolve a hostname, validate all IPs, and return the first safe IP. For IP literals, validates directly and returns the canonical form. - For DNS names, resolves via ``getaddrinfo`` and checks every result. + For DNS names, resolves via ``getaddrinfo`` (offloaded to a thread pool + to avoid blocking the event loop) and checks every result. The returned IP is used by ``_SSRFSafeTransport`` to **pin** the TCP connection, eliminating the TOCTOU gap between DNS validation and the @@ -100,10 +102,16 @@ def _resolve_and_validate(hostname: str) -> str: raise ValueError(f"Connection to blocked IP: {hostname}") return str(parsed_ip) - # DNS name — resolve and validate all addresses + # DNS name — resolve in a thread pool to avoid blocking the event loop + loop = asyncio.get_running_loop() try: - addrinfos = socket.getaddrinfo( - hostname, None, socket.AF_UNSPEC, socket.SOCK_STREAM + addrinfos = await loop.run_in_executor( + None, + socket.getaddrinfo, + hostname, + None, + socket.AF_UNSPEC, + socket.SOCK_STREAM, ) except socket.gaierror: raise ValueError(f"Could not resolve hostname: {hostname}") @@ -120,7 +128,7 @@ def _resolve_and_validate(hostname: str) -> str: return addrinfos[0][4][0] -def _validate_url_target(url: str) -> None: +async def _validate_url_target(url: str) -> None: """Resolve a URL's hostname and reject if any resolved IP is internal or port is blocked. Raises: @@ -132,7 +140,7 @@ def _validate_url_target(url: str) -> None: if not hostname: raise ValueError(f"URL has no hostname: {url}") _validate_port(parsed.port) - _resolve_and_validate(hostname) + await _resolve_and_validate(hostname) def is_url(value: str) -> bool: @@ -190,7 +198,7 @@ async def _check_redirect(response: httpx.Response) -> None: location = response.headers.get("location", "") if location: try: - _validate_url_target(location) + await _validate_url_target(location) except ValueError: # TooManyRedirects aborts the redirect chain — httpx # has no "redirect rejected" error type. @@ -228,7 +236,7 @@ async def handle_async_request(self, request: httpx.Request) -> httpx.Response: _validate_port(request.url.port) # Resolve DNS and validate — returns the first safe IP - resolved_ip = _resolve_and_validate(hostname) + resolved_ip = await _resolve_and_validate(hostname) # Pin the URL to the validated IP so the inner transport connects # directly without a second (unvalidated) DNS lookup. @@ -276,7 +284,7 @@ async def fetch_csv_from_url(url: str) -> pd.DataFrame: httpx.HTTPStatusError: On non-2xx responses. """ url = _normalise_google_sheets_url(url) - _validate_url_target(url) + await _validate_url_target(url) async with httpx.AsyncClient( transport=_SSRFSafeTransport(), diff --git a/everyrow-mcp/tests/test_utils.py b/everyrow-mcp/tests/test_utils.py index 4663f5b2..67237a4e 100644 --- a/everyrow-mcp/tests/test_utils.py +++ b/everyrow-mcp/tests/test_utils.py @@ -2,7 +2,7 @@ import socket from pathlib import Path -from unittest.mock import patch +from unittest.mock import AsyncMock, patch import httpx import pandas as pd @@ -259,45 +259,50 @@ def test_allows_public_ip(self): def test_allows_public_ip_2(self): assert _is_blocked_ip("93.184.216.34") is False - def test_validate_url_target_blocks_localhost(self): + @pytest.mark.asyncio + async def test_validate_url_target_blocks_localhost(self): with patch( "everyrow_mcp.utils.socket.getaddrinfo", return_value=_mock_resolve("localhost", "127.0.0.1"), ): with pytest.raises(ValueError, match="not permitted"): - _validate_url_target("http://localhost/secret") + await _validate_url_target("http://localhost/secret") - def test_validate_url_target_blocks_10_x(self): + @pytest.mark.asyncio + async def test_validate_url_target_blocks_10_x(self): with patch( "everyrow_mcp.utils.socket.getaddrinfo", return_value=_mock_resolve("internal.corp", "10.0.0.5"), ): with pytest.raises(ValueError, match="not permitted"): - _validate_url_target("http://internal.corp/data") + await _validate_url_target("http://internal.corp/data") - def test_validate_url_target_blocks_metadata_endpoint(self): + @pytest.mark.asyncio + async def test_validate_url_target_blocks_metadata_endpoint(self): with patch( "everyrow_mcp.utils.socket.getaddrinfo", return_value=_mock_resolve("metadata", "169.254.169.254"), ): with pytest.raises(ValueError, match="not permitted"): - _validate_url_target("http://metadata/latest/api-token") + await _validate_url_target("http://metadata/latest/api-token") - def test_validate_url_target_allows_public(self): + @pytest.mark.asyncio + async def test_validate_url_target_allows_public(self): with patch( "everyrow_mcp.utils.socket.getaddrinfo", return_value=_mock_resolve("example.com", "93.184.216.34"), ): # Should not raise - _validate_url_target("https://example.com/data.csv") + await _validate_url_target("https://example.com/data.csv") - def test_validate_url_target_blocks_unresolvable(self): + @pytest.mark.asyncio + async def test_validate_url_target_blocks_unresolvable(self): with patch( "everyrow_mcp.utils.socket.getaddrinfo", side_effect=socket.gaierror("Name resolution failed"), ): with pytest.raises(ValueError, match="Could not resolve"): - _validate_url_target("http://nonexistent.invalid/data") + await _validate_url_target("http://nonexistent.invalid/data") # ── Port restriction tests ──────────────────────────────────── @@ -330,27 +335,30 @@ def test_arbitrary_high_port_blocked(self): with pytest.raises(ValueError, match="not permitted"): _validate_port(9090) - def test_validate_url_target_blocks_redis_port(self): + @pytest.mark.asyncio + async def test_validate_url_target_blocks_redis_port(self): with patch( "everyrow_mcp.utils.socket.getaddrinfo", return_value=_mock_resolve("example.com", "93.184.216.34"), ): with pytest.raises(ValueError, match="not permitted"): - _validate_url_target("http://example.com:6379/data") + await _validate_url_target("http://example.com:6379/data") - def test_validate_url_target_allows_port_443(self): + @pytest.mark.asyncio + async def test_validate_url_target_allows_port_443(self): with patch( "everyrow_mcp.utils.socket.getaddrinfo", return_value=_mock_resolve("example.com", "93.184.216.34"), ): - _validate_url_target("https://example.com:443/data.csv") + await _validate_url_target("https://example.com:443/data.csv") - def test_validate_url_target_allows_port_8080(self): + @pytest.mark.asyncio + async def test_validate_url_target_allows_port_8080(self): with patch( "everyrow_mcp.utils.socket.getaddrinfo", return_value=_mock_resolve("example.com", "93.184.216.34"), ): - _validate_url_target("http://example.com:8080/data.csv") + await _validate_url_target("http://example.com:8080/data.csv") # ── DNS-pinning tests ──────────────────────────────────────── @@ -359,57 +367,67 @@ def test_validate_url_target_allows_port_8080(self): class TestResolveAndValidate: """Tests for _resolve_and_validate (IP pinning).""" - def test_returns_ip_for_public_hostname(self): + @pytest.mark.asyncio + async def test_returns_ip_for_public_hostname(self): with patch( "everyrow_mcp.utils.socket.getaddrinfo", return_value=_mock_resolve("example.com", "93.184.216.34"), ): - ip = _resolve_and_validate("example.com") + ip = await _resolve_and_validate("example.com") assert ip == "93.184.216.34" - def test_returns_ip_literal_directly(self): - ip = _resolve_and_validate("8.8.8.8") + @pytest.mark.asyncio + async def test_returns_ip_literal_directly(self): + ip = await _resolve_and_validate("8.8.8.8") assert ip == "8.8.8.8" - def test_blocks_private_ip_literal(self): + @pytest.mark.asyncio + async def test_blocks_private_ip_literal(self): with pytest.raises(ValueError, match="blocked IP"): - _resolve_and_validate("127.0.0.1") + await _resolve_and_validate("127.0.0.1") - def test_blocks_metadata_hostname(self): + @pytest.mark.asyncio + async def test_blocks_metadata_hostname(self): with pytest.raises(ValueError, match="not permitted"): - _resolve_and_validate("metadata.google.internal") + await _resolve_and_validate("metadata.google.internal") - def test_blocks_hostname_resolving_to_private(self): + @pytest.mark.asyncio + async def test_blocks_hostname_resolving_to_private(self): with patch( "everyrow_mcp.utils.socket.getaddrinfo", return_value=_mock_resolve("evil.com", "10.0.0.1"), ): with pytest.raises(ValueError, match="not permitted"): - _resolve_and_validate("evil.com") + await _resolve_and_validate("evil.com") - def test_blocks_unresolvable(self): + @pytest.mark.asyncio + async def test_blocks_unresolvable(self): with patch( "everyrow_mcp.utils.socket.getaddrinfo", side_effect=socket.gaierror("Name resolution failed"), ): with pytest.raises(ValueError, match="Could not resolve"): - _resolve_and_validate("nonexistent.invalid") + await _resolve_and_validate("nonexistent.invalid") - def test_unwraps_ipv4_mapped_ipv6(self): + @pytest.mark.asyncio + async def test_unwraps_ipv4_mapped_ipv6(self): with pytest.raises(ValueError, match="blocked IP"): - _resolve_and_validate("::ffff:127.0.0.1") + await _resolve_and_validate("::ffff:127.0.0.1") - def test_allows_public_ipv6_literal(self): - ip = _resolve_and_validate("2001:db8::1") + @pytest.mark.asyncio + async def test_allows_public_ipv6_literal(self): + ip = await _resolve_and_validate("2001:db8::1") assert ip == "2001:db8::1" - def test_blocks_ipv6_ula(self): + @pytest.mark.asyncio + async def test_blocks_ipv6_ula(self): with pytest.raises(ValueError, match="blocked IP"): - _resolve_and_validate("fd12:3456:789a::1") + await _resolve_and_validate("fd12:3456:789a::1") - def test_blocks_ipv6_link_local(self): + @pytest.mark.asyncio + async def test_blocks_ipv6_link_local(self): with pytest.raises(ValueError, match="blocked IP"): - _resolve_and_validate("fe80::1") + await _resolve_and_validate("fe80::1") # ── IPv6 Host header tests ─────────────────────────────────── @@ -431,6 +449,7 @@ async def test_ipv6_host_header_no_port(self): ) as mock: with patch( "everyrow_mcp.utils._resolve_and_validate", + new_callable=AsyncMock, return_value="2001:db8::1", ): await transport.handle_async_request(request) @@ -451,6 +470,7 @@ async def test_ipv6_host_header_with_non_standard_port(self): ) as mock: with patch( "everyrow_mcp.utils._resolve_and_validate", + new_callable=AsyncMock, return_value="2001:db8::1", ): with patch("everyrow_mcp.utils._validate_port"): @@ -472,6 +492,7 @@ async def test_ipv4_host_header_no_brackets(self): ) as mock: with patch( "everyrow_mcp.utils._resolve_and_validate", + new_callable=AsyncMock, return_value="93.184.216.34", ): with patch("everyrow_mcp.utils._validate_port"):