fix(security): complete SSRF blocklist with missing IANA-reserved ranges [MEDIUM]#1557
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughUpdated the SSRF denylist by adding two IPv4 CIDR ranges: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nemoclaw/src/blueprint/ssrf.ts`:
- Around line 13-23: Add unit tests exercising the SSRF denylist entries defined
with cidr and cidr6: assert that addresses in 0.0.0.0/8 and 198.18.0.0/15 are
blocked, that IPv6 fe80::/10 is blocked, and that the widened fc00::/7 correctly
blocks fc00::1 while allowing fe00::1; implement these tests against the same
function used by existing tests to evaluate host/IP blocking (the SSRF denylist
checker used alongside the cidr/cidr6 definitions) so the new ranges get
explicit regression coverage.
🪄 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
Run ID: ee788826-d082-46e6-a25f-7c4636909e09
📒 Files selected for processing (1)
nemoclaw/src/blueprint/ssrf.ts
|
✨ Thanks for submitting this fix, which proposes a way to strengthen SSRF protection by adding missing IANA-reserved IP ranges to the blocklist. |
cv
left a comment
There was a problem hiding this comment.
Security Review — WARNING (needs rebase)
The two net-new ranges are correct and should be blocked:
0.0.0.0/8(RFC 1122 "This network") — on many systems resolves to localhost198.18.0.0/15(RFC 2544 Benchmark testing) — /15 is correct for this two-/16 block
Required change
Rebase onto current main. PR #898 already landed and added fc00::/7 and fe80::/10 to the blocklist with overlapping test coverage. After rebase:
- Remove the now-redundant
fc00::/7andfe80::/10additions - De-duplicate tests that overlap with #898's boundary tests
- Update PR description to reflect reduced scope (two new ranges, not four)
What's good
- CIDR notation is correct for all ranges
- Tests cover both boundary IPs and
validateEndpointUrlintegration - No logic changes beyond the blocklist array
- No risk of blocking legitimate traffic
…IUM]
The PRIVATE_NETWORKS blocklist was missing two IANA-reserved ranges:
- 0.0.0.0/8 ("This network", RFC 1122) — resolves to localhost on
many systems, enabling the same class of attacks as 127.0.0.0/8
- 198.18.0.0/15 (Benchmark testing, RFC 2544) — reserved range that
should not be reachable in production
Add both ranges and regression tests covering their boundaries and
IPv4-mapped IPv6 equivalents.
Reported-by: FailSafe Security Researcher
Co-Authored-By: Joshua Medvinsky <joshua-medvinsky@users.noreply.github.com>
8bccf50 to
0158281
Compare
|
Updated per review feedback:
|
ericksoa
left a comment
There was a problem hiding this comment.
LGTM — purely additive blocklist entries for IANA-reserved ranges (0.0.0.0/8, 198.18.0.0/15). No logic changes, low regression risk.
Security Finding: Incomplete SSRF Blocklist in Endpoint Validation
Severity: MEDIUM
Reported by: FailSafe Security Researcher
Component:
nemoclaw/src/blueprint/ssrf.ts—PRIVATE_NETWORKSblocklistDescription
The
isPrivateIp()function in the SSRF protection module maintains a hardcoded blocklist of private IP ranges used to prevent the sandbox from connecting to internal network services. The blocklist is missing several reserved address ranges defined by IANA, allowing an attacker to route requests to internal infrastructure through addresses that bypass the filter.Missing ranges:
0.0.0.0/8— "This network" (RFC 1122). On many systems,0.0.0.0resolves to localhost, allowing the same class of attacks as127.0.0.0/8.198.18.0.0/15— Network benchmark testing (RFC 2544).fe80::/10— IPv6 link-local addresses. Entirely absent from the IPv6 portion of the blocklist.fc00::/7— Onlyfd00::/8is blocked; the parentfc00::/7unique local range is not fully covered.The
validateEndpointUrl()function resolves the hostname via DNS and passes each resolved address throughisPrivateIp(). Any address falling in the missing ranges passes the check and is accepted as a valid endpoint URL.Fix
0.0.0.0/8(RFC 1122 "this network")198.18.0.0/15(RFC 2544 benchmark)fe80::/10(IPv6 link-local)fd00::/8tofc00::/7(full unique-local range)Test plan
0.0.0.0is now blocked198.18.0.1is now blockedfe80::1is now blocked (IPv6 link-local)fc00::1is now blocked (unique-local, was previously outsidefd00::/8)Summary by CodeRabbit