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.}