Skip to content

fix(ext/fetch,ext/websocket): check resolved IPs against net deny list#34236

Merged
bartlomieju merged 3 commits into
mainfrom
fix/fetch-check-net-resolved
May 27, 2026
Merged

fix(ext/fetch,ext/websocket): check resolved IPs against net deny list#34236
bartlomieju merged 3 commits into
mainfrom
fix/fetch-check-net-resolved

Conversation

@bartlomieju

@bartlomieju bartlomieju commented May 19, 2026

Copy link
Copy Markdown
Member

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.

`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 fibibot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 fibibot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  1. 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 allow fetch("http://localhost:<port>/") after localhost resolves to 127.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.
@bartlomieju bartlomieju changed the title fix(ext/fetch): check resolved IPs against net deny list fix(ext/fetch,ext/websocket): check resolved IPs against net deny list May 26, 2026

@fibibot fibibot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

  1. check_resolved_permissions(&addrs, permissions.as_ref())? therefore calls check_net_resolved() with port 0 for default fetch/WebSocket DNS resolution. --allow-net --deny-net=127.0.0.1:<port> can still allow fetch("http://localhost:<port>/"). Please run the check after set_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 fibibot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@bartlomieju
bartlomieju merged commit 3d6c614 into main May 27, 2026
136 checks passed
@bartlomieju
bartlomieju deleted the fix/fetch-check-net-resolved branch May 27, 2026 05:59
Comment thread ext/fetch/dns.rs
Comment on lines +20 to +27
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;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems kind of terrible, can't we just pass this somehow?

littledivy pushed a commit to crowlKats/deno that referenced this pull request Jun 10, 2026
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`.
bartlomieju added a commit that referenced this pull request Jun 17, 2026
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants