fix(ext/fetch,ext/websocket): check resolved IPs against net deny list#34236
Conversation
`fetch()` only ran the static `check_net_url` before delegating to hyper. The DNS resolver in `ext/fetch/dns.rs` then returned addresses straight to the HTTP connector without running `check_net_resolved`, so a hostname like `localhost` could pass the URL check and still connect to a denied IP after DNS lookup. `Deno.connect`, `Deno.listen`, etc. in `ext/net/ops.rs` already perform this post-resolution check. Thread the `PermissionsContainer` into the fetch DNS resolver and run `check_net_resolved` on every resolved address before yielding it to the connector. Applies to the default client, `Deno.createHttpClient`, and any custom `Resolve` implementations.
fibibot
left a comment
There was a problem hiding this comment.
The DNS-layer check loses the destination port, so port-scoped IP denies are still bypassable through a hostname. Service<Name> only receives the host name, and both hickory and custom resolver paths construct/check resolved addresses with port 0; GAI addresses are also produced by the DNS resolver before the HTTP connector applies the request port. That means --allow-net --deny-net=127.0.0.1:<port> can still allow fetch("http://localhost:<port>/"), while Deno.connect() would deny it because it checks the resolved address with the actual port.
Please move or add the post-resolution deny check somewhere that has both the resolved IP and the request port, and add a regression test for a port-specific deny rule.
fibibot
left a comment
There was a problem hiding this comment.
permissions.check_net((addr.ip().to_string(), Some(0)), "fetch()")?still loses the request port. The resolver only has the hostname, so--allow-net --deny-net=127.0.0.1:<port>can still allowfetch("http://localhost:<port>/")afterlocalhostresolves to127.0.0.1. Put the post-resolution deny check where both the resolved IP and target port are available, and add a port-specific regression test.
WebSocket reuses the fetch HTTP client, so the default and custom-client paths already pick up the post-DNS check added in the previous commit. The remaining gap was create_client_from_websocket_options, used when opening a WebSocket with ca_certs or unsafelyIgnoreCertificateErrors: it built a deno_fetch::Client directly with options.resolver.clone() and no permissions, so a hostname that passes check_net_url but resolves to a denied IP would still connect. Thread the PermissionsContainer through that helper as well, mirroring create_client_from_options in ext/fetch. Add a spec test that exercises the WebSocket post-DNS deny path.
fibibot
left a comment
There was a problem hiding this comment.
The new check still runs before hyper-util applies the destination port, so port-scoped resolved-IP denies remain bypassable. Name only contains the host; GAI resolves (&*name.host, 0), Hickory builds SocketAddr::new(x, 0), and HttpConnector::call_async calls set_port(&mut addr, port, ...) after Resolver::call returns.
check_resolved_permissions(&addrs, permissions.as_ref())?therefore callscheck_net_resolved()with port0for default fetch/WebSocket DNS resolution.--allow-net --deny-net=127.0.0.1:<port>can still allowfetch("http://localhost:<port>/"). Please run the check afterset_port(or pass the target port into the resolver path) and add a port-specific regression; the new specs only cover host-wide denies.
The DNS-layer post-resolution check ran inside `Service<Name>::call`, which only sees the hostname. hyper-util's `HttpConnector` sets the destination port after the resolver returns, so the previous check always ran with port `0` and could not enforce port-scoped deny rules like `--deny-net=127.0.0.1:<port>`. Thread the destination port through via a `tokio::task_local` that is scoped by a new `PermissionedHttpConnector<Uri>` adapter sitting in front of `HttpConnector`. The resolver reads the port synchronously before `tokio::spawn`, so the spawned check sees the real request port. The wrapper also covers the proxy paths that route through the inner HTTP connector. Add a regression spec that proves a port-scoped deny rule blocks the matching port via both literal IP and DNS-resolved hostname while a different port on the same host stays reachable.
fibibot
left a comment
There was a problem hiding this comment.
The current head fixes the port-scoped bypass: PermissionedHttpConnector captures the URI port before HttpConnector strips it down to Name, and check_resolved_permissions() now calls check_net_resolved() with that real port for GAI, Hickory, and custom resolver results. The new deny_net_fetch_resolved_port spec covers both denied and different-port-allowed paths; CI is green.
| tokio::task_local! { | ||
| /// The destination port for the in-flight HTTP/WS connection. Set by | ||
| /// [`PermissionedHttpConnector`] before invoking the inner connector so | ||
| /// that the DNS resolver below can run the post-resolution deny check | ||
| /// against the real request port instead of the placeholder `0` that | ||
| /// hyper-util's `HttpConnector` carries through `Service<Name>`. | ||
| static REQUEST_PORT: u16; | ||
| } |
There was a problem hiding this comment.
Seems kind of terrible, can't we just pass this somehow?
denoland#34236) `fetch()` and `WebSocket` both ran the static `check_net_url` and then handed the URL to hyper, whose DNS resolver in `ext/fetch/dns.rs` returned addresses straight to the connector. That meant a hostname like `localhost` could pass the URL check and still connect to a denied IP after DNS lookup. `Deno.connect`, `Deno.listen`, and friends already run `check_net_resolved` on every resolved address in `ext/net/ops.rs`; the HTTP/WS paths were the odd ones out. Thread the `PermissionsContainer` through to the fetch DNS resolver and run the same post-resolution check on each address before yielding it to the HTTP connector. Applies to the default client, clients created via `Deno.createHttpClient`, and any custom `Resolve` implementation. WebSocket reuses the fetch HTTP client, so the default and custom-client paths inherit the fix automatically. The one remaining gap was `create_client_from_websocket_options` (used when a WebSocket is opened with `ca_certs` or `unsafelyIgnoreCertificateErrors`), which built a `deno_fetch::Client` directly without threading permissions. Patched to mirror `create_client_from_options` in `ext/fetch`.
…34418) Follow-up to #34236, addressing #34236 (comment). #34236 introduced a `REQUEST_PORT` `tokio::task_local!` to smuggle the real destination port from `PermissionedHttpConnector` (which sees the `Uri`) into the DNS resolver (which is only handed a `Name` by hyper-util's `HttpConnector`). The DNS resolver then ran the post-resolution net-deny check against every resolved address with that port. This was load-bearing but pretty ugly: it relied on the task-local being read before each `tokio::spawn` inside the resolver and scoped around the inner connector's future. Instead, run both the resolution and the check in `PermissionedHttpConnector` itself, which sees the full `Uri` (and therefore the real port). The connector now owns the `Resolver` and the connector config (local address) rather than a pre-built `HttpConnector`: - For hostnames, it resolves via the `Resolver`, checks every resolved address with `check_net_resolved` before any socket is opened (matching what `Deno.connect` does in `ext/net/ops.rs`), and then hands the vetted addresses to a hyper-util `HttpConnector` through a pre-resolved resolver. The connection goes to exactly the addresses that were checked, so no second DNS query can race with a record change. - For IP-literal hosts, the literal is checked directly and resolution is skipped (hyper-util connects to literals without consulting the resolver). - Resolution errors are wrapped in a `DnsError` type that mirrors hyper-util's `ConnectError` display ("dns error" with the `io::Error` as source), so fetch error messages keep their existing `client error (Connect): dns error: ...` format. This removes the `REQUEST_PORT` task-local, the `permissions` field on `Resolver`, `Resolver::with_permissions`, and the in-resolver `check_resolved_permissions` helper. Callers pass `permissions: Option<PermissionsContainer>` via `CreateHttpClientOptions`, and `ext/fetch` and `ext/websocket` set it there. No behavior change for the existing `tests/specs/permission/deny_net_*` regression tests. ## Follow-up: proxied request destinations A request routed through a proxy only opens a socket to the proxy, so the post-resolution deny check on the connector previously only saw the proxy address. An IP-level `--deny-net` rule on the destination could therefore be bypassed by routing through an allowed proxy. `ProxyConnector` now runs a best-effort net-deny check against the real destination before connecting to the proxy: it resolves the destination host (via the new `CheckDst` trait on `PermissionedHttpConnector`) and checks every resolved address. It is best-effort because a proxy may be able to reach a host this process cannot resolve locally, so a local resolution failure is not fatal; the connection to the proxy itself is still checked separately. Covered by a new `ext/fetch` test that denies a proxied destination whose hostname resolves to a denied IP.
fetch()andWebSocketboth ran the staticcheck_net_urland thenhanded the URL to hyper, whose DNS resolver in
ext/fetch/dns.rsreturned addresses straight to the connector. That meant a hostname
like
localhostcould pass the URL check and still connect to a deniedIP after DNS lookup.
Deno.connect,Deno.listen, and friends alreadyrun
check_net_resolvedon every resolved address inext/net/ops.rs;the HTTP/WS paths were the odd ones out.
Thread the
PermissionsContainerthrough to the fetch DNS resolver andrun the same post-resolution check on each address before yielding it
to the HTTP connector. Applies to the default client, clients created
via
Deno.createHttpClient, and any customResolveimplementation.WebSocket reuses the fetch HTTP client, so the default and
custom-client paths inherit the fix automatically. The one remaining
gap was
create_client_from_websocket_options(used when a WebSocketis opened with
ca_certsorunsafelyIgnoreCertificateErrors), whichbuilt a
deno_fetch::Clientdirectly without threading permissions.Patched to mirror
create_client_from_optionsinext/fetch.