mirror of
https://github.com/ComposioHQ/composio.git
synced 2026-09-22 11:46:35 +08:00
d544006a25
Fixes #4151. ## The problem Both SDKs validated a URL by resolving its hostname, and then handed the *hostname* to the HTTP client, which resolved it again when it opened the socket. Two lookups, two answers: a short-TTL record under an attacker's control answers publicly for the check and with `169.254.169.254`, `127.0.0.1`, or RFC 1918 space for the connect. The guard passes and the connection lands inside the network — classic TOCTOU DNS rebinding, documented in both modules until now as a known residual. ```mermaid sequenceDiagram participant SDK participant DNS as Attacker DNS participant Meta as 169.254.169.254 Note over SDK,Meta: before SDK->>DNS: resolve evil.example.com (validate) DNS-->>SDK: 93.184.216.34 — passes the guard SDK->>DNS: resolve evil.example.com (connect) DNS-->>SDK: 169.254.169.254 SDK->>Meta: GET /latest/meta-data/… Meta-->>SDK: credentials ``` ## The fix Resolve once, validate every answer, then connect to the address that was validated. There is no second lookup left to rebind. - **Python** — `safe_get` / `safe_request` mount a transport adapter that swaps the connect target for the duration of the socket connect only. The `Host` header and TLS SNI keep the hostname, so certificate verification is unchanged; rewriting `conn._dns_host` for the whole connection would have sent `Host: <ip>` and offered the IP as SNI, failing against every real origin. Every fetch call site now goes through those two helpers, so no `requests.get` sits next to a bare check any more: - `_files.py::_fetch_file_from_url`, `_files.py::FileDownloadable.download` - `tool_router_session_files.py::_fetch_url_bytes` - `safe_request`, per redirect hop - **TypeScript** — `assertSafeFetchTarget` returns the validated address and `ssrfSafeFetch` hands `fetch` a dispatcher pinned to it, re-pinned per redirect hop. The dispatcher goes to the runtime's own `fetch`, so callers that stub `globalThis.fetch` keep working. The pinned `lookup` answers both shapes Node calls it with — the address *list* it uses for Happy Eyeballs, and the single `(address, family)` it uses when `autoSelectFamily` is off — since answering in the wrong shape is rejected as an invalid address. - A fail-closed peer assertion runs on the Python side before a byte is written to the socket — redundant while pinning works, and a tripwire if a urllib3 upgrade ever breaks it. - `workerd` is unchanged: it already fails closed for user-supplied URLs. Redirect *validation* already existed in both SDKs (`safe_request` / `ssrfSafeFetch`); what was missing was re-pinning each hop. ## Tests The existing suites could not express this bug: they mock both the resolver and the HTTP client, so check and use are the same mock. The new tests use real sockets. - `python/tests/test_url_safety_pinning.py` — two loopback servers and a resolver that answers the first lookup with one endpoint and every later one with another, which is what a short-TTL rebinding record does. Asserts the rebound endpoint receives **zero** connections, and that `Host` still carries the hostname. Both tests fail on `next` and pass here. - `ts/packages/core/test/utils/pinnedDispatcher.node.test.ts` — a real server plus a hostname under `.invalid`, which RFC 2606 guarantees never resolves. A request that arrives proves the connect used the pinned address and never consulted DNS. The third case shows the contrast: unpinned, the same fetch cannot resolve at all. - `ssrfGuard.test.ts` gains assertions that each hop is pinned to that hop's own validated address. - `pinnedDispatcher.node.test.ts` also pins with `setDefaultAutoSelectFamily(false)`, which is the branch Node takes for the single-address callback. ## Notes - Supersedes #4157, which diagnosed this correctly. Its post-response peer check turned out not to hold: with an HTTP/1.0 or `Connection: close` server, urllib3 detaches the socket (`conn.sock is None`) while `r.content` still returns the full body, so the check fails open exactly where exfiltration succeeds. That is why the assertion here runs at connect time instead. - The Python package now declares `urllib3>=2` directly. `url_safety` imports it for `NameResolutionError`, which only exists from 2.0, and the pinning adapter reaches into 2.x connection internals; `requests` alone allows 1.x, where `import composio` would have failed outright. - `@composio/core` gains an `undici` dependency, pinned to `^7`: undici 8 dispatchers are rejected by the `fetch` in every Node version this package supports (22/24/25, verified). The real-socket test runs on the full CI matrix, so a future incompatibility fails loudly instead of silently un-pinning. - `undici` is imported on first pinned request rather than at module load: importing it installs a process-wide global dispatcher, which would have handed the host application's own unrelated `fetch` calls this package's undici merely because it imported `@composio/core`. - Residuals, now documented in the modules: - Requests routed through an environment proxy keep the pre-flight check only. The proxy resolves the hostname itself and the SDK cannot see or pin that resolution. - A process that does perform a pinned fetch still ends up on this package's `Agent` if nothing had claimed the global dispatcher slot yet. undici defines that slot non-configurable, so it cannot be handed back — assigning `undefined` leaves the runtime's own `fetch` asserting on a missing dispatcher.
277 lines
10 KiB
Python
277 lines
10 KiB
Python
"""Regression tests for the DNS-rebinding window in the SSRF guard.
|
|
|
|
The guard resolves a hostname to decide whether a URL is safe. If the HTTP
|
|
client then resolves that hostname a *second* time to open the socket, an
|
|
attacker who controls the authoritative DNS can answer with a public address
|
|
for the check and an internal one for the connect — the check and the use are
|
|
about different addresses (issue #4151).
|
|
|
|
These tests run against real sockets on loopback rather than mocking the HTTP
|
|
client, because a mocked client cannot resolve anything twice and so cannot
|
|
express the bug at all. The resolver is the only thing faked: it answers the
|
|
first lookup of the hostname with one endpoint and every later lookup with
|
|
another, which is exactly what a short-TTL rebinding record does.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import ipaddress
|
|
import os
|
|
import socket
|
|
import threading
|
|
import typing as t
|
|
from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer
|
|
from unittest.mock import MagicMock, patch
|
|
|
|
import pytest
|
|
import requests
|
|
import urllib3.connection
|
|
|
|
from composio.exceptions import BlockedInternalUrlError
|
|
from composio.utils.url_safety import _assert_pinned_peer, safe_get
|
|
|
|
HOSTNAME = "rebind.test"
|
|
|
|
|
|
def _is_ip_literal(host: str) -> bool:
|
|
try:
|
|
ipaddress.ip_address(host)
|
|
except ValueError:
|
|
return False
|
|
return True
|
|
|
|
|
|
class _RecordingServer:
|
|
"""A loopback HTTP server that records what actually reached it."""
|
|
|
|
def __init__(self, body: bytes) -> None:
|
|
self.hits: t.List[str] = []
|
|
recorder = self
|
|
|
|
class Handler(BaseHTTPRequestHandler):
|
|
protocol_version = "HTTP/1.1"
|
|
|
|
def do_GET(self) -> None: # noqa: N802 - stdlib naming
|
|
recorder.hits.append(self.headers.get("Host", ""))
|
|
self.send_response(200)
|
|
self.send_header("Content-Type", "text/plain")
|
|
self.send_header("Content-Length", str(len(body)))
|
|
self.end_headers()
|
|
self.wfile.write(body)
|
|
|
|
def log_message(self, *args: t.Any) -> None:
|
|
pass
|
|
|
|
self._server = ThreadingHTTPServer(("127.0.0.1", 0), Handler)
|
|
# Keep-alive connections would otherwise hold `shutdown()` open.
|
|
self._server.daemon_threads = True
|
|
self.port = self._server.server_address[1]
|
|
self._thread = threading.Thread(target=self._server.serve_forever, daemon=True)
|
|
self._thread.start()
|
|
|
|
def close(self) -> None:
|
|
self._server.shutdown()
|
|
self._server.server_close()
|
|
|
|
|
|
@pytest.fixture
|
|
def validated_server() -> t.Iterator[_RecordingServer]:
|
|
server = _RecordingServer(b"public payload")
|
|
yield server
|
|
server.close()
|
|
|
|
|
|
@pytest.fixture
|
|
def rebound_server() -> t.Iterator[_RecordingServer]:
|
|
server = _RecordingServer(b"internal secret")
|
|
yield server
|
|
server.close()
|
|
|
|
|
|
@pytest.fixture
|
|
def no_inherited_proxy(monkeypatch: pytest.MonkeyPatch) -> None:
|
|
"""Make ``_proxy_applies`` deterministic: no proxy, from env or system.
|
|
|
|
An inherited ``HTTP_PROXY`` (or a macOS/Windows system proxy, which
|
|
``getproxies`` also reads) would disable the pinning adapter, and the
|
|
request would dial the proxy's answer to the hostname instead of the
|
|
validated address — failing these tests on machines that carry one.
|
|
"""
|
|
monkeypatch.setattr(
|
|
"requests.utils.get_environ_proxies",
|
|
lambda url: {},
|
|
)
|
|
|
|
|
|
@pytest.fixture
|
|
def rebinding_dns(
|
|
no_inherited_proxy: None,
|
|
validated_server: _RecordingServer,
|
|
rebound_server: _RecordingServer,
|
|
) -> t.Iterator[None]:
|
|
"""Answer the first lookup of the hostname benignly, later ones with the victim.
|
|
|
|
Both endpoints live on 127.0.0.1 because a test cannot bind a public
|
|
address, so they are told apart by port; `is_blocked_ip` is stubbed out for
|
|
the same reason. Neither substitution touches the property under test,
|
|
which is whether the fetch connects to the endpoint that was validated or
|
|
re-resolves and connects somewhere else. The blocklist itself is covered by
|
|
``test_url_safety.py``.
|
|
"""
|
|
lookups: t.List[str] = []
|
|
real_getaddrinfo = socket.getaddrinfo
|
|
|
|
def fake_getaddrinfo(host, port, *args, **kwargs): # type: ignore[no-untyped-def]
|
|
# A real resolver hands an IP literal straight back, which is what the
|
|
# fix relies on when it connects to the address it already validated.
|
|
if _is_ip_literal(host):
|
|
return real_getaddrinfo(host, port, *args, **kwargs)
|
|
|
|
lookups.append(host)
|
|
victim = rebound_server if len(lookups) > 1 else validated_server
|
|
return [(socket.AF_INET, socket.SOCK_STREAM, 6, "", ("127.0.0.1", victim.port))]
|
|
|
|
with patch("socket.getaddrinfo", side_effect=fake_getaddrinfo):
|
|
with patch("composio.utils.url_safety.is_blocked_ip", return_value=False):
|
|
yield
|
|
|
|
|
|
def test_proxy_environment_keeps_only_the_pre_flight_check(
|
|
validated_server: _RecordingServer,
|
|
) -> None:
|
|
"""Behind a proxy the SDK cannot pin: the proxy resolves the hostname.
|
|
|
|
Documented residual — ``_proxy_applies`` disables the pinning adapter, so
|
|
the request dials the proxy. A proxy nothing can reach makes that
|
|
observable: the request must fail by trying the proxy, never by dialling
|
|
the validated address. (This test deliberately does not use
|
|
``no_inherited_proxy`` — it needs ``HTTP_PROXY`` visible — so it patches
|
|
the resolver inline instead.)
|
|
"""
|
|
real_getaddrinfo = socket.getaddrinfo
|
|
|
|
def fake_getaddrinfo(host, port, *args, **kwargs): # type: ignore[no-untyped-def]
|
|
if _is_ip_literal(host):
|
|
return real_getaddrinfo(host, port, *args, **kwargs)
|
|
return [
|
|
(
|
|
socket.AF_INET,
|
|
socket.SOCK_STREAM,
|
|
6,
|
|
"",
|
|
("127.0.0.1", validated_server.port),
|
|
)
|
|
]
|
|
|
|
# Empty NO_PROXY so no machine-level bypass list can cover the hostname.
|
|
env = {"HTTP_PROXY": "http://127.0.0.1:9", "NO_PROXY": "", "no_proxy": ""}
|
|
with patch("socket.getaddrinfo", side_effect=fake_getaddrinfo):
|
|
with patch("composio.utils.url_safety.is_blocked_ip", return_value=False):
|
|
with patch("composio.utils.url_safety._PinnedAddressAdapter") as adapter:
|
|
with patch.dict(os.environ, env):
|
|
with pytest.raises(requests.exceptions.RequestException):
|
|
safe_get(
|
|
f"http://{HOSTNAME}:{validated_server.port}/payload",
|
|
timeout=(1, 1),
|
|
)
|
|
|
|
adapter.assert_not_called()
|
|
assert validated_server.hits == []
|
|
|
|
|
|
@pytest.mark.usefixtures("rebinding_dns")
|
|
def test_fetch_connects_to_the_address_it_validated(
|
|
validated_server: _RecordingServer, rebound_server: _RecordingServer
|
|
) -> None:
|
|
response = safe_get(
|
|
f"http://{HOSTNAME}:{validated_server.port}/payload", timeout=(5, 5)
|
|
)
|
|
|
|
assert response.content == b"public payload"
|
|
assert len(validated_server.hits) == 1
|
|
# The whole point: the rebound endpoint is never even connected to, so
|
|
# neither its body nor a bare TCP connection to it is available to the
|
|
# attacker.
|
|
assert rebound_server.hits == []
|
|
|
|
|
|
@pytest.mark.usefixtures("rebinding_dns")
|
|
def test_pinning_keeps_the_hostname_on_the_wire(
|
|
validated_server: _RecordingServer,
|
|
) -> None:
|
|
"""The address is pinned; the hostname is not replaced by it.
|
|
|
|
Pinning by rewriting the connection's host would send ``Host: 127.0.0.1``
|
|
and offer the IP as TLS SNI, which fails certificate verification against
|
|
every real origin.
|
|
"""
|
|
safe_get(f"http://{HOSTNAME}:{validated_server.port}/payload", timeout=(5, 5))
|
|
|
|
assert validated_server.hits == [f"{HOSTNAME}:{validated_server.port}"]
|
|
|
|
|
|
def test_pinning_still_has_the_urllib3_internals_it_relies_on() -> None:
|
|
"""Fail loudly if a urllib3 upgrade removes what the adapter hooks into.
|
|
|
|
Silently losing either of these would silently un-pin every connection.
|
|
"""
|
|
connection = urllib3.connection.HTTPConnection("example.com")
|
|
|
|
assert hasattr(connection, "_new_conn")
|
|
assert hasattr(connection, "_dns_host")
|
|
|
|
|
|
def test_peer_mismatch_fails_closed() -> None:
|
|
"""The last line of defence, checked before a byte is written to the socket."""
|
|
sock = MagicMock()
|
|
sock.getpeername.return_value = ("169.254.169.254", 80)
|
|
|
|
with pytest.raises(BlockedInternalUrlError, match="169.254.169.254"):
|
|
_assert_pinned_peer(sock, "93.184.216.34", HOSTNAME)
|
|
|
|
sock.close.assert_called_once()
|
|
|
|
|
|
def test_connect_falls_back_to_the_next_validated_address(
|
|
no_inherited_proxy: None,
|
|
validated_server: _RecordingServer,
|
|
) -> None:
|
|
"""A dual-stack host must not be stranded on its first answer.
|
|
|
|
``::1`` is validated but nothing listens there, so the connect has to move
|
|
on to the second answer the way the HTTP client would have. urllib3 rewraps
|
|
connect failures into its own exception hierarchy, none of which inherits
|
|
from ``OSError``, so this also pins down which exceptions the fallback has
|
|
to catch.
|
|
"""
|
|
real_getaddrinfo = socket.getaddrinfo
|
|
|
|
def fake_getaddrinfo(host, port, *args, **kwargs): # type: ignore[no-untyped-def]
|
|
if _is_ip_literal(host):
|
|
return real_getaddrinfo(host, port, *args, **kwargs)
|
|
return [
|
|
(
|
|
socket.AF_INET6,
|
|
socket.SOCK_STREAM,
|
|
6,
|
|
"",
|
|
("::1", validated_server.port, 0, 0),
|
|
),
|
|
(
|
|
socket.AF_INET,
|
|
socket.SOCK_STREAM,
|
|
6,
|
|
"",
|
|
("127.0.0.1", validated_server.port),
|
|
),
|
|
]
|
|
|
|
with patch("socket.getaddrinfo", side_effect=fake_getaddrinfo):
|
|
with patch("composio.utils.url_safety.is_blocked_ip", return_value=False):
|
|
response = safe_get(
|
|
f"http://{HOSTNAME}:{validated_server.port}/payload", timeout=(5, 5)
|
|
)
|
|
|
|
assert response.content == b"public payload"
|
|
assert len(validated_server.hits) == 1
|