mirror of
https://github.com/usestrix/strix.git
synced 2026-10-16 04:28:23 +00:00
Fix CSPT fingerprint collision and correct fixture normalization paths
- SARIF class fingerprint now prefers the structured finding_class over title-keyword matching, so a CSPT finding whose title contains "path traversal" no longer collapses onto the server-side traversal fingerprint - Correct documented normalized paths in positive_direct/positive_encoded fixtures, the evidence runbook, and the skill (leading ../ segments pop both path prefixes, e.g. /admin/keys/detail, not /api/admin/keys) - Add regression tests covering the fingerprint precedence and the corrected fixture paths Addresses PR #789 review (Closes #776)
This commit is contained in:
parent
47c27aef2e
commit
302e3442b6
6 changed files with 151 additions and 24 deletions
|
|
@ -41,7 +41,7 @@ agent-browser network requests # note the intended path, e.g. /api/
|
|||
# 3b. TRAVERSAL — drive the same source with a crafted value.
|
||||
agent-browser open "<target>/#/orders/..%2f..%2fadmin%2fkeys"
|
||||
agent-browser wait --load networkidle
|
||||
agent-browser network requests # note the traversed path, e.g. /api/admin/keys
|
||||
agent-browser network requests # note the traversed path, e.g. /admin/keys/detail
|
||||
|
||||
# 4. Stop the capture and keep the HAR as the primary artifact.
|
||||
agent-browser network har stop /workspace/.agent-browser-screenshots/cspt.har
|
||||
|
|
@ -71,7 +71,7 @@ agent-browser open "<target>"
|
|||
A finding is a real CSPT only when **all** of these hold, evidenced from the HAR:
|
||||
|
||||
1. **Traversed path on the wire** — the outbound request path is the traversed
|
||||
target (e.g. `/api/admin/keys`), not the intended one
|
||||
target (e.g. `/admin/keys/detail`), not the intended one
|
||||
(`/api/orders/12345/detail`).
|
||||
2. **Ambient credentials** — that request carried the victim's session
|
||||
(`Cookie` / `Authorization` header present; read it from the HAR entry).
|
||||
|
|
@ -103,7 +103,7 @@ Intended (benign control):
|
|||
|
||||
Traversed (CSPT):
|
||||
GET /api/orders/../../admin/keys/detail
|
||||
→ browser normalizes to: GET /api/admin/keys
|
||||
→ browser normalizes to: GET /admin/keys/detail
|
||||
Cookie: session=<redacted>
|
||||
→ 200 OK (admin key material reachable via a low-priv user's fragment)
|
||||
```
|
||||
|
|
@ -119,9 +119,10 @@ python -m http.server 8000 --directory tests/fixtures/cspt
|
|||
agent-browser network har start
|
||||
agent-browser open "http://localhost:8000/positive_direct.html#/orders/../../admin/keys"
|
||||
agent-browser wait --load networkidle
|
||||
agent-browser network requests # observe the normalized /api/admin/keys request
|
||||
agent-browser network requests # observe the normalized /admin/keys/detail request
|
||||
```
|
||||
|
||||
The positive fixtures emit a traversed `/api/...` request; the negative fixtures
|
||||
The positive fixtures emit a traversed request whose path escapes the intended
|
||||
`/api/...` prefix (e.g. `/admin/keys/detail`); the negative fixtures
|
||||
(`negative_validated.html`, `negative_safe_construction.html`) either reject the
|
||||
input before the sink or keep the value out of the path entirely.
|
||||
|
|
|
|||
|
|
@ -918,20 +918,36 @@ def _class_fingerprint(rule_id: str, report: dict[str, Any]) -> str | None:
|
|||
primary fingerprint (typical case: file rename, or a fix that moves
|
||||
the vulnerable code to a new module).
|
||||
|
||||
Composite of (rule_id, vuln-class keyword extracted from title).
|
||||
Title is LLM-authored so it's stochastic at the prose level, but
|
||||
the class keyword extraction picks up the discrete vulnerability
|
||||
category, which is much more stable than the full title.
|
||||
Composite of (rule_id, vuln-class token). The class token is the
|
||||
structured ``finding_class`` when it carries signal (i.e. is present
|
||||
and not the default ``dynamic``), otherwise a keyword extracted from
|
||||
the title.
|
||||
|
||||
Falls back to the first 5 lowercased words of the title when no
|
||||
curated keyword matches. Acceptable as a fallback because the
|
||||
class fingerprint is a tiebreaker, not a primary reconciliation key.
|
||||
Preferring ``finding_class`` matters when a finding's title happens to
|
||||
contain a broader curated keyword: a CSPT report titled "Path Traversal
|
||||
via fragment" carries ``finding_class="client_side_path_traversal"``
|
||||
but its title matches the generic ``path traversal`` keyword, which
|
||||
would otherwise collapse it onto the same fingerprint as a server-side
|
||||
path-traversal finding (both key on CWE-22). The structured field is
|
||||
authoritative, so it wins; title-keyword extraction is the fallback
|
||||
only when ``finding_class`` is absent or ``dynamic``.
|
||||
|
||||
Title is LLM-authored so it's stochastic at the prose level, but the
|
||||
keyword extraction picks up the discrete vulnerability category, which
|
||||
is much more stable than the full title. Falls back to the first 5
|
||||
lowercased words of the title when no curated keyword matches.
|
||||
Acceptable as a fallback because the class fingerprint is a tiebreaker,
|
||||
not a primary reconciliation key.
|
||||
"""
|
||||
title = _string_value(report.get("title")) or ""
|
||||
keyword = _class_keyword(title) if title else ""
|
||||
if not keyword:
|
||||
finding_class = _string_value(report.get("finding_class"))
|
||||
if finding_class and finding_class != "dynamic":
|
||||
token = finding_class
|
||||
else:
|
||||
title = _string_value(report.get("title")) or ""
|
||||
token = _class_keyword(title) if title else ""
|
||||
if not token:
|
||||
return None
|
||||
composite = f"rule:{rule_id}|class:{keyword}"
|
||||
composite = f"rule:{rule_id}|class:{token}"
|
||||
return hashlib.sha256(composite.encode("utf-8")).hexdigest()
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -88,8 +88,10 @@ axios.get("/api/orders/" + params.id) // id from route param
|
|||
xhr.open("GET", baseApi + "/" + segment) // segment from postMessage
|
||||
```
|
||||
|
||||
If `userId` = `../../admin/keys#`, the browser normalizes
|
||||
`/api/users/../../admin/keys` → `/api/admin/keys`, sent with the victim's session.
|
||||
If `userId` = `../../admin/keys#` (the trailing `#` starts a fragment, dropping
|
||||
the appended `/avatar`), the browser normalizes
|
||||
`/api/users/../../admin/keys` → `/admin/keys` (the two `../` segments pop `users`
|
||||
then `api`), sent with the victim's session.
|
||||
|
||||
## Reconnaissance
|
||||
|
||||
|
|
@ -166,7 +168,7 @@ agent-browser screenshot
|
|||
**Confirmation criteria** — you have a real CSPT when *all* hold:
|
||||
|
||||
1. The **outbound request path in the proxy/HAR** is the traversed target
|
||||
(`/api/admin/keys`), not the intended one (`/api/users/<id>/avatar`).
|
||||
(e.g. `/admin/keys`), not the intended one (`/api/users/<id>/avatar`).
|
||||
2. That request carried the **victim's credentials** (session cookie /
|
||||
`Authorization` header present on the request — read it from the HAR).
|
||||
3. There is a **benign same-endpoint control**: the same flow with a normal
|
||||
|
|
|
|||
7
tests/fixtures/cspt/positive_direct.html
vendored
7
tests/fixtures/cspt/positive_direct.html
vendored
|
|
@ -4,8 +4,11 @@
|
|||
Source: URL fragment (location.hash)
|
||||
Sink: fetch()
|
||||
The fragment segment is concatenated straight into the request path with no
|
||||
validation. A hash of `../../admin/keys` normalizes the outbound request to
|
||||
/api/admin/keys, sent with the page's ambient session.
|
||||
validation. With a hash of `#/orders/../../admin/keys`, orderId becomes
|
||||
`../../admin/keys` and the request URL is built as
|
||||
`/api/orders/../../admin/keys/detail`. The browser normalizes that outbound
|
||||
path to /admin/keys/detail (the two `../` segments pop `orders` then `api`),
|
||||
sent with the page's ambient session.
|
||||
-->
|
||||
<html lang="en">
|
||||
<head><meta charset="utf-8" /><title>Order detail</title></head>
|
||||
|
|
|
|||
6
tests/fixtures/cspt/positive_encoded.html
vendored
6
tests/fixtures/cspt/positive_encoded.html
vendored
|
|
@ -4,8 +4,10 @@
|
|||
Source: query parameter (?resource=)
|
||||
Sink: XMLHttpRequest.open()
|
||||
The value is URL-decoded once, then joined into the path. A payload of
|
||||
%2e%2e%2f%2e%2e%2fadmin%2fconfig decodes to ../../admin/config and the browser
|
||||
normalizes the outbound path to /api/admin/config.
|
||||
%2e%2e%2f%2e%2e%2fadmin%2fconfig decodes to ../../admin/config, the request
|
||||
URL is built as `/api/resources/../../admin/config`, and the browser
|
||||
normalizes the outbound path to /admin/config (the two `../` segments pop
|
||||
`resources` then `api`).
|
||||
-->
|
||||
<html lang="en">
|
||||
<head><meta charset="utf-8" /><title>Resource viewer</title></head>
|
||||
|
|
|
|||
|
|
@ -178,7 +178,7 @@ def _create_kwargs(**overrides: Any) -> dict[str, Any]:
|
|||
"poc_description": "Open the app with a crafted fragment.",
|
||||
"poc_script_code": "location.hash = '#/orders/../../admin/keys'",
|
||||
"remediation_steps": "Validate the segment against an allowlist.",
|
||||
"evidence": "Outbound request path was /api/admin/keys with the session cookie.",
|
||||
"evidence": "Outbound request path was /admin/keys/detail with the session cookie.",
|
||||
"assumptions": "Assumes the victim opens a crafted link while authenticated.",
|
||||
"fix_effort": "low",
|
||||
"cvss_breakdown": _cvss(),
|
||||
|
|
@ -298,3 +298,106 @@ def test_sarif_separates_cspt_from_server_side_traversal_class(tmp_path: Path) -
|
|||
hashes = [r["properties"].get("strix_vuln_class_hash") for r in results]
|
||||
assert all(hashes), "expected a class fingerprint on both findings"
|
||||
assert hashes[0] != hashes[1], "CSPT and server-side traversal share a class fingerprint"
|
||||
|
||||
|
||||
def test_sarif_class_fingerprint_uses_finding_class_over_colliding_title(tmp_path: Path) -> None:
|
||||
# Regression: a CSPT finding whose *title* contains the generic "path
|
||||
# traversal" keyword must still be separated from a genuine server-side
|
||||
# path-traversal finding. Before the fix the class fingerprint was derived
|
||||
# from the title keyword alone, so both collapsed onto the same hash even
|
||||
# though the structured finding_class distinguished them. The structured
|
||||
# field must win.
|
||||
write_sarif(
|
||||
tmp_path,
|
||||
[
|
||||
{
|
||||
"id": "vuln-0001",
|
||||
"title": "Path Traversal via fragment", # collides on the generic keyword
|
||||
"severity": "high",
|
||||
"cwe": "CWE-22",
|
||||
"finding_class": "client_side_path_traversal",
|
||||
"timestamp": "2026-07-16 10:00:00 UTC",
|
||||
"code_locations": [{"file": "app.js", "start_line": 12}],
|
||||
},
|
||||
{
|
||||
"id": "vuln-0002",
|
||||
"title": "Path Traversal in download file parameter",
|
||||
"severity": "high",
|
||||
"cwe": "CWE-22",
|
||||
"finding_class": "dynamic",
|
||||
"timestamp": "2026-07-16 10:00:00 UTC",
|
||||
"code_locations": [{"file": "server.py", "start_line": 30}],
|
||||
},
|
||||
],
|
||||
)
|
||||
results = json.loads((tmp_path / "findings.sarif").read_text(encoding="utf-8"))["runs"][0][
|
||||
"results"
|
||||
]
|
||||
hashes = [r["properties"].get("strix_vuln_class_hash") for r in results]
|
||||
assert all(hashes), "expected a class fingerprint on both findings"
|
||||
assert hashes[0] != hashes[1], (
|
||||
"CSPT finding with a 'path traversal' title collapsed onto the "
|
||||
"server-side traversal fingerprint; finding_class must take precedence"
|
||||
)
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# Fixture path normalization (documented expected paths must be correct)
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
||||
|
||||
def _normalize_url_path(path: str) -> str:
|
||||
"""Resolve ``.`` / ``..`` segments the way a browser normalizes a URL path.
|
||||
|
||||
Leading ``..`` that would escape the root are clamped (dropped), matching
|
||||
the URL parser rather than a filesystem join.
|
||||
"""
|
||||
parts = path.split("/")
|
||||
out: list[str] = []
|
||||
for part in parts:
|
||||
if part == "..":
|
||||
if out and out[-1] not in ("", ".."):
|
||||
out.pop()
|
||||
elif part != ".":
|
||||
out.append(part)
|
||||
return "/".join(out)
|
||||
|
||||
|
||||
def test_normalize_helper_matches_documented_examples() -> None:
|
||||
# Guard the helper itself so the assertions below mean something.
|
||||
assert _normalize_url_path("/api/orders/../../admin/keys/detail") == "/admin/keys/detail"
|
||||
assert _normalize_url_path("/api/resources/../../admin/config") == "/admin/config"
|
||||
assert _normalize_url_path("/api/users/../../admin/keys") == "/admin/keys"
|
||||
|
||||
|
||||
def test_positive_direct_fixture_documents_correct_normalized_path() -> None:
|
||||
text = (FIXTURES / "positive_direct.html").read_text(encoding="utf-8")
|
||||
# The fixture builds this request URL from orderId="../../admin/keys":
|
||||
assert _normalize_url_path("/api/orders/../../admin/keys/detail") == "/admin/keys/detail"
|
||||
# ...and the comment must document that correct result, not the wrong
|
||||
# /api/admin/keys.
|
||||
assert "/admin/keys/detail" in text
|
||||
assert "/api/admin/keys" not in text
|
||||
|
||||
|
||||
def test_positive_encoded_fixture_documents_correct_normalized_path() -> None:
|
||||
text = (FIXTURES / "positive_encoded.html").read_text(encoding="utf-8")
|
||||
assert _normalize_url_path("/api/resources/../../admin/config") == "/admin/config"
|
||||
assert "/admin/config" in text
|
||||
assert "/api/admin/config" not in text
|
||||
|
||||
|
||||
def test_no_fixture_or_doc_claims_the_wrong_api_admin_path() -> None:
|
||||
# Belt-and-suspenders: the incorrect "/api/admin/..." normalized paths must
|
||||
# not reappear in any fixture, the skill, or the runbook.
|
||||
repo = Path(__file__).parent.parent
|
||||
targets = [
|
||||
FIXTURES / "positive_direct.html",
|
||||
FIXTURES / "positive_encoded.html",
|
||||
repo / "docs" / "cspt-evidence-runbook.md",
|
||||
repo / "strix" / "skills" / "vulnerabilities" / "client_side_path_traversal.md",
|
||||
]
|
||||
for path in targets:
|
||||
text = path.read_text(encoding="utf-8")
|
||||
assert "/api/admin/keys" not in text, f"{path.name} claims wrong path /api/admin/keys"
|
||||
assert "/api/admin/config" not in text, f"{path.name} claims wrong path /api/admin/config"
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue