fix(rules): improve precision and recall of java opengrep rules

[David Larsen (dc-larsen)]

Why

A customer SAST evaluation reported roughly 90% false positives from our Java rules and compared them unfavourably to CodeQL. Their engineer's read was that Semgrep matches patterns in single files while CodeQL traces paths across files. Part of that is structural, but most of what they saw was fixable rule defects.

I reproduced it. On six mature, heavily reviewed open source Java projects (guava, netty, spring-framework, commons-lang, commons-io, spring-petclinic — about 17,400 Java files) the current rules emit 1,631 findings. I hand adjudicated a random sample of 40 of them: zero true positives. Three rules produced 74% of the volume.

This is the Java equivalent of the .NET work in #63, using the same method.

What was wrong

Noise. java-empty-catch-block (645 findings) fired on catch (NullPointerException tolerated) {} and on every catch block documented with a comment. java-reflection-injection (296) matched every method.invoke(), every newInstance() factory call, and Class.forName("sun.misc.Cleaner") on a string constant. java-system-out-usage (263) claimed "sensitive information written to log files" but matched any println. java-hardcoded-credentials matched on variable name alone — the exact defect fixed for .NET in #63 — flagging KEY_ATTRIBUTE = "key" and SEC_WEBSOCKET_KEY1 = "Sec-WebSocket-Key1".

Two systematic bugs silently suppressed whole categories.

Patterns written with simple type names never matched fully qualified call sites:

MessageDigest.getInstance("MD5")                  // matched
java.security.MessageDigest.getInstance("MD5")    // did NOT match
new Random()                                       // matched
new java.util.Random()                             // did NOT match

And the crypto rules matched exact algorithm literals, so Cipher.getInstance("DES/CBC/PKCS5Padding") never matched a rule looking for "DES". Together these meant weakrand, hash, crypto and securecookie scored zero recall on the benchmark despite having rules for them.

Results

Benchmarked with opengrep 1.25.0 against OWASP Benchmark v1.2 (2,740 annotated servlets, 1,415 real vulnerabilities and 1,325 deliberate non-vulnerabilities, with published ground truth).

	Before	After
Precision	64.5%	76.5%
Recall	12.4%	63.4%
Benchmark score (TPR - FPR)	5.1	42.6
True positives found	176	897

securecookie, weakrand, crypto and hash now run at 100% precision (recall 100%, 91.7%, 74.6%, 69.0%).

On the mature open source corpus, which is the honest proxy for what a customer has to triage:

	Before	After	Change
Total findings	1,631	130	-92%
Unique findings, mature libraries only	1,536	85	-94.5%

java-empty-catch-block, java-reflection-injection, java-system-out-usage and java-hardcoded-credentials now emit zero findings across all six mature libraries.

WebGoat goes 87 → 45. I checked every removed finding: they are lint noise plus three reflection matches on factory calls and a JDK dynamic proxy. The planted vulnerabilities still fire, including the Zip Slip in ProfileZipSlip, the default credentials in DefaultCredentialsTask, and the weak PRNG in PasswordResetLink.

Changes

Precision: java-empty-catch-block, java-reflection-injection (→ taint), java-system-out-usage, java-hardcoded-credentials, java-unsafe-deserialization, java-insecure-random (→ taint), java-hardcoded-ip, java-insecure-cookie.

Recall: qualified-name variants throughout, metavariable-regex over crypto transformation strings, provider overloads of getInstance, and java-ldap-injection / java-path-traversal converted to taint with Zip Slip and Spring multipart sources.

New rules: java-xss and java-xpath-injection, both taint mode. XSS was the single largest recall gap at 246 missed real vulnerabilities.

Two changes worth calling out because they are subtle:

• java-insecure-cookie's setSecure(true) exclusion is now bound to the same metavariable. Previously any setSecure(true) in scope exonerated every other cookie in the method — this is the same class of bug as the StartsWith sanitizer fix in fix(rules): improve precision of 4 high-FP dotnet opengrep rules #63.
• java-empty-catch-block excludes commented blocks with pattern-not-regex. Comments are not AST nodes, so a documented catch block still looks empty to the matcher.

Reproducing

docs/java-sast-benchmark.md has the method, the per-category numbers, and scripts/score_owasp_benchmark.py scores an opengrep JSON run against the benchmark CSV.

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

