Skip to content

Fix redirect SSRF parsing, DNS rebinding, and private-target redirects - #3

Merged
Phantom9869 merged 2 commits into
cursor/security-review-fixes-9f2bfrom
cursor/redirect-dns-safety-714f
Oct 21, 2026
Merged

Phantom9869 merged 2 commits into
cursor/security-review-fixes-9f2bfrom
cursor/redirect-dns-safety-714f

Conversation

@Phantom9869

@Phantom9869 Phantom9869 commented Oct 21, 2026 •

Copy link
Copy Markdown
Owner

Fixes the four findings from the independent review of cursor/security-review-fixes-9f2b (954e811), plus the follow-up review of head 111307d. This branch is based on that branch. Base is cursor/security-review-fixes-9f2b, not main, so it can be reviewed and folded into PR #2. Nothing here is merged, and PR #2's branch is untouched.

1. Redirect SSRF via URL parser differential (high)

urllib.parse.urlsplit and the parser requests/urllib3 connect with do not agree on a backslash in the authority. Location: http://127.0.0.1:18080\@example.com/ is host example.com to urlsplit and http://127.0.0.1:18080/%5C@example.com/ to requests, so the old same-host check passed and the client dialed loopback.

Redirects are decided on requests.Request(...).prepare().url parsed with urllib3.util.parse_url. That prepared URL is what the next hop sends, and it must survive a second prepare unchanged. The host that is checked is the host that hop connects to. Location values that contain a raw backslash, whitespace, or a control character are refused as well; the parser check still blocks the backslash case when that filter is bypassed.

%5C and @ are covered too. Both parsers agree on those, so they are not the bypass: %5C@example.com stays on example.com, and example.com@127.0.0.1 is a different host and is not followed.

2. DNS rebinding / TOCTOU (medium)

The safety check resolved with dnspython, then requests resolved again with getaddrinfo at connect time. The connection now opens a socket to the exact addresses that decision vetted. The URL host is left in place for the Host header and TLS SNI. There is no second name lookup between the check and the connect, including on redirect hops.

Residual risk:

  • A proxy resolves the name itself, so the pin would not be the address that is contacted. If this URL would actually be proxied (proxies=, or HTTP_PROXY / HTTPS_PROXY / ALL_PROXY and the host is not bypassed), redirects are not followed and that one request uses requests' normal proxy path. NO_PROXY is consulted per URL; a bypassed host keeps the pin and may follow redirects.
  • The vetted address is whatever the resolver returned for that hop. A public address that later proxies into a private network is the server itself, not a DNS rebind, and is not detectable here.
  • getaddrinfo cannot be cancelled. A lookup that exceeds its deadline is abandoned; the daemon thread stays blocked in libc until the call returns. The scan does not wait, and the thread does not keep the process alive.

3. System resolver regression (medium)

resolve_ips only used dnspython, so /etc/hosts, mDNS, and NSS were ignored (localhost could come back as a non-loopback address). That broke host_reachable, port scans, and TLS for lab names.

Resolution is one getaddrinfo (hosts file, then DNS, A and AAAA together) on a daemon thread with a single deadline. The deadline is not spent once per record type. Address literals still skip the lookup. connect still dials the resolved address rather than the name. ports.run resolves the host once and passes that list into every port connect, so a scan does not start one resolver thread per port. connect(..., resolved=ips) is keyword-only; other callers still resolve themselves.

4. Private-target redirects (medium)

Same-host redirects required every address to be public, so an internal HTTP to HTTPS hop stopped on the 301.

Choice: the user-supplied scan target is treated as private when it is a non-public address literal, or when every address from its first resolution is non-public. That covers 127.0.0.1 / 10.x / 192.168.x / ::1 and hosts-file lab names such as target.htb and localhost. An opt-in flag was the alternative; it would leave the common lab case (a name in the hosts file, HTTP to HTTPS on the same host) broken, which is the bug. The scan target is already the host the operator asked to scan.

A same-host redirect is followed when every vetted address is public, or when that original target is private and every address from a fresh lookup of the next hop is non-public. The next hop is resolved again and is not allowed to reuse the first snapshot, so a name whose first answer is public (or mixed) and whose redirect answer is private is still a pivot and is not followed. Mixed addresses fail closed in both directions. The port must be the original port or the other side of the 80/443 pair; a hop from one high port to another is not followed. If the followed hop raises, the 3xx already in hand is returned instead of the exception.

5. CA bundle and TLS verification downgrade (medium, follow-up)

The pinned session sets trust_env = False so an environment proxy cannot replace the dialed address. requests only reads REQUESTS_CA_BUNDLE / CURL_CA_BUNDLE when trust_env is true, so a target that verifies with plain requests and an internal CA failed here with CERTIFICATE_VERIFY_FAILED, and get_for_host treated that SSLError as a dead HTTPS port and retried over plain HTTP.

trust_env stays false. The bundle is copied into verify when the caller did not pass verify=False or an explicit path (REQUESTS_CA_BUNDLE, then CURL_CA_BUNDLE). A certificate verification failure, including a hostname mismatch, is re-raised as requests.exceptions.SSLError with not retrying over HTTP in the message. The preferred scheme is not cached. Fetch modules that already catch RequestException record that text on the existing "Could not fetch site" finding. A protocol mismatch (WRONG_VERSION_NUMBER on an HTTP-only port) still falls back to HTTP.

6. NO_PROXY, redirect ports, and port-scan DNS (low, follow-up)

Proxy use is decided per prepared URL with requests.utils.should_bypass_proxies and get_environ_proxies. A host listed in NO_PROXY (or proxies["no_proxy"]) keeps pinning and same-host redirects. Any other configured proxy still skips the pin and does not follow redirects. The pinned send drops the proxies mapping so select_proxy cannot send a bypassed host through the proxy anyway (trust_env false does not strip an explicit proxy dict).

Redirects may change port only for the 80↔443 pair, compared with the original request's port. A different high port is returned as the 3xx and is not fetched. A followed hop that errors (connection failure, TLS failure, and the rest of RequestException) returns that 3xx with gambit_redirects still counting only hops that succeeded. The redirect body is buffered for the caller.

Tests

python3 -m pytest -q: 180 passed.

New coverage includes a real test CA (OpenSSL) on the pinned HTTPS path: REQUESTS_CA_BUNDLE verifies pinned.gambit.test, a hostname mismatch is rejected (so this is not verify=False), and get_for_host does not open a second connection for plain HTTP. NO_PROXY is tested with a proxy pointed at a closed port and getaddrinfo blackholed, while 127.0.0.1 still follows a same-host redirect. Port changes cover same-port and 80↔443 follows, and a different high port against a local server is not contacted. Also covered: a private-only name (target.htb) and localhost follow same-host redirects; a public-then-private resolution does not; a failed hop returns the 3xx; ports.run resolves once. The CA test skips when openssl is not on PATH, so Windows CI without it stays green. pip-audit on the unchanged pins reports no known vulnerabilities for the direct requirements. CI still runs the full audit, plus pytest on 3.11 and 3.12.

Open in Web Open in Cursor 

cursoragent and others added 2 commits October 21, 2026 07:13
Validate each redirect against the URL requests will connect to, dial
the addresses that check vetted, resolve names with the system resolver
under one deadline, and follow same-host redirects for private literals.

Co-authored-by: Kamal Choubey <Phantom9869@users.noreply.github.com>
The pinned session kept trust_env off, so REQUESTS_CA_BUNDLE was ignored
and a certificate failure was retried as plain HTTP. Apply the env CA
bundle explicitly, and surface TLS verification errors instead of
downgrading. Follow same-host redirects for names that resolve only to
non-public addresses, still blocking a public-to-private pivot. Check
proxies per URL, limit redirect ports to the same port or 80/443, return
the 3xx when a followed hop fails, and resolve a port scan's host once.

Co-authored-by: Kamal Choubey <Phantom9869@users.noreply.github.com>
@Phantom9869
Phantom9869 merged commit 884c041 into cursor/security-review-fixes-9f2b Oct 21, 2026
8 checks passed
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.

2 participants