diff --git a/.github/workflows/python-tests.yml b/.github/workflows/python-tests.yml index 251aed1..03b5a9f 100644 --- a/.github/workflows/python-tests.yml +++ b/.github/workflows/python-tests.yml @@ -6,6 +6,8 @@ on: paths: - "socket_basics/**/*.py" - "tests/**/*.py" + - "tests/fixtures/**" + - "socket_basics/rules/**" - "pyproject.toml" - "uv.lock" - "action.yml" @@ -19,6 +21,8 @@ on: paths: - "socket_basics/**/*.py" - "tests/**/*.py" + - "tests/fixtures/**" + - "socket_basics/rules/**" - "pyproject.toml" - "uv.lock" - "action.yml" @@ -57,6 +61,20 @@ jobs: run: | python -m pip install --upgrade pip uv uv sync --locked --extra dev + - name: ๐Ÿ”Ž Install opengrep + # The Java rule regression tests skip when opengrep is not on PATH. + # Install the exact release the images ship (the Dockerfile ARG) so the + # tests exercise the engine users get, and require it below so a broken + # install fails the job instead of quietly skipping the module. + run: | + version="$(sed -n 's/^ARG OPENGREP_VERSION=//p' Dockerfile)" + test -n "$version" + install -d "$RUNNER_TEMP/opengrep" + curl -fsSL --retry 3 -o "$RUNNER_TEMP/opengrep/opengrep" \ + "https://github.com/opengrep/opengrep/releases/download/${version}/opengrep_manylinux_x86" + chmod +x "$RUNNER_TEMP/opengrep/opengrep" + echo "$RUNNER_TEMP/opengrep" >> "$GITHUB_PATH" + "$RUNNER_TEMP/opengrep/opengrep" --version - name: ๐Ÿ” Assert uv.lock is in sync with pyproject.toml # Catches dependency PRs (Dependabot or maintainer) that change # pyproject.toml without regenerating the lock, or vice versa. @@ -66,4 +84,6 @@ jobs: - name: ๐Ÿ“š Assert current-release docs are in sync run: python3 scripts/check_release_docs.py --check - name: ๐Ÿงช Run tests + env: + SOCKET_BASICS_REQUIRE_OPENGREP: "1" run: uv run --no-sync pytest -q tests/ diff --git a/CHANGELOG.md b/CHANGELOG.md index b5ee9a0..8595b72 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). match the Dockerfile pins; `--write` updates both. (CE-445) - Documentation for the `-heavy` image variant and for when the standard image is the right choice. (CE-445) +- Java SAST: `java-xss` (CWE-79) and `java-xpath-injection` (CWE-643) taint + rules; an OWASP Benchmark scorer (`scripts/score_owasp_benchmark.py`) with the + method and results in `docs/java-sast-benchmark.md`; and annotated Java rule + regression fixtures under `tests/fixtures/opengrep/java`, which CI now runs + against the opengrep release pinned in the Dockerfile. (#112) + +### Changed +- Socket Python CLI 2.7.0 โ†’ 2.8.0 in the heavy and app-tests images. (#112) ### Removed - The `workspace` and `GITHUB_API_URL` GitHub Action inputs. Neither had an @@ -57,6 +65,24 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). pre-commit hook examples use the published image name, and the installation guide states the Python 3.10 requirement and the npm install path for the Socket CLI. New guidance covers large repositories and facts-file size. +- **Java SAST precision and recall.** Twelve Java rules were rewritten after a + customer evaluation reported roughly 90% false positives. On six mature open + source projects (~17,400 files) the rule set now emits about 95% fewer + findings, and the lint-style rules (`java-empty-catch-block`, + `java-system-out-usage`, `java-reflection-injection`, + `java-hardcoded-credentials`) report nothing there. On OWASP Benchmark v1.2, + recall rises from 13% to 71% while precision improves from 64.5% to 76.7%. + Two systematic defects drove the recall gap: patterns written with simple + type names never matched fully qualified call sites, and crypto rules matched + exact algorithm literals instead of transformation strings. (#112) +- Java SAST false positives removed along the way: `RSA/ECB/...` is no longer a + weak cipher; a hardened cookie no longer hides an unhardened neighbour; + parameterized `JdbcTemplate`/`PreparedStatement` calls, the four-argument + LDAP `search(base, filter, args, controls)` form, `MessageDigest.update()`, + and the `Path.startsWith`/canonical-path containment idioms are no longer + reported; SnakeYAML `SafeConstructor` loads are excluded (including the 2.0 + `LoaderOptions` form) while `loadAs`/`loadAll` are now sinks; `"10.0.0.1"` is + reported as a hardcoded IP and `"10.2.3"` is not. (#112) ## [3.1.0] - 2026-09-02 diff --git a/Dockerfile.heavy b/Dockerfile.heavy index ff3bdd3..6e9c577 100644 --- a/Dockerfile.heavy +++ b/Dockerfile.heavy @@ -4,7 +4,7 @@ ARG TRUFFLEHOG_VERSION=3.96.0 ARG UV_VERSION=0.12.1 ARG OPENGREP_VERSION=v1.26.0 ARG SOCKET_NPM_CLI_VERSION=1.1.165 -ARG SOCKET_PYTHON_CLI_VERSION=2.7.0 +ARG SOCKET_PYTHON_CLI_VERSION=2.8.0 # Socket-built Trivy, pinned by digest โ€” see the note in ./Dockerfile. ARG TRIVY_IMAGE=ghcr.io/socketdev/trivy:0.73.0@sha256:e3d9d5f10250cb73b0ea9446ae1191c0f2da2f5e6173eac08a840b1812f02e0b diff --git a/app_tests/Dockerfile b/app_tests/Dockerfile index 15327de..2cf3355 100644 --- a/app_tests/Dockerfile +++ b/app_tests/Dockerfile @@ -13,7 +13,7 @@ ARG UV_VERSION=0.12.1 ARG GOSEC_VERSION=v2.28.0 ARG OPENGREP_VERSION=v1.26.0 ARG SOCKET_NPM_CLI_VERSION=1.1.165 -ARG SOCKET_PYTHON_CLI_VERSION=2.7.0 +ARG SOCKET_PYTHON_CLI_VERSION=2.8.0 # # NOT Dependabot-trackable โ€” Socket-built Trivy, pinned by digest; updated by # Socket's trivy-dist release process. See the note in the root ./Dockerfile. diff --git a/docs/java-sast-benchmark.md b/docs/java-sast-benchmark.md new file mode 100644 index 0000000..b3f5981 --- /dev/null +++ b/docs/java-sast-benchmark.md @@ -0,0 +1,297 @@ +# Java SAST rule benchmarking + +This document records how `socket_basics/rules/java.yml` is measured and what the +current numbers are. It exists so the next person changing a Java rule can tell +whether they improved it or just moved the noise around. + +## Why + +A customer SAST evaluation reported roughly 90% false positives from our Java +rules, and compared us unfavourably to CodeQL. Reproducing that on real code +confirmed it: on six mature, heavily reviewed open source Java projects +(~17,400 Java files) the rule set emitted **1,631 findings**, and a hand +adjudicated random sample of 40 of them contained **zero true positives**. + +## Corpora + +Two corpora, because they answer different questions. + +| Corpus | What it answers | Source | +|---|---|---| +| OWASP Benchmark v1.2 | Precision and recall against ground truth | `github.com/OWASP-Benchmark/BenchmarkJava` | +| Mature OSS Java projects | How much noise a real user has to triage | guava, netty, spring-framework, commons-lang, commons-io, spring-petclinic | +| WebGoat | Whether we still catch deliberately planted vulnerabilities | `github.com/WebGoat/WebGoat` | + +OWASP Benchmark ships 2,740 annotated servlets (1,415 real vulnerabilities, +1,325 deliberate non-vulnerabilities) with an `expectedresults-1.2.csv` giving +the category, CWE, and whether each case is genuinely vulnerable. It is the +standard scoring corpus for Java SAST. + +The mature-OSS corpus is the honest proxy for customer experience. These +libraries are not web applications and contain essentially no reachable +instances of these vulnerability classes, so nearly every finding is noise. +Alert volume there is the number that maps to triage burden. + +## Running it + +Pin both the engine and the corpus, or the numbers below will not reproduce. + +- **Engine.** Measured with **opengrep 1.26.0**, the release the images pin + (`OPENGREP_VERSION` in `Dockerfile` and `Dockerfile.heavy`). Rule behaviour + was identical on 1.19.0, 1.25.0 and 1.26.0 everywhere it was checked, but + re-measure if you change the pin. +- **Corpus.** BenchmarkJava at commit + [`51f0a7c`](https://github.com/OWASP-Benchmark/BenchmarkJava/commit/51f0a7cf8bb9d17ce1f6d72598c1d1c6ce90f661) + (2026-08-31). A `--depth 1` clone of `main` moves, and both the test cases and + the `expectedresults` CSV have changed between Benchmark releases. + +```bash +git clone https://github.com/OWASP-Benchmark/BenchmarkJava.git +git -C BenchmarkJava checkout 51f0a7cf8bb9d17ce1f6d72598c1d1c6ce90f661 + +opengrep --json --dataflow-traces --quiet --no-git-ignore \ + --config socket_basics/rules/java.yml \ + --output results.json \ + BenchmarkJava/src/main/java + +python3 scripts/score_owasp_benchmark.py results.json \ + BenchmarkJava/expectedresults-1.2.csv +``` + +The scorer reports per-category precision, recall, false positive rate and the +OWASP Benchmark score (`TPR - FPR`), plus per-rule TP/FP counts so you can see +which rule is responsible for a regression. + +## Regression tests + +The full corpora are not needed to catch the regressions that are easiest to +reintroduce. `tests/fixtures/opengrep/java` holds one small Java file per rule +area, annotated `// ruleid: ` for a line that must be reported and +`// ok: ` for one that must not. `tests/test_java_opengrep_rules.py` +scans them and diffs against the annotations: + +```bash +pytest tests/test_java_opengrep_rules.py +``` + +Locally the tests skip when `opengrep` is not on `PATH`. CI installs the +release pinned in `Dockerfile` and sets `SOCKET_BASICS_REQUIRE_OPENGREP=1`, so +there a missing engine fails the job instead of skipping the module. Add a case +to the relevant fixture whenever you change a rule; the substring, +qualified-name and containment-check bugs found in review are all covered there +now. + +Three behaviours worth knowing when editing these: + +- `metavariable-regex` **anchors at the start** of the metavariable text, so + every alternation branch needs its own leading `.*`. +- Prefer scoped `(?i:...)` groups over a leading global `(?i)`. A global flag + also lowercases the deliberately case-sensitive camelCase branches, which is + how `pivot`, `divisor`, `spinner` and `monkey` were matching `iv`, `pin` and + `key`. +- opengrep's default ignore list skips any path under a `tests/` directory, and + on some releases (1.19.0) that applies even to explicitly listed files. The + harness therefore scans a temporary copy of the fixtures and asserts that + every fixture was actually scanned, so an ignored file fails loudly instead + of passing every negative annotation by scanning nothing. + +## Results + +Measured with opengrep 1.26.0. + +Scan time on BenchmarkJava went from 6s to 11s, roughly +70%. The extra cost is +the taint-mode conversions and the wider sink lists. It is small in absolute +terms on a 2,766 file corpus, but it is not free, and it is worth re-checking if +more rules move to taint mode. Run the corpora sequentially: seven concurrent +opengrep processes on one machine thrash badly. + +### OWASP Benchmark v1.2 (ground truth) + +Both columns are produced by the current `scripts/score_owasp_benchmark.py`, +so they share a denominator. An earlier revision of this table quoted a Before +column from a scorer that still mapped the non-existent +`java-trust-boundary-violation` rule, which counted Benchmark's 126 +`trustbound` cases against recall on the Before side only. + +| | Before | After | +|---|---|---| +| Precision | 64.5% | **76.7%** | +| Recall | 13.2% | **71.3%** | +| False positive rate | 7.6% | 22.5% | +| Benchmark score (TPR - FPR) | 5.6 | **48.9** | +| True positives found | 176 | **950** | + +Per category, after the change: + +| Category | Precision | Recall | +|---|---|---| +| securecookie | 100.0% | 100.0% | +| weakrand | 100.0% | 100.0% | +| crypto | 100.0% | 74.6% | +| hash | 100.0% | 69.0% | +| xpathi | 60.0% | 80.0% | +| ldapi | 58.3% | 77.8% | +| xss | 67.4% | 70.7% | +| pathtraver | 56.1% | 69.2% | +| sqli | 66.8% | 57.0% | +| cmdi | 63.6% | 44.4% | + +**Cross-category findings are not counted.** Following the OWASP method, the +scorer only credits a finding when the rule's category matches the CWE the test +case targets; a finding of a different class in that file is discarded rather +than counted as a false positive. That is worth stating because it hid a real +bug. Before this change, 93 discarded pairs were `java-sql-injection` firing on +`hash` test cases, because the untyped `$TEMPLATE.update(...)` sink matched +`MessageDigest.update(input)`. Subtracting the crypto receivers fixed it and +raised `sqli` recall at the same time. 40 discarded pairs remain, all `java-xss` +on `sqli` and `xpathi` cases, where the test genuinely echoes the tainted value +into the response; those look like real findings the OWASP scoring model +discards. Counting all 40 as false positives would put overall precision at +74.3% rather than 76.7%. + +Making the SQL string the only sink argument (so parameterized +`update("... = ?", input)` calls stop being reported) and adding +`prepareStatement()` as a sink moved `sqli` from 142 to 155 true positives at +slightly higher precision. Demoting the `File` and `Path` constructors from +sinks to propagators, with filesystem operations such as `exists()` as the +sinks instead, left `pathtraver` exactly where it was: 107 of Benchmark's 268 +path traversal cases never open the file, they only probe it. + +The headline false positive rate rises because the rule set now detects seven +categories it previously scored zero on. Precision, which is the share of +emitted findings that are real, is the comparable number and it improved. + +### Mature open source Java projects (triage burden) + +| | Before | After | Change | +|---|---|---|---| +| Total findings | 1,631 | 126 | **-92%** | +| Unique findings, mature libraries only | 1,536 | 83 | **-94.6%** | + +Two caveats on these counts, both of which apply equally to the Before and +After columns: + +- **Part of the reduction is scoping, not rule logic.** Several rules gained a + `paths: exclude` block for test, benchmark and example directories. 429 of the + 1,631 baseline findings (26%) sit in paths that are now excluded, concentrated + in `java-system-out-usage` (212 of its 263, mostly netty's `example` module), + `java-insecure-random` (82 of 102) and `java-hardcoded-ip` (35 of 48). Those + three rules would still be much quieter without the exclusions, but "zero + findings" for them is scoping plus logic, not logic alone. +- **guava is counted twice.** The repository ships `guava/` and a near-identical + `android/guava/` mirror, and both are scanned. Exactly half of guava's + findings are mirror duplicates (245 of 490 before, 10 of 20 after), and the + same applies to its share of the ~17,400 file count. + +Three rules produced 74% of the original noise: `java-empty-catch-block` (645), +`java-reflection-injection` (296) and `java-system-out-usage` (263). All three +now emit zero findings on the mature-library corpus. + +### WebGoat (deliberately vulnerable) + +87 findings before, 43 after. The removed findings were lint noise +(`java-system-out-usage` 26, `java-hardcoded-ip` 5, `java-empty-catch-block` 4), +three `java-reflection-injection` matches on factory `newInstance()` calls and a +JDK dynamic proxy, and two `java-path-traversal` duplicates on `new File(...)` +constructors (one of them only ever passed to a log statement) now that the +filesystem operation rather than the constructor is the sink. The security +findings, including the Zip Slip in `ProfileZipSlip`, the default credentials in +`DefaultCredentialsTask`, and the weak PRNG in `PasswordResetLink`, are still +reported. + +## Known limits + +**OWASP Benchmark's designated false positives are adversarial toward +pattern-based engines.** A large share of them are unreachable-branch traps: + +```java +String guess = "ABC"; +char switchTarget = guess.charAt(1); // always 'B', the safe branch +switch (switchTarget) { + case 'A': bar = param; break; // tainted, but dead code + case 'B': bar = "bob"; break; // always taken +} +``` + +Resolving these requires constant propagation plus path sensitivity. opengrep's +taint analysis is path insensitive, so it reports the dead tainted branch. This +is the structural difference behind the "Semgrep matches patterns, CodeQL traces +paths" comparison, and it caps achievable precision on `sqli`, `cmdi` and +`pathtraver` regardless of how the rules are written. Do not read the remaining +FPs in those categories as fixable rule defects without checking the test case +first. + +**Declared types are all the matcher sees.** A typed metavariable such as +`(Random $R)` matches on the *declared* type, so a `SecureRandom` assigned into +a `Random`-typed variable is still treated as a weak PRNG: + +```java +Random rng = new SecureRandom(); // declared Random +String token = Long.toString(rng.nextLong(), 36); // reported +``` + +There is no clean pattern-level fix. Narrowing the source to +`new Random(...)` only would lose every case where the PRNG is held in a field, +which is the common real-world shape, so this is accepted as a known false +positive rather than traded for the recall. + +**Comment exclusions are statement scoped.** `java-empty-catch-block` excludes +a catch block carrying an explanatory comment with `pattern-not-regex`, because +comments are not AST nodes. The regex is applied to the whole matched `try` +statement, so a comment in one catch clause suppresses an empty broad catch +beside it: + +```java +try { work(); } +catch (IOException e) { // fine, fall through +} catch (Exception e) { } // not reported +``` + +Anchoring the regex to the clause that binds the exception variable is not +expressible in a single rule. The same code without the comment on the first +clause is reported. + +**Containment checks must be visible as one expression.** `java-path-traversal` +recognises `Path.startsWith`, `normalize().startsWith` and +`getCanonicalPath().startsWith` as containment when the check is written on the +variable that later reaches the sink, and the `File`/`Path` constructors are +propagators rather than sinks so the check gets a chance to run. The String +form, `String canon = f.getCanonicalPath(); if (!canon.startsWith(base))`, is +indistinguishable from a bypassable prefix blacklist such as +`name.startsWith("..")` and is not a sanitizer, so that spelling of the idiom is +still reported at the open. + +**Ambiguous names in `java-insecure-random`.** The sink matches a security +value by name, and `key` is genuinely ambiguous in Java. `rememberMeKey` (a +Benchmark true positive) and `shardKey` or `cacheKey` (map keys, not secrets) +are the same shape, so the rule reports all of them. Narrowing `key` to compound +forms only, as `java-hardcoded-credentials` does, would drop `weakrand` recall +from 100%. The short words `iv`, `pin`, `otp`, `key` and `auth` are matched only +at a word boundary, so `pivot`, `divisor`, `spinner`, `monkey` and `author` are +not reported, but `keyIndex` and `ivLen` still are. + +**`java-unsafe-deserialization` still reports library plumbing.** Guava's and +commons-lang's serialization helpers take a caller-supplied `ObjectInputStream` +and call `readObject()` on it, outside the `Serializable` contract methods the +rule excludes. Whether that is a finding depends on who calls the helper, which +is not visible to the rule. + +**Remaining noise not addressed here.** On the mature-library corpus these rules +are the largest remaining sources: + +| Rule | Findings | Cause | +|---|---|---| +| `java-template-injection` | 20 | Matches any `.process(...)` call | +| `java-xxe-vulnerability` | 14 | Matches `DocumentBuilderFactory.newInstance()` unconditionally, ignoring whether secure features are set | +| `java-unsafe-deserialization` | 24 | Library serialization helpers that accept a caller-supplied `ObjectInputStream`, plus Spring's `YamlProcessor`, whose restrictive `Constructor` and tag inspector are configured in a different method from the `loadAll()` call | +| `java-jndi-injection` | 8 | Matches any `.lookup(...)` call | +| `java-sql-injection` | 2 | `$STMT.execute(...)` still matches any method named `execute` with a tainted first argument. Two SpEL `ConstructorExecutor.execute()` matches dropped out once the SQL string became the only sink argument | +| `java-unvalidated-redirect` | 4 | All four are Spring's own `RedirectView`, which is the framework's redirect implementation | + +Every one of these is the same defect class the change fixes elsewhere: an +untyped receiver on a common method name. The pattern to follow is a typed +metavariable plus a fully qualified variant, as in `java-ldap-injection`. + +`trustbound` (CWE-501) has no rule at all; OWASP Benchmark scores 126 cases for +it, and `scripts/score_owasp_benchmark.py` deliberately omits the category so it +reads as unscored rather than as a rule at 0% recall. diff --git a/scripts/score_owasp_benchmark.py b/scripts/score_owasp_benchmark.py new file mode 100755 index 0000000..2abd9de --- /dev/null +++ b/scripts/score_owasp_benchmark.py @@ -0,0 +1,145 @@ +#!/usr/bin/env python3 +"""Score an opengrep JSON run against the OWASP Benchmark v1.2 expected results. + +Two scores are produced: + 1. Category-matched precision/recall (official OWASP Benchmark style): a finding + only counts for the CWE the test case actually targets. + 2. Raw finding volume: everything the tool emitted, which is what a customer + actually has to triage. +""" +import csv +import json +import re +import sys +from collections import defaultdict +from pathlib import Path + +# Map socket-basics java rule ids -> OWASP Benchmark category. +# Benchmark's trustbound (CWE-501) category is deliberately absent: java.yml +# has no rule for it, so it should read as unscored rather than as 0% recall. +RULE_CATEGORY = { + "java-sql-injection": "sqli", + "java-jpa-sql-injection": "sqli", + "java-command-injection": "cmdi", + "java-path-traversal": "pathtraver", + "java-ldap-injection": "ldapi", + "java-insecure-random": "weakrand", + "java-weak-crypto-md5": "hash", + "java-weak-crypto-sha1": "hash", + "java-weak-cipher": "crypto", + "java-insecure-cookie": "securecookie", + "java-xpath-injection": "xpathi", + "java-xss": "xss", + "java-template-injection": "xss", +} + +TEST_RE = re.compile(r"(BenchmarkTest\d{5})") + + +def load_expected(csv_path): + expected = {} + with open(csv_path) as fh: + for row in csv.reader(fh): + if not row or row[0].startswith("#"): + continue + name, category, real, cwe = row[0].strip(), row[1].strip(), row[2].strip(), row[3].strip() + expected[name] = {"category": category, "real": real.lower() == "true", "cwe": cwe} + return expected + + +def main(results_json, expected_csv): + expected = load_expected(expected_csv) + data = json.loads(Path(results_json).read_text()) + findings = data.get("results", []) + + # Deduplicate: one (test case, rule) pair counts once, matching how a + # reviewer triages "does this tool flag this file for this issue". + hits = set() + per_rule_total = defaultdict(int) + off_benchmark = 0 + + for f in findings: + rule = f.get("check_id", "").split(".")[-1] + per_rule_total[rule] += 1 + m = TEST_RE.search(f.get("path", "")) + if not m: + off_benchmark += 1 + continue + hits.add((m.group(1), rule)) + + # Category-matched scoring + cat_stats = defaultdict(lambda: {"tp": 0, "fp": 0, "fn": 0, "tn": 0}) + rule_stats = defaultdict(lambda: {"tp": 0, "fp": 0}) + + flagged = defaultdict(set) # test -> {categories flagged} + for test, rule in hits: + cat = RULE_CATEGORY.get(rule) + if cat is None: + continue + flagged[test].add(cat) + exp = expected.get(test) + if not exp or exp["category"] != cat: + continue + if exp["real"]: + rule_stats[rule]["tp"] += 1 + else: + rule_stats[rule]["fp"] += 1 + + scored_cats = set(RULE_CATEGORY.values()) + for test, exp in expected.items(): + cat = exp["category"] + if cat not in scored_cats: + continue + did_flag = cat in flagged.get(test, set()) + s = cat_stats[cat] + if exp["real"] and did_flag: + s["tp"] += 1 + elif exp["real"] and not did_flag: + s["fn"] += 1 + elif not exp["real"] and did_flag: + s["fp"] += 1 + else: + s["tn"] += 1 + + def pct(n, d): + return f"{100.0 * n / d:5.1f}%" if d else " -" + + print(f"\n=== {Path(results_json).name} ===") + print(f"Total raw findings emitted: {len(findings)}") + print(f"Unique (testcase, rule) pairs: {len(hits)}") + print(f"Findings outside benchmark test files: {off_benchmark}\n") + + print("--- Per-category (OWASP Benchmark scoring: CWE-matched) ---") + print(f"{'category':14} {'TP':>5} {'FP':>5} {'FN':>5} {'TN':>5} {'prec':>7} {'recall':>7} {'FP rate':>8} {'score':>7}") + tot = {"tp": 0, "fp": 0, "fn": 0, "tn": 0} + for cat in sorted(cat_stats): + s = cat_stats[cat] + for k in tot: + tot[k] += s[k] + tpr = s["tp"] / (s["tp"] + s["fn"]) if (s["tp"] + s["fn"]) else 0 + fpr = s["fp"] / (s["fp"] + s["tn"]) if (s["fp"] + s["tn"]) else 0 + print(f"{cat:14} {s['tp']:5} {s['fp']:5} {s['fn']:5} {s['tn']:5} " + f"{pct(s['tp'], s['tp'] + s['fp'])} {pct(s['tp'], s['tp'] + s['fn'])} " + f"{pct(s['fp'], s['fp'] + s['tn'])} {100 * (tpr - fpr):6.1f}") + tpr = tot["tp"] / (tot["tp"] + tot["fn"]) if (tot["tp"] + tot["fn"]) else 0 + fpr = tot["fp"] / (tot["fp"] + tot["tn"]) if (tot["fp"] + tot["tn"]) else 0 + print(f"{'TOTAL':14} {tot['tp']:5} {tot['fp']:5} {tot['fn']:5} {tot['tn']:5} " + f"{pct(tot['tp'], tot['tp'] + tot['fp'])} {pct(tot['tp'], tot['tp'] + tot['fn'])} " + f"{pct(tot['fp'], tot['fp'] + tot['tn'])} {100 * (tpr - fpr):6.1f}") + + print("\n--- Per-rule (CWE-matched TP/FP) ---") + print(f"{'rule':38} {'TP':>5} {'FP':>5} {'prec':>7} {'raw findings':>13}") + for rule in sorted(per_rule_total, key=lambda r: -per_rule_total[r]): + s = rule_stats.get(rule, {"tp": 0, "fp": 0}) + print(f"{rule:38} {s['tp']:5} {s['fp']:5} {pct(s['tp'], s['tp'] + s['fp'])} {per_rule_total[rule]:13}") + + +if __name__ == "__main__": + if len(sys.argv) != 3: + print( + "usage: score_owasp_benchmark.py " + "", + file=sys.stderr, + ) + sys.exit(2) + main(sys.argv[1], sys.argv[2]) diff --git a/socket_basics/rules/java.yml b/socket_basics/rules/java.yml index 3be0c86..184f4bc 100644 --- a/socket_basics/rules/java.yml +++ b/socket_basics/rules/java.yml @@ -3,22 +3,79 @@ rules: # Code injection via reflection - id: java-reflection-injection - message: "Code injection vulnerability detected. User-controlled input is passed to a code evaluation function, allowing arbitrary code execution. Avoid eval/exec with user input; use safe alternatives." + message: "Code injection via reflection detected. User-controlled data flows into dynamic class loading or script evaluation, letting an attacker load arbitrary classes or run arbitrary code. Resolve the value through a fixed allowlist instead of passing request data to Class.forName or a script engine." severity: CRITICAL languages: [java] - pattern-either: - - pattern: Class.forName($USER_INPUT) - - pattern: $CLASS.newInstance() - - pattern: $METHOD.invoke($OBJ, $USER_INPUT) - - pattern: Runtime.getRuntime().exec($USER_INPUT) + mode: taint + pattern-sources: + # Servlet request sources + - pattern: $REQ.getParameter(...) + - pattern: $REQ.getParameterValues(...) + - pattern: $REQ.getHeader(...) + - pattern: $REQ.getHeaders(...) + - pattern: $REQ.getQueryString() + - pattern: $REQ.getPathInfo() + - pattern: $REQ.getRequestURI() + # Type constrained: an untyped $REQ.getInputStream() also matched + # Resource.getInputStream(), which is a classpath resource, not input. + - pattern: (HttpServletRequest $R).getInputStream() + - pattern: (HttpServletRequest $R).getReader() + - pattern: (javax.servlet.http.HttpServletRequest $R).getInputStream() + - pattern: (javax.servlet.http.HttpServletRequest $R).getReader() + - pattern: (Cookie $C).getValue() + # Spring MVC parameter binding + - patterns: + - pattern: $PARAM + - pattern-inside: | + $RET $METHOD(..., @RequestParam(...) $TYPE $PARAM, ...) { ... } + - patterns: + - pattern: $PARAM + - pattern-inside: | + $RET $METHOD(..., @PathVariable(...) $TYPE $PARAM, ...) { ... } + - patterns: + - pattern: $PARAM + - pattern-inside: | + $RET $METHOD(..., @RequestBody $TYPE $PARAM, ...) { ... } + pattern-propagators: + - pattern: (String $A) + (String $B) + from: $B + to: $A + - pattern: (String $A).concat($B) + from: $B + to: $A + - pattern: String.format($FMT, ..., $X, ...) + from: $X + to: String.format + - pattern: $B.append($X) + from: $X + to: $B + pattern-sinks: + # Dynamic class loading + - pattern: Class.forName(...) + - pattern: java.lang.Class.forName(...) + - pattern: $LOADER.loadClass(...) + # Script and expression evaluation + - pattern: (ScriptEngine $E).eval(...) + - pattern: (GroovyShell $S).evaluate(...) + - pattern: (GroovyShell $S).parse(...) + - pattern: (ExpressionParser $P).parseExpression(...) + pattern-sanitizers: + # Resolving the value through an allowlist removes attacker control + - pattern: $MAP.get(...) + # String.valueOf is a conversion, not an allowlist lookup, so it must not + # launder taint. Integer.valueOf and Long.valueOf stay sanitizers: a + # parsed number cannot name a class. + - patterns: + - pattern: $ENUM.valueOf(...) + - pattern-not: String.valueOf(...) metadata: category: security - cwe: CWE-94 - confidence: medium + cwe: CWE-470 + confidence: high subcategory: injection vulnerability_class: "Injection Vulnerability" owasp: "A03:2021" - fix: "Avoid ScriptEngine.eval() with user input. Use a sandboxed interpreter or template engine. Restrict class loading with a SecurityManager." + fix: "Map the user-supplied value to a class or handler through an explicit allowlist (for example a Map>) rather than passing it to Class.forName(). Never pass request data to ScriptEngine.eval()." # SQL injection - using taint mode for accurate detection - id: java-sql-injection @@ -72,22 +129,59 @@ rules: from: $B to: $A pattern-sinks: - # JDBC statement execution - - pattern: $STMT.executeQuery(...) - - pattern: $STMT.execute(...) - - pattern: $STMT.executeUpdate(...) - - pattern: $STMT.executeLargeUpdate(...) - # JPA/Hibernate query creation - - pattern: $EM.createQuery(...) - - pattern: $EM.createNativeQuery(...) - - pattern: $SESSION.createQuery(...) - - pattern: $SESSION.createSQLQuery(...) - # JDBC template methods - - pattern: $TEMPLATE.query(...) - - pattern: $TEMPLATE.queryForObject(...) - - pattern: $TEMPLATE.queryForList(...) - - pattern: $TEMPLATE.execute(...) - - pattern: $TEMPLATE.update(...) + # Only the SQL string is the sink. The other arguments of these APIs are + # bind parameters, row mappers and result types, so a parameterized call + # such as update("... = ?", input) is the remediation, not an injection. + - patterns: + - pattern-either: + # JDBC statement execution and preparation. The prepare sinks are + # what catch a concatenated query handed to prepareStatement() + # and then run with a no-argument execute(). + - pattern: $STMT.executeQuery($SQL, ...) + - pattern: $STMT.execute($SQL, ...) + - pattern: $STMT.executeUpdate($SQL, ...) + - pattern: $STMT.executeLargeUpdate($SQL, ...) + - pattern: $STMT.addBatch($SQL) + - pattern: $CONN.prepareStatement($SQL, ...) + - pattern: $CONN.prepareCall($SQL, ...) + # JPA/Hibernate query creation + - pattern: $EM.createQuery($SQL, ...) + - pattern: $EM.createNativeQuery($SQL, ...) + - pattern: $SESSION.createQuery($SQL, ...) + - pattern: $SESSION.createSQLQuery($SQL, ...) + # Spring JDBC template methods. Typing the receiver is not + # workable here: the template is routinely reached through a + # static field or an interface, so a typed metavariable misses + # most real call sites. Keep the broad receiver and subtract the + # collisions, which are all update() on a crypto primitive. + - pattern: $TEMPLATE.query($SQL, ...) + - pattern: $TEMPLATE.queryForObject($SQL, ...) + - pattern: $TEMPLATE.queryForList($SQL, ...) + - pattern: $TEMPLATE.queryForMap($SQL, ...) + - pattern: $TEMPLATE.queryForRowSet($SQL, ...) + - pattern: $TEMPLATE.update($SQL, ...) + - pattern: $TEMPLATE.batchUpdate($SQL, ...) + - pattern-not: (MessageDigest $D).update(...) + - pattern-not: (java.security.MessageDigest $D).update(...) + - pattern-not: (Mac $M).update(...) + - pattern-not: (javax.crypto.Mac $M).update(...) + - pattern-not: (Cipher $C).update(...) + - pattern-not: (javax.crypto.Cipher $C).update(...) + - pattern-not: (Signature $S).update(...) + - pattern-not: (java.security.Signature $S).update(...) + - pattern-not: (Checksum $K).update(...) + - pattern-not: (java.util.zip.Checksum $K).update(...) + # An inline chain has no declared receiver for the typed subtraction + # to see, so name the factory calls as well. + - pattern-not: MessageDigest.getInstance(...).update(...) + - pattern-not: java.security.MessageDigest.getInstance(...).update(...) + - pattern-not: Mac.getInstance(...).update(...) + - pattern-not: javax.crypto.Mac.getInstance(...).update(...) + - pattern-not: Cipher.getInstance(...).update(...) + - pattern-not: javax.crypto.Cipher.getInstance(...).update(...) + - pattern-not: Signature.getInstance(...).update(...) + - pattern-not: java.security.Signature.getInstance(...).update(...) + - focus-metavariable: $SQL pattern-sanitizers: # PreparedStatement parameter binding - pattern: $STMT.setString(...) @@ -114,14 +208,70 @@ rules: # Deserialization vulnerabilities - id: java-unsafe-deserialization - message: "Unsafe deserialization detected. Deserializing untrusted data can lead to remote code execution or denial of service. Use safe serialization formats like JSON or validate data before deserializing." + message: "Unsafe deserialization detected. Java native deserialization of attacker-controlled bytes leads to remote code execution through gadget chains. Use a data-only format such as JSON or Protocol Buffers, or install an ObjectInputFilter (JEP 290) that allowlists deserializable classes." severity: CRITICAL languages: [java] + paths: + exclude: + - "*/src/test/*" + - "*/src/testFixtures/*" + - "*/test/java/*" + - "*/testsuite/*" + - "*/microbench/*" + - "*/jmh/*" + - "*Test.java" + - "*Tests.java" + - "*TestCase.java" + - "*Benchmark.java" pattern-either: - - pattern: new ObjectInputStream($STREAM).readObject() - - pattern: $OIS.readObject() - - pattern: XMLDecoder.readObject() - - pattern: Yaml.load($INPUT) + # Java native deserialization and XMLDecoder. The receiver must actually + # be an ObjectInputStream: a bare $X.readObject() also matches unrelated + # APIs such as BouncyCastle's PEMParser and JBoss Marshalling's + # Unmarshaller, which are not Java deserialization. + - patterns: + - pattern-either: + - pattern: (ObjectInputStream $OIS).readObject() + - pattern: new ObjectInputStream(...).readObject() + - pattern: (XMLDecoder $D).readObject() + - pattern: new XMLDecoder(...).readObject() + # Implementing the java.io.Serializable contract requires calling + # readObject() on the stream the JVM hands you. That is not a finding. + - pattern-not-inside: | + private void readObject($T $S) { ... } + - pattern-not-inside: | + private void readObject($T $S) throws $EX { ... } + - pattern-not-inside: | + public void readExternal($T $S) { ... } + - pattern-not-inside: | + public void readExternal($T $S) throws $EX { ... } + # SnakeYAML without a SafeConstructor deserializes arbitrary types, + # through load(), loadAs() and loadAll() alike. + - patterns: + - pattern: new Yaml().$LOAD(...) + - metavariable-regex: + metavariable: $LOAD + regex: ^(load|loadAs|loadAll)$ + - patterns: + - pattern: (Yaml $Y).$LOAD(...) + - metavariable-regex: + metavariable: $LOAD + regex: ^(load|loadAs|loadAll)$ + # A SafeConstructor load is the remediation this rule recommends. + # SnakeYAML 2.0 removed the no-arg SafeConstructor, so the argument + # list has to stay open here. + - pattern-not: new Yaml(new SafeConstructor(...), ...).$LOAD(...) + - pattern-not: new Yaml(new org.yaml.snakeyaml.constructor.SafeConstructor(...), ...).$LOAD(...) + # Bound to the receiver: $Y here is the $Y of the load above. These + # exclusions live in their own branch because a metavariable the + # positive pattern does not bind is free inside pattern-not-inside, + # and a free $Y let one SafeConstructor field exclude every + # readObject() finding in the same class. + - pattern-not-inside: | + $T $Y = new Yaml(new SafeConstructor(...), ...); + ... + - pattern-not-inside: | + $T $Y = new Yaml(new org.yaml.snakeyaml.constructor.SafeConstructor(...), ...); + ... metadata: category: security cwe: CWE-502 @@ -129,7 +279,7 @@ rules: subcategory: integrity vulnerability_class: "Insecure Deserialization" owasp: "A08:2021" - fix: "Use ObjectInputFilter (JEP 290) to restrict deserializable classes. Prefer JSON (Jackson/Gson) or Protocol Buffers for data interchange." + fix: "Use ObjectInputFilter (JEP 290) to restrict deserializable classes. Prefer JSON (Jackson/Gson) or Protocol Buffers for data interchange. For SnakeYAML use new Yaml(new SafeConstructor())." # Command injection - using taint mode for accurate detection - id: java-command-injection @@ -192,40 +342,237 @@ rules: # LDAP injection - id: java-ldap-injection - message: "LDAP injection vulnerability detected. User input in LDAP queries without sanitization allows attackers to modify query logic. Escape special characters in LDAP filters." + message: "LDAP injection vulnerability detected. User-controlled data is concatenated into an LDAP search filter or distinguished name, letting an attacker rewrite the query and retrieve or modify directory entries they should not reach. Escape the value or bind it as a filter argument." severity: CRITICAL languages: [java] - pattern-either: - - pattern: $CTX.search($FILTER + $USER_INPUT, ...) - - pattern: new SearchFilter($FILTER + $USER_INPUT) - - pattern: LdapName($DN + $USER_INPUT) + mode: taint + paths: + exclude: + - "*/src/test/*" + - "*/src/testFixtures/*" + - "*/test/java/*" + - "*/testsuite/*" + - "*/microbench/*" + - "*/jmh/*" + - "*Test.java" + - "*Tests.java" + - "*TestCase.java" + - "*Benchmark.java" + pattern-sources: + - pattern: $REQ.getParameter(...) + - pattern: $REQ.getParameterValues(...) + - pattern: $REQ.getParameterMap() + - pattern: $REQ.getHeader(...) + - pattern: $REQ.getHeaders(...) + - pattern: $REQ.getQueryString() + - pattern: $REQ.getPathInfo() + - pattern: $REQ.getRequestURI() + # Type constrained: an untyped $REQ.getInputStream() also matched + # Resource.getInputStream(), which is a classpath resource, not input. + - pattern: (HttpServletRequest $R).getInputStream() + - pattern: (HttpServletRequest $R).getReader() + - pattern: (javax.servlet.http.HttpServletRequest $R).getInputStream() + - pattern: (javax.servlet.http.HttpServletRequest $R).getReader() + # Cookie values are attacker controlled and are the source in a large + # share of real path traversal and injection findings. + - pattern: (Cookie $C).getValue() + - pattern: (javax.servlet.http.Cookie $C).getValue() + # Archive entry names and uploaded filenames are attacker controlled. + # ZipEntry.getName() is the Zip Slip source; MultipartFile carries the + # client-supplied filename verbatim. + - pattern: (ZipEntry $E).getName() + - pattern: (java.util.zip.ZipEntry $E).getName() + - pattern: (ArchiveEntry $E).getName() + - pattern: (MultipartFile $F).getOriginalFilename() + - pattern: (org.springframework.web.multipart.MultipartFile $F).getOriginalFilename() + - pattern: (Part $P).getSubmittedFileName() + - patterns: + - pattern: $PARAM + - pattern-inside: | + $RET $METHOD(..., @RequestParam(...) $TYPE $PARAM, ...) { ... } + - patterns: + - pattern: $PARAM + - pattern-inside: | + $RET $METHOD(..., @PathVariable(...) $TYPE $PARAM, ...) { ... } + pattern-propagators: + - pattern: (String $A) + (String $B) + from: $B + to: $A + - pattern: (String $A).concat($B) + from: $B + to: $A + - pattern: String.format($FMT, ..., $X, ...) + from: $X + to: String.format + - pattern: $B.append($X) + from: $X + to: $B + - pattern: (StringBuilder $B).toString() + from: $B + to: (StringBuilder $B).toString() + # URL decoding is not sanitisation; it preserves attacker control and is + # applied to almost every header-sourced value in real servlet code. + - pattern: URLDecoder.decode($X, ...) + from: $X + to: URLDecoder.decode + - pattern: java.net.URLDecoder.decode($X, ...) + from: $X + to: java.net.URLDecoder.decode + - pattern: new String($X) + from: $X + to: new String + - pattern: new String($X, ...) + from: $X + to: new String + - pattern: (String $S).substring(...) + from: $S + to: (String $S).substring(...) + - pattern: (String $S).trim() + from: $S + to: (String $S).trim() + - pattern: (String $S).toLowerCase(...) + from: $S + to: (String $S).toLowerCase(...) + - pattern: (String $S).replace(...) + from: $S + to: (String $S).replace(...) + - pattern: (String $S).getBytes(...) + from: $S + to: (String $S).getBytes(...) + - pattern: $E.nextElement() + from: $E + to: $E.nextElement() + - pattern: Base64.decodeBase64($X) + from: $X + to: Base64.decodeBase64 + - pattern: org.apache.commons.codec.binary.Base64.decodeBase64($X) + from: $X + to: org.apache.commons.codec.binary.Base64.decodeBase64 + - pattern: Base64.encodeBase64($X) + from: $X + to: Base64.encodeBase64 + - pattern: org.apache.commons.codec.binary.Base64.encodeBase64($X) + from: $X + to: org.apache.commons.codec.binary.Base64.encodeBase64 + pattern-sinks: + - patterns: + - pattern-either: + - pattern: (DirContext $CTX).search(...) + - pattern: (InitialDirContext $CTX).search(...) + - pattern: (javax.naming.directory.DirContext $CTX).search(...) + - pattern: (javax.naming.directory.InitialDirContext $CTX).search(...) + - pattern: (LdapContext $CTX).search(...) + - pattern: (javax.naming.ldap.LdapContext $CTX).search(...) + - pattern: (InitialLdapContext $CTX).search(...) + - pattern: (javax.naming.ldap.InitialLdapContext $CTX).search(...) + - pattern: (LdapOperations $T).search(...) + - pattern: (org.springframework.ldap.core.LdapOperations $T).search(...) + - pattern: (LdapTemplate $T).search(...) + # The four-argument form with a literal filter expression is the + # parameterized API: values passed through filterArgs are escaped by + # the provider. It is the remediation this rule's own fix text + # recommends, so it must not be reported. + - pattern-not: $CTX.search($NAME, "...", $ARGS, $CONTROLS) + - pattern: new SearchFilter(...) + - pattern: new LdapName(...) + - pattern: new javax.naming.ldap.LdapName(...) + - pattern: (LdapQueryBuilder $Q).filter(...) + pattern-sanitizers: + - pattern: LdapEncoder.filterEncode(...) + - pattern: org.springframework.ldap.support.LdapEncoder.filterEncode(...) + - pattern: LdapEncoder.nameEncode(...) + - pattern: ESAPI.encoder().encodeForLDAP(...) + - pattern: $ENC.encodeForLDAP(...) + - pattern: $ENC.encodeForDN(...) + - pattern: Integer.parseInt(...) metadata: category: security cwe: CWE-90 - confidence: medium + confidence: high subcategory: injection vulnerability_class: "Injection Vulnerability" owasp: "A03:2021" - fix: "Use javax.naming.ldap with properly escaped filter values. Use LdapEncoder.filterEncode() from Spring LDAP for escaping." - - # === High Severity Rules === + fix: "Escape the value with LdapEncoder.filterEncode() from Spring LDAP, or pass it as a bound filter argument via search(base, '(uid={0})', new Object[]{ value }, controls)." # Hardcoded credentials - id: java-hardcoded-credentials message: "Hard-coded credentials detected. Embedding secrets in source code makes them easily discoverable and impossible to rotate. Use environment variables or a secrets manager instead." severity: HIGH languages: [java] - patterns: - - pattern-either: - - pattern: | - private static final String $VAR = "..."; - - pattern: | - public static final String $VAR = "..."; - - pattern: | - String $VAR = "..."; - - metavariable-regex: - metavariable: $VAR - regex: (?i).*(password|passwd|pwd|secret|token|key|api_key).* + paths: + exclude: + - "*/src/test/*" + - "*/src/testFixtures/*" + - "*/test/java/*" + - "*/testsuite/*" + - "*/microbench/*" + - "*/jmh/*" + - "*Test.java" + - "*Tests.java" + - "*TestCase.java" + - "*Benchmark.java" + pattern-either: + # Pattern 1: name suggests a credential AND the value looks like a real secret + - patterns: + - pattern-either: + - pattern: private static final String $VAR = "$VALUE"; + - pattern: public static final String $VAR = "$VALUE"; + - pattern: static final String $VAR = "$VALUE"; + - pattern: private String $VAR = "$VALUE"; + - pattern: String $VAR = "$VALUE"; + - metavariable-regex: + metavariable: $VAR + # A bare "key" matches constants such as KEY_ATTRIBUTE, PARENT_KEY + # and SEC_WEBSOCKET_KEY1, which are map keys and header names, not + # secrets, so "key" is only honoured in compound credential words. + # "token", "password" and "secret" are specific enough to stand alone. + regex: (?i).*(password|passwd|pwd|secret|credential|token|api_?key|apikey|secret_?key|private_?key|access_?key|encryption_?key|signing_?key|connection_?string).* + - metavariable-regex: + metavariable: $VALUE + # Non-empty. Default credentials such as "admin" are short and are + # exactly the finding that matters, so the floor stays low and the + # shape exclusions below carry the precision. + regex: ^.{4,}$ + - metavariable-regex: + metavariable: $VALUE + # Exclude identifiers, header names, property paths, format strings + # and placeholder expressions, which are configuration, not secrets. + regex: ^(?!.*[ :;,{}<>()\[\]])(?!.*\$\{)(?!.*%[sdf])(?!(?i)(none|null|true|false|unset|example|placeholder|changeit)$).*$ + - metavariable-regex: + metavariable: $VALUE + # A dotted identifier is a system property or a class name + # (java.net.socks.password, sun.misc.SharedSecrets), not a secret. + regex: ^(?!^[A-Za-z][A-Za-z0-9]*(\.[A-Za-z0-9]+)+$).*$ + - metavariable-regex: + metavariable: $VALUE + # A hyphenated value with no digit is a header name + # (Access-Control-Allow-Credentials, X-CSRF-TOKEN). Digits are the + # cheap discriminator: header names essentially never contain one + # and real keys almost always do, so sk-live-9f8e7d6c5b4a, + # xoxb-... and STAGING-TOKEN-42 are still reported. + regex: ^(?!^(?!.*[0-9])[A-Za-z][A-Za-z0-9]*(-[A-Za-z0-9]+)+$).*$ + - metavariable-regex: + metavariable: $VALUE + # Any x- prefixed value is a custom header name. + regex: ^(?!(?i:x-).*$).*$ + - metavariable-regex: + metavariable: $VALUE + # HTTP auth scheme names, not credentials. + regex: ^(?!(?i:bearer|basic|digest|oauth|negotiate)$).*$ + - metavariable-regex: + metavariable: $VALUE + # A value that just restates the credential keyword is naming a + # field, a UI label or an auth method ("password", "Password", + # "accesskey", "stompCredentials", "access_token", "j_password"), + # not holding one. Real default credentials such as "admin", + # "webgoat" or "password123" do not restate the keyword alone. + regex: ^(?!(?=[A-Za-z])(?!.*[0-9])(?i:[a-z_]*(password|passwd|credential|secret|token|apikey|accesskey|auth)[a-z_]*)$).*$ + # Pattern 2: credential APIs called with string literals + - pattern: new PasswordAuthentication($USER, "...".toCharArray()) + - pattern: DriverManager.getConnection($URL, $USER, "...") + - pattern: java.sql.DriverManager.getConnection($URL, $USER, "...") + - pattern: new BasicAWSCredentials("...", "...") + - pattern: $CTX.addToEnvironment(Context.SECURITY_CREDENTIALS, "...") metadata: category: security cwe: CWE-798 @@ -237,13 +584,24 @@ rules: # Weak cryptography - id: java-weak-crypto-md5 - message: "Weak cryptographic algorithm detected. Using broken or outdated algorithms may allow attackers to decrypt data or forge signatures. Use modern algorithms like AES-256, SHA-256, or Ed25519." + message: "Weak hash algorithm (MD5) detected. MD5 is collision-broken and must not be used for signatures, integrity checks, or password storage. Use SHA-256 or better, and a password hash such as bcrypt, scrypt or Argon2 for credentials." severity: HIGH languages: [java] pattern-either: - - pattern: MessageDigest.getInstance("MD5") - - pattern: MessageDigest.getInstance("md5") + - patterns: + - pattern-either: + # Covers the one-, two- and three-argument getInstance overloads, + # both imported and fully qualified. Real code frequently writes + # java.security.MessageDigest.getInstance("MD5", "SUN"). + - pattern: MessageDigest.getInstance("$ALGO", ...) + - pattern: java.security.MessageDigest.getInstance("$ALGO", ...) + - metavariable-regex: + metavariable: $ALGO + regex: (?i)^(md5|md-5|md2|md4)$ - pattern: DigestUtils.md5($DATA) + - pattern: DigestUtils.md5Hex($DATA) + - pattern: org.apache.commons.codec.digest.DigestUtils.md5($DATA) + - pattern: org.apache.commons.codec.digest.DigestUtils.md5Hex($DATA) metadata: category: security cwe: CWE-327 @@ -251,17 +609,23 @@ rules: subcategory: crypto vulnerability_class: "Cryptographic Weakness" owasp: "A02:2021" - fix: "Use MessageDigest.getInstance('SHA-256') instead of MD5/SHA1. Use AES/GCM/NoPadding for encryption. Use Cipher from javax.crypto with strong algorithms." + fix: "Use MessageDigest.getInstance('SHA-256'). For password storage use BCrypt, SCrypt or Argon2 rather than a raw digest." - id: java-weak-crypto-sha1 - message: "Weak cryptographic algorithm detected. Using broken or outdated algorithms may allow attackers to decrypt data or forge signatures. Use modern algorithms like AES-256, SHA-256, or Ed25519." + message: "Weak hash algorithm (SHA-1) detected. SHA-1 is collision-broken and must not be used for signatures or integrity checks. Use SHA-256 or better." severity: HIGH languages: [java] pattern-either: - - pattern: MessageDigest.getInstance("SHA-1") - - pattern: MessageDigest.getInstance("SHA1") - - pattern: MessageDigest.getInstance("sha1") + - patterns: + - pattern-either: + - pattern: MessageDigest.getInstance("$ALGO", ...) + - pattern: java.security.MessageDigest.getInstance("$ALGO", ...) + - metavariable-regex: + metavariable: $ALGO + regex: (?i)^(sha1|sha-1)$ - pattern: DigestUtils.sha1($DATA) + - pattern: DigestUtils.sha1Hex($DATA) + - pattern: org.apache.commons.codec.digest.DigestUtils.sha1($DATA) metadata: category: security cwe: CWE-327 @@ -269,29 +633,129 @@ rules: subcategory: crypto vulnerability_class: "Cryptographic Weakness" owasp: "A02:2021" - fix: "Use MessageDigest.getInstance('SHA-256') instead of MD5/SHA1. Use AES/GCM/NoPadding for encryption. Use Cipher from javax.crypto with strong algorithms." + fix: "Use MessageDigest.getInstance('SHA-256') or stronger." # Insecure random - id: java-insecure-random - message: "Insecure random number generator used. Non-cryptographic PRNGs produce predictable values that attackers can guess. Use a cryptographically secure random generator for security-sensitive operations." + message: "Insecure random number generator used for a security value. java.util.Random and Math.random() are linear congruential generators whose output is predictable from a few observed values, so tokens, session identifiers, salts and keys derived from them can be guessed. Use java.security.SecureRandom." severity: HIGH languages: [java] - pattern-either: - - pattern: new Random() - - pattern: new Random($SEED) + mode: taint + paths: + exclude: + - "*/src/test/*" + - "*/src/testFixtures/*" + - "*/test/java/*" + - "*/testsuite/*" + - "*/microbench/*" + - "*/jmh/*" + - "*Test.java" + - "*Tests.java" + - "*TestCase.java" + - "*Benchmark.java" + pattern-sources: + # Both the imported and fully qualified spellings; benchmark and real code + # both write new java.util.Random() in full. + - pattern: new Random(...).$M(...) + - pattern: new java.util.Random(...).$M(...) + - pattern: (Random $R).$M(...) + - pattern: (java.util.Random $R).$M(...) - pattern: Math.random() - pattern-not-inside: - pattern: | - // This is not for cryptographic use - ... + - pattern: java.lang.Math.random() + - pattern: ThreadLocalRandom.current().$M(...) + - pattern: (RandomStringUtils $U).random(...) + - pattern: RandomStringUtils.random(...) + - pattern: RandomStringUtils.randomAlphanumeric(...) + # nextBytes returns void and fills the array in place. Taint has to be + # attached to the argument or it never reaches a key or IV sink. + - patterns: + - pattern-either: + - pattern: (Random $R).nextBytes($ARR) + - pattern: (java.util.Random $R).nextBytes($ARR) + - pattern: new Random(...).nextBytes($ARR) + - pattern: new java.util.Random(...).nextBytes($ARR) + - focus-metavariable: $ARR + by-side-effect: true + pattern-propagators: + - pattern: Float.toString($X) + from: $X + to: Float.toString + - pattern: Double.toString($X) + from: $X + to: Double.toString + - pattern: Long.toString($X, ...) + from: $X + to: Long.toString + - pattern: Integer.toString($X, ...) + from: $X + to: Integer.toString + - pattern: String.valueOf($X) + from: $X + to: String.valueOf + - pattern: (String $S).substring(...) + from: $S + to: (String $S).substring(...) + - pattern: (String $A) + (String $B) + from: $B + to: $A + - pattern: $B.append($X) + from: $X + to: $B + - pattern: $ENC.encodeToString($X) + from: $X + to: $ENC.encodeToString($X) + pattern-sinks: + # A weak PRNG is only a vulnerability when its output becomes a security + # value. Shuffling a list, jittering a retry, or seeding a JMH benchmark + # is not. Require the value to land somewhere security relevant. + - patterns: + - pattern: $VAR = $SRC + - focus-metavariable: $SRC + - metavariable-regex: + metavariable: $VAR + # Anchored. Unanchored short words matched as substrings of + # ordinary identifiers: iv in pivot and divisor, pin in spinner, + # key in monkey, auth in author. seed is dropped entirely because + # reseeding one PRNG from another is not a security value. The + # case-insensitive flag is scoped per alternative: a leading + # global (?i) made the camelCase branch match lowercase too, + # which reintroduced every one of those substring matches. + # Each alternative also carries its own leading .* because + # metavariable-regex anchors at the start of the name. + regex: (?i:.*(token|session|nonce|salt|secret|password|passwd|credential|apikey|api_key|privatekey|private_key|pincode|csrf|initvector|cookie|verifier|challenge|resetcode|activation|captcha|guid|uuid))|(?:.*[._])?(?i:iv|pin|otp|key|auth)(?:$|[_0-9]|[A-Z])|.*[a-z](?:Iv|Pin|Otp|Key|Auth)(?:[A-Z0-9_]|$) + - patterns: + - pattern: $TYPE $VAR = $SRC; + - focus-metavariable: $SRC + - metavariable-regex: + metavariable: $VAR + regex: (?i:.*(token|session|nonce|salt|secret|password|passwd|credential|apikey|api_key|privatekey|private_key|pincode|csrf|initvector|cookie|verifier|challenge|resetcode|activation|captcha|guid|uuid))|(?:.*[._])?(?i:iv|pin|otp|key|auth)(?:$|[_0-9]|[A-Z])|.*[a-z](?:Iv|Pin|Otp|Key|Auth)(?:[A-Z0-9_]|$) + # A weak PRNG inside a class whose whole purpose is a security mechanism + # is a finding regardless of what the local variable is called. This is + # what catches session-id and CSRF-token generators that assign to `id`. + - patterns: + - pattern-inside: | + class $CLS { ... } + - metavariable-regex: + metavariable: $CLS + regex: (?i).*(session|csrf|xsrf|token|password|passwd|credential|authentication|crypto|cipher|secret|nonce|otp|pincode|resetlink).* + - pattern: $VAR = $SRC + - focus-metavariable: $SRC + # Direct use in security APIs + - pattern: new Cookie($NAME, ...) + - pattern: new javax.servlet.http.Cookie($NAME, ...) + - pattern: (Cookie $C).setValue(...) + - pattern: new SecretKeySpec(...) + - pattern: new javax.crypto.spec.SecretKeySpec(...) + - pattern: new IvParameterSpec(...) + - pattern: (MessageDigest $D).update(...) metadata: category: security cwe: CWE-338 - confidence: medium + confidence: high subcategory: crypto vulnerability_class: "Cryptographic Weakness" owasp: "A02:2021" - fix: "Use java.security.SecureRandom instead of java.util.Random for security-sensitive operations." + fix: "Use java.security.SecureRandom for tokens, session identifiers, salts, IVs and keys. SecureRandom.getInstanceStrong() is appropriate for long-lived secrets." # XXE vulnerabilities - id: java-xxe-vulnerability @@ -320,71 +784,229 @@ rules: # Path traversal - using taint mode for accurate detection - id: java-path-traversal - message: "Path traversal vulnerability detected. User-controlled data flows into file operations without proper validation. Use Path.normalize() and validate the result is within allowed directory." + message: "Path traversal vulnerability detected. User-controlled data flows into a file operation without validation, letting an attacker use ../ sequences to read or write files outside the intended directory. Resolve the path and verify it stays under the intended base directory before opening it." severity: HIGH languages: [java] mode: taint + paths: + exclude: + - "*/src/test/*" + - "*/src/testFixtures/*" + - "*/test/java/*" + - "*/testsuite/*" + - "*/microbench/*" + - "*/jmh/*" + - "*Test.java" + - "*Tests.java" + - "*TestCase.java" + - "*Benchmark.java" pattern-sources: - # Servlet request sources - pattern: $REQ.getParameter(...) - pattern: $REQ.getParameterValues(...) + - pattern: $REQ.getParameterMap() - pattern: $REQ.getHeader(...) + - pattern: $REQ.getHeaders(...) - pattern: $REQ.getQueryString() - - pattern: $REQ.getRequestURI() - pattern: $REQ.getPathInfo() - # User object getters that typically return user input - - pattern: $OBJ.getFilename() - - pattern: $OBJ.getName() - - pattern: $OBJ.getPath() - - pattern: $OBJ.getValue() - - pattern: $OBJ.getData() + - pattern: $REQ.getRequestURI() + # Type constrained: an untyped $REQ.getInputStream() also matched + # Resource.getInputStream(), which is a classpath resource, not input. + - pattern: (HttpServletRequest $R).getInputStream() + - pattern: (HttpServletRequest $R).getReader() + - pattern: (javax.servlet.http.HttpServletRequest $R).getInputStream() + - pattern: (javax.servlet.http.HttpServletRequest $R).getReader() + # Cookie values are attacker controlled and are the source in a large + # share of real path traversal and injection findings. + - pattern: (Cookie $C).getValue() + - pattern: (javax.servlet.http.Cookie $C).getValue() + # Archive entry names and uploaded filenames are attacker controlled. + # ZipEntry.getName() is the Zip Slip source; MultipartFile carries the + # client-supplied filename verbatim. + - pattern: (ZipEntry $E).getName() + - pattern: (java.util.zip.ZipEntry $E).getName() + - pattern: (ArchiveEntry $E).getName() + - pattern: (MultipartFile $F).getOriginalFilename() + - pattern: (org.springframework.web.multipart.MultipartFile $F).getOriginalFilename() + - pattern: (Part $P).getSubmittedFileName() + - patterns: + - pattern: $PARAM + - pattern-inside: | + $RET $METHOD(..., @RequestParam(...) $TYPE $PARAM, ...) { ... } + - patterns: + - pattern: $PARAM + - pattern-inside: | + $RET $METHOD(..., @PathVariable(...) $TYPE $PARAM, ...) { ... } pattern-propagators: - # String concatenation - pattern: (String $A) + (String $B) from: $B to: $A - pattern: (String $A).concat($B) from: $B to: $A - # Path operations that propagate taint + - pattern: String.format($FMT, ..., $X, ...) + from: $X + to: String.format + - pattern: $B.append($X) + from: $X + to: $B + - pattern: (StringBuilder $B).toString() + from: $B + to: (StringBuilder $B).toString() + # URL decoding is not sanitisation; it preserves attacker control and is + # applied to almost every header-sourced value in real servlet code. + - pattern: URLDecoder.decode($X, ...) + from: $X + to: URLDecoder.decode + - pattern: java.net.URLDecoder.decode($X, ...) + from: $X + to: java.net.URLDecoder.decode + - pattern: new String($X) + from: $X + to: new String + - pattern: new String($X, ...) + from: $X + to: new String + - pattern: (String $S).substring(...) + from: $S + to: (String $S).substring(...) + - pattern: (String $S).trim() + from: $S + to: (String $S).trim() + - pattern: (String $S).toLowerCase(...) + from: $S + to: (String $S).toLowerCase(...) + - pattern: (String $S).replace(...) + from: $S + to: (String $S).replace(...) + - pattern: (String $S).getBytes(...) + from: $S + to: (String $S).getBytes(...) + - pattern: $E.nextElement() + from: $E + to: $E.nextElement() + - pattern: Base64.decodeBase64($X) + from: $X + to: Base64.decodeBase64 + - pattern: org.apache.commons.codec.binary.Base64.decodeBase64($X) + from: $X + to: org.apache.commons.codec.binary.Base64.decodeBase64 + - pattern: Base64.encodeBase64($X) + from: $X + to: Base64.encodeBase64 + - pattern: org.apache.commons.codec.binary.Base64.encodeBase64($X) + from: $X + to: org.apache.commons.codec.binary.Base64.encodeBase64 - pattern: Paths.get(..., $X, ...) from: $X to: Paths.get - - pattern: Path.resolve($X) + - pattern: java.nio.file.Paths.get(..., $X, ...) from: $X - to: Path.resolve - # File path operations + to: java.nio.file.Paths.get + - pattern: $P.resolve($X) + from: $X + to: $P.resolve($X) - pattern: new File($X) from: $X to: new File - pattern: new File($DIR, $X) from: $X to: new File + - pattern: new java.io.File($X) + from: $X + to: new java.io.File + - pattern: new java.io.File($DIR, $X) + from: $X + to: new java.io.File pattern-sinks: - # File operations - - pattern: new File(...) + # Both the imported and fully qualified spellings. Servlet code and + # generated code routinely write java.io.* in full, and the previous + # short-name-only sinks missed every one of those call sites. + # Constructing a File or a Path touches nothing, so new File(...) and + # Paths.get(...) are propagators (above) rather than sinks. Making the + # constructor a sink reported the canonical-path containment idiom + # (construct, canonicalize, check, then open) at the constructor, before + # the check could run. The filesystem operations below are the sinks. - pattern: new FileInputStream(...) + - pattern: new java.io.FileInputStream(...) - pattern: new FileOutputStream(...) + - pattern: new java.io.FileOutputStream(...) - pattern: new FileReader(...) + - pattern: new java.io.FileReader(...) - pattern: new FileWriter(...) + - pattern: new java.io.FileWriter(...) - pattern: new RandomAccessFile(...) - - pattern: Files.readAllBytes(...) - - pattern: Files.readAllLines(...) - - pattern: Files.write(...) - - pattern: Files.copy(...) - - pattern: Files.move(...) - - pattern: Files.delete(...) - - pattern: Files.deleteIfExists(...) - - pattern: Paths.get(...) - # Legacy IO operations - - pattern: $FILE.createNewFile(...) - - pattern: $FILE.delete() - - pattern: $FILE.renameTo(...) + - pattern: new java.io.RandomAccessFile(...) + # Filesystem operations on a File built from the tainted value. exists() + # and friends are included: probing an attacker-chosen path discloses the + # filesystem, and it is the sink in a large share of real traversal code. + - patterns: + - pattern-either: + - pattern: (File $F).$M(...) + - pattern: (java.io.File $F).$M(...) + - metavariable-regex: + metavariable: $M + regex: ^(exists|isFile|isDirectory|canRead|canWrite|canExecute|length|lastModified|list|listFiles|createNewFile|delete|deleteOnExit|mkdir|mkdirs|renameTo|setLastModified|setReadable|setWritable|setExecutable)$ + - focus-metavariable: $F + - patterns: + - pattern-either: + - pattern: new File(...).$M(...) + - pattern: new java.io.File(...).$M(...) + - metavariable-regex: + metavariable: $M + regex: ^(exists|isFile|isDirectory|canRead|canWrite|canExecute|length|lastModified|list|listFiles|createNewFile|delete|deleteOnExit|mkdir|mkdirs|renameTo|setLastModified|setReadable|setWritable|setExecutable)$ + - patterns: + - pattern-either: + - pattern: Files.$M(...) + - pattern: java.nio.file.Files.$M(...) + - metavariable-regex: + metavariable: $M + regex: ^(newInputStream|newOutputStream|newBufferedReader|newBufferedWriter|newByteChannel|newDirectoryStream|readAllBytes|readAllLines|readString|lines|write|writeString|copy|move|delete|deleteIfExists|createFile|createDirectory|createDirectories|exists|notExists|isDirectory|isRegularFile|isReadable|isWritable|size|list|walk|find|getLastModifiedTime)$ pattern-sanitizers: - # Path validation - - pattern: $PATH.normalize() - - pattern: $PATH.toRealPath(...) - - pattern: FilenameUtils.normalize(...) + # normalize() alone collapses ../ but does not confine the result, so it + # is only a sanitiser when paired with a containment check. The + # containment check is what actually makes the path safe. + - patterns: + # Typed to Path deliberately. Path.startsWith is component-wise + # containment; String.startsWith is a prefix test, and an untyped + # receiver let a bypassable blacklist such as + # name.startsWith("..") sanitize the value. + - pattern-either: + - pattern: (Path $PATH).startsWith($BASE) + - pattern: (java.nio.file.Path $PATH).startsWith($BASE) + - focus-metavariable: $PATH + # Without by-side-effect the sanitizer only cleans the startsWith + # expression itself, so the checked variable stayed tainted at every + # later sink and the containment check suppressed nothing. + by-side-effect: true + # The same containment check with normalize() inside the condition. + - patterns: + - pattern-either: + - pattern: (Path $PATH).normalize().startsWith($BASE) + - pattern: (Path $PATH).toAbsolutePath().normalize().startsWith($BASE) + - pattern: (Path $PATH).normalize().toAbsolutePath().startsWith($BASE) + - pattern: (java.nio.file.Path $PATH).normalize().startsWith($BASE) + - pattern: (java.nio.file.Path $PATH).toAbsolutePath().normalize().startsWith($BASE) + - focus-metavariable: $PATH + by-side-effect: true + # The java.io idiom: canonicalize, then prefix test against the canonical + # base. getCanonicalPath() resolves ../ so, unlike a prefix test on the + # raw string, this is containment. The String form (canon = f.getCanonicalPath(); + # canon.startsWith(base)) is indistinguishable from a blacklist and is not + # a sanitizer; see the known limits in docs/java-sast-benchmark.md. + - patterns: + - pattern-either: + - pattern: (File $F).getCanonicalPath().startsWith($BASE) + - pattern: (java.io.File $F).getCanonicalPath().startsWith($BASE) + - pattern: (File $F).getCanonicalFile().toPath().startsWith($BASE) + - pattern: (java.io.File $F).getCanonicalFile().toPath().startsWith($BASE) + - pattern: (File $F).toPath().normalize().startsWith($BASE) + - pattern: (java.io.File $F).toPath().normalize().startsWith($BASE) + - focus-metavariable: $F + by-side-effect: true + - pattern: FilenameUtils.getName(...) + - pattern: org.apache.commons.io.FilenameUtils.getName(...) + - pattern: Integer.parseInt(...) + - pattern: UUID.fromString(...) metadata: category: security cwe: CWE-22 @@ -392,7 +1014,7 @@ rules: subcategory: access-control vulnerability_class: "Access Control Violation" owasp: "A01:2021" - fix: "Use File.getCanonicalPath() and verify the result starts with the allowed base directory. Use java.nio.file.Path.normalize() and resolve()." + fix: "Resolve the path with Path.normalize() and then assert the result startsWith() the intended base directory, or strip the value to a bare filename with FilenameUtils.getName()." # SSL/TLS bypass - id: java-ssl-bypass @@ -427,15 +1049,22 @@ rules: # Weak cipher algorithms - id: java-weak-cipher - message: "Weak cryptographic algorithm detected. Using broken or outdated algorithms may allow attackers to decrypt data or forge signatures. Use modern algorithms like AES-256, SHA-256, or Ed25519." + message: "Weak or unauthenticated cipher detected. DES, 3DES, RC2, RC4 and Blowfish are broken or deprecated, and ECB mode leaks plaintext structure. Use AES-256 in GCM mode (AES/GCM/NoPadding)." severity: MEDIUM languages: [java] - pattern-either: - - pattern: Cipher.getInstance("DES") - - pattern: Cipher.getInstance("RC4") - - pattern: Cipher.getInstance("RC2") - - pattern: Cipher.getInstance("DESede") - - pattern: Cipher.getInstance("Blowfish") + patterns: + - pattern-either: + # Cipher.getInstance takes a transformation string such as + # "DES/CBC/PKCS5Padding", not a bare algorithm name, and is commonly + # written fully qualified with an optional provider argument. + - pattern: Cipher.getInstance("$TRANSFORM", ...) + - pattern: javax.crypto.Cipher.getInstance("$TRANSFORM", ...) + - pattern: new SecretKeySpec($KEY, "$TRANSFORM") + - pattern: new javax.crypto.spec.SecretKeySpec($KEY, "$TRANSFORM") + - pattern: KeyGenerator.getInstance("$TRANSFORM", ...) + - metavariable-regex: + metavariable: $TRANSFORM + regex: (?i)^(des|desede|tripledes|3des|rc2|rc4|arcfour|blowfish)(/.*)?$|^(?!rsa/)[^/]+/ecb/.*$ metadata: category: security cwe: CWE-327 @@ -443,6 +1072,7 @@ rules: subcategory: crypto vulnerability_class: "Cryptographic Weakness" owasp: "A02:2021" + fix: "Use Cipher.getInstance('AES/GCM/NoPadding') with a 256-bit key and a unique 12-byte IV per message." # Weak SSL/TLS versions - id: java-weak-ssl-version @@ -518,83 +1148,191 @@ rules: # Cookie security issues - id: java-insecure-cookie - message: "Sensitive cookie missing the Secure flag. The cookie may be transmitted over unencrypted HTTP, allowing interception. Set the Secure flag on all sensitive cookies." + message: "Cookie created without the Secure flag. The cookie may be transmitted over unencrypted HTTP, allowing interception. Call setSecure(true) and setHttpOnly(true) before adding the cookie to the response." severity: MEDIUM languages: [java] + paths: + exclude: + - "*/src/test/*" + - "*/src/testFixtures/*" + - "*/test/java/*" + - "*/testsuite/*" + - "*/microbench/*" + - "*/jmh/*" + - "*Test.java" + - "*Tests.java" + - "*TestCase.java" + - "*Benchmark.java" pattern-either: - - pattern: | - Cookie $COOKIE = new Cookie($NAME, $VALUE); - - pattern: | - new Cookie($NAME, $VALUE); - pattern-not-inside: - pattern: | - ... - $COOKIE.setSecure(true); - ... + # Branch 1: the cookie is assigned to a variable or field. Matching the + # declaration (rather than the bare constructor) forces $COOKIE to unify + # between the match and the exclusion, so the exclusion is scoped to this + # cookie. A region-based exclusion dropped an unhardened cookie whenever + # a neighbouring cookie in the same method was hardened. + - patterns: + - pattern-either: + - pattern: $T $COOKIE = new Cookie($NAME, $VALUE); + - pattern: $T $COOKIE = new javax.servlet.http.Cookie($NAME, $VALUE); + - pattern: $T $COOKIE = new jakarta.servlet.http.Cookie($NAME, $VALUE); + - pattern: $COOKIE = new Cookie($NAME, $VALUE); + - pattern: $COOKIE = new javax.servlet.http.Cookie($NAME, $VALUE); + - pattern: $COOKIE = new jakarta.servlet.http.Cookie($NAME, $VALUE); + - pattern-not-inside: | + $T $COOKIE = new Cookie(...); + ... + $COOKIE.setSecure(true); + - pattern-not-inside: | + $T $COOKIE = new javax.servlet.http.Cookie(...); + ... + $COOKIE.setSecure(true); + - pattern-not-inside: | + $T $COOKIE = new jakarta.servlet.http.Cookie(...); + ... + $COOKIE.setSecure(true); + # Assignment form, which also covers hardening through a field. + - pattern-not-inside: | + $COOKIE = new Cookie(...); + ... + $COOKIE.setSecure(true); + - pattern-not-inside: | + $COOKIE = new javax.servlet.http.Cookie(...); + ... + $COOKIE.setSecure(true); + - pattern-not-inside: | + $COOKIE = new jakarta.servlet.http.Cookie(...); + ... + $COOKIE.setSecure(true); + # Branch 2: constructed inline and never assigned, so it can never be + # hardened, e.g. response.addCookie(new Cookie(name, value)). + - patterns: + - pattern-either: + - pattern: $RESP.addCookie(new Cookie($NAME, $VALUE)) + - pattern: $RESP.addCookie(new javax.servlet.http.Cookie($NAME, $VALUE)) + - pattern: $RESP.addCookie(new jakarta.servlet.http.Cookie($NAME, $VALUE)) metadata: category: security cwe: CWE-614 - confidence: low + confidence: medium subcategory: configuration vulnerability_class: "Security Misconfiguration" owasp: "A05:2021" - - # === Low Severity Rules === + fix: "Call cookie.setSecure(true) and cookie.setHttpOnly(true), or set server.servlet.session.cookie.secure=true in Spring Boot." # System.out usage in production - id: java-system-out-usage - message: "Sensitive information written to log files. Passwords, tokens, or personal data in logs can be exposed to unauthorized parties. Redact sensitive values before logging." + message: "Sensitive information written to console output. Passwords, tokens, or personal data printed to stdout or stderr end up in container logs and CI output where they are broadly readable. Redact the value or remove the statement." severity: LOW languages: [java] - pattern-either: - - pattern: System.out.println($MSG) - - pattern: System.out.print($MSG) - - pattern: System.err.println($MSG) - - pattern: $THROWABLE.printStackTrace() + paths: + exclude: + - "*/src/test/*" + - "*/src/testFixtures/*" + - "*/test/java/*" + - "*/testsuite/*" + - "*/microbench/*" + - "*/jmh/*" + - "*Test.java" + - "*Tests.java" + - "*TestCase.java" + - "*Benchmark.java" + - "*/example/*" + - "*/examples/*" + patterns: + - pattern-either: + - pattern: System.out.println($MSG) + - pattern: System.out.print($MSG) + - pattern: System.err.println($MSG) + - pattern: System.err.print($MSG) + # The rule is about leaking secrets, not about console I/O. Printing a + # progress message is not a security finding, so require the printed + # expression to reference something credential bearing. + - metavariable-regex: + metavariable: $MSG + regex: (?i).*(password|passwd|pwd|secret|credential|api_?key|apikey|private_?key|access_?key|auth_?token|access_?token|refresh_?token|session_?id|sessionid|ssn|creditcard|credit_card|cvv|passphrase).* metadata: category: security cwe: CWE-532 - confidence: low + confidence: medium subcategory: logging vulnerability_class: "Sensitive Data Exposure" owasp: "A09:2021" + fix: "Remove the statement or redact the value before printing. Route diagnostics through a logger with a redaction filter rather than System.out." # Empty catch blocks - id: java-empty-catch-block - message: "Improper error handling detected. The application does not properly handle exceptions, which may cause crashes or information leaks. Catch specific exceptions and handle them gracefully." + message: "Broad exception silently swallowed. Catching Exception or Throwable and discarding it without comment or logging hides failures, which can mask a security control that did not run. Log the exception, rethrow it, or document why it is safe to ignore." severity: LOW languages: [java] - pattern: | - try { - ... - } catch ($EXCEPTION $VAR) { - } + paths: + exclude: + - "*/src/test/*" + - "*/src/testFixtures/*" + - "*/test/java/*" + - "*/testsuite/*" + - "*/microbench/*" + - "*/jmh/*" + - "*Test.java" + - "*Tests.java" + - "*TestCase.java" + - "*Benchmark.java" + patterns: + - pattern: | + try { + ... + } catch ($EXCEPTION $VAR) { + } + # Catching a specific, expected exception and continuing is legitimate + # control flow. Only a swallowed broad exception is worth reporting. + - metavariable-regex: + metavariable: $EXCEPTION + regex: ^(Exception|Throwable|RuntimeException|java\.lang\.Exception|java\.lang\.Throwable|java\.lang\.RuntimeException)$ + # The conventional Java names for a deliberately discarded exception. + - metavariable-regex: + metavariable: $VAR + regex: ^(?!(ignored|ignore|expected|tolerated|unused|nop|noop|swallowed|discard|discarded)$).*$ + # A catch block carrying an explanatory comment is a reviewed decision, + # not an oversight. Comments are not AST nodes, so the body still looks + # empty to the matcher and has to be excluded textually. + - pattern-not-regex: 'catch\s*\([^)]*\)\s*\{\s*(//|/\*)' metadata: category: security cwe: CWE-703 - confidence: high + confidence: medium subcategory: error-handling vulnerability_class: "Improper Error Handling" + fix: "Log the exception with context, rethrow it, rename the variable to 'ignored', or add a comment explaining why discarding it is safe." # Hardcoded IP addresses - id: java-hardcoded-ip - message: "Hard-coded credentials detected. Embedding secrets in source code makes them easily discoverable and impossible to rotate. Use environment variables or a secrets manager instead." + message: "Hard-coded private network address detected. Embedding infrastructure addresses in source ties the build to one environment and leaks internal network layout. Move the address to configuration." severity: LOW languages: [java] - pattern-either: - - pattern: '"192.168.$IP"' - - pattern: '"10.$IP"' - - pattern: '"172.16.$IP"' - - pattern: '"127.0.0.1"' + paths: + exclude: + - "*/src/test/*" + - "*/src/testFixtures/*" + - "*/test/java/*" + - "*/testsuite/*" + - "*/microbench/*" + - "*/jmh/*" + - "*Test.java" + - "*Tests.java" + - "*TestCase.java" + - "*Benchmark.java" + - "*/example/*" + - "*/examples/*" + # Requires a full dotted quad. Matching a partial address also matched + # version strings such as "10.0" and "10.4". Loopback (127.x) and the + # wildcard bind address are not infrastructure disclosure and are excluded + # by the leading-octet alternation. + pattern-regex: '"(?:10\.[0-9]{1,3}|192\.168|172\.(?:1[6-9]|2[0-9]|3[01]))\.[0-9]{1,3}\.[0-9]{1,3}(?::[0-9]{1,5})?"' metadata: category: security - cwe: CWE-798 + cwe: CWE-547 confidence: low - subcategory: authentication - vulnerability_class: "Authentication Weakness" - owasp: "A07:2021" - - # === Framework-specific Rules === + subcategory: configuration + vulnerability_class: "Security Misconfiguration" + fix: "Read the address from configuration (environment variable, application.properties, or service discovery) instead of hard-coding it." # Spring Security bypass - id: java-spring-security-bypass @@ -770,4 +1508,272 @@ rules: subcategory: upload vulnerability_class: "Unrestricted File Upload" owasp: "A04:2021" - fix: "Validate file extension, MIME type, and content. Store uploads outside the web root. Use Apache Tika for content-type detection." \ No newline at end of file + fix: "Validate file extension, MIME type, and content. Store uploads outside the web root. Use Apache Tika for content-type detection." + + - id: java-xss + message: "Cross-site scripting (XSS) vulnerability detected. User-controlled data is written into the HTTP response without HTML encoding, letting an attacker inject script that runs in other users' browsers. Encode the value for the output context before writing it." + severity: HIGH + languages: [java] + mode: taint + paths: + exclude: + - "*/src/test/*" + - "*/src/testFixtures/*" + - "*/test/java/*" + - "*/testsuite/*" + - "*/microbench/*" + - "*/jmh/*" + - "*Test.java" + - "*Tests.java" + - "*TestCase.java" + - "*Benchmark.java" + pattern-sources: + - pattern: $REQ.getParameter(...) + - pattern: $REQ.getParameterValues(...) + - pattern: $REQ.getParameterMap() + - pattern: $REQ.getHeader(...) + - pattern: $REQ.getHeaders(...) + - pattern: $REQ.getQueryString() + - pattern: $REQ.getPathInfo() + - pattern: $REQ.getRequestURI() + - pattern: $REQ.getRequestURL() + # Type constrained: an untyped $REQ.getInputStream() also matched + # Resource.getInputStream(), which is a classpath resource, not input. + - pattern: (HttpServletRequest $R).getInputStream() + - pattern: (HttpServletRequest $R).getReader() + - pattern: (javax.servlet.http.HttpServletRequest $R).getInputStream() + - pattern: (javax.servlet.http.HttpServletRequest $R).getReader() + - pattern: (Cookie $C).getValue() + - pattern: (Cookie $C).getName() + # Spring MVC parameter binding + - patterns: + - pattern: $PARAM + - pattern-inside: | + $RET $METHOD(..., @RequestParam(...) $TYPE $PARAM, ...) { ... } + - patterns: + - pattern: $PARAM + - pattern-inside: | + $RET $METHOD(..., @PathVariable(...) $TYPE $PARAM, ...) { ... } + - patterns: + - pattern: $PARAM + - pattern-inside: | + $RET $METHOD(..., @RequestBody $TYPE $PARAM, ...) { ... } + pattern-propagators: + - pattern: (String $A) + (String $B) + from: $B + to: $A + - pattern: (String $A).concat($B) + from: $B + to: $A + - pattern: String.format($FMT, ..., $X, ...) + from: $X + to: String.format + - pattern: $B.append($X) + from: $X + to: $B + - pattern: (StringBuilder $B).toString() + from: $B + to: (StringBuilder $B).toString() + - pattern: (StringBuffer $B).toString() + from: $B + to: (StringBuffer $B).toString() + - pattern: URLDecoder.decode($X, ...) + from: $X + to: URLDecoder.decode + - pattern: java.net.URLDecoder.decode($X, ...) + from: $X + to: java.net.URLDecoder.decode + - pattern: (String $S).substring(...) + from: $S + to: (String $S).substring(...) + - pattern: (String $S).trim() + from: $S + to: (String $S).trim() + - pattern: $E.nextElement() + from: $E + to: $E.nextElement() + pattern-sinks: + - pattern: $RESP.getWriter().println(...) + - pattern: $RESP.getWriter().print(...) + - pattern: $RESP.getWriter().write(...) + - pattern: $RESP.getWriter().format(...) + - pattern: $RESP.getWriter().printf(...) + - pattern: $RESP.getWriter().append(...) + - pattern: $RESP.getOutputStream().write(...) + - pattern: (PrintWriter $W).println(...) + - pattern: (PrintWriter $W).print(...) + - pattern: (PrintWriter $W).write(...) + - pattern: (PrintWriter $W).format(...) + - pattern: (JspWriter $W).println(...) + - pattern: (JspWriter $W).print(...) + # Type constrained: an untyped $RESP.setHeader() also matched Spring + # messaging header accessors, which never reach a browser. + - pattern: (HttpServletResponse $R).setHeader($NAME, ...) + - pattern: (HttpServletResponse $R).addHeader($NAME, ...) + - pattern: (javax.servlet.http.HttpServletResponse $R).setHeader($NAME, ...) + pattern-sanitizers: + - pattern: ESAPI.encoder().encodeForHTML(...) + - pattern: $ENC.encodeForHTML(...) + - pattern: $ENC.encodeForHTMLAttribute(...) + - pattern: $ENC.encodeForJavaScript(...) + - pattern: Encode.forHtml(...) + - pattern: Encode.forHtmlAttribute(...) + - pattern: org.owasp.encoder.Encode.forHtml(...) + - pattern: StringEscapeUtils.escapeHtml4(...) + - pattern: StringEscapeUtils.escapeHtml(...) + - pattern: org.apache.commons.text.StringEscapeUtils.escapeHtml4(...) + - pattern: HtmlUtils.htmlEscape(...) + - pattern: org.springframework.web.util.HtmlUtils.htmlEscape(...) + - pattern: Jsoup.clean(...) + # Values coerced to a non-string type cannot carry markup + - pattern: Integer.parseInt(...) + - pattern: Long.parseLong(...) + - pattern: Double.parseDouble(...) + - pattern: UUID.fromString(...) + metadata: + category: security + cwe: CWE-79 + confidence: high + subcategory: xss + vulnerability_class: "Cross-Site Scripting (XSS)" + owasp: "A03:2021" + fix: "HTML-encode the value at the point of output with OWASP Encoder (Encode.forHtml) or HtmlUtils.htmlEscape. In JSP use ; in Thymeleaf use th:text, which encodes by default." + + - id: java-xpath-injection + message: "XPath injection vulnerability detected. User-controlled data is concatenated into an XPath expression, letting an attacker rewrite the query and read arbitrary nodes from the document. Bind the value as a variable through an XPathVariableResolver instead of building the expression by concatenation." + severity: HIGH + languages: [java] + mode: taint + paths: + exclude: + - "*/src/test/*" + - "*/src/testFixtures/*" + - "*/test/java/*" + - "*/testsuite/*" + - "*/microbench/*" + - "*/jmh/*" + - "*Test.java" + - "*Tests.java" + - "*TestCase.java" + - "*Benchmark.java" + pattern-sources: + - pattern: $REQ.getParameter(...) + - pattern: $REQ.getParameterValues(...) + - pattern: $REQ.getParameterMap() + - pattern: $REQ.getHeader(...) + - pattern: $REQ.getHeaders(...) + - pattern: $REQ.getQueryString() + - pattern: $REQ.getPathInfo() + - pattern: $REQ.getRequestURI() + # Type constrained: an untyped $REQ.getInputStream() also matched + # Resource.getInputStream(), which is a classpath resource, not input. + - pattern: (HttpServletRequest $R).getInputStream() + - pattern: (HttpServletRequest $R).getReader() + - pattern: (javax.servlet.http.HttpServletRequest $R).getInputStream() + - pattern: (javax.servlet.http.HttpServletRequest $R).getReader() + # Cookie values are attacker controlled and are the source in a large + # share of real path traversal and injection findings. + - pattern: (Cookie $C).getValue() + - pattern: (javax.servlet.http.Cookie $C).getValue() + # Archive entry names and uploaded filenames are attacker controlled. + # ZipEntry.getName() is the Zip Slip source; MultipartFile carries the + # client-supplied filename verbatim. + - pattern: (ZipEntry $E).getName() + - pattern: (java.util.zip.ZipEntry $E).getName() + - pattern: (ArchiveEntry $E).getName() + - pattern: (MultipartFile $F).getOriginalFilename() + - pattern: (org.springframework.web.multipart.MultipartFile $F).getOriginalFilename() + - pattern: (Part $P).getSubmittedFileName() + - patterns: + - pattern: $PARAM + - pattern-inside: | + $RET $METHOD(..., @RequestParam(...) $TYPE $PARAM, ...) { ... } + - patterns: + - pattern: $PARAM + - pattern-inside: | + $RET $METHOD(..., @PathVariable(...) $TYPE $PARAM, ...) { ... } + pattern-propagators: + - pattern: (String $A) + (String $B) + from: $B + to: $A + - pattern: (String $A).concat($B) + from: $B + to: $A + - pattern: String.format($FMT, ..., $X, ...) + from: $X + to: String.format + - pattern: $B.append($X) + from: $X + to: $B + - pattern: (StringBuilder $B).toString() + from: $B + to: (StringBuilder $B).toString() + # URL decoding is not sanitisation; it preserves attacker control and is + # applied to almost every header-sourced value in real servlet code. + - pattern: URLDecoder.decode($X, ...) + from: $X + to: URLDecoder.decode + - pattern: java.net.URLDecoder.decode($X, ...) + from: $X + to: java.net.URLDecoder.decode + - pattern: new String($X) + from: $X + to: new String + - pattern: new String($X, ...) + from: $X + to: new String + - pattern: (String $S).substring(...) + from: $S + to: (String $S).substring(...) + - pattern: (String $S).trim() + from: $S + to: (String $S).trim() + - pattern: (String $S).toLowerCase(...) + from: $S + to: (String $S).toLowerCase(...) + - pattern: (String $S).replace(...) + from: $S + to: (String $S).replace(...) + - pattern: (String $S).getBytes(...) + from: $S + to: (String $S).getBytes(...) + - pattern: $E.nextElement() + from: $E + to: $E.nextElement() + - pattern: Base64.decodeBase64($X) + from: $X + to: Base64.decodeBase64 + - pattern: org.apache.commons.codec.binary.Base64.decodeBase64($X) + from: $X + to: org.apache.commons.codec.binary.Base64.decodeBase64 + - pattern: Base64.encodeBase64($X) + from: $X + to: Base64.encodeBase64 + - pattern: org.apache.commons.codec.binary.Base64.encodeBase64($X) + from: $X + to: org.apache.commons.codec.binary.Base64.encodeBase64 + pattern-sinks: + - pattern: (XPath $XP).evaluate(...) + - pattern: (XPath $XP).compile(...) + - pattern: (javax.xml.xpath.XPath $XP).evaluate(...) + - pattern: (javax.xml.xpath.XPath $XP).compile(...) + - pattern: (XPathExpression $XE).evaluate(...) + - pattern: XPathFactory.newInstance().newXPath().evaluate(...) + - pattern: javax.xml.xpath.XPathFactory.newInstance().newXPath().evaluate(...) + - pattern: $XP.evaluateExpression(...) + - pattern: $NODE.selectNodes(...) + - pattern: $NODE.selectSingleNode(...) + - pattern: DocumentHelper.createXPath(...) + pattern-sanitizers: + - pattern: ESAPI.encoder().encodeForXPath(...) + - pattern: $ENC.encodeForXPath(...) + - pattern: Integer.parseInt(...) + - pattern: Long.parseLong(...) + metadata: + category: security + cwe: CWE-643 + confidence: high + subcategory: injection + vulnerability_class: "Injection Vulnerability" + owasp: "A03:2021" + fix: "Use XPath.setXPathVariableResolver() and reference the value as $var in the expression, or validate the input against a strict allowlist before interpolating." diff --git a/tests/fixtures/opengrep/java/HardcodedCredentials.java b/tests/fixtures/opengrep/java/HardcodedCredentials.java new file mode 100644 index 0000000..559be7b --- /dev/null +++ b/tests/fixtures/opengrep/java/HardcodedCredentials.java @@ -0,0 +1,56 @@ +// Fixtures for java-hardcoded-credentials. +public class HardcodedCredentials { + // ruleid: java-hardcoded-credentials + private static final String DEFAULT_PASSWORD = "webgoat"; + // ruleid: java-hardcoded-credentials + private static final String ADMIN_PASSWORD = "admin"; + // ruleid: java-hardcoded-credentials + static final String LEAKED_TOKEN = "STAGING-TOKEN-42"; + // Hyphenated but digit bearing, so a real key rather than a header name. + // ruleid: java-hardcoded-credentials + static final String API_KEY = "sk-live-9f8e7d6c5b4a"; + // ruleid: java-hardcoded-credentials + static final String SLACK_TOKEN = "xoxb-2409januaryfake-99"; + + // Weak defaults that contain the keyword but also a digit, so they are + // values rather than a restatement of the field name. + // ruleid: java-hardcoded-credentials + static final String LEGACY_PASSWORD = "password123"; + // ruleid: java-hardcoded-credentials + static final String DB_SECRET = "secret_2024"; + + // Header names, not secrets. + // ok: java-hardcoded-credentials + public static final String ACCESS_CONTROL_ALLOW_CREDENTIALS = "Access-Control-Allow-Credentials"; + // ok: java-hardcoded-credentials + public static final String SEC_TOKEN_BINDING = "Sec-Token-Binding"; + // ok: java-hardcoded-credentials + public static final String CSRF_TOKEN_HEADER = "X-CSRF-TOKEN"; + // Auth scheme names. + // ok: java-hardcoded-credentials + public static final String TOKEN_TYPE = "Bearer"; + // Property paths and class names. + // ok: java-hardcoded-credentials + public static final String JAVA_NET_SOCKS_PASSWORD = "java.net.socks.password"; + // ok: java-hardcoded-credentials + static final String SHARED_SECRETS_CLASSNAME = "sun.misc.SharedSecrets"; + // Values that merely restate the keyword, including snake_case forms. + // ok: java-hardcoded-credentials + private static final String AUTH_PASSWORD = "password"; + // A capitalised restatement is a UI label. + // ok: java-hardcoded-credentials + private static final String PASSWORD_LABEL = "Password"; + // ok: java-hardcoded-credentials + public static final String ACCESSKEY_ATTRIBUTE = "accesskey"; + // ok: java-hardcoded-credentials + static final String TOKEN_PARAM = "access_token"; + // ok: java-hardcoded-credentials + static final String PASSWORD_FIELD = "j_password"; + // ok: java-hardcoded-credentials + private static final String CREDENTIALS_HEADER = "stompCredentials"; + // Map keys and attribute names, matched only by a bare "key". + // ok: java-hardcoded-credentials + public static final String KEY_ATTRIBUTE = "key"; + // ok: java-hardcoded-credentials + public static final String SEC_WEBSOCKET_KEY1 = "Sec-WebSocket-Key1"; +} diff --git a/tests/fixtures/opengrep/java/HardcodedIp.java b/tests/fixtures/opengrep/java/HardcodedIp.java new file mode 100644 index 0000000..d20cecb --- /dev/null +++ b/tests/fixtures/opengrep/java/HardcodedIp.java @@ -0,0 +1,22 @@ +// Fixtures for java-hardcoded-ip. +public class HardcodedIp { + // ruleid: java-hardcoded-ip + static final String A = "10.0.0.1"; + // ruleid: java-hardcoded-ip + static final String B = "10.0.0.1:8080"; + // ruleid: java-hardcoded-ip + static final String C = "192.168.1.1"; + // ruleid: java-hardcoded-ip + static final String D = "172.16.4.9"; + + // Version strings, not addresses. + // ok: java-hardcoded-ip + static final String E = "10.0"; + // ok: java-hardcoded-ip + static final String F = "10.2.3"; + // Loopback and wildcard bind are not infrastructure disclosure. + // ok: java-hardcoded-ip + static final String G = "127.0.0.1"; + // ok: java-hardcoded-ip + static final String H = "0.0.0.0"; +} diff --git a/tests/fixtures/opengrep/java/InsecureCookie.java b/tests/fixtures/opengrep/java/InsecureCookie.java new file mode 100644 index 0000000..59b78bd --- /dev/null +++ b/tests/fixtures/opengrep/java/InsecureCookie.java @@ -0,0 +1,38 @@ +// Fixtures for java-insecure-cookie. +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletResponse; + +public class InsecureCookie { + private Cookie field; + + // A hardened cookie must not exonerate an unhardened neighbour. + void neighbour(HttpServletResponse resp) { + Cookie a = new Cookie("a", "1"); + // ruleid: java-insecure-cookie + Cookie b = new Cookie("b", "2"); + a.setSecure(true); + resp.addCookie(a); + resp.addCookie(b); + } + + void hardened(HttpServletResponse resp) { + // ok: java-insecure-cookie + Cookie c = new Cookie("c", "3"); + c.setSecure(true); + resp.addCookie(c); + } + + // Hardened through a field. + void viaField(HttpServletResponse resp) { + // ok: java-insecure-cookie + this.field = new Cookie("d", "4"); + this.field.setSecure(true); + resp.addCookie(this.field); + } + + // Never assigned to a variable at all. + void inline(HttpServletResponse resp) { + // ruleid: java-insecure-cookie + resp.addCookie(new Cookie("e", "5")); + } +} diff --git a/tests/fixtures/opengrep/java/InsecureRandom.java b/tests/fixtures/opengrep/java/InsecureRandom.java new file mode 100644 index 0000000..db8b116 --- /dev/null +++ b/tests/fixtures/opengrep/java/InsecureRandom.java @@ -0,0 +1,93 @@ +// Fixtures for java-insecure-random. +import java.util.Random; +import java.security.SecureRandom; +import javax.crypto.spec.SecretKeySpec; +import javax.crypto.spec.IvParameterSpec; + +public class InsecureRandom { + void weakSecurityValues() { + Random rnd = new Random(); + // ruleid: java-insecure-random + String sessionToken = Long.toString(rnd.nextLong(), 36); + // ruleid: java-insecure-random + String csrf = Long.toString(rnd.nextLong(), 36); + // ruleid: java-insecure-random + String otp = Integer.toString(rnd.nextInt(999999)); + // ruleid: java-insecure-random + String apiKey = Long.toString(rnd.nextLong(), 36); + } + + void weakKeyMaterial() { + byte[] key = new byte[16]; + new Random().nextBytes(key); + // ruleid: java-insecure-random + new SecretKeySpec(key, "AES"); + } + + void weakIvMaterial() { + byte[] iv = new byte[12]; + new java.util.Random().nextBytes(iv); + // ruleid: java-insecure-random + new IvParameterSpec(iv); + } + + // Ordinary non-security uses. The short credential words must not match as + // substrings of these identifiers. + void ordinary() { + Random rnd = new Random(); + // ok: java-insecure-random + int pivot = rnd.nextInt(10); + // ok: java-insecure-random + int divisor = rnd.nextInt(10); + // ok: java-insecure-random + int spinner = rnd.nextInt(4); + // ok: java-insecure-random + int monkey = rnd.nextInt(4); + // ok: java-insecure-random + String author = Integer.toString(rnd.nextInt(4)); + // ok: java-insecure-random + int jitterMs = rnd.nextInt(250); + } + + // Short credential words in leading position, with a camelCase suffix. + void leadingShortWords() { + Random rnd = new Random(); + // ruleid: java-insecure-random + String otpCode = Integer.toString(rnd.nextInt(999999)); + // ruleid: java-insecure-random + int pinNumber = rnd.nextInt(9999); + // ruleid: java-insecure-random + String keyMaterial = Long.toString(rnd.nextLong()); + } + + // Assignment to a field rather than a local. + private long key; + private String otp; + + void fields() { + Random rnd = new Random(); + // ruleid: java-insecure-random + this.key = rnd.nextLong(); + // ruleid: java-insecure-random + this.otp = Integer.toString(rnd.nextInt(999999)); + } + + // The receiver type written fully qualified. + void qualifiedReceiver(java.util.Random rnd) { + // ruleid: java-insecure-random + String sessionToken = Long.toString(rnd.nextLong(), 36); + byte[] material = new byte[16]; + rnd.nextBytes(material); + // ruleid: java-insecure-random + new SecretKeySpec(material, "AES"); + } + + // camelCase and SNAKE_CASE forms of the short words still count. + void shortWordsThatDoCount() { + Random rnd = new Random(); + // ruleid: java-insecure-random + String resetPin = Integer.toString(rnd.nextInt(9999)); + // ruleid: java-insecure-random + String AUTH_VALUE = Long.toString(rnd.nextLong()); + } +} diff --git a/tests/fixtures/opengrep/java/LdapInjection.java b/tests/fixtures/opengrep/java/LdapInjection.java new file mode 100644 index 0000000..1fdaa6a --- /dev/null +++ b/tests/fixtures/opengrep/java/LdapInjection.java @@ -0,0 +1,46 @@ +// Fixtures for java-ldap-injection. +import javax.naming.directory.DirContext; +import javax.naming.directory.SearchControls; +import javax.servlet.http.HttpServletRequest; + +public class LdapInjection { + Object tainted(DirContext ctx, HttpServletRequest request) throws Exception { + String user = request.getParameter("u"); + // ruleid: java-ldap-injection + return ctx.search("ou=people", "(uid=" + user + ")", new SearchControls()); + } + + // Servlet code commonly declares the context fully qualified. + Object taintedQualified(javax.naming.directory.InitialDirContext idc, + HttpServletRequest request) throws Exception { + String user = request.getParameter("u"); + // ruleid: java-ldap-injection + return idc.search("ou=people", "(uid=" + user + ")", new SearchControls()); + } + + Object escaped(DirContext ctx, HttpServletRequest request) throws Exception { + String user = org.springframework.ldap.support.LdapEncoder.filterEncode(request.getParameter("u")); + // ok: java-ldap-injection + return ctx.search("ou=people", "(uid=" + user + ")", new SearchControls()); + } + + // A Lucene search is not an LDAP search. An untyped $CTX.search() sink + // turned this into a CRITICAL finding. + Object luceneSearch(org.apache.lucene.search.IndexSearcher searcher, + org.apache.lucene.queryparser.classic.QueryParser parser, + HttpServletRequest request) throws Exception { + // Integer.parseInt is a listed sanitizer, so the tainted value has to + // reach the sink unparsed for this to guard the untyped-sink defect. + String text = request.getParameter("q"); + // ok: java-ldap-injection + return searcher.search(parser.parse(text), 10); + } + + // The four-argument form binds the value through filterArgs, which the + // provider escapes. It is the remediation the rule's fix text recommends. + Object parameterized(DirContext ctx, HttpServletRequest request) throws Exception { + String user = request.getParameter("u"); + // ok: java-ldap-injection + return ctx.search("ou=people", "(uid={0})", new Object[]{user}, new SearchControls()); + } +} diff --git a/tests/fixtures/opengrep/java/PathTraversal.java b/tests/fixtures/opengrep/java/PathTraversal.java new file mode 100644 index 0000000..02878e9 --- /dev/null +++ b/tests/fixtures/opengrep/java/PathTraversal.java @@ -0,0 +1,104 @@ +// Fixtures for java-path-traversal. +import java.io.*; +import java.nio.file.*; +import java.util.zip.*; +import javax.servlet.http.HttpServletRequest; + +public class PathTraversal { + static final Path BASE = Paths.get("/var/data"); + static final File BASE_DIR = new File("/var/data"); + + void unvalidated(HttpServletRequest request) throws Exception { + String name = request.getParameter("f"); + // ruleid: java-path-traversal + Files.newInputStream(BASE.resolve(name)); + } + + void qualifiedSink(HttpServletRequest request) throws Exception { + String name = request.getParameter("f"); + // ruleid: java-path-traversal + new java.io.FileInputStream("/var/data/" + name); + } + + // Zip Slip: the archive entry name is attacker controlled. Constructing + // the File is a propagator, not a sink; the copy is the finding. + void zipSlip(File zipFile, File dir) throws Exception { + ZipFile zip = new ZipFile(zipFile); + ZipEntry e = zip.entries().nextElement(); + // ok: java-path-traversal + File f = new File(dir, e.getName()); + // ruleid: java-path-traversal + Files.copy(zip.getInputStream(e), f.toPath()); + } + + // Probing an attacker-chosen path is a finding even without an open. + void probe(HttpServletRequest request) { + File f = new File(BASE_DIR, request.getParameter("f")); + // ruleid: java-path-traversal + f.exists(); + } + + // A containment check must suppress the finding at the later sink. + void contained(HttpServletRequest request) throws Exception { + String name = request.getParameter("f"); + Path p = BASE.resolve(name).normalize(); + if (!p.startsWith(BASE)) { + throw new IOException("outside base"); + } + // ok: java-path-traversal + Files.newInputStream(p); + } + + // normalize() inside the condition is the same containment check. + void normalizeInCheck(HttpServletRequest request) throws Exception { + String name = request.getParameter("f"); + Path p = BASE.resolve(name); + if (!p.normalize().startsWith(BASE)) { + throw new IOException("outside base"); + } + // ok: java-path-traversal + Files.newInputStream(p); + } + + // The java.io idiom: canonicalize, then prefix test against the canonical + // base. getCanonicalPath() resolves ../ so this is containment. + void canonical(HttpServletRequest request) throws Exception { + String name = request.getParameter("f"); + // ok: java-path-traversal + File f = new File(BASE_DIR, name); + if (!f.getCanonicalPath().startsWith(BASE_DIR.getCanonicalPath() + File.separator)) { + throw new IOException("outside base"); + } + // ok: java-path-traversal + new FileInputStream(f); + } + + // A checked variable that is reassigned from new input is tainted again. + void reassigned(HttpServletRequest request) throws Exception { + Path p = BASE.resolve(request.getParameter("f")).normalize(); + if (!p.startsWith(BASE)) { + throw new IOException("outside base"); + } + p = BASE.resolve(request.getParameter("g")); + // ruleid: java-path-traversal + Files.newInputStream(p); + } + + // A String prefix test is a bypassable blacklist, not containment, and + // must not sanitize. Only Path.startsWith is component-wise containment. + void blacklistedPrefix(HttpServletRequest request) throws Exception { + String name = request.getParameter("f"); + if (name.startsWith("..")) { + throw new IOException("rejected"); + } + // ruleid: java-path-traversal + new java.io.FileInputStream("/var/data/" + name); + } + + // Reducing to a bare filename removes the traversal. + void basename(HttpServletRequest request) throws Exception { + String name = org.apache.commons.io.FilenameUtils.getName(request.getParameter("f")); + // ok: java-path-traversal + Files.newInputStream(BASE.resolve(name)); + } +} diff --git a/tests/fixtures/opengrep/java/ReflectionInjection.java b/tests/fixtures/opengrep/java/ReflectionInjection.java new file mode 100644 index 0000000..47a8f8a --- /dev/null +++ b/tests/fixtures/opengrep/java/ReflectionInjection.java @@ -0,0 +1,41 @@ +// Fixtures for java-reflection-injection. +import java.util.Map; +import javax.servlet.http.HttpServletRequest; + +public class ReflectionInjection { + Map> allowlist; + + Class tainted(HttpServletRequest request) throws Exception { + // ruleid: java-reflection-injection + return Class.forName(request.getParameter("c")); + } + + // String.valueOf is a conversion, not an allowlist lookup. + Class taintedThroughValueOf(HttpServletRequest request) throws Exception { + // ruleid: java-reflection-injection + return Class.forName(String.valueOf(request.getParameter("c"))); + } + + // Resolving through an allowlist removes attacker control. + Class allowlisted(HttpServletRequest request) throws Exception { + // ok: java-reflection-injection + return allowlist.get(request.getParameter("c")); + } + + // Constant class names and ordinary reflection are not injection. + Class constant() throws Exception { + // ok: java-reflection-injection + return Class.forName("sun.misc.Cleaner"); + } + + Object proxyDispatch(java.lang.reflect.Method method, Object target, Object[] args) throws Exception { + // ok: java-reflection-injection + return method.invoke(target, args); + } + + // A classpath resource stream is not request input. + Object scriptFromResource(javax.script.ScriptEngine engine, org.springframework.core.io.Resource resource) throws Exception { + // ok: java-reflection-injection + return engine.eval(new java.io.InputStreamReader(resource.getInputStream())); + } +} diff --git a/tests/fixtures/opengrep/java/SqlInjection.java b/tests/fixtures/opengrep/java/SqlInjection.java new file mode 100644 index 0000000..4a98754 --- /dev/null +++ b/tests/fixtures/opengrep/java/SqlInjection.java @@ -0,0 +1,60 @@ +// Fixtures for java-sql-injection. +import java.security.MessageDigest; +import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.Statement; +import javax.servlet.http.HttpServletRequest; +import org.springframework.jdbc.core.JdbcTemplate; + +public class SqlInjection { + private JdbcTemplate jdbcTemplate; + + void concatenated(Statement stmt, HttpServletRequest request) throws Exception { + String id = request.getParameter("id"); + // ruleid: java-sql-injection + stmt.executeQuery("SELECT * FROM t WHERE id = '" + id + "'"); + } + + // A concatenated query handed to prepareStatement() and run with a + // no-argument execute() is reported at the preparation. + void preparedFromConcat(Connection conn, HttpServletRequest request) throws Exception { + String sql = "SELECT * FROM t WHERE id = '" + request.getParameter("id") + "'"; + // ruleid: java-sql-injection + PreparedStatement ps = conn.prepareStatement(sql); + ps.execute(); + } + + void templateConcat(HttpServletRequest request) { + String id = request.getParameter("id"); + // ruleid: java-sql-injection + jdbcTemplate.update("UPDATE t SET x = '" + id + "'"); + } + + // Bind parameters are the remediation, not an injection. Only the SQL + // string argument is the sink. + void templateParameterized(HttpServletRequest request) { + String id = request.getParameter("id"); + // ok: java-sql-injection + jdbcTemplate.update("UPDATE t SET x = ?", id); + // ok: java-sql-injection + jdbcTemplate.queryForObject("SELECT c FROM t WHERE id = ?", Integer.class, id); + } + + void preparedParameterized(Connection conn, HttpServletRequest request) throws Exception { + // ok: java-sql-injection + PreparedStatement ps = conn.prepareStatement("SELECT * FROM t WHERE id = ?"); + ps.setString(1, request.getParameter("id")); + ps.execute(); + } + + // update() on a digest is not a query, whether the receiver is a declared + // variable or the factory call is chained inline. + void digests(HttpServletRequest request) throws Exception { + String input = request.getParameter("p"); + MessageDigest md = MessageDigest.getInstance("SHA-256"); + // ok: java-sql-injection + md.update(input.getBytes()); + // ok: java-sql-injection + MessageDigest.getInstance("SHA-256").update(input.getBytes()); + } +} diff --git a/tests/fixtures/opengrep/java/UnsafeDeserialization.java b/tests/fixtures/opengrep/java/UnsafeDeserialization.java new file mode 100644 index 0000000..1413d53 --- /dev/null +++ b/tests/fixtures/opengrep/java/UnsafeDeserialization.java @@ -0,0 +1,81 @@ +// Fixtures for java-unsafe-deserialization. +import java.io.*; +import org.yaml.snakeyaml.Yaml; +import org.yaml.snakeyaml.constructor.SafeConstructor; +import org.yaml.snakeyaml.LoaderOptions; + +public class UnsafeDeserialization implements Serializable { + // Field-held instances are the common real-world shape. + private final Yaml safeYaml = new Yaml(new SafeConstructor(new LoaderOptions())); + private final Yaml unsafeYaml = new Yaml(); + + Object fromStream(ObjectInputStream ois) throws Exception { + // ruleid: java-unsafe-deserialization + return ois.readObject(); + } + + Object inline(InputStream in) throws Exception { + // ruleid: java-unsafe-deserialization + return new ObjectInputStream(in).readObject(); + } + + Object yamlUnsafe(String s) { + // ruleid: java-unsafe-deserialization + return new Yaml().load(s); + } + + Object yamlUnsafeField(String s) { + // ruleid: java-unsafe-deserialization + return unsafeYaml.load(s); + } + + // loadAs and loadAll deserialize the same way load does. + Object yamlUnsafeLoadAs(String s) { + // ruleid: java-unsafe-deserialization + return new Yaml().loadAs(s, Object.class); + } + + // The rule's own fix text recommends SafeConstructor, so it must not fire. + Object yamlSafeInline(String s) { + // ok: java-unsafe-deserialization + return new Yaml(new SafeConstructor()).load(s); + } + + Object yamlSafeVariable(String s) { + Yaml y = new Yaml(new SafeConstructor()); + // ok: java-unsafe-deserialization + return y.load(s); + } + + Object yamlSafeField(String s) { + // ok: java-unsafe-deserialization + return safeYaml.load(s); + } + + // SnakeYAML 2.0 removed the no-arg SafeConstructor. + Object yamlSafeLoaderOptions(String s) { + // ok: java-unsafe-deserialization + return new Yaml(new SafeConstructor(new LoaderOptions())).load(s); + } + + Iterable yamlSafeLoadAll(String s) { + // ok: java-unsafe-deserialization + return new Yaml(new SafeConstructor(new LoaderOptions())).loadAll(s); + } + + // Implementing the Serializable contract, including the standard throws. + private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException { + // ok: java-unsafe-deserialization + Object ignoredValue = in.readObject(); + } + + // Not Java deserialization at all. + Object pem(PemParser pemParser) throws Exception { + // ok: java-unsafe-deserialization + return pemParser.readObject(); + } +} + +class PemParser { + Object readObject() { return null; } +} diff --git a/tests/fixtures/opengrep/java/WeakCipher.java b/tests/fixtures/opengrep/java/WeakCipher.java new file mode 100644 index 0000000..2e2c574 --- /dev/null +++ b/tests/fixtures/opengrep/java/WeakCipher.java @@ -0,0 +1,34 @@ +// Fixtures for java-weak-cipher. +// "ruleid:" marks a line that must be reported; "ok:" a line that must not be. +import javax.crypto.Cipher; +import javax.crypto.spec.SecretKeySpec; + +public class WeakCipher { + void broken() throws Exception { + // ruleid: java-weak-cipher + Cipher.getInstance("DES/CBC/PKCS5Padding"); + // ruleid: java-weak-cipher + javax.crypto.Cipher.getInstance("DESede/CBC/PKCS5Padding", "SunJCE"); + // ruleid: java-weak-cipher + Cipher.getInstance("Blowfish"); + // ruleid: java-weak-cipher + Cipher.getInstance("RC4"); + // ECB really is a block mode here. + // ruleid: java-weak-cipher + Cipher.getInstance("AES/ECB/PKCS5Padding"); + // ruleid: java-weak-cipher + new SecretKeySpec(new byte[8], "DES"); + } + + void fine() throws Exception { + // ok: java-weak-cipher + Cipher.getInstance("AES/GCM/NoPadding"); + // In an RSA transformation "ECB" is a JCA placeholder, not a block mode. + // ok: java-weak-cipher + Cipher.getInstance("RSA/ECB/PKCS1Padding"); + // ok: java-weak-cipher + Cipher.getInstance("RSA/ECB/OAEPWithSHA-256AndMGF1Padding"); + // ok: java-weak-cipher + new SecretKeySpec(new byte[32], "AES"); + } +} diff --git a/tests/test_java_opengrep_rules.py b/tests/test_java_opengrep_rules.py new file mode 100644 index 0000000..bbdf195 --- /dev/null +++ b/tests/test_java_opengrep_rules.py @@ -0,0 +1,185 @@ +"""Regression tests for the bundled Java opengrep rules. + +Each fixture under ``tests/fixtures/opengrep/java`` annotates the line that +follows it with either ``// ruleid: `` (the rule must report that line) or +``// ok: `` (the rule must not report it). These are cheap guards against +the regex and taint regressions that are easy to reintroduce when editing +``java.yml``; they do not need the OWASP Benchmark corpus. + +Skipped when ``opengrep`` is not on PATH, so they are a no-op for contributors +who only touch Python. +""" + +from __future__ import annotations + +import json +import os +import re +import shutil +import subprocess +import tempfile +from collections import defaultdict +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parent.parent +RULES = REPO_ROOT / "socket_basics" / "rules" / "java.yml" +FIXTURES = REPO_ROOT / "tests" / "fixtures" / "opengrep" / "java" + +ANNOTATION = re.compile(r"//\s*(ruleid|ok):\s*([\w-]+)\s*$") + +# CI sets this so a missing opengrep fails the job instead of silently +# skipping every test in this module. +REQUIRE_OPENGREP_ENV = "SOCKET_BASICS_REQUIRE_OPENGREP" + +_HAVE_OPENGREP = shutil.which("opengrep") is not None +if not _HAVE_OPENGREP and os.environ.get(REQUIRE_OPENGREP_ENV): + pytest.fail( + f"{REQUIRE_OPENGREP_ENV} is set but opengrep is not on PATH", pytrace=False + ) + +pytestmark = pytest.mark.skipif( + not _HAVE_OPENGREP, + reason="opengrep is not installed; Java rule regression tests skipped", +) + + +def _expectations() -> tuple[set[tuple[str, str, int]], set[tuple[str, str, int]]]: + """Return (must_report, must_not_report) as {(file, rule, line)} sets.""" + expected: set[tuple[str, str, int]] = set() + forbidden: set[tuple[str, str, int]] = set() + for path in sorted(FIXTURES.glob("*.java")): + lines = path.read_text().splitlines() + for idx, line in enumerate(lines): + match = ANNOTATION.search(line) + if not match: + continue + kind, rule = match.group(1), match.group(2) + # The annotation refers to the next non-comment line. + target = idx + 1 + while target < len(lines) and lines[target].strip().startswith("//"): + target += 1 + if target >= len(lines): + continue + entry = (path.name, rule, target + 1) # 1-indexed + (expected if kind == "ruleid" else forbidden).add(entry) + return expected, forbidden + + +def _scan() -> set[tuple[str, str, int]]: + """Run opengrep over a copy of the fixtures and return {(file, rule, line)}.""" + fixtures = sorted(FIXTURES.glob("*.java")) + # Scan a copy outside the repository. opengrep's default ignore list skips + # any path with a tests/ directory in it, and on some versions (1.19.0) + # that applies even to explicitly listed files, so scanning in place + # silently scanned nothing and every positive annotation "failed". + with tempfile.TemporaryDirectory(prefix="opengrep-java-fixtures-") as tmp: + for path in fixtures: + shutil.copy(path, tmp) + out = Path(tmp) / "results.json" + proc = subprocess.run( + [ + "opengrep", "--json", "--quiet", "--no-git-ignore", + "--config", str(RULES), "--output", str(out), tmp, + ], + capture_output=True, + text=True, + check=False, + ) + assert out.exists(), ( + f"opengrep wrote no output (exit {proc.returncode}): {proc.stderr[-2000:]}" + ) + data = json.loads(out.read_text() or "{}") + + errors = [e.get("message", str(e))[:200] for e in data.get("errors", [])] + assert not errors, f"opengrep reported errors while scanning the fixtures: {errors}" + scanned = {Path(p).name for p in data.get("paths", {}).get("scanned", [])} + expected = {p.name for p in fixtures} + assert scanned == expected, ( + f"opengrep scanned {sorted(scanned)} but the fixtures are {sorted(expected)}" + ) + + found: set[tuple[str, str, int]] = set() + for result in data.get("results", []): + rule = result.get("check_id", "").split(".")[-1] + name = Path(result.get("path", "")).name + start = result.get("start", {}).get("line") + end = result.get("end", {}).get("line", start) + # A match can span several lines; credit every line it covers so an + # annotation on the first line of a multi-line statement still matches. + for line in range(start, (end or start) + 1): + found.add((name, rule, line)) + return found + + +@pytest.fixture(scope="module") +def scan_results() -> set[tuple[str, str, int]]: + return _scan() + + +def test_fixtures_have_annotations() -> None: + expected, forbidden = _expectations() + assert expected, "no positive fixture annotations were collected" + assert forbidden, "no negative fixture annotations were collected" + + +def test_expected_findings_are_reported(scan_results) -> None: + expected, _ = _expectations() + missing = sorted(entry for entry in expected if entry not in scan_results) + assert not missing, "rules failed to report annotated true positives: " + ", ".join( + f"{name}:{line} {rule}" for name, rule, line in missing + ) + + +def test_forbidden_findings_are_not_reported(scan_results) -> None: + _, forbidden = _expectations() + reported = sorted(entry for entry in forbidden if entry in scan_results) + assert not reported, "rules reported annotated false positives: " + ", ".join( + f"{name}:{line} {rule}" for name, rule, line in reported + ) + + +def test_rules_config_is_valid() -> None: + result = subprocess.run( + ["opengrep", "--validate", "--config", str(RULES)], + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 0, result.stderr or result.stdout + + +def test_every_annotated_rule_exists() -> None: + import yaml + + ids = {rule["id"] for rule in yaml.safe_load(RULES.read_text())["rules"]} + expected, forbidden = _expectations() + referenced = {rule for _, rule, _ in expected | forbidden} + unknown = sorted(referenced - ids) + assert not unknown, f"fixtures reference rules that do not exist: {unknown}" + + +def test_no_unannotated_findings(scan_results) -> None: + """Every finding must land on an annotated line. + + Without this, an annotation can be satisfied by an unrelated finding that + happens to cover the same line, and the fixture silently stops guarding + the behaviour it was written for. + """ + expected, forbidden = _expectations() + annotated = {(name, rule, line) for name, rule, line in expected | forbidden} + stray = sorted(entry for entry in scan_results if entry not in annotated) + # A multi-line match credits every line it spans, so only report a finding + # when none of its lines carry an annotation for that rule. + by_rule_file = defaultdict(set) + for name, rule, line in annotated: + by_rule_file[(name, rule)].add(line) + unexplained = [ + (name, rule, line) + for name, rule, line in stray + if line not in by_rule_file.get((name, rule), set()) + ] + assert not unexplained, "findings on unannotated lines: " + ", ".join( + f"{name}:{line} {rule}" for name, rule, line in unexplained + )