Two things I want to be honest about

A chunk of OWASP Benchmark's designated false positives are not fixable by any pattern engine. They are unreachable-branch traps:

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
}

Solving that needs constant propagation plus path sensitivity. opengrep's taint analysis is path insensitive, so it reports the dead branch. This caps achievable precision on sqli, cmdi and pathtraver no matter how the rules are written, and it is the real substance behind the CodeQL comparison. Please don't read the residual FPs in those categories as rule defects without opening the test case.

I deliberately left five rules alone, and they are now the largest remaining noise sources on real code. Documented in the doc, listed here so they don't get lost: java-template-injection (20 findings, matches any .process(...)), java-xxe-vulnerability (14, matches DocumentBuilderFactory.newInstance() without checking whether secure features are set), java-unsafe-deserialization (14 residual, library serialization helpers), java-jndi-injection (8, matches any .lookup(...)), and java-sql-injection (4, $STMT.execute(...) matches any method named execute). There is also no trustbound rule at all, which is 126 unscored benchmark cases.

Testing

opengrep --validate clean at 32 rules. Full pytest suite: 339 passed.

---

Note

Medium Risk
Large changes to customer-facing SAST behavior can miss real issues or change alert volume sharply; changes are rule/config only with documented benchmarks, not runtime security code.

Overview
Overhauls socket_basics/rules/java.yml to cut false positives on mature OSS (~92% fewer findings) while raising OWASP Benchmark recall (12.4% → 63.4%) and precision (64.5% → 76.5%).

Precision: Noisy lint-style rules are tightened—java-empty-catch-block, java-system-out-usage, and java-hardcoded-credentials use stricter metavariable/regex filters and test-path excludes; java-reflection-injection moves to taint (drops blanket invoke/newInstance/Runtime.exec); java-insecure-random only flags weak PRNG output flowing into security-sensitive sinks; deserialization, cookies, and hardcoded-IP rules get narrower patterns and exclusions.

Recall: Crypto/hash/random/cookie rules gain fully qualified call sites and metavariable-regex on algorithm/transformation strings; LDAP and path traversal become taint rules with servlet/Spring sources, Zip Slip upload sources, and expanded java.io/Files sinks; new java-xss and java-xpath-injection taint rules fill benchmark gaps.

Tooling: Adds docs/java-sast-benchmark.md (corpora, commands, before/after metrics) and scripts/score_owasp_benchmark.py to score opengrep JSON against OWASP Benchmark v1.2 with CWE-matched category stats and per-rule TP/FP.

^{Reviewed by Cursor Bugbot for commit 667bf9c. Configure here.}

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 7 potential issues.

^{Bugbot Autofix is ON, but it could not run because the branch was deleted or merged before autofix could start.}

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 667bf9c. Configure here.}

[cursor]

Weak cipher flags standard RSA

High Severity

The second alternative of the $TRANSFORM regex matches any literal containing /ecb/, so ordinary RSA transformations such as RSA/ECB/PKCS1Padding are reported as broken ECB. In Java those strings use ECB only as a placeholder, not the block-cipher mode the finding describes.

 

^{Reviewed by Cursor Bugbot for commit 667bf9c. Configure here.}

[cursor]

Cookie exclusion spans other constructors

High Severity

pattern-not-inside still matches a region from one cookie assignment through that cookie's setSecure(true). A second new Cookie sitting between those statements is inside the region, so an unsecured cookie is dropped whenever another cookie in the same method is hardened.

 

^{Reviewed by Cursor Bugbot for commit 667bf9c. Configure here.}

[cursor]

Private IP regex misses 10.x

High Severity

The 10 branch only allows two more octets before the closing quote, so "10.0.0.1" does not match while three-part versions such as "10.2.3" do. The 192.168 and 172.16–31 branches correctly require a dotted quad.

 

^{Reviewed by Cursor Bugbot for commit 667bf9c. Configure here.}

[cursor]

Weak random misses nextBytes keys

High Severity

Random.nextBytes is void and fills the caller-supplied array in place, but these sources taint only the call expression. That taint never reaches SecretKeySpec or IvParameterSpec, so generating keys or IVs with a weak PRNG is missed.

