fix(rules): improve precision of 4 high-FP dotnet opengrep rules - #63
Conversation
Addresses customer SAST evaluation feedback where 4 rules produced 150/170 false positives (88% of all FPs), inflating the reported FP rate to 91%. Rules fixed: - dotnet-xss-response-write: Convert to taint mode. Previously matched any .Write() call including Serilog ITextFormatter log sinks. Now requires data flow from user input sources to Response.Write sinks. - dotnet-hardcoded-credentials: Add value inspection and credential API patterns. Previously matched on variable names alone, flagging config key paths like "UseCaptchaOnResetPassword". - dotnet-crypto-failures: Target actual weak algorithms (3DES, DES, RC2, RijndaelManaged) instead of Encoding.UTF8.GetBytes() which flagged the recommended SHA256.HashData(Encoding.UTF8.GetBytes(...)) pattern. - dotnet-path-traversal: Convert to taint mode. Previously matched all Path.Combine() calls including those using framework-provided paths like _env.WebRootPath. Validated with opengrep v1.19.0 against NIST Juliet C# test suite: xss-response-write: Prec 41.6% -> 100%, Recall 47.8% -> 24.3% hardcoded-credentials: Prec 0.0% -> 100%, Recall 0.0% -> 3.6% crypto-failures: Prec 36.7% -> 100%, Recall 51.4% -> 50.0% path-traversal: Prec 0.0% -> 100%, Recall 0.0% -> 45.2%
4958b6f to
cdb7224
Compare
E2E Validation: dotnet rule precision (round 1)Validated the updated rules against two intentionally vulnerable .NET repositories using opengrep v1.19.0 and the full socket-basics pipeline. Test Targets
Resultsthe-most-vulnerable-dotnet-app
AspGoat
Pipeline Integration
ObservationThe taint-mode rules correctly eliminate pattern-match false positives. Initial taint sources target classic ASP.NET WebForms ( |
E2E Validation: ASP.NET Core coverage added (round 2)Extended taint sources and sinks to cover ASP.NET Core patterns, then re-validated against both test repos. ChangesSources (both
Sinks (
Sinks (
Resultsthe-most-vulnerable-dotnet-app
AspGoat
Validation
|
…net rules Add controller parameter binding sources ([FromQuery], [FromBody], [FromRoute], [FromForm]) and IFormFile.FileName to path-traversal and XSS taint rules. Add Response.WriteAsync and Html.Raw as XSS sinks. Add fully-qualified System.IO.File.* sink variants for ASP.NET Core code that uses explicit namespace qualification. E2E tested against two vulnerable .NET repos: 7 true positives found, zero false positives.
There was a problem hiding this comment.
David Larsen (@dc-larsen) Thanks for putting this together! Reducing 150 false positives from 4 rules is meaningful, and the move toward taint-style matching is directionally the right fix for the worst offenders here. I also validated the updated dotnet.yml on my end with opengrep --validate, and the config is syntactically valid.
I do see a few semantic issues worth tightening before merge - summarized here, but see my review comments for more details:
dotnet-path-traversal:Path.GetFullPath(...)is currently treated as a sanitizer atsocket_basics/rules/dotnet.yml:410dotnet-hardcoded-credentials: the variable-name regex got much narrower atsocket_basics/rules/dotnet.yml:161dotnet-crypto-failures: one broad pattern still looks FP-prone atsocket_basics/rules/dotnet.yml:805
Overall, I think the main idea in this PR is solid: move away from simple pattern matching where it was clearly overfiring, and add more context-aware logic. I’d just recommend tightening the three cases above so we don’t trade one class of false positives for a quieter set of false negatives or misclassifications.
- Remove Path.GetFullPath() as path-traversal sanitizer (normalizes but does not prevent traversal on its own) - Broaden hardcoded-credentials variable regex to cover idiomatic C# naming: apiKey, connectionString, privateKey, accessKey, authToken - Remove overly broad Base64 encoding pattern from crypto-failures (benign encoding/transport use generates noise)
|
David Larsen (@dc-larsen) Just one remaining tweak and we should be good to go 👍 |
The $X.StartsWith($BASE) sanitizer was matching the boolean expression
instead of marking the checked path variable as sanitized, so correctly
validated paths were still flagged as tainted.
Use focus-metavariable + by-side-effect so the sanitizer applies to $X
itself. Verified with a synthetic test case: scans of an unsanitized
File.ReadAllText still fire, but the same call guarded by
full.StartsWith("/var/data/") no longer does. Juliet CWE-23/36 results
unchanged at 432 findings (Juliet test cases do not exercise StartsWith
validation). opengrep --validate and pytest pass.
* 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. * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * fix(rules): match the fully qualified DigestUtils.sha1Hex spelling java-weak-crypto-sha1 listed DigestUtils.sha1Hex and the fully qualified sha1, but not org.apache.commons.codec.digest.DigestUtils.sha1Hex, so that spelling was missed while the MD5 rule covered all four. Adds a weak-hash fixture covering every spelling of both rules. OWASP Benchmark is unchanged. Of the four findings in this Bugbot round only this one reproduces. The other three (toLowerCase/replace in java-xss, StringBuilder.toString in java-reflection-injection, Long.toHexString in java-insecure-random) are reported on the current rules: opengrep propagates taint through any method call on a tainted receiver and any call with a tainted argument by default, so the explicit String propagators are belt-and-braces. The benchmark doc now says so, to save the next review round the same detour. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: lelia <2418071+lelia@users.noreply.github.com>
Summary
Fixes 4 dotnet opengrep rules that produced 150 of 170 total false positives (88%) in a customer SAST evaluation, inflating the reported FP rate to 91%.
.Write()including SerilogITextFormatterlog sinks (74 FPs). Now tracks data flow from user input toResponse.Write.UseCaptchaOnResetPassword(31 FPs).Encoding.UTF8.GetBytes()which triggers on the recommendedSHA256.HashData()pattern (30 FPs).Path.Combine()calls including framework paths like_env.WebRootPath(15 FPs).Benchmark data (NIST Juliet C# Test Suite)
All 4 rules achieve 100% precision (zero false positives) post-fix. Recall trade-offs are acceptable: taint-mode rules only fire when user input actually reaches the sink, which is the correct behavior for security analysis.
Customer impact
Eliminates all 150 FPs from these 4 rules. Remaining findings (36 total, 20 FP) produce a ~56% FP rate, consistent with pattern-matching SAST tools. Further tuning via community rules and per-language scoping can reduce this further.
Testing
opengrep --validatepasses on full dotnet.yml (40 rules, 0 errors)pytestpasses (139 tests, 0 failures)