From 667bf9cfc2a8586e0605b1ba9e031c8413becb73 Mon Sep 17 00:00:00 2001 From: David Larsen Date: Sat, 5 Sep 2026 16:03:24 -0400 Subject: [PATCH 1/7] fix(rules): improve precision and recall of java opengrep rules Addresses a customer SAST evaluation that reported roughly 90% false positives from the Java rules and compared them unfavourably to CodeQL. Reproduced on six mature open source Java projects (guava, netty, spring-framework, commons-lang, commons-io, spring-petclinic, ~17,400 Java files): the rule set emitted 1,631 findings, and a hand adjudicated random sample of 40 contained zero true positives. Three rules produced 74% of that volume. Precision fixes: - java-empty-catch-block: 645 findings, all noise. Restrict to swallowed broad exceptions, exclude the conventional "ignored"/"expected" variable names, and exclude blocks carrying an explanatory comment. Comments are not AST nodes, so a documented catch block still looks empty to the matcher and had to be excluded textually. - java-reflection-injection: 296 findings. Was matching every method.invoke(), every newInstance() factory call, and Class.forName() on string constants. Converted to taint mode with servlet and Spring MVC sources and dynamic-class-loading and script-eval sinks. - java-system-out-usage: 263 findings. The message claims sensitive data in logs but the rule matched any println. Now requires the printed expression to reference something credential bearing. - java-hardcoded-credentials: matched on variable name alone, flagging KEY_ATTRIBUTE = "key" and SEC_WEBSOCKET_KEY1. Ported the value inspection approach already applied to the dotnet rules in #63: bare "key" only counts in compound credential words, and values shaped like header names, property paths, or a restatement of the keyword itself are excluded. Now zero findings across all six mature libraries while still catching WebGoat's default credentials. - java-unsafe-deserialization: required the receiver to actually be an ObjectInputStream, and excluded calls inside a class's own readObject and readExternal implementations, which are the Serializable contract. - java-insecure-random: converted to taint mode. A weak PRNG is only a vulnerability when its output becomes a security value, not when it seeds a JMH benchmark or shuffles a list. - java-hardcoded-ip: required a full dotted quad and excluded loopback. - java-insecure-cookie: bound the setSecure(true) exclusion to the same variable, so one hardened cookie no longer exonerates every other cookie in the method. Recall fixes. Two systematic bugs suppressed entire categories: - Patterns using simple type names never matched fully qualified call sites, so java.security.MessageDigest.getInstance("MD5"), new java.util.Random() and new javax.servlet.http.Cookie() were all invisible. Added qualified variants throughout. - Crypto rules matched exact algorithm literals, so Cipher.getInstance("DES/CBC/PKCS5Padding") did not match "DES". Replaced with metavariable-regex over the transformation string, and covered the provider overloads of getInstance. Also added java-xss and java-xpath-injection, both taint mode, and converted java-ldap-injection and java-path-traversal to taint with Zip Slip and Spring multipart sources. Validated with opengrep 1.25.0. OWASP Benchmark v1.2 (2,740 annotated cases, ground truth): precision 64.5% -> 76.5% recall 12.4% -> 63.4% score 5.1 -> 42.6 securecookie, weakrand, crypto and hash reach 100% precision. Mature open source Java projects: 1,631 -> 130 findings (-92%) unique findings on mature libraries 1,536 -> 85 (-94.5%) WebGoat: 87 -> 45. The removed findings are lint noise and three reflection matches on factory calls; the planted vulnerabilities, including the Zip Slip, default credentials and weak PRNG, still fire. Methodology, per-category results, the known limits of OWASP Benchmark for pattern-based engines, and the remaining untouched noise sources are documented in docs/java-sast-benchmark.md, with a reusable scorer in scripts/score_owasp_benchmark.py. --- docs/java-sast-benchmark.md | 139 ++++ scripts/score_owasp_benchmark.py | 137 ++++ socket_basics/rules/java.yml | 1078 ++++++++++++++++++++++++++---- 3 files changed, 1227 insertions(+), 127 deletions(-) create mode 100644 docs/java-sast-benchmark.md create mode 100644 scripts/score_owasp_benchmark.py diff --git a/docs/java-sast-benchmark.md b/docs/java-sast-benchmark.md new file mode 100644 index 0000000..4de4bfc --- /dev/null +++ b/docs/java-sast-benchmark.md @@ -0,0 +1,139 @@ +# 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 + +```bash +git clone --depth 1 https://github.com/OWASP-Benchmark/BenchmarkJava.git + +opengrep --json --dataflow-traces --quiet -a --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. + +## Results + +Measured with opengrep 1.25.0. + +### OWASP Benchmark v1.2 (ground truth) + +| | Before | After | +|---|---|---| +| Precision | 64.5% | **76.5%** | +| Recall | 12.4% | **63.4%** | +| False positive rate | 7.3% | 20.8% | +| Benchmark score (TPR - FPR) | 5.1 | **42.6** | +| True positives found | 176 | **897** | + +Per category, after the change: + +| Category | Precision | Recall | +|---|---|---| +| securecookie | 100.0% | 100.0% | +| weakrand | 100.0% | 91.7% | +| 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% | +| cmdi | 63.6% | 44.4% | +| sqli | 64.9% | 44.1% | + +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 | 130 | **-92%** | +| Unique findings, mature libraries only | 1,536 | 85 | **-94.5%** | + +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, 45 after. The removed findings were lint noise +(`java-system-out-usage` 26, `java-hardcoded-ip` 5, `java-empty-catch-block` 4) +plus three `java-reflection-injection` matches on factory `newInstance()` calls +and a JDK dynamic proxy. 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. + +**Remaining noise not addressed here.** On the mature-library corpus these rules +were untouched by this change and are still 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` | 14 | Library serialization helpers that accept a caller-supplied `ObjectInputStream` | +| `java-jndi-injection` | 8 | Matches any `.lookup(...)` call | +| `java-sql-injection` | 4 | `$STMT.execute(...)` and `$TEMPLATE.query(...)` sinks match any method of those names | + +`trustbound` (CWE-501) has no rule at all; OWASP Benchmark scores 126 cases for it. diff --git a/scripts/score_owasp_benchmark.py b/scripts/score_owasp_benchmark.py new file mode 100644 index 0000000..de6a86e --- /dev/null +++ b/scripts/score_owasp_benchmark.py @@ -0,0 +1,137 @@ +#!/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 +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-trust-boundary-violation": "trustbound", + "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__": + main(sys.argv[1], sys.argv[2]) diff --git a/socket_basics/rules/java.yml b/socket_basics/rules/java.yml index 3be0c86..b10130f 100644 --- a/socket_basics/rules/java.yml +++ b/socket_basics/rules/java.yml @@ -3,22 +3,74 @@ 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(...) + - pattern: $ENUM.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 @@ -114,14 +166,43 @@ 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] - pattern-either: - - pattern: new ObjectInputStream($STREAM).readObject() - - pattern: $OIS.readObject() - - pattern: XMLDecoder.readObject() - - pattern: Yaml.load($INPUT) + paths: + exclude: + - "*/src/test/*" + - "*/src/testFixtures/*" + - "*/test/java/*" + - "*/testsuite/*" + - "*/microbench/*" + - "*/jmh/*" + - "*Test.java" + - "*Tests.java" + - "*TestCase.java" + - "*Benchmark.java" + patterns: + - pattern-either: + # 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. + - pattern: (ObjectInputStream $OIS).readObject() + - pattern: new ObjectInputStream(...).readObject() + - pattern: (XMLDecoder $D).readObject() + - pattern: new XMLDecoder(...).readObject() + # SnakeYAML without a SafeConstructor deserializes arbitrary types + - pattern: new Yaml().load(...) + - pattern: (Yaml $Y).load(...) + # 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 { ... } metadata: category: security cwe: CWE-502 @@ -129,7 +210,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 +273,214 @@ 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: + - pattern: (DirContext $CTX).search(...) + - pattern: (InitialDirContext $CTX).search(...) + - pattern: (javax.naming.directory.DirContext $CTX).search(...) + - pattern: $CTX.search($BASE, $FILTER, ...) + - pattern: new SearchFilter(...) + - pattern: new LdapName(...) + - pattern: new javax.naming.ldap.LdapName(...) + - pattern: (LdapTemplate $T).search(...) + - 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 name containing lowercase is an HTTP header name + # (Access-Control-Allow-Credentials, Sec-Token-Binding). An + # all-caps hyphenated value such as STAGING-TOKEN-42 is still + # reported, because that shape really is a secret. + regex: ^(?!^(?=.*[a-z])[A-Za-z][A-Za-z0-9]*(-[A-Za-z0-9]+)+$).*$ + - metavariable-regex: + metavariable: $VALUE + # A lowercase value that just restates the credential keyword is + # naming a field or an auth method ("password", "accesskey", + # "stompCredentials"), not holding one. Real default credentials + # such as "admin" or "webgoat" do not restate the keyword. + regex: ^(?!(?=[a-z])(?i:[a-z0-9]*(password|passwd|credential|secret|token|apikey|accesskey|auth)[a-z0-9]*)$).*$ + # 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 +492,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 +517,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 +541,109 @@ 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: 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(...) + 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 + regex: (?i).*(token|session|nonce|salt|secret|password|passwd|credential|apikey|api_key|privatekey|private_key|otp|pin|pincode|csrf|iv|initvector|cookie|auth|verifier|challenge|resetcode|activation|captcha|guid|uuid|seed|key) + - 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|otp|pin|pincode|csrf|iv|initvector|cookie|auth|verifier|challenge|resetcode|activation|captcha|guid|uuid|key) + # 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,54 +672,159 @@ 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 + # 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. - pattern: new File(...) + - pattern: new java.io.File(...) - 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: new java.io.RandomAccessFile(...) + - pattern: Files.newInputStream(...) + - pattern: Files.newOutputStream(...) + - pattern: Files.newBufferedReader(...) + - pattern: Files.newBufferedWriter(...) - pattern: Files.readAllBytes(...) - pattern: Files.readAllLines(...) - pattern: Files.write(...) @@ -375,16 +832,24 @@ rules: - pattern: Files.move(...) - pattern: Files.delete(...) - pattern: Files.deleteIfExists(...) + - pattern: java.nio.file.Files.readAllBytes(...) + - pattern: java.nio.file.Files.write(...) + - pattern: java.nio.file.Files.copy(...) + - pattern: java.nio.file.Files.newInputStream(...) + - pattern: java.nio.file.Files.newOutputStream(...) - pattern: Paths.get(...) - # Legacy IO operations - - pattern: $FILE.createNewFile(...) - - pattern: $FILE.delete() - - pattern: $FILE.renameTo(...) + - pattern: java.nio.file.Paths.get(...) 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: + - pattern: $PATH.startsWith($BASE) + - focus-metavariable: $PATH + - pattern: FilenameUtils.getName(...) + - pattern: org.apache.commons.io.FilenameUtils.getName(...) + - pattern: Integer.parseInt(...) + - pattern: UUID.fromString(...) metadata: category: security cwe: CWE-22 @@ -392,7 +857,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 +892,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)([/].*)?$|^.*/ecb/.*$ metadata: category: security cwe: CWE-327 @@ -443,6 +915,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 +991,166 @@ 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] - pattern-either: - - pattern: | - Cookie $COOKIE = new Cookie($NAME, $VALUE); - - pattern: | - new Cookie($NAME, $VALUE); - pattern-not-inside: - pattern: | - ... - $COOKIE.setSecure(true); - ... + paths: + exclude: + - "*/src/test/*" + - "*/src/testFixtures/*" + - "*/test/java/*" + - "*/testsuite/*" + - "*/microbench/*" + - "*/jmh/*" + - "*Test.java" + - "*Tests.java" + - "*TestCase.java" + - "*Benchmark.java" + patterns: + - pattern-either: + # Servlet code very often writes the constructor fully qualified. + - pattern: new Cookie($NAME, $VALUE) + - pattern: new javax.servlet.http.Cookie($NAME, $VALUE) + - pattern: new jakarta.servlet.http.Cookie($NAME, $VALUE) + # The exclusion is bound to the same variable. A scope-wide + # "any setSecure(true) nearby" check let one hardened cookie exonerate + # every other cookie in the same method. + - 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); 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|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 +1326,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." From 87b814f9c6a30a9519f3a4b6a9c9a8c17a6a027f Mon Sep 17 00:00:00 2001 From: David Larsen Date: Wed, 9 Sep 2026 09:00:19 -0400 Subject: [PATCH 2/7] fix(rules): address review feedback on java rule precision Six of Bugbot's seven findings reproduced and are fixed; the seventh (the readObject throws clause) did not reproduce and is dismissed. Five further defects found in review are fixed alongside them. Precision: - java-weak-cipher: RSA/ECB/PKCS1Padding was reported as a broken ECB cipher. In a JCA RSA transformation "ECB" is a placeholder, not a block mode, and is the standard spelling. AES/ECB/... is still reported. - java-insecure-cookie: the exclusion region ran from one cookie's declaration to its setSecure call, so a second cookie constructed inside that region was dropped whenever a neighbour was hardened. The positive pattern is now a declaration, which forces $COOKIE to unify between the match and the exclusion. Added an assignment form so hardening through a field is recognised, and a branch for a cookie constructed inline and never assigned. - java-insecure-random: the sink name regex was unanchored, so the short words matched as substrings of ordinary identifiers: iv in pivot and divisor, pin in spinner, key in monkey, auth in author. The short words now require a word boundary and seed is dropped entirely. Two details worth remembering: metavariable-regex anchors at the start of the name, so each alternation branch needs its own leading .*, and a leading global (?i) also lowercased the deliberately case-sensitive camelCase branch, which is what let those substrings through. - java-unsafe-deserialization: a SnakeYAML load using new Yaml(new SafeConstructor()) was reported even though that is the remediation the rule's own fix text recommends. - java-ldap-injection: an untyped $CTX.search(...) sink turned a Lucene IndexSearcher.search() into a CRITICAL LDAP finding. Sinks are now type constrained. Dropping the untyped sink initially halved Benchmark recall, which turned out to be the same qualified-name bug fixed elsewhere in this branch: the corpus declares javax.naming.directory.InitialDirContext and only the simple name was covered. Recall is restored at 77.8%. - java-reflection-injection: $ENUM.valueOf(...) also matched String.valueOf, so taint laundered through a plain string conversion escaped detection. Integer.valueOf and Long.valueOf remain sanitizers. Recall: - java-hardcoded-ip: the 10 branch allowed only two more octets, so "10.0.0.1" was missed while the version string "10.2.3" was reported. Requires a full dotted quad. - java-insecure-random: Random.nextBytes is void and fills the caller's array, so tainting the call expression never reached SecretKeySpec or IvParameterSpec. Added a by-side-effect source focused on the argument. weakrand now scores 100% precision at 100% recall. - java-path-traversal: the startsWith containment sanitizer lacked by-side-effect, so it only cleaned the startsWith expression itself and the checked variable stayed tainted at every later sink. The containment check suppressed nothing. - java-hardcoded-credentials: the hyphen exclusion treated any hyphenated lowercase value as a header name, which dropped sk-live-... and xoxb-... style keys. Digits are the discriminator: header names essentially never contain one. Added explicit exclusions for x- prefixed names and HTTP auth scheme words, and widened the restatement check to underscores so "access_token" and "j_password" are excluded. Tooling and docs: - Added Java rule regression fixtures under tests/fixtures/opengrep/java with // ruleid: and // ok: annotations, and tests/test_java_opengrep_rules.py to score them. Every defect above is covered. The tests skip when opengrep is absent. Note that opengrep's default ignore list skips any directory named tests/, so the harness passes explicit file paths. - scripts/score_owasp_benchmark.py: added a usage guard, set the exec bit, and removed the java-trust-boundary-violation mapping for a rule that does not exist, so trustbound reads as unscored rather than 0% recall. - Pinned the engine version and the BenchmarkJava commit in the doc, added the two newly confirmed known limits (a SecureRandom held in a Random-typed variable, and the statement-scoped comment exclusion in java-empty-catch-block), and re-ran every number. - Added a CHANGELOG entry. Re-measured on opengrep 1.25.0, BenchmarkJava at 51f0a7c: precision 64.5% -> 76.8% (was 76.5% before this commit) recall 12.4% -> 68.7% (was 63.4%) score 5.1 -> 47.2 (was 42.6) securecookie and weakrand both 100% precision at 100% recall. Mature open source Java projects: 1,631 -> 129 findings, and 1,536 -> 84 unique findings on the mature libraries alone. WebGoat 87 -> 45. Full suite: 344 passed. opengrep --validate clean at 32 rules. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 58 +++++++ docs/java-sast-benchmark.md | 96 ++++++++++-- scripts/score_owasp_benchmark.py | 12 +- socket_basics/rules/java.yml | 145 +++++++++++++----- .../opengrep/java/HardcodedCredentials.java | 46 ++++++ tests/fixtures/opengrep/java/HardcodedIp.java | 22 +++ .../opengrep/java/InsecureCookie.java | 38 +++++ .../opengrep/java/InsecureRandom.java | 60 ++++++++ .../fixtures/opengrep/java/LdapInjection.java | 36 +++++ .../fixtures/opengrep/java/PathTraversal.java | 48 ++++++ .../opengrep/java/ReflectionInjection.java | 41 +++++ .../opengrep/java/UnsafeDeserialization.java | 49 ++++++ tests/fixtures/opengrep/java/WeakCipher.java | 34 ++++ tests/test_java_opengrep_rules.py | 144 +++++++++++++++++ 14 files changed, 782 insertions(+), 47 deletions(-) mode change 100644 => 100755 scripts/score_owasp_benchmark.py create mode 100644 tests/fixtures/opengrep/java/HardcodedCredentials.java create mode 100644 tests/fixtures/opengrep/java/HardcodedIp.java create mode 100644 tests/fixtures/opengrep/java/InsecureCookie.java create mode 100644 tests/fixtures/opengrep/java/InsecureRandom.java create mode 100644 tests/fixtures/opengrep/java/LdapInjection.java create mode 100644 tests/fixtures/opengrep/java/PathTraversal.java create mode 100644 tests/fixtures/opengrep/java/ReflectionInjection.java create mode 100644 tests/fixtures/opengrep/java/UnsafeDeserialization.java create mode 100644 tests/fixtures/opengrep/java/WeakCipher.java create mode 100644 tests/test_java_opengrep_rules.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 470706c..3cf7b05 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,64 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Added +- Two new Java SAST rules, both taint mode: `java-xss` (CWE-79) and + `java-xpath-injection` (CWE-643). XSS was the largest recall gap in the Java + rule set, accounting for 246 missed true positives on the OWASP Benchmark + corpus. (#112) +- `scripts/score_owasp_benchmark.py` scores an OpenGrep JSON run against the + OWASP Benchmark v1.2 `expectedresults` CSV, reporting per-category precision, + recall, false positive rate and the Benchmark score, plus per-rule TP/FP + counts. (#112) +- `docs/java-sast-benchmark.md` documents the benchmarking method, the + before/after numbers, the known limits of OWASP Benchmark for pattern-based + engines, and the remaining noise sources in the Java rules. (#112) +- Java rule regression fixtures under `tests/fixtures/opengrep/java` with + `// ruleid:` and `// ok:` annotations, exercised by + `tests/test_java_opengrep_rules.py`. The tests skip when `opengrep` is not on + `PATH`, so they are a no-op for contributors who only touch Python. (#112) + +### Fixed +- **Java SAST precision.** Twelve Java rules were rewritten after a customer + evaluation reported roughly 90% false positives. On six mature open source + Java projects (~17,400 files) the rule set previously emitted 1,631 findings, + of which a hand adjudicated random sample of 40 contained no true positives. + `java-empty-catch-block`, `java-reflection-injection`, + `java-system-out-usage` and `java-hardcoded-credentials` produced most of that + volume and now report nothing on those projects. `java-reflection-injection`, + `java-insecure-random`, `java-path-traversal` and `java-ldap-injection` were + converted to taint mode. (#112) +- **Java SAST recall.** Two systematic defects suppressed whole categories. + Patterns written with simple type names never matched fully qualified call + sites, so `java.security.MessageDigest.getInstance("MD5")`, + `new java.util.Random()` and `new javax.servlet.http.Cookie(...)` were + invisible; qualified variants were added throughout. Crypto rules matched + exact algorithm literals, so `Cipher.getInstance("DES/CBC/PKCS5Padding")` + never matched a rule looking for `"DES"`; these now use `metavariable-regex` + over the transformation string and cover the provider overloads of + `getInstance`. (#112) +- `java-insecure-cookie` no longer drops an unhardened cookie when a + neighbouring cookie in the same method calls `setSecure(true)`. The exclusion + is bound per variable and also recognises hardening through a field. (#112) +- `java-weak-cipher` no longer reports `RSA/ECB/PKCS1Padding`, where `ECB` is a + JCA placeholder rather than a block mode. `AES/ECB/...` is still reported. + (#112) +- `java-hardcoded-ip` now requires a full dotted quad, so version strings such + as `"10.0"` and `"10.2.3"` are no longer reported and `"10.0.0.1"` is. (#112) +- `java-path-traversal` honours a `startsWith` containment check at later file + sinks, and detects Zip Slip via `ZipEntry.getName()` and Spring multipart + uploads via `MultipartFile.getOriginalFilename()`. (#112) +- `java-insecure-random` detects weak key and IV material generated through + `Random.nextBytes(array)`, and no longer treats `pivot`, `divisor`, `spinner`, + `monkey` or `author` as security-relevant names. (#112) +- `java-unsafe-deserialization` no longer reports SnakeYAML loads that use + `new Yaml(new SafeConstructor())`, which is the remediation the rule itself + recommends, and no longer matches unrelated `readObject()` APIs such as + BouncyCastle's `PEMParser`. (#112) +- `java-ldap-injection` no longer reports a Lucene `IndexSearcher.search()` call + as a CRITICAL LDAP injection. Sinks are type constrained to the LDAP APIs. + (#112) + ## [3.1.0] - 2026-09-02 ### Added diff --git a/docs/java-sast-benchmark.md b/docs/java-sast-benchmark.md index 4de4bfc..e7d97a0 100644 --- a/docs/java-sast-benchmark.md +++ b/docs/java-sast-benchmark.md @@ -34,8 +34,20 @@ 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.25.0**. The images pin + `OPENGREP_VERSION=v1.26.0` 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 --depth 1 https://github.com/OWASP-Benchmark/BenchmarkJava.git +git clone https://github.com/OWASP-Benchmark/BenchmarkJava.git +git -C BenchmarkJava checkout 51f0a7cf8bb9d17ce1f6d72598c1d1c6ce90f661 opengrep --json --dataflow-traces --quiet -a --no-git-ignore \ --config socket_basics/rules/java.yml \ @@ -50,6 +62,33 @@ 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 +``` + +The tests skip when `opengrep` is not on `PATH`. 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. + +Two 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 directory named `tests/`, so the test + harness passes explicit file paths rather than the fixture directory. + ## Results Measured with opengrep 1.25.0. @@ -58,18 +97,18 @@ Measured with opengrep 1.25.0. | | Before | After | |---|---|---| -| Precision | 64.5% | **76.5%** | -| Recall | 12.4% | **63.4%** | -| False positive rate | 7.3% | 20.8% | -| Benchmark score (TPR - FPR) | 5.1 | **42.6** | -| True positives found | 176 | **897** | +| Precision | 64.5% | **76.8%** | +| Recall | 12.4% | **68.7%** | +| False positive rate | 7.3% | 21.5% | +| Benchmark score (TPR - FPR) | 5.1 | **47.2** | +| True positives found | 176 | **915** | Per category, after the change: | Category | Precision | Recall | |---|---|---| | securecookie | 100.0% | 100.0% | -| weakrand | 100.0% | 91.7% | +| weakrand | 100.0% | 100.0% | | crypto | 100.0% | 74.6% | | hash | 100.0% | 69.0% | | xpathi | 60.0% | 80.0% | @@ -87,8 +126,8 @@ emitted findings that are real, is the comparable number and it improved. | | Before | After | Change | |---|---|---|---| -| Total findings | 1,631 | 130 | **-92%** | -| Unique findings, mature libraries only | 1,536 | 85 | **-94.5%** | +| Total findings | 1,631 | 129 | **-92%** | +| Unique findings, mature libraries only | 1,536 | 84 | **-94.5%** | 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 @@ -125,6 +164,36 @@ paths" comparison, and it caps achievable precision on `sqli`, `cmdi` and 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. + **Remaining noise not addressed here.** On the mature-library corpus these rules were untouched by this change and are still the largest remaining sources: @@ -135,5 +204,12 @@ were untouched by this change and are still the largest remaining sources: | `java-unsafe-deserialization` | 14 | Library serialization helpers that accept a caller-supplied `ObjectInputStream` | | `java-jndi-injection` | 8 | Matches any `.lookup(...)` call | | `java-sql-injection` | 4 | `$STMT.execute(...)` and `$TEMPLATE.query(...)` sinks match any method of those names | +| `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. +`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 old mode 100644 new mode 100755 index de6a86e..2abd9de --- a/scripts/score_owasp_benchmark.py +++ b/scripts/score_owasp_benchmark.py @@ -14,7 +14,9 @@ from collections import defaultdict from pathlib import Path -# Map socket-basics java rule ids -> OWASP Benchmark category +# 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", @@ -27,7 +29,6 @@ "java-weak-cipher": "crypto", "java-insecure-cookie": "securecookie", "java-xpath-injection": "xpathi", - "java-trust-boundary-violation": "trustbound", "java-xss": "xss", "java-template-injection": "xss", } @@ -134,4 +135,11 @@ def pct(n, d): 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 b10130f..e62d4a8 100644 --- a/socket_basics/rules/java.yml +++ b/socket_basics/rules/java.yml @@ -62,7 +62,12 @@ rules: pattern-sanitizers: # Resolving the value through an allowlist removes attacker control - pattern: $MAP.get(...) - - pattern: $ENUM.valueOf(...) + # 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-470 @@ -203,6 +208,15 @@ rules: public void readExternal($T $S) { ... } - pattern-not-inside: | public void readExternal($T $S) throws $EX { ... } + # A SafeConstructor load is the remediation this rule recommends. + - pattern-not: new Yaml(new SafeConstructor(), ...).load(...) + - pattern-not: new Yaml(new org.yaml.snakeyaml.constructor.SafeConstructor(), ...).load(...) + - 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 @@ -389,7 +403,11 @@ rules: - pattern: (DirContext $CTX).search(...) - pattern: (InitialDirContext $CTX).search(...) - pattern: (javax.naming.directory.DirContext $CTX).search(...) - - pattern: $CTX.search($BASE, $FILTER, ...) + - pattern: (javax.naming.directory.InitialDirContext $CTX).search(...) + - pattern: (LdapContext $CTX).search(...) + - pattern: (javax.naming.ldap.LdapContext $CTX).search(...) + - pattern: (LdapOperations $T).search(...) + - pattern: (org.springframework.ldap.core.LdapOperations $T).search(...) - pattern: new SearchFilter(...) - pattern: new LdapName(...) - pattern: new javax.naming.ldap.LdapName(...) @@ -463,18 +481,28 @@ rules: regex: ^(?!^[A-Za-z][A-Za-z0-9]*(\.[A-Za-z0-9]+)+$).*$ - metavariable-regex: metavariable: $VALUE - # A hyphenated name containing lowercase is an HTTP header name - # (Access-Control-Allow-Credentials, Sec-Token-Binding). An - # all-caps hyphenated value such as STAGING-TOKEN-42 is still - # reported, because that shape really is a secret. - regex: ^(?!^(?=.*[a-z])[A-Za-z][A-Za-z0-9]*(-[A-Za-z0-9]+)+$).*$ + # 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 lowercase value that just restates the credential keyword is # naming a field or an auth method ("password", "accesskey", - # "stompCredentials"), not holding one. Real default credentials - # such as "admin" or "webgoat" do not restate the keyword. - regex: ^(?!(?=[a-z])(?i:[a-z0-9]*(password|passwd|credential|secret|token|apikey|accesskey|auth)[a-z0-9]*)$).*$ + # "stompCredentials", "access_token", "j_password"), not holding + # one. Real default credentials such as "admin" or "webgoat" do + # not restate the keyword. + regex: ^(?!(?=[a-z])(?i:[a-z0-9_]*(password|passwd|credential|secret|token|apikey|accesskey|auth)[a-z0-9_]*)$).*$ # Pattern 2: credential APIs called with string literals - pattern: new PasswordAuthentication($USER, "...".toCharArray()) - pattern: DriverManager.getConnection($URL, $USER, "...") @@ -573,6 +601,15 @@ rules: - 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: 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 @@ -610,13 +647,22 @@ rules: - focus-metavariable: $SRC - metavariable-regex: metavariable: $VAR - regex: (?i).*(token|session|nonce|salt|secret|password|passwd|credential|apikey|api_key|privatekey|private_key|otp|pin|pincode|csrf|iv|initvector|cookie|auth|verifier|challenge|resetcode|activation|captcha|guid|uuid|seed|key) + # 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](?: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|otp|pin|pincode|csrf|iv|initvector|cookie|auth|verifier|challenge|resetcode|activation|captcha|guid|uuid|key) + 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](?: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`. @@ -846,6 +892,10 @@ rules: - patterns: - pattern: $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 - pattern: FilenameUtils.getName(...) - pattern: org.apache.commons.io.FilenameUtils.getName(...) - pattern: Integer.parseInt(...) @@ -907,7 +957,7 @@ rules: - pattern: KeyGenerator.getInstance("$TRANSFORM", ...) - metavariable-regex: metavariable: $TRANSFORM - regex: (?i)^(des|desede|tripledes|3des|rc2|rc4|arcfour|blowfish)([/].*)?$|^.*/ecb/.*$ + regex: (?i)^(des|desede|tripledes|3des|rc2|rc4|arcfour|blowfish)(/.*)?$|^(?!rsa/)[^/]+/ecb/.*$ metadata: category: security cwe: CWE-327 @@ -1006,27 +1056,52 @@ rules: - "*Tests.java" - "*TestCase.java" - "*Benchmark.java" - patterns: - - pattern-either: - # Servlet code very often writes the constructor fully qualified. - - pattern: new Cookie($NAME, $VALUE) - - pattern: new javax.servlet.http.Cookie($NAME, $VALUE) - - pattern: new jakarta.servlet.http.Cookie($NAME, $VALUE) - # The exclusion is bound to the same variable. A scope-wide - # "any setSecure(true) nearby" check let one hardened cookie exonerate - # every other cookie in the same method. - - 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); + pattern-either: + # 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 @@ -1143,7 +1218,7 @@ rules: # 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|192\.168|172\.(?:1[6-9]|2[0-9]|3[01]))\.[0-9]{1,3}\.[0-9]{1,3}(?::[0-9]{1,5})?"' + 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-547 diff --git a/tests/fixtures/opengrep/java/HardcodedCredentials.java b/tests/fixtures/opengrep/java/HardcodedCredentials.java new file mode 100644 index 0000000..5499527 --- /dev/null +++ b/tests/fixtures/opengrep/java/HardcodedCredentials.java @@ -0,0 +1,46 @@ +// 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"; + + // 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"; + // 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..f7390a6 --- /dev/null +++ b/tests/fixtures/opengrep/java/InsecureRandom.java @@ -0,0 +1,60 @@ +// 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); + } + + // 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..909609c --- /dev/null +++ b/tests/fixtures/opengrep/java/LdapInjection.java @@ -0,0 +1,36 @@ +// 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.search.Query query, + HttpServletRequest request) throws Exception { + String text = request.getParameter("q"); + // ok: java-ldap-injection + return searcher.search(query, Integer.parseInt(text)); + } +} diff --git a/tests/fixtures/opengrep/java/PathTraversal.java b/tests/fixtures/opengrep/java/PathTraversal.java new file mode 100644 index 0000000..0e1ce57 --- /dev/null +++ b/tests/fixtures/opengrep/java/PathTraversal.java @@ -0,0 +1,48 @@ +// 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"); + + 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. + void zipSlip(File zipFile, File dir) throws Exception { + ZipFile zip = new ZipFile(zipFile); + ZipEntry e = zip.entries().nextElement(); + // ruleid: java-path-traversal + File f = new File(dir, e.getName()); + Files.copy(zip.getInputStream(e), f.toPath()); + } + + // 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); + } + + // 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/UnsafeDeserialization.java b/tests/fixtures/opengrep/java/UnsafeDeserialization.java new file mode 100644 index 0000000..a1554af --- /dev/null +++ b/tests/fixtures/opengrep/java/UnsafeDeserialization.java @@ -0,0 +1,49 @@ +// Fixtures for java-unsafe-deserialization. +import java.io.*; +import org.yaml.snakeyaml.Yaml; +import org.yaml.snakeyaml.constructor.SafeConstructor; + +public class UnsafeDeserialization implements Serializable { + 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); + } + + // 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); + } + + // 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..6b06c6e --- /dev/null +++ b/tests/test_java_opengrep_rules.py @@ -0,0 +1,144 @@ +"""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 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*$") + +pytestmark = pytest.mark.skipif( + shutil.which("opengrep") is None, + 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 the fixtures and return {(file, rule, line)}.""" + with tempfile.NamedTemporaryFile(suffix=".json", delete=False) as handle: + out = Path(handle.name) + # Pass explicit file paths. opengrep's default ignore list skips any + # directory named tests/, so handing it FIXTURES scans nothing. + targets = [str(path) for path in sorted(FIXTURES.glob("*.java"))] + try: + subprocess.run( + [ + "opengrep", "--json", "--quiet", "-a", "--no-git-ignore", + "--config", str(RULES), "--output", str(out), *targets, + ], + capture_output=True, + text=True, + check=False, + ) + data = json.loads(out.read_text() or "{}") + finally: + out.unlink(missing_ok=True) + + 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 _group(entries): + grouped = defaultdict(list) + for name, rule, line in entries: + grouped[name].append((rule, line)) + return grouped From cb43e0033a89f225cb66b5eee22a991c3736973c Mon Sep 17 00:00:00 2001 From: David Larsen Date: Wed, 9 Sep 2026 09:24:56 -0400 Subject: [PATCH 3/7] fix(rules): correct regressions found in second-pass review An independent re-review of 87b814f found that three of the review fixes had over-corrected and one was incomplete. Verified each against a probe before changing anything; all four reproduced. Regressions introduced by the previous commit, now fixed: - java-insecure-random: anchoring the short credential words fixed the substring matches but broke leading position. otpCode, pinNumber, keyMaterial and every field assignment (this.key, this.otp, this.pin) stopped being reported, while shardKey still was. The word-boundary check now allows a camelCase suffix and a field-access prefix. pivot, divisor, spinner, monkey and author remain clean. - java-path-traversal: typing the startsWith sanitizer receiver. The untyped form let any String.startsWith sanitize, so a bypassable blacklist such as name.startsWith("..") suppressed the finding, in either branch and with any argument. Path.startsWith is component-wise containment; String.startsWith is a prefix test. Only the former is a sanitizer now. - java-hardcoded-credentials: widening the restatement exclusion to underscores also swallowed real weak defaults, dropping "password123", "secret_2024" and a planted WebGoat credential. Reused the digit discriminator already applied to the hyphen rule: a value that restates the field name never carries a digit. - java-unsafe-deserialization: the SafeConstructor exclusion only matched the no-arg constructor, which SnakeYAML 2.0 removed. It now accepts arguments, so new Yaml(new SafeConstructor(new LoaderOptions())) is excluded. Defects predating this branch, found by the same review: - java-sql-injection: the untyped $TEMPLATE.update(...) sink matched MessageDigest.update(input), producing 93 CRITICAL findings on the Benchmark's hash test cases. Typing the receiver was not workable, because the template is routinely reached through a static field, so the crypto receivers are subtracted instead. Added queryForRowSet and batchUpdate as sinks while there. sqli recall 44.1% -> 52.2% and precision 64.9% -> 66.0%. - java-insecure-random and java-ldap-injection were missing qualified-name variants (java.util.Random, javax.naming.ldap.InitialLdapContext). This is the defect class the branch claims to fix throughout. Tests and docs: - Fixtures cover every regression above: leading short words, field assignment, a qualified java.util.Random receiver, a String.startsWith blacklist, "password123", and SafeConstructor(LoaderOptions). The Lucene fixture was passing a value through Integer.parseInt, which is a listed sanitizer, so it guarded nothing; it now reaches the sink unparsed. - Added a test asserting every finding lands on an annotated line, so an annotation cannot be satisfied by an unrelated finding covering the same line. - Doc corrections: the Before column now comes from the current scorer (recall 13.2%, score 5.6; the old Before counted the 126 trustbound cases against recall on one side only), and it discloses the cross-category findings the OWASP method discards, the 26% of baseline volume that sits in now-excluded test and example paths, the duplicated guava android mirror, and the +70% scan time. Added known limits for the ambiguity of "key" as a name and for the deserialization helpers that remain. Re-measured on opengrep 1.25.0, BenchmarkJava at 51f0a7c: precision 64.5% -> 76.7% recall 13.2% -> 70.3% score 5.6 -> 48.2 true positives 176 -> 937 securecookie and weakrand 100% precision at 100% recall. Mature open source Java projects: 1,631 -> 129 findings, 1,536 -> 84 unique on the mature libraries. WebGoat 87 -> 45. Full suite: 345 passed. opengrep --validate clean at 32 rules. Co-Authored-By: Claude Opus 5 --- docs/java-sast-benchmark.md | 67 +++++++++++++++++-- socket_basics/rules/java.yml | 57 ++++++++++++---- .../opengrep/java/HardcodedCredentials.java | 7 ++ .../opengrep/java/InsecureRandom.java | 33 +++++++++ .../fixtures/opengrep/java/LdapInjection.java | 6 +- .../fixtures/opengrep/java/PathTraversal.java | 12 ++++ .../opengrep/java/UnsafeDeserialization.java | 7 ++ tests/test_java_opengrep_rules.py | 28 ++++++-- 8 files changed, 191 insertions(+), 26 deletions(-) diff --git a/docs/java-sast-benchmark.md b/docs/java-sast-benchmark.md index e7d97a0..0745de9 100644 --- a/docs/java-sast-benchmark.md +++ b/docs/java-sast-benchmark.md @@ -93,15 +93,27 @@ Two behaviours worth knowing when editing these: Measured with opengrep 1.25.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.8%** | -| Recall | 12.4% | **68.7%** | -| False positive rate | 7.3% | 21.5% | -| Benchmark score (TPR - FPR) | 5.1 | **47.2** | -| True positives found | 176 | **915** | +| Precision | 64.5% | **76.7%** | +| Recall | 13.2% | **70.3%** | +| False positive rate | 7.6% | 22.2% | +| Benchmark score (TPR - FPR) | 5.6 | **48.2** | +| True positives found | 176 | **937** | Per category, after the change: @@ -115,8 +127,21 @@ Per category, after the change: | ldapi | 58.3% | 77.8% | | xss | 67.4% | 70.7% | | pathtraver | 56.1% | 69.2% | +| sqli | 66.0% | 52.2% | | cmdi | 63.6% | 44.4% | -| sqli | 64.9% | 44.1% | + +**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.0% rather than 76.7%. 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 @@ -129,6 +154,21 @@ emitted findings that are real, is the comparable number and it improved. | Total findings | 1,631 | 129 | **-92%** | | Unique findings, mature libraries only | 1,536 | 84 | **-94.5%** | +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. @@ -194,6 +234,21 @@ 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. +**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 were untouched by this change and are still the largest remaining sources: diff --git a/socket_basics/rules/java.yml b/socket_basics/rules/java.yml index e62d4a8..26d02db 100644 --- a/socket_basics/rules/java.yml +++ b/socket_basics/rules/java.yml @@ -140,11 +140,30 @@ rules: - pattern: $SESSION.createQuery(...) - pattern: $SESSION.createSQLQuery(...) # JDBC template methods - - pattern: $TEMPLATE.query(...) - - pattern: $TEMPLATE.queryForObject(...) - - pattern: $TEMPLATE.queryForList(...) - - pattern: $TEMPLATE.execute(...) - - pattern: $TEMPLATE.update(...) + # 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. Instead + # keep the broad receiver and subtract the collisions, which are all + # update() on a crypto primitive rather than a query. + - patterns: + - pattern-either: + - pattern: $TEMPLATE.query(...) + - pattern: $TEMPLATE.queryForObject(...) + - pattern: $TEMPLATE.queryForList(...) + - pattern: $TEMPLATE.queryForRowSet(...) + - pattern: $TEMPLATE.execute(...) + - pattern: $TEMPLATE.update(...) + - pattern: $TEMPLATE.batchUpdate(...) + - 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(...) pattern-sanitizers: # PreparedStatement parameter binding - pattern: $STMT.setString(...) @@ -209,13 +228,15 @@ rules: - pattern-not-inside: | public void readExternal($T $S) throws $EX { ... } # A SafeConstructor load is the remediation this rule recommends. - - pattern-not: new Yaml(new SafeConstructor(), ...).load(...) - - pattern-not: new Yaml(new org.yaml.snakeyaml.constructor.SafeConstructor(), ...).load(...) + # 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(...) - pattern-not-inside: | - $T $Y = new Yaml(new SafeConstructor(), ...); + $T $Y = new Yaml(new SafeConstructor(...), ...); ... - pattern-not-inside: | - $T $Y = new Yaml(new org.yaml.snakeyaml.constructor.SafeConstructor(), ...); + $T $Y = new Yaml(new org.yaml.snakeyaml.constructor.SafeConstructor(...), ...); ... metadata: category: security @@ -406,6 +427,8 @@ rules: - 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: new SearchFilter(...) @@ -502,7 +525,7 @@ rules: # "stompCredentials", "access_token", "j_password"), not holding # one. Real default credentials such as "admin" or "webgoat" do # not restate the keyword. - regex: ^(?!(?=[a-z])(?i:[a-z0-9_]*(password|passwd|credential|secret|token|apikey|accesskey|auth)[a-z0-9_]*)$).*$ + regex: ^(?!(?=[a-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, "...") @@ -595,6 +618,7 @@ rules: - pattern: new Random(...).$M(...) - pattern: new java.util.Random(...).$M(...) - pattern: (Random $R).$M(...) + - pattern: (java.util.Random $R).$M(...) - pattern: Math.random() - pattern: java.lang.Math.random() - pattern: ThreadLocalRandom.current().$M(...) @@ -606,6 +630,7 @@ rules: - 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 @@ -656,13 +681,13 @@ rules: # 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](?:Iv|Pin|Otp|Key|Auth)(?:[A-Z0-9_]|$) + 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](?:Iv|Pin|Otp|Key|Auth)(?:[A-Z0-9_]|$) + 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`. @@ -890,7 +915,13 @@ rules: # is only a sanitiser when paired with a containment check. The # containment check is what actually makes the path safe. - patterns: - - pattern: $PATH.startsWith($BASE) + # 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 diff --git a/tests/fixtures/opengrep/java/HardcodedCredentials.java b/tests/fixtures/opengrep/java/HardcodedCredentials.java index 5499527..7fa1545 100644 --- a/tests/fixtures/opengrep/java/HardcodedCredentials.java +++ b/tests/fixtures/opengrep/java/HardcodedCredentials.java @@ -12,6 +12,13 @@ public class HardcodedCredentials { // 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"; diff --git a/tests/fixtures/opengrep/java/InsecureRandom.java b/tests/fixtures/opengrep/java/InsecureRandom.java index f7390a6..db8b116 100644 --- a/tests/fixtures/opengrep/java/InsecureRandom.java +++ b/tests/fixtures/opengrep/java/InsecureRandom.java @@ -49,6 +49,39 @@ void ordinary() { 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(); diff --git a/tests/fixtures/opengrep/java/LdapInjection.java b/tests/fixtures/opengrep/java/LdapInjection.java index 909609c..f4cc177 100644 --- a/tests/fixtures/opengrep/java/LdapInjection.java +++ b/tests/fixtures/opengrep/java/LdapInjection.java @@ -27,10 +27,12 @@ Object escaped(DirContext ctx, HttpServletRequest request) throws Exception { // 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.search.Query query, + 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(query, Integer.parseInt(text)); + return searcher.search(parser.parse(text), 10); } } diff --git a/tests/fixtures/opengrep/java/PathTraversal.java b/tests/fixtures/opengrep/java/PathTraversal.java index 0e1ce57..71c71b1 100644 --- a/tests/fixtures/opengrep/java/PathTraversal.java +++ b/tests/fixtures/opengrep/java/PathTraversal.java @@ -25,6 +25,7 @@ void zipSlip(File zipFile, File dir) throws Exception { ZipEntry e = zip.entries().nextElement(); // ruleid: java-path-traversal File f = new File(dir, e.getName()); + // ruleid: java-path-traversal Files.copy(zip.getInputStream(e), f.toPath()); } @@ -39,6 +40,17 @@ void contained(HttpServletRequest request) throws Exception { 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")); diff --git a/tests/fixtures/opengrep/java/UnsafeDeserialization.java b/tests/fixtures/opengrep/java/UnsafeDeserialization.java index a1554af..84fbeb2 100644 --- a/tests/fixtures/opengrep/java/UnsafeDeserialization.java +++ b/tests/fixtures/opengrep/java/UnsafeDeserialization.java @@ -2,6 +2,7 @@ import java.io.*; import org.yaml.snakeyaml.Yaml; import org.yaml.snakeyaml.constructor.SafeConstructor; +import org.yaml.snakeyaml.LoaderOptions; public class UnsafeDeserialization implements Serializable { Object fromStream(ObjectInputStream ois) throws Exception { @@ -31,6 +32,12 @@ Object yamlSafeVariable(String s) { return y.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); + } + // Implementing the Serializable contract, including the standard throws. private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException { // ok: java-unsafe-deserialization diff --git a/tests/test_java_opengrep_rules.py b/tests/test_java_opengrep_rules.py index 6b06c6e..77c32af 100644 --- a/tests/test_java_opengrep_rules.py +++ b/tests/test_java_opengrep_rules.py @@ -137,8 +137,26 @@ def test_every_annotated_rule_exists() -> None: assert not unknown, f"fixtures reference rules that do not exist: {unknown}" -def _group(entries): - grouped = defaultdict(list) - for name, rule, line in entries: - grouped[name].append((rule, line)) - return grouped +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 + ) From bdafbb14b86b8a23615367e243341d0dee6b841a Mon Sep 17 00:00:00 2001 From: David Larsen Date: Wed, 9 Sep 2026 09:25:24 -0400 Subject: [PATCH 4/7] docs(changelog): note the sql-injection and SnakeYAML 2.0 fixes These two entries were dropped from the previous commit when the editing script aborted partway through. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3cf7b05..af74941 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,11 +60,18 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). `monkey` or `author` as security-relevant names. (#112) - `java-unsafe-deserialization` no longer reports SnakeYAML loads that use `new Yaml(new SafeConstructor())`, which is the remediation the rule itself - recommends, and no longer matches unrelated `readObject()` APIs such as + recommends, including the SnakeYAML 2.0 `SafeConstructor(LoaderOptions)` + form, and no longer matches unrelated `readObject()` APIs such as BouncyCastle's `PEMParser`. (#112) - `java-ldap-injection` no longer reports a Lucene `IndexSearcher.search()` call as a CRITICAL LDAP injection. Sinks are type constrained to the LDAP APIs. (#112) +- `java-sql-injection` no longer reports `MessageDigest.update(input)` as SQL + injection. The untyped `$TEMPLATE.update(...)` sink matched any method named + `update`, which produced 93 CRITICAL findings on the OWASP Benchmark's hash + test cases. The crypto receivers (`MessageDigest`, `Mac`, `Cipher`, + `Signature`, `Checksum`) are now subtracted, and `queryForRowSet` and + `batchUpdate` were added as sinks. (#112) ## [3.1.0] - 2026-09-02 From c18c24d277413015d273223ef35bddb09acdb258 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Thu, 10 Sep 2026 01:01:21 -0400 Subject: [PATCH 5/7] test(rules): run the Java opengrep fixtures in CI and make the harness portable The regression tests added for the Java rules skipped in CI, because the python-tests workflow never installed opengrep, so the fixtures guarded nothing. The workflow now installs the release pinned by OPENGREP_VERSION in the Dockerfile and sets SOCKET_BASICS_REQUIRE_OPENGREP=1, so a missing engine fails the job instead of silently skipping the module. Rule and fixture paths are added to the workflow's path filters, since a java.yml-only change did not trigger it before. The harness scans a temporary copy of the fixtures and asserts that every fixture was scanned and that opengrep reported no errors. opengrep's default ignore list skips any path under tests/, and on 1.19.0 that applies even to explicitly listed files, so scanning in place returned zero files and every positive annotation failed as if the rules had regressed. Drop -a from the scan command. It is --autofix, inert today only because every fix: key in java.yml sits under metadata. Co-Authored-By: Claude Fable 5.1 --- .github/workflows/python-tests.yml | 20 ++++++++++++ tests/test_java_opengrep_rules.py | 49 ++++++++++++++++++++++-------- 2 files changed, 56 insertions(+), 13 deletions(-) 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/tests/test_java_opengrep_rules.py b/tests/test_java_opengrep_rules.py index 77c32af..bbdf195 100644 --- a/tests/test_java_opengrep_rules.py +++ b/tests/test_java_opengrep_rules.py @@ -13,6 +13,7 @@ from __future__ import annotations import json +import os import re import shutil import subprocess @@ -28,8 +29,18 @@ 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( - shutil.which("opengrep") is None, + not _HAVE_OPENGREP, reason="opengrep is not installed; Java rule regression tests skipped", ) @@ -57,25 +68,37 @@ def _expectations() -> tuple[set[tuple[str, str, int]], set[tuple[str, str, int] def _scan() -> set[tuple[str, str, int]]: - """Run opengrep over the fixtures and return {(file, rule, line)}.""" - with tempfile.NamedTemporaryFile(suffix=".json", delete=False) as handle: - out = Path(handle.name) - # Pass explicit file paths. opengrep's default ignore list skips any - # directory named tests/, so handing it FIXTURES scans nothing. - targets = [str(path) for path in sorted(FIXTURES.glob("*.java"))] - try: - subprocess.run( + """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", "-a", "--no-git-ignore", - "--config", str(RULES), "--output", str(out), *targets, + "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 "{}") - finally: - out.unlink(missing_ok=True) + + 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", []): From 11679dc11b18ea38d0be8884660e542e2585a8c2 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Thu, 10 Sep 2026 01:01:22 -0400 Subject: [PATCH 6/7] build(docker): bump Socket Python CLI to 2.8.0 in the heavy and app-tests images socketsecurity 2.8.0 (PyPI, 2026-09-09) is required by the heavy image. The app-tests image pins the same tool and is kept in step, as in the 2.7.0 bump. Co-Authored-By: Claude Fable 5.1 --- Dockerfile.heavy | 2 +- app_tests/Dockerfile | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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. From 36ac76ac08fb63ac34932d0afa92f163043e36ee Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Thu, 10 Sep 2026 01:03:42 -0400 Subject: [PATCH 7/7] fix(rules): address third-pass review findings on the Java rules java-sql-injection: only the SQL string argument is the sink, so a parameterized JdbcTemplate or PreparedStatement call such as update("... = ?", input) is no longer reported. prepareStatement(), prepareCall(), addBatch() and queryForMap() are added as sinks, which is what catches a concatenated query prepared once and run with a no-argument execute(). Inline MessageDigest/Mac/Cipher/Signature.getInstance(...).update() chains are subtracted alongside the typed receivers. OWASP Benchmark sqli 142 -> 155 true positives at 66.0% -> 66.8% precision. java-path-traversal: the File and Path constructors are propagators rather than sinks; filesystem operations (File.exists() and friends, the Files.* family) are the sinks. The canonical-path idiom (construct, canonicalize, check, open) was previously reported at the constructor before the check could run. normalize().startsWith(...) and getCanonicalPath().startsWith(...) now sanitize the checked variable by side effect. Benchmark pathtraver is unchanged at 92 TP / 72 FP: 107 of its 268 cases only ever call exists() on the File, and the new sinks cover them exactly. java-ldap-injection: the four-argument search(base, "literal", args, controls) form is the parameterized API and the remediation the fix text recommends, so it is excluded. java-unsafe-deserialization: loadAs() and loadAll() are sinks. The rule is split into bound branches: a metavariable the positive pattern does not bind is free inside pattern-not-inside, so one SafeConstructor Yaml field excluded every readObject() finding in the same class. The new fixture caught it. java-hardcoded-credentials: a capitalised restatement of the keyword such as "Password" is a UI label, not a secret. Fixtures cover each case. The doc is re-measured on opengrep 1.26.0, the release the images pin: Benchmark recall 70.3% -> 71.3% and score 48.2 -> 48.9 at unchanged precision; mature-corpus findings 84 -> 83; WebGoat 45 -> 43, with the Zip Slip, default-credential and weak-PRNG lessons still reported. The CHANGELOG entry is condensed to fit the 3.2.0 bundle. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 87 ++---- docs/java-sast-benchmark.md | 85 ++++-- socket_basics/rules/java.yml | 262 +++++++++++------- .../opengrep/java/HardcodedCredentials.java | 3 + .../fixtures/opengrep/java/LdapInjection.java | 8 + .../fixtures/opengrep/java/PathTraversal.java | 48 +++- .../fixtures/opengrep/java/SqlInjection.java | 60 ++++ .../opengrep/java/UnsafeDeserialization.java | 25 ++ 8 files changed, 393 insertions(+), 185 deletions(-) create mode 100644 tests/fixtures/opengrep/java/SqlInjection.java diff --git a/CHANGELOG.md b/CHANGELOG.md index af74941..e015c20 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,69 +9,34 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] ### Added -- Two new Java SAST rules, both taint mode: `java-xss` (CWE-79) and - `java-xpath-injection` (CWE-643). XSS was the largest recall gap in the Java - rule set, accounting for 246 missed true positives on the OWASP Benchmark - corpus. (#112) -- `scripts/score_owasp_benchmark.py` scores an OpenGrep JSON run against the - OWASP Benchmark v1.2 `expectedresults` CSV, reporting per-category precision, - recall, false positive rate and the Benchmark score, plus per-rule TP/FP - counts. (#112) -- `docs/java-sast-benchmark.md` documents the benchmarking method, the - before/after numbers, the known limits of OWASP Benchmark for pattern-based - engines, and the remaining noise sources in the Java rules. (#112) -- Java rule regression fixtures under `tests/fixtures/opengrep/java` with - `// ruleid:` and `// ok:` annotations, exercised by - `tests/test_java_opengrep_rules.py`. The tests skip when `opengrep` is not on - `PATH`, so they are a no-op for contributors who only touch Python. (#112) +- 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) ### Fixed -- **Java SAST precision.** Twelve Java rules were rewritten after a customer - evaluation reported roughly 90% false positives. On six mature open source - Java projects (~17,400 files) the rule set previously emitted 1,631 findings, - of which a hand adjudicated random sample of 40 contained no true positives. - `java-empty-catch-block`, `java-reflection-injection`, - `java-system-out-usage` and `java-hardcoded-credentials` produced most of that - volume and now report nothing on those projects. `java-reflection-injection`, - `java-insecure-random`, `java-path-traversal` and `java-ldap-injection` were - converted to taint mode. (#112) -- **Java SAST recall.** Two systematic defects suppressed whole categories. - Patterns written with simple type names never matched fully qualified call - sites, so `java.security.MessageDigest.getInstance("MD5")`, - `new java.util.Random()` and `new javax.servlet.http.Cookie(...)` were - invisible; qualified variants were added throughout. Crypto rules matched - exact algorithm literals, so `Cipher.getInstance("DES/CBC/PKCS5Padding")` - never matched a rule looking for `"DES"`; these now use `metavariable-regex` - over the transformation string and cover the provider overloads of - `getInstance`. (#112) -- `java-insecure-cookie` no longer drops an unhardened cookie when a - neighbouring cookie in the same method calls `setSecure(true)`. The exclusion - is bound per variable and also recognises hardening through a field. (#112) -- `java-weak-cipher` no longer reports `RSA/ECB/PKCS1Padding`, where `ECB` is a - JCA placeholder rather than a block mode. `AES/ECB/...` is still reported. - (#112) -- `java-hardcoded-ip` now requires a full dotted quad, so version strings such - as `"10.0"` and `"10.2.3"` are no longer reported and `"10.0.0.1"` is. (#112) -- `java-path-traversal` honours a `startsWith` containment check at later file - sinks, and detects Zip Slip via `ZipEntry.getName()` and Spring multipart - uploads via `MultipartFile.getOriginalFilename()`. (#112) -- `java-insecure-random` detects weak key and IV material generated through - `Random.nextBytes(array)`, and no longer treats `pivot`, `divisor`, `spinner`, - `monkey` or `author` as security-relevant names. (#112) -- `java-unsafe-deserialization` no longer reports SnakeYAML loads that use - `new Yaml(new SafeConstructor())`, which is the remediation the rule itself - recommends, including the SnakeYAML 2.0 `SafeConstructor(LoaderOptions)` - form, and no longer matches unrelated `readObject()` APIs such as - BouncyCastle's `PEMParser`. (#112) -- `java-ldap-injection` no longer reports a Lucene `IndexSearcher.search()` call - as a CRITICAL LDAP injection. Sinks are type constrained to the LDAP APIs. - (#112) -- `java-sql-injection` no longer reports `MessageDigest.update(input)` as SQL - injection. The untyped `$TEMPLATE.update(...)` sink matched any method named - `update`, which produced 93 CRITICAL findings on the OWASP Benchmark's hash - test cases. The crypto receivers (`MessageDigest`, `Mac`, `Cipher`, - `Signature`, `Checksum`) are now subtracted, and `queryForRowSet` and - `batchUpdate` were added as sinks. (#112) +- **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/docs/java-sast-benchmark.md b/docs/java-sast-benchmark.md index 0745de9..b3f5981 100644 --- a/docs/java-sast-benchmark.md +++ b/docs/java-sast-benchmark.md @@ -36,10 +36,10 @@ Alert volume there is the number that maps to triage burden. Pin both the engine and the corpus, or the numbers below will not reproduce. -- **Engine.** Measured with **opengrep 1.25.0**. The images pin - `OPENGREP_VERSION=v1.26.0` 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. +- **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 @@ -49,7 +49,7 @@ Pin both the engine and the corpus, or the numbers below will not reproduce. git clone https://github.com/OWASP-Benchmark/BenchmarkJava.git git -C BenchmarkJava checkout 51f0a7cf8bb9d17ce1f6d72598c1d1c6ce90f661 -opengrep --json --dataflow-traces --quiet -a --no-git-ignore \ +opengrep --json --dataflow-traces --quiet --no-git-ignore \ --config socket_basics/rules/java.yml \ --output results.json \ BenchmarkJava/src/main/java @@ -74,11 +74,14 @@ scans them and diffs against the annotations: pytest tests/test_java_opengrep_rules.py ``` -The tests skip when `opengrep` is not on `PATH`. 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. +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. -Two behaviours worth knowing when editing these: +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 `.*`. @@ -86,12 +89,15 @@ Two behaviours worth knowing when editing these: 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 directory named `tests/`, so the test - harness passes explicit file paths rather than the fixture directory. +- 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.25.0. +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 @@ -110,10 +116,10 @@ column from a scorer that still mapped the non-existent | | Before | After | |---|---|---| | Precision | 64.5% | **76.7%** | -| Recall | 13.2% | **70.3%** | -| False positive rate | 7.6% | 22.2% | -| Benchmark score (TPR - FPR) | 5.6 | **48.2** | -| True positives found | 176 | **937** | +| 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: @@ -127,7 +133,7 @@ Per category, after the change: | ldapi | 58.3% | 77.8% | | xss | 67.4% | 70.7% | | pathtraver | 56.1% | 69.2% | -| sqli | 66.0% | 52.2% | +| sqli | 66.8% | 57.0% | | cmdi | 63.6% | 44.4% | **Cross-category findings are not counted.** Following the OWASP method, the @@ -141,7 +147,15 @@ 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.0% rather than 76.7%. +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 @@ -151,8 +165,8 @@ emitted findings that are real, is the comparable number and it improved. | | Before | After | Change | |---|---|---|---| -| Total findings | 1,631 | 129 | **-92%** | -| Unique findings, mature libraries only | 1,536 | 84 | **-94.5%** | +| 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: @@ -175,12 +189,15 @@ now emit zero findings on the mature-library corpus. ### WebGoat (deliberately vulnerable) -87 findings before, 45 after. The removed findings were lint noise -(`java-system-out-usage` 26, `java-hardcoded-ip` 5, `java-empty-catch-block` 4) -plus three `java-reflection-injection` matches on factory `newInstance()` calls -and a JDK dynamic proxy. The security findings, including the Zip Slip in -`ProfileZipSlip`, the default credentials in `DefaultCredentialsTask`, and the -weak PRNG in `PasswordResetLink`, are still reported. +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 @@ -234,6 +251,16 @@ 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) @@ -250,15 +277,15 @@ 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 -were untouched by this change and are still the largest remaining sources: +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` | 14 | Library serialization helpers that accept a caller-supplied `ObjectInputStream` | +| `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` | 4 | `$STMT.execute(...)` and `$TEMPLATE.query(...)` sinks match any method of those names | +| `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 diff --git a/socket_basics/rules/java.yml b/socket_basics/rules/java.yml index 26d02db..184f4bc 100644 --- a/socket_basics/rules/java.yml +++ b/socket_basics/rules/java.yml @@ -129,31 +129,38 @@ 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 - # 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. Instead - # keep the broad receiver and subtract the collisions, which are all - # update() on a crypto primitive rather than a query. + # 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: - - pattern: $TEMPLATE.query(...) - - pattern: $TEMPLATE.queryForObject(...) - - pattern: $TEMPLATE.queryForList(...) - - pattern: $TEMPLATE.queryForRowSet(...) - - pattern: $TEMPLATE.execute(...) - - pattern: $TEMPLATE.update(...) - - pattern: $TEMPLATE.batchUpdate(...) + # 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(...) @@ -164,6 +171,17 @@ rules: - 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(...) @@ -205,39 +223,55 @@ rules: - "*Tests.java" - "*TestCase.java" - "*Benchmark.java" - patterns: - - pattern-either: - # 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. - - pattern: (ObjectInputStream $OIS).readObject() - - pattern: new ObjectInputStream(...).readObject() - - pattern: (XMLDecoder $D).readObject() - - pattern: new XMLDecoder(...).readObject() - # SnakeYAML without a SafeConstructor deserializes arbitrary types - - pattern: new Yaml().load(...) - - pattern: (Yaml $Y).load(...) - # 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 { ... } - # 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(...) - - pattern-not-inside: | - $T $Y = new Yaml(new SafeConstructor(...), ...); - ... - - pattern-not-inside: | - $T $Y = new Yaml(new org.yaml.snakeyaml.constructor.SafeConstructor(...), ...); - ... + pattern-either: + # 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 @@ -421,20 +455,27 @@ rules: from: $X to: org.apache.commons.codec.binary.Base64.encodeBase64 pattern-sinks: - - 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(...) + - 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: (LdapTemplate $T).search(...) - pattern: (LdapQueryBuilder $Q).filter(...) pattern-sanitizers: - pattern: LdapEncoder.filterEncode(...) @@ -520,12 +561,12 @@ rules: regex: ^(?!(?i:bearer|basic|digest|oauth|negotiate)$).*$ - metavariable-regex: metavariable: $VALUE - # A lowercase value that just restates the credential keyword is - # naming a field or an auth method ("password", "accesskey", - # "stompCredentials", "access_token", "j_password"), not holding - # one. Real default credentials such as "admin" or "webgoat" do - # not restate the keyword. - regex: ^(?!(?=[a-z])(?!.*[0-9])(?i:[a-z_]*(password|passwd|credential|secret|token|apikey|accesskey|auth)[a-z_]*)$).*$ + # 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, "...") @@ -880,8 +921,11 @@ rules: # 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. - - pattern: new File(...) - - pattern: new java.io.File(...) + # 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(...) @@ -892,24 +936,31 @@ rules: - pattern: new java.io.FileWriter(...) - pattern: new RandomAccessFile(...) - pattern: new java.io.RandomAccessFile(...) - - pattern: Files.newInputStream(...) - - pattern: Files.newOutputStream(...) - - pattern: Files.newBufferedReader(...) - - pattern: Files.newBufferedWriter(...) - - pattern: Files.readAllBytes(...) - - pattern: Files.readAllLines(...) - - pattern: Files.write(...) - - pattern: Files.copy(...) - - pattern: Files.move(...) - - pattern: Files.delete(...) - - pattern: Files.deleteIfExists(...) - - pattern: java.nio.file.Files.readAllBytes(...) - - pattern: java.nio.file.Files.write(...) - - pattern: java.nio.file.Files.copy(...) - - pattern: java.nio.file.Files.newInputStream(...) - - pattern: java.nio.file.Files.newOutputStream(...) - - pattern: Paths.get(...) - - pattern: java.nio.file.Paths.get(...) + # 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: # normalize() alone collapses ../ but does not confine the result, so it # is only a sanitiser when paired with a containment check. The @@ -927,6 +978,31 @@ rules: # 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(...) diff --git a/tests/fixtures/opengrep/java/HardcodedCredentials.java b/tests/fixtures/opengrep/java/HardcodedCredentials.java index 7fa1545..559be7b 100644 --- a/tests/fixtures/opengrep/java/HardcodedCredentials.java +++ b/tests/fixtures/opengrep/java/HardcodedCredentials.java @@ -37,6 +37,9 @@ public class HardcodedCredentials { // 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 diff --git a/tests/fixtures/opengrep/java/LdapInjection.java b/tests/fixtures/opengrep/java/LdapInjection.java index f4cc177..1fdaa6a 100644 --- a/tests/fixtures/opengrep/java/LdapInjection.java +++ b/tests/fixtures/opengrep/java/LdapInjection.java @@ -35,4 +35,12 @@ Object luceneSearch(org.apache.lucene.search.IndexSearcher searcher, // 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 index 71c71b1..02878e9 100644 --- a/tests/fixtures/opengrep/java/PathTraversal.java +++ b/tests/fixtures/opengrep/java/PathTraversal.java @@ -6,6 +6,7 @@ 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"); @@ -19,16 +20,24 @@ void qualifiedSink(HttpServletRequest request) throws Exception { new java.io.FileInputStream("/var/data/" + name); } - // Zip Slip: the archive entry name is attacker controlled. + // 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(); - // ruleid: java-path-traversal + // 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"); @@ -40,6 +49,41 @@ void contained(HttpServletRequest request) throws Exception { 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 { 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 index 84fbeb2..1413d53 100644 --- a/tests/fixtures/opengrep/java/UnsafeDeserialization.java +++ b/tests/fixtures/opengrep/java/UnsafeDeserialization.java @@ -5,6 +5,10 @@ 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(); @@ -20,6 +24,17 @@ Object yamlUnsafe(String s) { 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 @@ -32,12 +47,22 @@ Object yamlSafeVariable(String s) { 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