Additional Locations (1)

• socket_basics/rules/java.yml#L634-L637

 

^{Reviewed by Cursor Bugbot for commit 667bf9c. Configure here.}

[cursor]

SecureRandom treated as weak source

Medium Severity

(Random $R).$M(...) matches any method on a Random-typed receiver. SecureRandom is commonly stored in a Random field, so correct token or key generation is still reported when the class or variable name looks security-related.

Additional Locations (1)

• socket_basics/rules/java.yml#L622-L630

 

^{Reviewed by Cursor Bugbot for commit 667bf9c. Configure here.}

[cursor]

Serializable exclusion misses standard throws

Medium Severity

The readObject and readExternal exclusions only allow zero or one thrown type. The standard Serializable signature throws IOException, ClassNotFoundException, so legitimate readObject() calls inside those methods still match.

 

^{Reviewed by Cursor Bugbot for commit 667bf9c. Configure here.}

[cursor]

Path sanitizer does not persist

Medium Severity

The startsWith sanitizer focuses $PATH but omits by-side-effect: true, so later file sinks that use the same path stay tainted. A containment check therefore does not suppress the finding the way the nearby comment describes.

 

^{Reviewed by Cursor Bugbot for commit 667bf9c. Configure here.}

[lelia]

David Larsen (@dc-larsen) Thanks for this. The direction and the measurement discipline are exactly right, and the benchmark doc is a real asset. I reproduced the rule behavior with small Java fixtures on opengrep 1.19.0 (the image ships 1.26.0; behavior matched everywhere I checked) rather than reading the patterns alone, and that turned up a handful of things that should land before merge. All of them are localized regex or pattern edits inside java.yml; nothing touches Python code, and the test suite passes on this branch.

Bugbot cross-check. Six of its seven findings reproduce: RSA/ECB flagged as weak, a hardened cookie hiding a neighbouring unhardened one, the 10.x address regex, nextBytes taint never reaching key specs, SecureRandom in a Random-typed variable, and the startsWith sanitizer not persisting. The readObject throws-clause finding does not reproduce and can be dismissed. Each confirmed one has a tested fix in the inline comments.

Additional defects Bugbot missed (all reproduced, details inline):

• Unanchored short tokens in the weak-random sink regex flag pivot, divisor, spinner and monkey as security values.
• The credential rule still reports "Bearer", "X-CSRF-TOKEN", "access_token" and "j_password", while missing an sk-live-... shaped key.
• A SafeConstructor SnakeYAML load is reported even though the rule's fix text recommends it.
• The untyped .search() LDAP sink turns a Lucene search into a CRITICAL finding.
• String.valueOf is treated as a reflection sanitizer.

Nits: scorer usage guard and exec bit, a mapping for a rule that doesn't exist, and pinning the engine version and BenchmarkJava commit in the doc. I'd also suggest checking the fixtures in as a lightweight regression test; happy to hand mine over.

Release note: this is slated to ride along in 3.2.0 with #110 and #111 once the above is in. The numbers quoted in the doc will want a re-run after the regex changes since a couple of them (weak-random substrings, the cookie region) will move the mature-corpus and OWASP counts.

Review verification and write-up prepared with Claude Code.

[lelia]

Confirmed (opengrep 1.19.0): Bugbot's RSA/ECB finding reproduces. Cipher.getInstance("RSA/ECB/PKCS1Padding") and "RSA/ECB/OAEPWithSHA-256AndMGF1Padding" are both reported as weak ciphers. In JCA the ECB token in an RSA transformation is a placeholder, not a block mode, and RSA/ECB/... is the standard spelling. Suggested regex:

(?i)^(des|desede|tripledes|3des|rc2|rc4|arcfour|blowfish)(/.*)?$|^(?!rsa/)[^/]+/ecb/.*$

AES/ECB/PKCS5Padding and DES/CBC/PKCS5Padding still match with that change.

[lelia]

Confirmed: Bugbot's region finding reproduces. With

Cookie a = new Cookie("a", "1");
Cookie b = new Cookie("b", "2");
a.setSecure(true);
resp.addCookie(b);

cookie b is not reported, because its constructor sits inside the a ... a.setSecure(true) region. The fix that keeps the per-variable binding is to make the positive pattern a declaration too, so $COOKIE has to unify between the match and the exclusion:

