fix(providers): block IPv4-embedding IPv6 forms + restrict ports in SSRF defense#173
Open
ruhex wants to merge 2 commits into
Open
fix(providers): block IPv4-embedding IPv6 forms + restrict ports in SSRF defense#173ruhex wants to merge 2 commits into
ruhex wants to merge 2 commits into
Conversation
…SRF defense (#158) Two gaps in PrefixSourceUrlValidator: 1. IsBlockedAddress normalized IPv4-mapped IPv6 but did not cover IPv6 forms that EMBED a non-public IPv4 — 6to4 (2002::/16, where the last 32 bits encode an IPv4 e.g. 2002:ac10:1:: → 172.16.0.1), Teredo (2001::/32), and the deprecated IPv4-compatible form (::a.b.c.d). An attacker controlling DNS could return such an address for a peer- supplied URL and reach an internal IPv4 host via the embedding. 2. ValidateUrlAsync checked scheme but not port — a peer could supply http://internal-host:9000/... and the ConnectCallback validates only the IP, leaving the port attacker-controlled. Added 2002::/16 (6to4), 2001::/32 (Teredo), ::ffff:0:0/96 (IPv4-mapped, defense in depth), and ::/96 (IPv4-compatible) to BlockedRanges. ValidateUrlAsync now restricts the port to 80/443 (scheme defaults or their explicit forms).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPrefixSourceUrlValidator now blocks additional IPv6 forms that can embed private IPv4 addresses and rejects non-standard ports in both URL validation and live connection paths. Tests cover the new address forms and port allowlist behavior. ChangesSSRF Validator Hardening
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@BGPLite.Providers/PrefixSourceUrlValidator.cs`:
- Around line 148-156: The port allowlist in PrefixSourceUrlValidator only
covers ValidateUrlAsync, but the live fetch path still uses
CreateValidatedConnectionAsync as the SocketsHttpHandler.ConnectCallback and
passes context.DnsEndPoint.Port directly to socket.ConnectAsync. Update
CreateValidatedConnectionAsync (and any shared helper it uses) to enforce the
same 80/443 allowlist before connecting, so non-standard ports are rejected at
fetch time too.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e49d31a7-2c3a-4752-9398-1850551f0bf7
📒 Files selected for processing (2)
BGPLite.Providers/PrefixSourceUrlValidator.csBGPLite.Tests/PrefixSourceUrlValidatorTests.cs
…CodeRabbit #173) ValidateUrlAsync checked the port, but the live fetch path (CreateValidatedConnectionAsync, the SocketsHttpHandler.ConnectCallback) passed context.DnsEndPoint.Port straight to socket.ConnectAsync — so a peer URL on a non-standard port still reached internal services at fetch time. Extracted IsAllowedPort (80/443 only) and call it from both the API-submission validator and the live connect callback so the two layers cannot drift.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #158
Problem
Two gaps in
PrefixSourceUrlValidator(the #144 SSRF defense):1. IPv6 normalization was incomplete —
IsBlockedAddressmapped IPv4-mapped-to-IPv6 but did not cover IPv6 forms that embed a non-public IPv4:2002::/16(6to4) —2002:ac10:0001::encodes172.16.0.1(RFC 1918 private).2001::/32(Teredo) — can embed private IPv4 in the last 32 bits.::a.b.c.d, deprecated but parseable).An attacker controlling a DNS record can return such a 6to4 address for a peer-supplied URL and reach the connect path to an internal host.
2. No port restriction —
ValidateUrlAsyncchecked scheme is http/https but did not restrict the port. A peer can supplyhttp://internal-host:9000/...; the ConnectCallback validates the IP, but the port is attacker-controlled — internal services on non-standard ports remain reachable.Fix
BGPLite.Providers/PrefixSourceUrlValidator.cs:2002::/16(6to4),2001::/32(Teredo),::ffff:0:0/96(IPv4-mapped — also caught byIsIPv4MappedToIPv6, defense in depth),::/96(IPv4-compatible) toBlockedRanges.ValidateUrlAsyncnow restricts the port to80/443(scheme defaults or their explicit:80/:443forms).Verification
dotnet build BGPLite.sln✅dotnet test BGPLite.sln✅ 440 passed (+10 new inPrefixSourceUrlValidatorTests)IsBlockedAddress_Rejects_IPv4EmbeddingForms(Theory: 6to4 encoding private/loopback/metadata, Teredo-style embedding, IPv4-compatible, IPv4-mapped private),ValidateUrlAsync_Rejects_NonStandardPort(:9000→ rejected),ValidateUrlAsync_Accepts_StandardPorts(Theory: implicit/explicit 80 and 443).Risk / behavior change
ValidateUrlAsync. This is acceptable — CIDR-list endpoints serve on standard ports; an operator with a legitimate non-standard port can extend the allowlist later. The ConnectCallback (live defense) is unaffected — it still validates the IP for whatever port the named client was configured with.ValidateUrlAsyncis still not wired into the live fetch path (it's the API-submission-time validator) — wiring it is a separate concern tracked in the issue. This PR closes the validation-logic gaps; the live defense (ConnectCallback) already covers the IP, and now also covers the new IPv6 ranges viaIsBlockedAddress.Summary by CodeRabbit