fix(filter): pin webhook dispatch to the checked IP to close DNS-rebind TOCTOU #343

2026-07-31T19:13:31Z

KofTwentyTwo commented

2026-07-31 19:13:31 +00:00

(Migrated from github.com)

Summary

Closes #307 (WS-D10). WebhookDispatcher::dispatch_with_options previously resolved the webhook host, checked each resolved IP against is_blocked_webhook_target (the SSRF block from #203), and then handed the raw URL to reqwest, which independently re-resolved and connected — leaving a narrow DNS-rebind TOCTOU window where a malicious resolver could answer the check with a safe IP and the connect with a private one.

The dispatcher now resolves the host once via resolve_host, runs every candidate address through is_blocked_webhook_target (rejecting the whole dispatch with WebhookError::SecurityViolation if any is blocked, when block_private_webhooks is set — unchanged from #203), and then builds a per-dispatch reqwest::Client via ClientBuilder::resolve(host, checked_socket_addr) so the connection is pinned to exactly the address that was checked.
reqwest's DNS override only maps the transport-level connect address; the original hostname is still sent as the TLS SNI and Host header, so certificate validation is unaffected — no weakening of TLS.
WebhookDispatcher no longer caches a shared Client; each dispatch builds its own pinned client since the pin target varies per call.
Updated the doc comment on ValidationOptions::block_private_webhooks to describe the closed TOCTOU.

Residual caveat

Pinning removes the second-resolution TOCTOU (the one #203 left open) by construction: the checked address is the only one reqwest is permitted to connect to. What remains out of scope, and is inherent to any SSRF defense that trusts a live DNS answer: a compromised/malicious authoritative resolver could still return a bad address on the single lookup the dispatcher performs, and there is no protection against a redirect chain re-introducing new hosts — mitigated separately by redirect::Policy::none(). Both are pre-existing, out-of-scope classes, not new residuals from this change.

Test plan

cargo fmt --all -- --check
cargo check-all (clippy --all-targets --workspace -D warnings) — 0 warnings
cargo test-all — full workspace green, 0 ignored
New nuncio_filter::webhook::tests::test_pinned_client_connects_to_checked_ip_not_dns: builds a pinned_client for host checked-ip-pin.invalid (an RFC 2606 .invalid domain guaranteed to never resolve via real DNS) pointed at a wiremock::MockServer bound to loopback, and asserts the POST reaches the mock server — the only way that can succeed is via the resolve override, not DNS.
New nuncio_filter::webhook::tests::test_dispatch_reaches_pinned_loopback_mock_server: drives dispatch_with_options end-to-end against a wiremock::MockServer and asserts the signed POST is received and returns the mocked status.
Existing is_blocked_webhook_target / first_blocked_address / pass-6 unit tests unchanged and green.
No live DNS or network calls in tests — only loopback wiremock servers and synthetic address lists.

Gate output (abbreviated)

cargo fmt --all -- --check   -> clean
cargo check-all               -> Finished, 0 warnings
cargo test-all                -> all suites ok, 0 failed, 0 ignored

## Summary Closes #307 (WS-D10). `WebhookDispatcher::dispatch_with_options` previously resolved the webhook host, checked each resolved IP against `is_blocked_webhook_target` (the SSRF block from #203), and then handed the raw URL to `reqwest`, which independently re-resolved and connected — leaving a narrow DNS-rebind TOCTOU window where a malicious resolver could answer the check with a safe IP and the connect with a private one. - The dispatcher now resolves the host once via `resolve_host`, runs every candidate address through `is_blocked_webhook_target` (rejecting the whole dispatch with `WebhookError::SecurityViolation` if any is blocked, when `block_private_webhooks` is set — unchanged from #203), and then builds a per-dispatch `reqwest::Client` via `ClientBuilder::resolve(host, checked_socket_addr)` so the connection is pinned to exactly the address that was checked. - `reqwest`'s DNS override only maps the transport-level connect address; the original hostname is still sent as the TLS SNI and `Host` header, so certificate validation is unaffected — no weakening of TLS. - `WebhookDispatcher` no longer caches a shared `Client`; each dispatch builds its own pinned client since the pin target varies per call. - Updated the doc comment on `ValidationOptions::block_private_webhooks` to describe the closed TOCTOU. ## Residual caveat Pinning removes the *second-resolution* TOCTOU (the one #203 left open) by construction: the checked address is the only one `reqwest` is permitted to connect to. What remains out of scope, and is inherent to any SSRF defense that trusts a live DNS answer: a compromised/malicious authoritative resolver could still return a bad address on the *single* lookup the dispatcher performs, and there is no protection against a redirect chain re-introducing new hosts — mitigated separately by `redirect::Policy::none()`. Both are pre-existing, out-of-scope classes, not new residuals from this change. ## Test plan - [x] `cargo fmt --all -- --check` - [x] `cargo check-all` (clippy `--all-targets --workspace -D warnings`) — 0 warnings - [x] `cargo test-all` — full workspace green, 0 ignored - [x] New `nuncio_filter::webhook::tests::test_pinned_client_connects_to_checked_ip_not_dns`: builds a `pinned_client` for host `checked-ip-pin.invalid` (an RFC 2606 `.invalid` domain guaranteed to never resolve via real DNS) pointed at a `wiremock::MockServer` bound to loopback, and asserts the POST reaches the mock server — the only way that can succeed is via the resolve override, not DNS. - [x] New `nuncio_filter::webhook::tests::test_dispatch_reaches_pinned_loopback_mock_server`: drives `dispatch_with_options` end-to-end against a `wiremock::MockServer` and asserts the signed POST is received and returns the mocked status. - [x] Existing `is_blocked_webhook_target` / `first_blocked_address` / pass-6 unit tests unchanged and green. - [x] No live DNS or network calls in tests — only loopback wiremock servers and synthetic address lists. ### Gate output (abbreviated) ``` cargo fmt --all -- --check -> clean cargo check-all -> Finished, 0 warnings cargo test-all -> all suites ok, 0 failed, 0 ignored ```

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/nuncio#343