- patterns:
    - pattern: $T $COOKIE = new Cookie($N, $V);
    - pattern-not-inside: |
        $T $COOKIE = new Cookie(...);
        ...
        $COOKIE.setSecure(true);

(same for the qualified spellings), plus a second pattern-either branch for a bare new Cookie(...) expression that is never assigned, e.g. resp.addCookie(new Cookie(...)).

Related: a cookie hardened through a field is still reported: this.cookie = new Cookie(...); this.cookie.setSecure(true); resp.addCookie(this.cookie). An assignment form of the exclusion ($COOKIE = new Cookie(...); ... $COOKIE.setSecure(true);) covers it.

[lelia]

Confirmed: Bugbot's 10.x finding reproduces. "10.0.0.1" and "10.0.0.1:8080" are not reported, while "10.2.3" is. The 10 branch only allows two more octets. Suggested 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})?"

The 192.168 and 172.16-31 branches behave correctly as written.

[lelia]

Confirmed: two of Bugbot's findings reproduce here.

1. nextBytes: byte[] key = new byte[16]; new Random().nextBytes(key); new SecretKeySpec(key, "AES") produces no finding, and the same for IvParameterSpec. nextBytes is void, so tainting the call expression never reaches the array. Add a by-side-effect source that focuses the argument:

   - 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

2. SecureRandom in a Random-typed variable (Random rng = new SecureRandom(); String token = Long.toString(rng.nextLong(), 36)) is reported. The declared type is what the typed metavariable sees, so there is no clean pattern-level fix. I'd accept this as a known limit and add it to the doc's Known limits section rather than weaken the source.

[lelia]

Confirmed: Bugbot's sanitizer finding reproduces. With

Path p = BASE.resolve(name).normalize();
if (!p.startsWith(BASE)) throw new IOException();
return Files.newInputStream(p);

the sink is still reported. Without by-side-effect: true the sanitizer only cleans the startsWith(...) expression itself, not the variable at later uses, so the containment check the comment describes never suppresses anything. Add by-side-effect: true under this patterns block. It will also suppress a startsWith whose result is ignored, which is an acceptable trade for a HIGH rule.

[lelia]

Nit (recall): the textual exclusion is range-scoped to the whole try statement, so a comment in a sibling catch clause suppresses an empty broad catch next to it:

try { work(); }
catch (IOException e) { // fine
} catch (Exception e) { }   // not reported

Without the comment on the IOException clause the same code is reported. Low priority; anchoring the regex to the catch that binds $EXCEPTION isn't expressible here, so this may just be worth a line in Known limits.

[lelia]

Nit: running without arguments raises IndexError instead of printing usage. A two-line guard (if len(sys.argv) != 3: print usage; sys.exit(2)) would match the other scripts. Also, the file has a shebang but is checked in as 100644, unlike prep_release.py (100755); either drop the shebang or chmod +x so both python3 scripts/... and ./scripts/... work.

[lelia]

Nit: java-trust-boundary-violation is mapped here but no such rule exists in java.yml, and the doc says trustbound has no rule. Harmless today, but it makes trustbound look scored at 0% recall in the per-category table rather than absent. Suggest remove the mapping, or at minimum, include a comment that it's a placeholder for a future rule.

[lelia]

Reproducibility: two things to pin so the next person gets the same numbers.

1. Engine version. The Dockerfile ships OPENGREP_VERSION=v1.26.0 and this was measured on 1.25.0; note the shipped pin here or re-measure on it. (FWIW every behavior I checked was identical on 1.19.0.)
2. BenchmarkJava checkout. git clone --depth 1 of main moves; record the commit or tag (v1.2) used, since the expectedresults CSV and test cases have changed between benchmark releases.

[lelia]

Suggestion: the rules have no regression coverage in tests/. Everything verified in this review came from a dozen small Java fixture files (one positive and one negative case per rule) scanned with a local opengrep binary. Checking those in under tests/fixtures/opengrep/java/ with a pytest that skips when opengrep is not on PATH would catch the regex regressions found here (RSA/ECB, 10.x, substring iv/pin/key) the next time someone edits java.yml, without needing the OWASP corpus.