webhook_bus's new DNS-rebind pin (f29f25) protects only the INITIAL request — the redirect leg still validates via the old resolve-then-discard _validate_hook_url, reopening the exact TOCTOU on any redirect hop (v1.4.0)
Reproduction steps:
- v1.4.0 fixes the historical webhook DNS-rebinding SSRF (commit f29f25):
post_json() now calls _vetted_addrinfo() once, then wraps the actual
connect in a _PinnedResolution(host, vetted) context manager that patches
socket.getaddrinfo to return ONLY the pre-vetted IPs for that exact host —
closing the "guard sees a public IP, connect re-resolves to a private one"
window on the FIRST hop.
- The community-fixed redirect-SSRF guard (#541f67, present since before
this session) re-runs a check on every 302/301/303 hop via
_redirect_guard_opener's _GuardedRedirect.redirect_request:
def redirect_request(_self, req, fp, code, msg, headers, newurl):
_validate_hook_url(newurl, allow_loopback=allow_loopback)
return urllib.request.HTTPRedirectHandler.redirect_request(...)
This still calls _validate_hook_url — the OLD function that resolves the
host ONCE via socket.getaddrinfo, checks it's public, and DISCARDS the
resolved IPs. It was never updated to call _vetted_addrinfo +
_PinnedResolution the way post_json's own initial-request path now does.
- After redirect_request validates and returns a plain new Request, urllib's
normal opener machinery goes on to actually CONNECT to it — triggering a
SECOND, fully independent socket.getaddrinfo call for the SAME redirect
host, completely outside any pin (the initial request's
_PinnedResolution(_host, _vetted) context only intercepts getaddrinfo
calls for the ORIGINAL host; its own _pinned wrapper explicitly falls
through to the real getaddrinfo for any other hostname).
- Run repro_webhook_redirect_dns_rebind_v140.py against a clean v1.4.0
extraction. It builds a real WebhookClient against a genuinely public
hook_url (example.com — the v1.4.0 initial-leg pinning fix is left
completely untouched and unchallenged), then calls the REAL
_GuardedRedirect.redirect_request() exactly as urllib invokes it on a 302,
with DNS rebinding simulated only on the redirect target (PUBLIC on the
first getaddrinfo call, LOOPBACK on any call after):
redirect_request(...) -> validated and returned a new Request
getaddrinfo('rebind.attacker.test', ...) call count during redirect_request: 1
CONFIRMED — the validation consumed its only resolution and pinned
nothing; urllib's subsequent connect-time resolution is unpinned.
Expected:
The redirect leg must get the SAME protection the initial leg just did —
resolve once, vet, and pin the connect to the vetted IPs — since a redirect
target is exactly as attacker-influenceable (the Location header of a
response from a server the operator approved) as the initial hostname is,
and the module's own docstring already treats redirect-following as a first-
class SSRF vector worth a dedicated guard.
Actual:
redirect_request validates the new host with a single, throwaway DNS lookup
and hands back a Request with no pinning attached. The connect that follows
is a fresh, independent resolution a rebinding attacker's authoritative DNS
server can answer differently from the validation lookup a moment earlier —
delivering the governed POST payload (which routinely carries workflow
data, per this same module's threat model) to loopback / 169.254.169.254 /
RFC1918, exactly the outcome f29f25 was written to prevent, exactly the
outcome #541f67 was written to prevent for a STATIC redirect target, and
still open for a REBINDING one.
Suggested fix:
Give the redirect leg the same treatment as the initial leg. In
redirect_request, replace the bare _validate_hook_url call with
_vetted_addrinfo + an active _PinnedResolution for the NEW host, and hold
that pin for the duration of the connect that follows:
def redirect_request(_self, req, fp, code, msg, headers, newurl):
host, vetted = _vetted_addrinfo(newurl, allow_loopback=allow_loopback)
# keep this pin active until the redirected request completes —
# e.g. stack it in a per-opener list of active _PinnedResolution
# contexts that post_json's outer call unwinds after r.read()
_active_pins.append(_PinnedResolution(host, vetted).__enter__())
return urllib.request.HTTPRedirectHandler.redirect_request(
_self, req, fp, code, msg, headers, newurl)
Because a redirect chain can have multiple hops, each hop needs its own pin
held simultaneously (or re-entered) until the whole request completes — a
single _PinnedResolution scoped only to the initial host, as post_json uses
today, is not enough once a redirect is involved.