diff --git a/harness/local_harness/benchmark/ground_truth/EXAMPLE.json b/harness/local_harness/benchmark/ground_truth/EXAMPLE.json.template similarity index 100% rename from harness/local_harness/benchmark/ground_truth/EXAMPLE.json rename to harness/local_harness/benchmark/ground_truth/EXAMPLE.json.template diff --git a/harness/local_harness/benchmark/ground_truth/NodeGoat.json b/harness/local_harness/benchmark/ground_truth/NodeGoat.json new file mode 100644 index 0000000..3450b1d --- /dev/null +++ b/harness/local_harness/benchmark/ground_truth/NodeGoat.json @@ -0,0 +1,14 @@ +[ + { + "finding_id": "NODEGOAT-001", + "type": "NoSQLInjection", + "source_code": "https://github.com/OWASP/NodeGoat/tree/c5cb68a7084e4ae7dcc60e6a98768720a81841e8", + "description": "In app/data/allocations-dao.js (getByUserIdAndThreshold, lines 78-79), the user-supplied 'threshold' query parameter is interpolated directly into a MongoDB $where clause: {$where: `this.userId == ${parsedUserId} && this.stocks > '${threshold}'`}. The $where operator executes arbitrary JavaScript server-side. An attacker can inject payloads such as \"0';while(true){}//\" (DoS via infinite loop) or \"1'; return 1 == '1\" (bypass threshold filter and return all records). No sanitization or type coercion is applied to 'threshold' before query construction. The commented-out fix in the same file shows the intended remediation: parseInt the threshold and validate its range before use." + }, + { + "finding_id": "NODEGOAT-002", + "type": "InsecureDirectObjectReference", + "source_code": "https://github.com/OWASP/NodeGoat/tree/c5cb68a7084e4ae7dcc60e6a98768720a81841e8", + "description": "In app/routes/allocations.js (displayAllocations handler, line 16), the userId is taken from req.params (the URL path) rather than req.session. A commented-out block in the same file documents the correct fix: use req.session.userId. Because the current code trusts the client-supplied URL parameter, any authenticated user can view another user's allocations by substituting a different userId in the URL (e.g. GET /allocations/2 instead of /allocations/1). No authorization check verifies that the requested userId matches the authenticated session." + } +] diff --git a/harness/local_harness/benchmark/ground_truth/README.md b/harness/local_harness/benchmark/ground_truth/README.md index 01eadea..a9fee42 100644 --- a/harness/local_harness/benchmark/ground_truth/README.md +++ b/harness/local_harness/benchmark/ground_truth/README.md @@ -3,12 +3,13 @@ This directory holds the benchmark corpus: one JSON file per target repository, each containing an array of known findings the scanner is expected to detect. -**You supply your own corpus.** This repo ships only `EXAMPLE.json` — a small, -**synthetic** sample that documents the schema and points at public, -deliberately-vulnerable applications (OWASP NodeGoat / Juice Shop / WebGoat). -The commit hashes in `EXAMPLE.json` are **illustrative placeholders**; set them to -the exact commit that contains the vulnerability in your own targets before -running the benchmark. Add your own `.json` files here. +This repo ships a real starter corpus covering OWASP Juice Shop, WebGoat, and +NodeGoat (`juice-shop.json`, `WebGoat.json`, `NodeGoat.json`), each pinned to a +specific commit hash with exact file and line references. These run out of the box. + +`EXAMPLE.json.template` documents the schema but uses placeholder commit hashes; +it is excluded from the benchmark glob (only `*.json` files are loaded). Copy and +rename it if you want a starting point for new entries. ## Schema @@ -27,7 +28,7 @@ Each file is a JSON array of finding objects: | Field | Meaning | |-------|---------| -| `finding_id` | Unique label for this finding. Any stable string works; the `VULN-NNN` scheme mirrors the IDs `/vulnhunt` emits in its report. | +| `finding_id` | Stable identifier, **globally unique across the entire corpus** (not just within one file). The benchmark keys judgments and history by this ID, so collisions across files silently corrupt results. Convention: prefix with the app name (e.g. `NODEGOAT-001`, `JUICE-002`) to guarantee uniqueness. | | `type` | Vulnerability class (free-form label used in the per-type scorecard). | | `source_code` | `https://github.com/{org}/{repo}/tree/{commit_hash}` — the benchmark clones the repo at exactly this commit (`git fetch --depth=1 origin `, full-clone fallback). | | `description` | The detail the judge compares the scanner's findings against. Be specific. | diff --git a/harness/local_harness/benchmark/ground_truth/WebGoat.json b/harness/local_harness/benchmark/ground_truth/WebGoat.json new file mode 100644 index 0000000..8b3d5b0 --- /dev/null +++ b/harness/local_harness/benchmark/ground_truth/WebGoat.json @@ -0,0 +1,14 @@ +[ + { + "finding_id": "WEBGOAT-001", + "type": "PathTraversal", + "source_code": "https://github.com/WebGoat/WebGoat/tree/5142935bf7c279882c3b0fc0ecec42c447de6fd5", + "description": "In src/main/java/org/owasp/webgoat/lessons/pathtraversal/ProfileUploadBase.java (execute method, line 51), the fullName request parameter is passed unsanitized to new File(uploadDirectory, fullName). No canonicalization or path separator filtering is applied before the file is created. An attacker can supply a fullName value containing ../ sequences (e.g. '../../../etc/evil') to write the uploaded file content outside the intended per-user upload directory under /PathTraversal/{username}/. The file is written via FileCopyUtils.copy() before the attemptWasMade() check runs, meaning the traversed write succeeds even though the lesson then detects it. The vulnerable endpoint is POST /PathTraversal/profile-upload with a multipart fullName field." + }, + { + "finding_id": "WEBGOAT-002", + "type": "PathTraversal", + "source_code": "https://github.com/WebGoat/WebGoat/tree/5142935bf7c279882c3b0fc0ecec42c447de6fd5", + "description": "In src/main/java/org/owasp/webgoat/lessons/pathtraversal/ProfileZipSlip.java, a ZIP archive uploaded via POST /PathTraversal/zip-slip is extracted using ZipFile without validating entry names. Each ZipEntry name is appended to the target directory via new File(tmpZipDirectory.toFile(), e.getName()) without checking for ../ sequences. A maliciously crafted ZIP with an entry named ../../webapps/ROOT/evil.jsp can write files outside the extraction directory to arbitrary locations writable by the application server process. This is the Zip Slip variant of path traversal (CVE-2018-1002200 pattern)." + } +] diff --git a/harness/local_harness/benchmark/ground_truth/juice-shop.json b/harness/local_harness/benchmark/ground_truth/juice-shop.json new file mode 100644 index 0000000..e5db718 --- /dev/null +++ b/harness/local_harness/benchmark/ground_truth/juice-shop.json @@ -0,0 +1,14 @@ +[ + { + "finding_id": "JUICE-001", + "type": "SQLInjection", + "source_code": "https://github.com/juice-shop/juice-shop/tree/a520e158cb65c43d24e2c55d84f09b05a2511a03", + "description": "In routes/login.ts (login function, line 34), the email field from the POST request body is interpolated directly into a raw Sequelize SQL query: SELECT * FROM Users WHERE email = '${req.body.email}' AND password = '${hash}' AND deletedAt IS NULL. The email value is not parameterized or escaped. An attacker can supply the email value \"' OR 1=1--\" to bypass authentication and log in as the first user in the database (typically the admin account). The query is executed via models.sequelize.query() with no bind parameters, making it vulnerable to classic SQL injection." + }, + { + "finding_id": "JUICE-002", + "type": "DOMXss", + "source_code": "https://github.com/juice-shop/juice-shop/tree/a520e158cb65c43d24e2c55d84f09b05a2511a03", + "description": "In frontend/src/app/search-result/search-result.component.ts (line 143), the URL query parameter 'q' is passed directly to Angular's DomSanitizer.bypassSecurityTrustHtml() and assigned to this.searchValue: this.searchValue = this.sanitizer.bypassSecurityTrustHtml(queryParam). The searchValue is then rendered as innerHTML in the component template. An attacker can craft a URL such as /#/search?q= or use an img onerror payload. Because bypassSecurityTrustHtml explicitly disables Angular's built-in XSS sanitization, the injected markup is executed verbatim in the victim's browser when they visit the crafted search link." + } +] diff --git a/harness/local_harness/benchmark/run.py b/harness/local_harness/benchmark/run.py index 35a7c07..bc79fc8 100644 --- a/harness/local_harness/benchmark/run.py +++ b/harness/local_harness/benchmark/run.py @@ -46,6 +46,45 @@ from .finding_history import get_stable_findings, update_history +_REQUIRED_FIELDS = {"finding_id", "type", "source_code", "description"} +_SOURCE_URL_PREFIX = "https://github.com/" +_SOURCE_URL_TREE = "/tree/" + + +def _validate_benchmarks(benchmarks): + """Validate all loaded benchmark findings before any network I/O. + + Returns a list of error strings. Empty list means all clear. + """ + errors = [] + seen_ids = {} + for filename, findings in benchmarks: + if not isinstance(findings, list): + errors.append(f"{filename}: top-level value must be a JSON array") + continue + for i, finding in enumerate(findings): + loc = f"{filename}[{i}]" + if not isinstance(finding, dict): + errors.append(f"{loc}: each entry must be a JSON object, got {type(finding).__name__}") + continue + missing = _REQUIRED_FIELDS - set(finding.keys()) + if missing: + errors.append(f"{loc}: missing required fields: {', '.join(sorted(missing))}") + continue + fid = finding["finding_id"] + if fid in seen_ids: + errors.append(f"{loc}: duplicate finding_id '{fid}' (also in {seen_ids[fid]})") + else: + seen_ids[fid] = filename + url = finding["source_code"] + if not url.startswith(_SOURCE_URL_PREFIX) or _SOURCE_URL_TREE not in url: + errors.append( + f"{loc} ({fid}): source_code must be " + f"https://github.com/{{org}}/{{repo}}/tree/{{commit_hash}}, got: {url!r}" + ) + return errors + + def load_all_benchmarks(): """Load all benchmark JSON files. @@ -57,12 +96,22 @@ def load_all_benchmarks(): for json_file in sorted(glob.glob(pattern)): with open(json_file) as f: findings = json.load(f) + results.append((os.path.basename(json_file), findings)) + + errors = _validate_benchmarks(results) + if errors: + print("Error: benchmark corpus validation failed:") + for err in errors: + print(f" {err}") + sys.exit(1) + + for _, findings in results: for finding in findings: repo_url, repo_name, commit_hash = parse_source_url(finding["source_code"]) finding["_repo_url"] = repo_url finding["_repo_name"] = repo_name finding["_commit_hash"] = commit_hash - results.append((os.path.basename(json_file), findings)) + return results diff --git a/harness/tests/test_corpus_integrity.py b/harness/tests/test_corpus_integrity.py new file mode 100644 index 0000000..5bc045e --- /dev/null +++ b/harness/tests/test_corpus_integrity.py @@ -0,0 +1,93 @@ +"""Integrity checks for the shipped benchmark corpus. + +Validates that the real ground_truth/ files satisfy the constraints the +benchmark runner assumes: valid JSON arrays, required fields present, globally +unique finding_id values, and well-formed source_code URLs. + +Runs in ~0.05s with no network, no model, and no cloning. +""" + +import json +import os + +import pytest + +from local_harness.benchmark.run import _validate_benchmarks + +GROUND_TRUTH_DIR = os.path.join( + os.path.dirname(__file__), "..", "local_harness", "benchmark", "ground_truth" +) + + +def _load_corpus(): + """Load all *.json files from ground_truth/ the same way the runner does.""" + results = [] + for name in sorted(os.listdir(GROUND_TRUTH_DIR)): + if not name.endswith(".json"): + continue + path = os.path.join(GROUND_TRUTH_DIR, name) + with open(path) as f: + results.append((name, json.load(f))) + return results + + +def test_corpus_loads_without_errors(): + """The shipped corpus must pass _validate_benchmarks with zero errors.""" + benchmarks = _load_corpus() + assert benchmarks, "No corpus files found in ground_truth/" + errors = _validate_benchmarks(benchmarks) + assert errors == [], "Corpus validation failed:\n" + "\n".join(errors) + + +def test_finding_ids_globally_unique(): + """finding_id must be unique across all corpus files.""" + benchmarks = _load_corpus() + seen = {} + duplicates = [] + for filename, findings in benchmarks: + if not isinstance(findings, list): + continue + for finding in findings: + if not isinstance(finding, dict): + continue + fid = finding.get("finding_id") + if fid is None: + continue + if fid in seen: + duplicates.append(f"{fid} appears in both {seen[fid]} and {filename}") + else: + seen[fid] = filename + assert duplicates == [], "Duplicate finding IDs found:\n" + "\n".join(duplicates) + + +def test_validate_benchmarks_rejects_non_dict_entry(): + """A JSON array containing a non-object entry should produce a clear error.""" + benchmarks = [("bad.json", ["not-a-dict"])] + errors = _validate_benchmarks(benchmarks) + assert any("JSON object" in e for e in errors) + + +def test_validate_benchmarks_rejects_non_list_file(): + """A corpus file whose top-level value is not an array should produce a clear error.""" + benchmarks = [("bad.json", {"finding_id": "X"})] + errors = _validate_benchmarks(benchmarks) + assert any("JSON array" in e for e in errors) + + +def test_validate_benchmarks_rejects_missing_fields(): + """A finding missing required fields should produce a clear error.""" + benchmarks = [("bad.json", [{"finding_id": "X", "type": "SQLi"}])] + errors = _validate_benchmarks(benchmarks) + assert any("missing required fields" in e for e in errors) + + +def test_validate_benchmarks_rejects_bad_source_url(): + """A finding with a malformed source_code URL should produce a clear error.""" + benchmarks = [("bad.json", [{ + "finding_id": "X-001", + "type": "SQLi", + "description": "test", + "source_code": "https://github.com/org/repo/blob/abc123/file.py", + }])] + errors = _validate_benchmarks(benchmarks) + assert any("source_code" in e for e in errors)