diff --git a/findings/2026-05-14-cgn-proxy-ssrf-multimodel-catch.md b/findings/2026-05-14-cgn-proxy-ssrf-multimodel-catch.md new file mode 100644 index 0000000..aabd888 --- /dev/null +++ b/findings/2026-05-14-cgn-proxy-ssrf-multimodel-catch.md @@ -0,0 +1,77 @@ +# Finding #79: Multi-Model Security Review Catches CGN + Proxy-Assisted SSRF Gaps + +**Date:** 2026-05-14 +**Task:** Review PR #129 (rodin/review-bot) — SSRF defense for Gitea client +**Task type:** Security review — multi-model pipeline (Config A: GPT-5 investigates, Opus judges) + +## Summary + +The multi-model review pipeline's security-reviewer role caught 3 MAJOR SSRF gaps in a PR whose core +intent was SSRF defense. These findings were against the **latest commit** (934c6728) after prior +review rounds had already addressed earlier findings. The PR author believed it was ready; two other +reviewers (Sonnet, GPT) approved. Only the security-focused review pass blocked the merge. + +## Findings Caught + +### Finding 1 & 2: Carrier-Grade NAT (CGN) Bypass in Python Preflight + +**File:** `.gitea/actions/review/action.yml` (lines ~209, ~354) +**Root cause:** Python's `ipaddress.IPv4Address.is_private` returns `False` for 100.64.0.0/10 (CGN), +and `is_reserved` does not cover it either. The SSRF pre-check had two validation blocks (initial +check + re-validation before download), both missing this range. +**Impact:** Attacker-controlled hostname resolving to a CGN address could exfiltrate ACTION_TOKEN +to non-public infrastructure. +**Fix:** Use `not ip.is_global` (which returns False for private, loopback, link-local, multicast, +reserved, unspecified, AND CGN) in both validation blocks. + +### Finding 3: Proxy-Assisted SSRF in Go Client + +**File:** `gitea/client.go` (line ~143) +**Root cause:** `newSafeHTTPClient` clones `http.DefaultTransport`, retaining `ProxyFromEnvironment`. +When an HTTP(S) proxy is configured in the environment, `DialContext` connects to the proxy — which +forwards the Authorization header through — and the target host/IP is resolved by the proxy, not +the client. `safeDialContext` never validates the proxied destination. +**Impact:** An attacker supplying a private IP (e.g., 169.254.169.254) as `baseURL` could use an +environment-configured proxy to bypass the IP blocklist entirely. +**Fix:** Set `Transport.Proxy = nil` in the safe transport, since baseURL is user-supplied and the +IP validation cannot be enforced client-side when a proxy intermediates the connection. + +## Why Other Reviewers Missed These + +- **Sonnet reviewer (structural):** Approved. Focused on code structure and defense-in-depth layers; + didn't check Python network library semantics or proxy interaction. +- **GPT reviewer (operational-gaps):** Approved with a note that security gaps "should be addressed + before merge" — but still voted APPROVE. Noted the gaps existed but didn't block. +- **Security-reviewer (dedicated):** Blocked. The CGN gap requires knowing that Python's `is_private` + and `is_reserved` don't cover 100.64/10 — a non-obvious library behavior. The proxy bypass + requires understanding that OS-level proxy settings interact with `http.DefaultTransport` cloning. + +**Key pattern:** Security gaps at the intersection of: +1. Specific library semantics (Python ipaddress module edge cases) +2. OS/environment interaction (proxy environment variables + HTTP transport inheritance) +...require a dedicated security reviewer role, not general code review. Multi-model pipeline +with explicit security role is the right call for auth/crypto/network security code. + +## Pipeline Configuration + +- **Config A (Even PR# = 129):** GPT-5 investigates, Opus judges (for multi-model phase) +- **Security review:** Dedicated `security-review-bot` pass (always runs, not A/B) +- **Result:** Security pass blocked merge, preventing a subtle but real SSRF bypass + +## Lessons + +1. **`is_global` > `is_private` + `is_reserved` for SSRF checks:** Python's positive allowlist check + (`not ip.is_global`) is more robust than negative blocklist approaches. CGN is the canonical + missed case; there may be others. + +2. **HTTP transport inheritance is a security footgun:** Cloning `http.DefaultTransport` in Go + preserves proxy settings. Security-sensitive transports should always explicitly set `Proxy: nil` + unless proxy use is intentional and the proxy is trusted. + +3. **Approval from non-security reviewers ≠ security clearance:** Sonnet and GPT both approved + despite these gaps. Only a reviewer configured with security-specific review criteria blocked + the merge. Dedicated security reviewer role provides value even when other models say APPROVE. + +4. **Multi-pass reviews on security code pay off:** PR #129 had already undergone one review round + with fixes. The security gaps persisted through that round. Security-focused second pass caught + what the first pass missed.