feat(analyzer): detect SSRF (cloud metadata / internal-network / dynamic-host requests)#63
Conversation
rng1995
left a comment
There was a problem hiding this comment.
Review: APPROVE
Nice, focused addition. The new static_patterns_ssrf analyzer is wired correctly into both the ALL_ANALYZERS list and the dispatch dict, and the registry test is updated to match, so the registration stays consistent.
Correctness
- The metadata/internal-host/dynamic-host split (SSRF1/SSRF2/SSRF3) is sensible, and severities (HIGH/MEDIUM) line up with the risk.
- De-duplication works as claimed: the link-local
169.254.169.254metadata address is only in the SSRF1 set, while SSRF2 covers loopback/0.0.0.0/RFC1918 ranges, so a metadata hit cannot double-fire as SSRF2. Thetest_metadata_ip_not_double_flaggedtest pins this behavior. - Private-range anchors are tight (
10\.\d,192\.168\.,172\.(?:1[6-9]|2\d|3[01])\.), which avoids obvious false positives like104.x. - I don't see catastrophic-backtracking risk: the alternations are bounded and there are no nested unbounded quantifiers, so ReDoS isn't a concern here.
Tests
- Good coverage for each rule, plus a false-positive guard (normal public HTTPS request stays clean) and the node entrypoint. Claims in the description match what the diff actually does.
Minor / optional (non-blocking) — these are reasonable to defer since the PR explicitly frames this as a first pass:
- SSRF2/SSRF3 require the URL literal to immediately follow the opening paren, so keyword-argument forms (e.g.
requests.get(url="http://127.0.0.1")orrequests.get(timeout=5, url=...)) won't match. Worth a follow-up. - SSRF2 covers the metadata IP but not the rest of the link-local
169.254.0.0/16range (or IPv6fe80::), so a non-metadata link-local target slips through both rules. - The natural-language SSRF1 pattern (
(?:read|fetch|get|query) ... metadata (service|endpoint|server)) runs withIGNORECASEand maps to HIGH; it could over-trigger on prose/docs that merely describe metadata services. A lower confidence or doc-context guard might reduce noise. curl/wgettokens are inside_REQbut SSRF2/SSRF3 require a(call form, so the shell-stylecurl http://127.0.0.1won't match via those rules.
None of the above block merge — the stated gap (metadata credential-theft going unflagged) is closed cleanly. Approving.
bebf253 to
d15b8db
Compare
…c-host requests) A skill that fetches the cloud metadata endpoint (169.254.169.254) to steal IAM credentials, or makes requests to internal hosts, was not flagged by any analyzer: existing analyzers match outbound data exfiltration or exec/subprocess sinks, not inbound-to-internal requests. Add a `static_patterns_ssrf` analyzer: SSRF1 (cloud metadata access), SSRF2 (requests to loopback/link-local/private hosts), SSRF3 (request target built from a dynamic/untrusted value). Adds a "Server-Side Request Forgery" category. Add tests for detection, same-line de-duplication, false-positive guard (normal public requests), and the node entrypoint. Signed-off-by: CharmingGroot <ohyes9711@gmail.com>
d15b8db to
eb0776f
Compare
|
Rebased onto current main — conflict in |
|
I do see conflicts due to other PRs getting merged. Sorry, can you resolve these ? |
Resolved three-way conflicts in __init__.py, pattern_defaults.py, and test_registry.py — keeping both the SSRF analyzer (our branch) and the anti-refusal analyzer (upstream feat/anti-refusal-analyzer, NVIDIA#65). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: CharmingGroot <ohyes9711@gmail.com>
|
Resolved merge conflicts with upstream/main (as of the anti-refusal analyzer merge, #65). Three files had conflicts — in each case both sides added new entries to the same list/dict,
No logic changes — purely additive resolution. Full unit test suite: 937 passed, 2 pre-existing |
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Re-review after rebase — re-confirming approval.
I re-checked the activity since my approval: it's a rebase onto current main plus conflict resolution that keeps both this PR's SSRF registry entries (SSRF1-SSRF3) and the upstream anti-refusal entries (AR1-AR3) in __init__.py, pattern_defaults.py, and test_registry.py. No change to the SSRF analyzer logic, mergeable_state: clean, tests pass. The SSRF1/SSRF2/SSRF3 detection I approved (with non-blocking nits about kwarg-form URLs and link-local coverage) is unchanged. Approving.
Fixes #62.
What this adds
A skill that performs SSRF — fetching the cloud metadata endpoint (
169.254.169.254) to steal IAM credentials, or making requests to internal hosts — was not flagged by any analyzer. Adds astatic_patterns_ssrfanalyzer under a new "Server-Side Request Forgery" category:169.254.169.254,metadata.google.internal, Alibaba100.100.100.200, IPv6 IMDS) — HIGHlocalhost,127.0.0.1,10.,192.168.,172.16–31.,::1) — MEDIUMrequests.get(f"http://{host}/...")) — MEDIUMThe reproduction from the issue (a metadata credential-theft script) went from
LOW(score 13) toCRITICAL(score 84).Scope / non-overlap
Verified the reproduction triggers none of
E1–E4, taint tracking, orbehavioral_ast— this is a distinct sink (inbound-to-internal request) fromE1(outbound exfil). The metadata IP is reported by SSRF1 only (no same-line duplicate with SSRF2). First-pass coverage anchors on Python/JS request functions and can be extended.Testing
ruff check src/ tests/andruff format --check src/ tests/pass.pytest -m 'not integration'passes (606 passed, 11 skipped).static_patterns_ssrfat 100% coverage. Tests cover: metadata / internal-host / dynamic-host detection, metadata-IP de-duplication, a false-positive guard (normal public HTTPS request is not flagged), and the node entrypoint.