fix(signing): block CGNAT and 6to4 relay ranges in SSRF validation - #974
Conversation
There was a problem hiding this comment.
Clean SSRF deny-list patch. Blocks two ranges the flag-based check provably misses — and it patches the holes without rewriting the flag approach into a brittle explicit prefix table, which is the right call: ipaddress tracks IANA for free.
Things I checked
- Membership check is version-safe.
_EXTRA_BLOCKED_NETWORKSholds only IPv4 networks, andany(ip in network ...)atjwks.py:293runs against a possibly-IPv6ip. CPython's_BaseNetwork.__contains__returnsFalseon a version mismatch — it does not raise (only</>do). So a v6 resolution falls through to the other flags, no crash, no gap.code-reviewerandsecurity-reviewerboth confirmed. - Metadata check still wins.
str(ip) in BLOCKED_METADATA_IPSatjwks.py:266runs unconditionally, before theif not allow_private and (...)block at:268. Alibaba's100.100.100.200sits inside100.64.0.0/10, so it's now covered twice — but the metadata arm fires first and never consultsallow_private.test_ssrf_alibaba_metadata_blocked_despite_allow_privatematches on"metadata", not"reserved range", which pins the ordering, not just the outcome. Right assertion. - Fail-closed on the wire.
default_jwks_fetcher→build_ip_pinned_transport→resolve_and_validate_host, and the validated IP is the pinned IP (ip_pinned_transport.py:156-162). The loop raises on the first reserved IP (jwks.py:276-282) rather than skip-and-pin-a-public-sibling, so a[public, CGNAT]host is rejected before any socket opens. Sync and async paths share the validator. - Load-bearing where it matters. On 3.10/3.11
is_privateisFalsefor100.64.0.0/10; the new term does the work there. On 3.12.4+/3.13 it's redundant-but-harmless. Either waymatch=\"reserved range\"holds because all reserved rejections share one message. - Semver signal correct: purely additive rejection of address space that is never a legitimate JWKS destination, no public API change —
fix:, notfix!:.
Follow-ups (non-blocking — file as issues)
- IPv6 embedded-IPv4 tunnels.
security-reviewer(Low, gated on a NAT64/6to4 gateway existing in the deployment path):64:ff9b::/96(NAT64, RFC 6052) and2002::/16(6to4, RFC 3056) embed an arbitrary IPv4 that the validator never decodes —64:ff9b::a9fe:a9fereaches169.254.169.254through a NAT64 gateway. The validator already unwrapsipv4_mappedatjwks.py:264-265; the fix mirrors that — decode the embedded IPv4 and re-run the metadata + reserved checks. Out of scope for a deny-list PR, but worth an issue.
Minor nits (non-blocking)
- v6-against-v4-network path is untested. All four params in
test_ssrf_blocks_ranges_python_flags_missare IPv4; the no-crash claim for a v6 resolution rests on CPython behavior, not a test. A one-line v6 case would document intent. 192.88.99.0/24tested only at.1. No bound coverage and noallow_privateoverride case (only CGNAT gets one). It's a /24, so an interior hit is representative — low value.
Approving on the strength of clean fail-closed ordering plus the metadata-wins regression guard.
|
Thanks for the careful read — the fail-closed ordering and metadata-precedence checks are exactly the parts I most wanted a second pair of eyes on. Both nits are addressed in b6b3f47:
On the NAT64/6to4 follow-up — I don't think there's an issue to file, and I'd rather flag that than leave a phantom vulnerability on the backlog. Both prefixes are already blocked wholesale, so there's nothing for the validator to decode: >>> ipaddress.ip_address('64:ff9b::a9fe:a9fe').is_private # NAT64 -> 169.254.169.254
True
>>> ipaddress.ip_address('2002:a9fe:a9fe::').is_private # 6to4 -> 169.254.169.254
TrueThrough the real validator, with Python classifies all of Worth noting this is the same class of gap the PR fixes, just in the other direction: CGNAT needed an explicit entry precisely because Happy to add the tunnel addresses as regression cases if you'd like them pinned against a future CPython change to those classifications — that'd be a cheap guard, and unlike the decode approach it tests the property we actually depend on. On CI: the one red job is |
There was a problem hiding this comment.
Correct fix, right shape. Reserved-range classification belongs in one central validator, and patching the two documented holes in the flag-based check — rather than rewriting to a full explicit prefix table — keeps tracking Python's IANA updates for free.
Things I checked
- IPv6-against-IPv4-network membership is safe.
any(ip in network for network in _EXTRA_BLOCKED_NETWORKS)atsrc/adcp/signing/jwks.py:293cannot raise whenipis anIPv6Address— CPython's_BaseNetwork.__contains__short-circuitsself._version != other._version → Falsebefore any netmask math (only the ordering operators raise).test_ssrf_ipv6_accepted_against_ipv4_only_extra_networkspins it rather than assuming it. Load-bearing behavior, now with a test — good. - Ordering: metadata wins. The unconditional metadata check at
jwks.py:266runs before theif not allow_private and (...)gate atjwks.py:268.100.100.100.200sits inside the new100.64.0.0/10range but is still rejected ascloud metadata IPeven underallow_private=True.test_ssrf_alibaba_metadata_blocked_despite_allow_private(match="metadata") guards exactly this. The escape hatch cannot unblock metadata. allow_privategate is correct. Both new ranges are the last term inside the existing reserved disjunction, so they short-circuit cleanly whenallow_private=True— same gate as every other reserved range. Classifying CGNAT/6to4 as reserved-class (gated) rather than metadata-class (unconditional) is the right call.- Version robustness.
100.64.0.0/10isis_private=Falseon 3.10–3.12.3, thenTrueon 3.11.9+/3.12.4+ after the IANA-registry update; the explicit range keeps the block identical across the whole 3.10–3.13 matrix.192.88.99.0/24is missed by every flag on every version — needed unconditionally. Both confirmed empirically bysecurity-reviewer. - Semver. No public API change —
_EXTRA_BLOCKED_NETWORKSis private, the diff is purely additive rejection of never-legitimate destinations.fix:is the right bump; no!warranted. - Test plan. Parametrized red-before/green-after on both ranges plus boundary bounds, IPv6 pass-through, both override cases, and the metadata-precedence regression.
getaddrinfomock shapes are right (AF_INET 2-tuple, AF_INET6 4-tuple). Full suite 5874 passed. No unchecked manual boxes.
Expert verdicts: code-reviewer — no blockers, no major, no blocking minor. security-reviewer — SHIP, no High/Medium, real hole closed, no bypass introduced.
Follow-ups (non-blocking — file as issues)
- Coverage of flag-missed special-use ranges is incomplete (
security-reviewer, Low). The two chosen are the highest-value, but on pre-fix interpreters several other IANA special-use ranges are alsois_private=False/is_reserved=False: IPv4 NAT64192.0.0.8and AS112/AMT192.31.196.0/24,192.52.193.0/24,192.175.48.0/24; and since_EXTRA_BLOCKED_NETWORKSis IPv4-only, IPv6 6to42002::/16, Teredo2001::/32, and ORCHIDv22001:20::/28. Either add them explicitly or gate onnot ip.is_globalon fixed interpreters — butis_globalis itself version-sensitive, so keeping an explicit list is defensible. Worth an issue, not a block.
Minor nits (non-blocking)
- Annotation is wider than the contents.
src/adcp/signing/jwks.py:73types the tuple astuple[ipaddress.IPv4Network | ipaddress.IPv6Network, ...]while both entries are IPv4. Harmless — reads as deliberate forward-compat for future v6 additions (see the follow-up above). No change required. A code comment noting the100.64.0.0/10entry is redundant-but-harmless on 3.11.9+/3.12.4+ would age well.
LGTM. Follow-ups noted below.
|
Follow-up and nit addressed in 61e6d92. I measured the classification across the full supported matrix (3.10.20, 3.11.13, 3.12.9, 3.13.11) rather than reasoning about it, which split the follow-up into four real gaps and three false alarms. Added — non-reserved under all six flags on every supported version:
The ORCHIDv2 entry also resolves nit 1 as a side effect — the Not added, because the flags already reject them on 3.10/3.11/3.12/3.13: 6to4 That block does depend on CPython's classification rather than on our own list, though, which is worth pinning. Two corrections, both measured:
So the entry is load-bearing on every supported version. I did not add the suggested "redundant-but-harmless on 3.11.9+/3.12.4+" comment — it would have recorded something untrue in the source, and a future reader might have deleted the entry on the strength of it. (gh-113171 did land in that window, but it moved
It would close none of the holes it was proposed for. The explicit list is not merely "defensible" here — it is the only option of the two that works. One thing your review prompted that I would not have caught: the parametrised cases were building Red-before/green-after verified on the four new ranges (exactly those four fail with the entries removed). Full suite 5889 passed, 41 skipped, 1 xfailed. |
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
|
Measurement-driven approach was the right call — the Generated by Claude Code |
`resolve_and_validate_host` classifies reserved addresses with Python's
`ipaddress` flags (`is_private`, `is_loopback`, `is_link_local`,
`is_multicast`, `is_reserved`, `is_unspecified`) plus an explicit
cloud-metadata set. Two ranges fall through every one of those flags:
* `100.64.0.0/10` — RFC 6598 carrier-grade NAT. `is_private` is False
here because RFC 6598 designates *shared* address space rather than
private space. AdCP 3.1.1 names this range explicitly in the deny list
a fetcher MUST apply before any outbound fetch to a counterparty URL
("Webhook URL validation (SSRF)", step 2), so the SDK was accepting
destinations its own spec requires it to reject. The range is routable
inside carrier and container networks, which makes it a live SSRF path
into an operator's internal network.
* `192.88.99.0/24` — RFC 7526 deprecated 6to4 relay anycast. Traffic sent
here is tunnelled by whichever relay answers, so the real destination is
unattributable. `is_reserved` does not cover it.
Adds `_EXTRA_BLOCKED_NETWORKS` and folds it into the existing check. The
new ranges sit behind the same `allow_private` gate as every other
reserved range, so on-prem and test deployments keep their escape hatch;
the cloud-metadata list stays unconditional.
Note the interaction: Alibaba's metadata IP `100.100.100.200` lives inside
`100.64.0.0/10`, so it was previously reachable only-by-name through the
metadata list. It is now covered twice, and a regression test pins that
the metadata check still wins under `allow_private=True`.
Behaviour change is purely additive rejection of address space that is
never a legitimate webhook or JWKS destination. No public API change.
Review nits on the CGNAT/6to4 deny-list change. `_EXTRA_BLOCKED_NETWORKS` holds only IPv4 networks while the membership test runs against a possibly-IPv6 address. That relies on CPython's `_BaseNetwork.__contains__` returning False across versions rather than raising — load-bearing behaviour that had no test, only an assumption. Adds two public-IPv6 cases that must pass rather than raise. Also fills the 6to4 relay range's coverage to match CGNAT's: both /24 bounds, and an `allow_private` override case, so the second range is pinned directly instead of inferred from the first. Tests only; no behaviour change.
Review follow-up: the deny list covered the two highest-value ranges the flag check misses, but not the rest of the flag-missed set. Verified across the whole supported interpreter matrix (3.10, 3.11, 3.12, 3.13) rather than assumed — four more ranges are non-reserved under all six flags on every one of them: 192.31.196.0/24 RFC 7535 AS112-v4 anycast 192.52.193.0/24 RFC 7450 AMT anycast 192.175.48.0/24 RFC 7534 AS112 direct delegation 2001:20::/28 RFC 7343 ORCHIDv2 None is ever a legitimate JWKS or webhook destination. The ORCHIDv2 entry also makes the tuple's IPv4|IPv6 annotation accurate rather than forward-looking. Three ranges named in the follow-up are NOT added, because the flags already reject them on every supported version: 6to4 2002::/16, Teredo 2001::/32, and IPv4 NAT64 192.0.0.8 (plus NAT64 64:ff9b::/96 from an earlier round). Their embedded IPv4 needs no decoding — the whole prefix is rejected before anything is unwrapped. Since that block depends on CPython's classification rather than on our list, test_ssrf_flag_covered_special_ranges_stay_blocked pins it, so a future reclassification surfaces as a failure instead of a silent hole. Two corrections to the follow-up, both measured: - 100.64.0.0/10 is is_private == False on 3.12.9 and 3.13.11, not just on pre-3.12.4 interpreters. The entry is load-bearing on every supported version, so no "redundant on newer Python" comment is warranted. - `not ip.is_global` is not a usable substitute gate: is_global reports True for the 6to4-relay, AS112, AMT and ORCHIDv2 ranges on every supported version, so it would close none of these holes. Also fixes the getaddrinfo mock shape: parametrised cases now build AF_INET 2-tuple vs AF_INET6 4-tuple sockaddrs via a helper, so the IPv6 cases exercise a record the stdlib would actually produce. Purely additive rejection of never-legitimate destinations; no public API change.
61e6d92 to
104c642
Compare
There was a problem hiding this comment.
Correct SSRF tightening. Fail-closed beats fail-open, and this closes six reserved ranges that Python's ipaddress flags genuinely miss — the deny list is the right escape hatch precisely because the flag-based default can't reach shared/anycast space.
Things I checked
- Ordering is right. The metadata check at
src/adcp/signing/jwks.py:266runs unconditionally, before theif not allow_privategate at :268. The newany(ip in network ...)folds into that gate at :320, so Alibaba100.100.100.200— which sits inside the new100.64.0.0/10— stays blocked by the metadata list even underallow_private=True.test_ssrf_alibaba_metadata_blocked_despite_allow_privatepins it. The escape hatch cannot unblock metadata. - No IPv4-mapped bypass. The
ipv4_mappedunwrap at :264-265 runs before both the metadata check and the reserved-range gate, so::ffff:100.64.0.1is unwrapped to100.64.0.1and matched against the IPv4 entries. No::ffff:path around_EXTRA_BLOCKED_NETWORKS. - No raise on the v4/v6 mismatch.
_BaseNetwork.__contains__returnsFalseon a version mismatch (only the ordering operators raise), so an IPv6 address tested against the IPv4 entries falls through to the flags rather than crashing.test_ssrf_ipv6_accepted_against_ipv4_only_extra_networkspins that load-bearing assumption instead of leaving it as a comment — right instinct. - ORCHIDv2
2001:20::/28is genuinely load-bearing. My two reviewers disagreed on this one, so I read the interpreter source:2001:20::/28is in_private_networks_exceptions(/usr/lib/python3.12/ipaddress.py:2341), andis_privateisin _private_networks AND NOT in _private_networks_exceptions(:2061-2064) — sois_privatereturns False for it despite2001::/23being private. The entry is required, not redundant, and the in-file comment is accurate. fix:is the right semver signal. Purely additive rejection of address space that is never a legitimate JWKS/webhook destination. No public-API change, no signature change,allow_privatestill covers on-prem. Security tightening is a patch-level fix, notfix!:.- Full suite reported 5874 passed;
ruff/mypyclean on both files.
Follow-ups (non-blocking — file as issues)
- The
2002::/16(6to4) reliance is patch-version dependent.2002::/16only entered CPython's IPv6_private_networks(ipaddress.py:2331) via gh-113171 — shipped in 3.10.14 / 3.11.9 / 3.12.4 / 3.13. On an unpatched sub-release still inside the declared>=3.10floor it's classified neither private nor reserved, and a host resolving to2002:a9fe:a9fe::(6to4-encapsulating169.254.169.254) would pass.test_ssrf_flag_covered_special_ranges_stay_blockedwould go red on such an interpreter, which is the pin doing its job — but if you want the block to not depend on patch level at all, add2002::/16(and belt-and-braces64:ff9b::/96) to_EXTRA_BLOCKED_NETWORKSand soften the "on every supported version" wording. Low severity: needs both an unpatched interpreter and working (deprecated) 6to4 relay routing. - PR body is stale relative to the diff. The "What" table documents 2 of the 6 added ranges and "Verified red before the fix (4 failed)" describes an earlier 4-case test; the committed change adds AS112 (
192.31.196.0/24,192.175.48.0/24), AMT (192.52.193.0/24), and ORCHIDv2, and the test carries 10 params. The in-file comment at :57-93 is the accurate record — worth syncing the body to it. Notable that the description tracks the first commit rather than the final state of a three-commit series.
Minor nits (non-blocking)
- Two sockaddr-building styles. The
_addrinfohelper (tests/conformance/signing/test_jwks.py:23) builds the correct AF_INET 2-tuple vs AF_INET6 4-tuple shapes, but the four hand-written tests (test_ssrf_ipv6_accepted_against_ipv4_only_extra_networks, bothallow_privateoverrides, the Alibaba guard) still hand-roll their tuples at :194-238. Routing them through_addrinfowould be more consistent; the raw tuples are correct as written.
LGTM. Follow-ups noted below.
Fixes #973.
What
resolve_and_validate_hostclassifies reserved addresses with Python'sipaddressflags plus an explicit cloud-metadata set. Two ranges fall through every flag:100.64.0.0/10is_privateis False — RFC 6598 designates shared address space, not private spacebuilding/by-layer/L1/security.mdx, "Webhook URL validation (SSRF)", step 2). Routable in carrier and container networks192.88.99.0/24is_reserveddoes not cover itSo the SDK was accepting counterparty destinations that its own pinned spec requires it to reject.
How
Adds
_EXTRA_BLOCKED_NETWORKSand folds it into the existing condition. Deliberately not a rewrite to a full explicit prefix table — the flag-based approach tracks Python's IANA updates for free and is the right default; this only patches the two documented holes in it.The new ranges sit behind the same
allow_privategate as every other reserved range, so on-prem and test deployments keep their escape hatch. The cloud-metadata list stays unconditional.Interaction worth noting
Alibaba's metadata IP
100.100.100.200lives inside100.64.0.0/10. It was previously reachable-by-name only throughBLOCKED_METADATA_IPS; it is now covered twice. A regression test pins that the metadata check still wins underallow_private=True, so the escape hatch cannot unblock metadata.Tests
test_ssrf_blocks_ranges_python_flags_miss— parametrised over both CGNAT bounds, a mid-range address, and the 6to4 relay range. Verified red before the fix (4 failed) and green after.Plus two behaviour-preservation guards:
test_ssrf_cgnat_honours_allow_private_overridetest_ssrf_alibaba_metadata_blocked_despite_allow_privateFull suite: 5874 passed, 41 skipped, 1 xfailed — no regressions.
ruff format,ruff check, andmypyclean on both files.Compatibility
No public API change. Purely additive rejection of address space that is never a legitimate webhook or JWKS destination — hence
fix:, notfix!:.