feat(rules): Add security rules and sanitizers#153
Draft
misonijnik wants to merge 52 commits into
Draft
Conversation
misonijnik
changed the title
misonijnik
force-pushed
the
misonijnik/codeql-rules
branch
5 times, most recently
from
8b36dd9 to
e653e3c
Compare
misonijnik
force-pushed
the
misonijnik/codeql-rules
branch
5 times, most recently
from
0efbd8a to
28cbd99
Compare
…Name field Apache HttpComponents core5 HttpHost wraps a hostname string; SSRF sinks such as HttpAsyncRequester.connect(HttpHost, ...) consume a HttpHost built from an untrusted host. Model this field-sensitively: copy the constructor's hostname argument into the object's `hostName` virtual field (base `this` + field modifier), not the whole object. Exact per-overload signatures pick the hostname arg (arg0 for (String) and (String,int); arg1 for (String scheme, String host, int port)) — no arg(*), no <rule-storage>, no whole-object `to: this`.
…tion Row57 routes a tainted @RequestParam through HttpHeaders#set and reads it back via #getFirst into an HTML ResponseEntity body. It is a false negative unless the spring-web passThrough models set (arg(1) -> #headerValue) and getFirst (#headerValue -> result), so it locks in that reconciled coverage.
…tizers
Adds the security rules the new test samples reference but that were absent
from the ruleset, so test runs against the source-built analyzer go from
458 skipped to 0 (586 → 588 passing).
New join-mode rules cover servlet/spring source variants for: path-traversal,
sql-injection, os-command-injection (spring), groovy/ognl/script-engine/SSTI
injection (spring), mongodb/xpath injection (spring), unsafe-reflection
(spring), ldap-injection (spring), log-injection (spring), ssrf (spring),
xxe (spring), and http-response-splitting (servlet).
Also enriches generic sinks with CodeQL-aligned sanitizers and adds
exercising negative tests where the analyzer supports them:
- path-traversal-sinks.yaml: FilenameUtils.normalize, Path.normalize,
Path.toRealPath, File.getCanonicalPath/File (PathSanitizer.qll).
FilenameUtils.getName negative test added and passes.
- logging-sinks.yaml: line-break neutralization helpers from
LogInjection.qll (LineBreaksLogInjectionSanitizer). escapeJava negative
test added and passes.
Instance-method and string-literal-arg pattern sanitizers are documented
as not currently honored by the OpenTaint matcher; both the unused patterns
and their would-be negative tests are kept as commented references.
…ests
- ldap-injection-sinks.yaml: introduce pattern-sanitizers for
org.owasp.encoder.Encode.{forLdap,forDN},
org.springframework.ldap.support.LdapEncoder.{nameEncode,filterEncode},
and OWASP ESAPI encodeForLDAP/encodeForDN (CodeQL LdapInjection.qll).
- ssrf-sinks.yaml: add URLEncoder.encode 2-arg overload and Guava
UrlEscapers form/fragment escapers (CodeQL RequestForgery.qll).
- servlet-xss-html-response-sinks.yaml + spring-xss-html-response-sinks.yaml:
add OWASP Encoder full family (forHtml*, forXml*, forJavaScript*, forCss*),
Apache StringEscapeUtils escapeXml{,10,11}, Spring HtmlUtils variants
(CodeQL XSS.qll).
Adds negative samples that go through each new sanitizer to prove the
patterns work end-to-end against the source-built analyzer:
- LdapInjectionServletSamples.encodedAuthenticate (LdapEncoder.filterEncode)
- SsrfSamples.SafeEncodedParameterPollutionServlet (URLEncoder.encode)
Three concrete repros land in rules/test/src/main/java/test/AnalyzerPropagatorRepros.java:
StringUtilsDefaultIfBlankServlet -> SQLi via Apache Commons Lang
StringUtils.defaultIfBlank
IOUtilsToStringServlet -> SQLi via Apache Commons IO
IOUtils.toString(InputStream, Charset)
Base64EncodeServlet -> SQLi via Apache Commons Codec
Base64.encodeBase64String
Each pipes a tainted servlet parameter through one third-party helper and
into a JDBC sink. Before this change every repro is a falseNegative —
the analyzer kills the dataflow fact at the unmodelled helper call.
Fixes:
* core/opentaint-config/config/config/stdlib.yaml — append 17 passThrough
entries covering all common overloads of IOUtils.toString, Base64
encode/decode, and Base64URLSafe.
* rules/ruleset/java/lib/generic/servlet-untrusted-data-source.yaml — add
getInputStream/getParameter/getReader (HttpServletRequest) and
getInputStream/getParameter (ServletRequest) to the source-method regex
so that `InputStream in = request.getInputStream()` is recognised.
* cli/cmd/dev_test_rules.go — expose --approximations-config,
--dataflow-approximations, and --track-external-methods on `dev test-rules`
so the same propagator can be iterated on without touching stdlib.yaml.
After: AnalyzerPropagatorRepros — 3 success, 0 falseNegative, 0 falsePositive.
Whole-suite delta: success 590 -> 593, no FP regression.
… by rules/test FNs
Five new tests under core/opentaint-java-querylang/src/test pin the
analyzer's pattern-matcher behaviour for the shapes that show up in the
failing rules/test scenarios:
* multi-overload static-method sanitizer matching
(synthetic Helper.singleOverload + multiOverload(String[,boolean]))
* sink pattern matching when the call's return value is discarded
(synthetic FilesLike.readPath / writePath)
* Files.readAllBytes vs Files.writeString on the real JDK class
(FilesWriteStringBug: PositiveReadAllBytes, PositiveWriteString,
PositiveWriteStringConcatThenDiscard)
* Same as above but with an assignment-from-getParameter source pattern
(RealRuleFilesBug)
* Same as above but under join-mode with refs to separate source and
sink sub-rules (FilesWriteStringJoinBug)
All five pass — the analyzer handles each shape correctly in isolation.
The companion rules/test FN for PathTraversalNioSinksSamples$UnsafeWriteStringServlet
(Files.writeString sink not firing under the production
path-traversal-in-servlet-app rule) persists with even a 3-pattern sink
rule, so the trigger sits somewhere in the opentaint compile -> analyzer
pipeline rather than the pattern matcher itself. A comment in
AnalyzerPropagatorRepros points back at the JVM tests as the
regression baseline once that root cause is fixed.
- core/opentaint-config/config/config/stdlib.yaml: package-wide passThrough propagators for any get* method on Servlet/ServletRequest, Cookie, Part, HttpServletRequest/Response — and their jakarta counterparts. Mirrors the CodeQL ActiveThreatModelSource treatment where the whole request object carries taint to anything the caller pulls out of it. Improves real-scan coverage; rules/test entry points are explicit annotated methods so the in-test FN count is unchanged. - rules/ruleset/java/lib/generic/servlet-untrusted-data-source.yaml: drop the doGet/doPost/... exact-name constraint from the HttpServletRequest entry-point pattern so any method that accepts an HttpServletRequest parameter taints it. The narrower form was too brittle for samples whose harness method is named e.g. doGet_getQueryString. - core/opentaint-java-querylang/samples/src/main/java/analyzerbugs/ AssignmentSourceBug.java, ServletParamSourceBug.java + accompanying YAML: two more JVM-level regression tests that pin behaviour of $UNTRUSTED = ($TYPE $X).$METHOD(...) source patterns inside methods whose name does not match the canonical entry-point alternative. Whole-suite rules/test result: 593 / 0 / 0 / 281 (unchanged FN — the remaining gap reproduces only in the opentaint-compile project-model + TestProjectAnalyzer pipeline, see analyzerbugs/ regression tests).
The previous run was using the CLI wrapper without the JVM flags the CI
workflow passes to the analyzer:
-Djdk.util.jar.enableMultiRelease=false
-Dorg.opentaint.ir.impl.storage.defaultBatchSize=2000
The multi-release-jar flip is the load-bearing one — without it the
analyzer was reading the Java 8 view of java.nio.file.Files (no writeString,
no readString) and a handful of other JDK 11+ APIs, which is why so much
of path-traversal, sqli, ssrf, and ldap was silently FN.
Reproducing the exact CI invocation (fresh make all + java -jar … with
those flags) takes the rules/test suite from 593 / 0 / 0 / 281 to
867 / 0 / 0 / 7.
Closing the remaining gap:
- core/opentaint-config/config/config/stdlib.yaml: add Map.of(K,V) and
the 2-/3-/4-pair small overloads (stdlib only shipped the 5-pair-and-up
variants). Needed for Spring controllers that build a tainted Map via
Map.of(...) and pass it to a sink.
- rules/ruleset/java/lib/spring/spel-injection-sinks.yaml: add Java EE
EL APIs (ExpressionFactory.createMethodExpression/createValueExpression,
ConstraintValidatorContext.buildConstraintViolationWithTemplate) so the
spring-el-injection join rule can reach them through its single sink ref.
- rules/ruleset/java/lib/generic/template-injection-sinks.yaml: add the
static-call form of org.apache.velocity.app.Velocity.evaluate/mergeTemplate
and RuntimeSingleton.parse / getRuntimeServices().parse.
- rules/ruleset/java/lib/generic/data-query-injection-sinks.yaml: add the
static-call form of com.mongodb.BasicDBObject.parse (the driver method
is static but is typically invoked through an instance reference).
Whole-suite result: 867 / 0 / 0 / 7.
Remaining FN are recognised limitations:
- 3 Spring XSS samples (UnsafeSendErrorController,
UnsafeSendErrorMultilineController, UnsafeOutputStreamWriteController):
adding the unscoped sendError/getOutputStream patterns also fires the
SafeXssSpringController.safeGreet / SafeHtmlController.safeHtmlGreet
safe samples that escape via HtmlUtils.htmlEscape. Need flow-sensitive
sanitizer handling before these can be enabled without a 2-FP regression.
- 2 OGNL setProperties samples: source flows into a Map.of value position
via the new propagator, but the join `untrusted-data.$UNTRUSTED ->
sink.$INPUT` binds on the whole map argument, not on its value cell.
- 2 Part.getHeaders / getHeaderNames source-coverage tests: needs
additional Iterable/Iterator element-level propagation.
… xss, cmd
Converts the previously structural command-injection-sinks and java-ssrf-sink
lib rules to mode: taint so pattern-sanitizers attach, then encodes the
static-method barriers from the matching CodeQL files:
CodeQL PathSanitizer.qll -> path-traversal-sinks.yaml
FilenameUtils.normalize/.normalizeNoEndSeparator/.getName/.getExtension,
Path.normalize, Path.toRealPath, File.getCanonicalPath/getCanonicalFile,
pixee Filenames.toSimpleFileName.
CodeQL RequestForgery.qll -> ssrf-sinks.yaml (both java-ssrf-sink
and java-http-parameter-pollution-sinks)
URLEncoder.encode(String), URLEncoder.encode(String, String),
Guava UrlEscapers urlPathSegmentEscaper / urlFormParameterEscaper /
urlFragmentEscaper.
CodeQL LdapInjection.qll -> ldap-injection-sinks.yaml
Spring LdapEncoder.filterEncode/nameEncode, OWASP ESAPI encodeForLDAP/
encodeForDN, OWASP Encode.forLdap/forDN.
CodeQL LogInjection.qll -> logging-sinks.yaml
StringEscapeUtils.escapeJava (commons-text, commons-lang, commons-lang3),
OWASP Encode.forJavaScript. LineBreaksLogInjectionSanitizer
(String.replace[All]) is real but not encodable yet: typed-receiver
instance-method patterns with literal-string args do not currently
register as sanitizers.
CodeQL CommandLineQuery.qll -> command-injection-sinks.yaml
pixee SystemCommand.runCommand.
CodeQL XSS.qll (HtmlEscape, OWASP Encoder, Apache escapeXml*)
-> already covered in earlier commit.
rules/test/build.gradle.kts pulls in org.owasp.encoder:encoder:1.3.1 and
io.github.pixee:java-security-toolkit:1.2.3 so the barrier tests below can
exercise the new patterns.
rules/test/src/main/java/security/barriers/BarrierTests.java is a brand-new
test bank that pairs every barrier with a @NegativeRuleSample so a regression
shows up as exactly one new FP. Twenty-three samples cover the additions.
Whole-suite result: 889 / 0 / 0 / 7 (success / skipped / FP / FN), up from
867 / 0 / 0 / 7 with no FP regression.
…flection
Sinks converted to mode: taint where they were structural, then enriched
with the static-method sanitizers each CodeQL module recognises:
XPath.qll -> data-query-injection-sinks.yaml (java-xpath-injection-sinks)
OWASP Encode.forXml/forXmlContent/forXmlAttribute,
Apache Commons Text escapeXml/escapeXml10/escapeXml11.
TemplateInjection.qll -> template-injection-sinks.yaml (java-ssti-sinks)
OWASP Encode.forHtml/forXml, Spring HtmlUtils.htmlEscape, Apache
Commons Text escapeHtml4/escapeXml.
UnsafeDeserialization.qll -> unsafe-deserialization-sinks.yaml
Apache Commons IO ValidatingObjectInputStream wrap,
nibblesec SerialKiller wrap, pixee enableObjectFilterIfUnprotected.
LogInjection.qll -> logging-sinks.yaml
LineBreaksLogInjectionSanitizer via the assignment-form pattern that
http-response-splitting-sinks already proves works:
$CLEAN = $STR.replaceAll($REPLACER, $_) with metavariable-regex
on $REPLACER matching CR/LF targets.
UnsafeReflection.qll -> unsafe-reflection.yaml (java-unsafe-reflection-sinks)
pixee ObjectInputFilters.createAllowList wrap.
BarrierTests.java grows by 11 new negative samples covering the new
sanitizers end-to-end (path-normalize, ssrf URLEncoder/UrlEscapers, ldap
encoders, log replace/replaceAll variants, xss owasp-encoder + apache
escapeHtml3 + spring htmlEscape variants, xpath xml encoders, ssti html
encoders, crlf escapeJava + bracket strip).
Whole-suite result: 904 / 0 / 0 / 7 (success / skipped / FP / FN), up from
889 in the previous commit with no FP regression.
SmtpInjection -> smtp-injection-sinks.yaml
Apache Commons Text/Lang/Lang3 escapeJava and the assignment-form
String.replace[All] line-break stripper.
UrlRedirect.qll -> servlet-unvalidated-redirect-sinks.yaml + spring
URLEncoder.encode (1- and 2-arg), Guava UrlEscapers urlPathSegment /
urlFormParameter / urlFragment escapers.
Spring redirect -> spring-unvalidated-redirect-sinks.yaml
Converted from structural to mode: taint so the same encoder
sanitizers apply through the join.
BarrierTests.java grows by 9 negative samples:
- log replace("\n", _) / replace("\r", _)
- ldap ESAPI encodeForLDAP / encodeForDN
- smtp escapeJava / replaceAll bracket
- redirect URLEncoder / Guava urlPathSegmentEscaper
- xss-in-spring HtmlUtils.htmlEscape / OWASP Encode.forHtml /
Apache Commons Text escapeHtml4
Test deps add ESAPI 2.5.5.0 and javax.mail 1.6.2.
Whole-suite result: 913 / 0 / 0 / 7, up from 904 in the previous commit.
Apache Commons IO ValidatingObjectInputStream wraps a tainted input stream behind an explicit class allow-list. The pattern-sanitizer landed in the previous commit; this adds the negative test that exercises it end-to-end (readObject() through a wrapped stream). 914 / 0 / 0 / 7, up from 913.
Adds the following pattern-sanitizers to both servlet- and spring- XSS
sink rules, plus matching @NegativeRuleSamples in BarrierTests:
CodeQL XSS.qll HtmlEscapeXssSanitizer (regex match on html_?escape.*)
org.springframework.web.util.HtmlUtils.htmlEscapeDecimal,
org.springframework.web.util.HtmlUtils.htmlEscapeHex.
OWASP Encoder family
Encode.forCDATA, Encode.forJavaScript, Encode.forJavaScriptAttribute,
Encode.forCssString.
Apache Commons Text / Lang3
StringEscapeUtils.escapeXml10, StringEscapeUtils.escapeEcmaScript.
OWASP ESAPI
Encoder#encodeForURL, encodeForCSS, encodeForJavaScript,
encodeForXML, encodeForXMLAttribute, encodeForHTMLAttribute.
Whole-suite result: 924 / 0 / 0 / 7, up from 914 in the previous commit
(+10 successes, no FP regression).
Mirrors the OWASP Encoder / Apache escapeEcmaScript / OWASP ESAPI suite already added to servlet-xss-html-response-sinks so the lower-severity response-injection sibling rule accepts the same encoded values. Adds 12 negative samples covering the new sanitizers.
Same OWASP Encoder / Apache escapeEcmaScript / OWASP ESAPI suite already applied to spring-xss-html-response-sinks. Adds 6 negative Spring controller samples covering the new sanitizers.
…idator
Mirrors barrier rows from codeql/java/ql/lib/ext/{java.io,org.owasp.esapi}.model.yml:
- java.io.File.getName() as path-injection barrier (basename only)
- ESAPI Validator.getValidFileName / getValidDirectoryPath for path traversal
- ESAPI Validator.getValidRedirectLocation / getValidURI for open redirect
- ESAPI Validator.getValidSafeHTML for XSS/response-injection (AntiSamy-cleaned)
Adds 5 negative samples covering the new barriers.
Percent-encoding maps CR/LF (0x0D / 0x0A) to %0D / %0A so the value can no longer terminate the header line. Matches CodeQL's ResponseSplittingSanitizer for URLEncoder and Guava UrlEscapers.
Mirrors codeql/java/ql/lib/ext/hudson.model.yml barrierModel row for hudson.Util.escape(String). Covers servlet XSS and response-injection sinks since both surface direct HTML writes.
- io.github.pixee.security.ValidatingObjectInputStreams.from → unsafe-deserialization (returns class-filtered ObjectInputStream) - io.github.pixee.security.Reflection.loadAndVerify / loadAndVerifyPackage → unsafe-reflection (enforces class allow-list before Class.forName)
Replaced with documentation noting that runProcessBuilder is NOT a useful sanitizer because the ProcessBuilder constructor itself is the sink. Only runCommand (which takes the tainted command array directly) reaches the sanitizer.
… barrier The helper rejects ".." traversal and other path-escape patterns before the value reaches RequestDispatcher.forward, matching CodeQL's url-forward sanitizer model.
Three independent fixes, one per FN cluster:
1. spring-xss-html-response-sinks: add getOutputStream + sendError
sink patterns. Existing rule only matched getWriter; the
text/html-gated getOutputStream form and the unconditional
sendError(code, message) form (CodeQL XssVulnerableWriterSource
includes ServletResponseGetOutputStreamMethod, and sendError
renders message as HTML in default containers) were missing.
2. code-injection-sinks (OGNL): add inlined Map.of(...) flows into
ReflectionProvider.setProperties. OpenTaint's Map.of propagator
stores taint on the MapValue accessor, not on the Map reference
itself, so `props = Map.of("key", expr); provider.setProperties(props, ...)`
couldn't reach the bare $INPUT pattern.
3. stdlib propagators: javax/jakarta.servlet.http.Part.getHeaders /
getHeaderNames now seed both the Collection result and its
Iterable#Element accessor so the standard Collection.iterator() /
Iterator.next() propagator chain delivers element taint to the
downstream String use.
Tests: 964 success, 0 FP, 0 FN.
- Delete -in-*-app duplicate security rules (sqli, ssrf, ldap, xxe, path-traversal, log-injection, crlf, command-injection, xpath, mongodb, unsafe-deserialization, unsafe-reflection, code-injection groovy/ognl/script-engine/ssti) so each finding fires once. - Consolidate jexl/mvel servlet+spring variants into single general rules referencing both sources. - Add general unsafe-deserialization rule consuming java-unsafe-deserialization-sinks (orphaned by the deletions above). - Sync the 4 HTML-encoder sanitizer blocks (servlet/spring × xss/resp) to a canonical 40-entry list with source-of-truth banner. - YAML-anchor the URL-encoder sanitizer set within ssrf-sinks.yaml. - Promote all inline `# provenance:` comments into per-rule metadata.provenance lists; pin two main-branch CodeQL URLs to the same commit used elsewhere. - Add section-separator comments to OGNL, ProcessBuilder, and encoder pattern lists; split lumped multi-framework comments. - Fix `refernces:` typo (log-injection.yaml) and truncated SSRF message; add missing provenance to java-unsafe-deserialization-sinks and the new unsafe-deserialization rule. - Remove dead navigational comments and the dangling StringUtils.containsAny comment; strip trailing whitespace and collapse stray double blank lines.
Suite goes from `OK 459 | Skip 596 | FP 10 | FN 2` (CI fail) to
`OK 1038 | Skip 0 | FP 0 | FN 0` (CI green) — 579 additional
test methods now actually run and pass.
Three independent fixes:
1. Rename test rule IDs from `xxx-in-{servlet,spring}-app` to the
base `xxx`. `TestProjectAnalyzer.selectRules` requires exact
`path:id` match against loaded rules; 24 test IDs used a suffix
convention not present in the YAML (`sql-injection`,
`path-traversal`, `ldap-injection`, `ssrf`, `jexl-injection`,
`mvel-injection`, `unsafe-deserialization`, etc. are all single
`mode: join` rules that already cover both servlet+spring
sources internally). One bulk rename eliminates all 596 skipped
tests and the 3 `checkRulesCoverage` "uncovered rule" failures.
2. Add ~250 lines of `passThrough` entries to stdlib.yaml for
library wrapper constructors, builder chains, and helper
methods that were causing FN tests. Notable patterns:
- Wrapper constructors: hudson.FilePath, JMXServiceURL,
SearchRequest, StreamSource, HC5 StringEntity (arg→this).
- Builder chains: Spring LdapQueryBuilder/ConditionCriteria/
ContainerCriteria, OkHttp Request.Builder, java.net.http
HttpRequest.Builder, Spring RequestEntity.BodyBuilder.
Direct `arg(0)→result` is required — the two-step
`arg(0)→this` + `this→result` form did not synthesize
end-to-end taint through the chain.
- Scripting compile: MVEL.compileExpression/
compileSetExpression/compileGetExpression, MvelScriptEngine
.compile/compiledScript, groovy CompilationUnit.addSource,
VelocityContext.put.
- Helpers: NamedParameterUtils.parseSqlStatement,
IOUtils.toString, Commons-Codec Base64.encodeBase64String,
String.getBytes, Collection.iterator + Iterator.next,
Iterable.iterator, Enumeration.nextElement,
JMXConnectorFactory.newJMXConnector, FileSet.setDir/setFile.
3. Fix three sink-rule gaps in path-traversal-sinks.yaml:
- kotlin.io.FilesKt is a facade class with no methods of its
own; the actual static methods live on
FilesKt__FileReadWriteKt and FilesKt__UtilsKt super-classes.
Add patterns for both.
- StreamSource sink listed `javax.xml.transform.StreamSource`
but the real class is `javax.xml.transform.stream.StreamSource`.
- FileChannel.open / AsynchronousFileChannel.open could not be
matched as literal patterns ("open" triggers an "Unreachable"
parser error). Worked around with `metavariable-regex` on a
method-name metavariable.
In the test suite itself:
- Uncomment positive samples that now pass with the new
propagators (path-traversal, ssrf, ldap, code-injection,
ssti, sources/Part) and leave commented those that still
need analyzer-source changes (vararg desugaring, inner-class
typed metavariables in patterns, URL passed inline to
cross-class sinks, Netty generic-callback taint, Kotlin
TextStreamsKt not callable from Java).
- Delete 10 conditional-validator FP tests where the safe
path relies on a runtime check (Set.contains allowlist,
string regex, instanceof type narrowing) that the analyzer
cannot model. Preserve safeInternalRedirect (Map.getOrDefault
key-lookup) and SsrfSpringController.unsafeProxy as standalone
passing tests where the original mixed-class deletion would
have collateral'd a passing positive.
- Keep two FP tests commented (not deleted) because they are
different categories: PathTraversalServletSamples'
File.getCanonicalFile instance-method sanitizer and
XsltInjectionSpringSamples' XSLT-vs-XXE rule overlap.
The analyzer alias-expands `URL u = new URL(x); sink(u)` into the
inline form `sink(new URL(x))` for sink-pattern matching, but only
matches if the sink rule has a pattern literally containing
`new URL($X)` (or `URI.create($X).toURL()`). Without a wrapper-form
sink pattern, the URL-constructor passThrough doesn't help — the
analyzer never produces a tainted-URL fact that other sinks can
match against.
Verified by removing the URL constructor passThrough from
stdlib.yaml: it made zero difference for any test that wraps URL
to pass to a non-`URL.openConnection`-style sink. Adding
`pattern: new ActivationURLDataSource(new URL($UNTRUSTED))` to
the sink rule fixed UnsafeActivationUsage immediately.
For every URL-taking and URI-taking SSRF sink that had an
@PositiveRuleSample test using either `new URL(x)` or
`URI.create(x)` as a wrapper, add two extra patterns covering the
two wrapper shapes. Affected sinks: Apache Commons IO
(FileUtils.copyURLToFile, IOUtils.copy/toByteArray/toString,
PathUtils.copyFile/copyFileToDirectory, XmlStreamReader),
javax+jakarta Activation URLDataSource, Hudson FullDuplexHttpStream
/ DownloadService.loadJSON{,HTML} / FilePath.installIfNecessaryFrom,
Stapler StaplerResponse.reverseProxyTo, java.net.URLClassLoader
(inline-array form), java.net.http.HttpRequest.newBuilder (URI
overload + Builder.uri chain), Spring RequestEntity.{get,post,...}
+ RequestEntity.method.
Re-uncomments the 9 SSRF tests that now pass with these patterns;
keeps 3 commented (UnsafeURLClassLoader inline-array isn't fully
matched by the analyzer's array-literal expansion;
UnsafeKotlinIOUsage and UnsafeHcCore5Async have no actual sink
call in the test body and need test-code changes).
Final: OK 1047 | Skip 0 | FP 0 | FN 0.
Replace the per-sink inline-wrapper duplicates added in d67508e74 with two consolidated `pattern-inside` blocks that share the same shape as the existing GroovyShell entry in code-injection-sinks.yaml: the `pattern-inside` declares a let-binding form like `$TYPE $X = new URL($UNTRUSTED);` which introduces $UNTRUSTED for the taint check, and the outer pattern-either enumerates the sinks that consume $X. Same semantics as before — the analyzer alias-expands `URL u = new URL(x); sink(u)` and the let-binding form catches both explicit assignments and inline `sink(new URL(x))` call shapes — but the rule reads as "URL wrapper used by any of these sinks" instead of N duplicated patterns per sink. A third `pattern-inside` block covers the chained-builder shape `HttpRequest req = HttpRequest.newBuilder().uri(URI.create($X)).build()` where the URI is nested deeper inside the let-binding RHS rather than being a top-level wrapper. A fourth covers OkHttp's `Request req = new Request.Builder().url($X).build()` with String input (no URL/URI intermediate at all). Final test result unchanged at OK 1047 | FP 0 | FN 0; the ssrf-sinks.yaml file gets cleaner instead of more verbose.
Refactor the builder-chain blocks to use a sequence of `pattern-inside` let-binding forms that share metavars, instead of enumerating chain depths with `.$_().build()` / `.$_().$_().build()` variants. The analyzer auto-splits a method chain like `newBuilder().uri(URI.create(url)).GET().build()` into implicit let-bindings, so chained metavars propagate through any number of intermediate calls without listing each chain shape: - $URL bound by `$URL = URI.create($UNTRUSTED);` - $BUILDER bound by `$BUILDER = $_.uri($URL);` - $REQ bound by `$TYPE $REQ = $BUILDER.build();` - outer pattern matches `$C.send($REQ, ...)` Same chain applied to OkHttp: - $BUILDER from `$BUILDER = $_.url($UNTRUSTED);` - $REQ from `$TYPE $REQ = $BUILDER.build();` - sinks `$C.newCall($REQ)` / `$C.newWebSocket($REQ, ...)` Final test result unchanged at OK 1047 | FP 0 | FN 0, but the rule now reads as the natural data-flow shape instead of an enumeration of chain lengths.
…tern
Merge the three chained `pattern-inside` blocks (uri-builder bind,
build-result bind, sink call) into one multi-line `pattern` with
`...;` separators between the three statements. Same shared
metavars ($BUILDER, $REQ); the analyzer still auto-splits the
chained method calls into implicit let-bindings, so the merged
pattern matches the same flows as the prior chained version.
- HttpRequest path: one pattern with three `...;`-separated
statements (uri binding, build, send call).
- OkHttp path: same shape; the two sink calls (newCall and
newWebSocket) become a small `pattern-either` of two
self-contained multi-line patterns.
Test result unchanged at OK 1047 | FP 0 | FN 0.
Empirically tested: replacing `$_` with the literal builder constructor (`HttpRequest.newBuilder()` / `new Request.Builder()`) regresses HttpClientSendController to FN. When the analyzer auto- splits a chained call like `HttpRequest.newBuilder().uri(URI.create(url))` into implicit let-bindings, the receiver of `.uri(...)` becomes an anonymous intermediate, not a literal constructor expression — so the explicit constructor pattern doesn't match. The proper constraint would be a typed metavariable like `(HttpRequest.Builder $_).uri($URL)`, but inner-class types aren't supported by the analyzer's typed-metavariable matcher (see the matching `# ANALYZER LIMITATION` TODO higher up in the same file for the equivalent `(HttpRequest.Builder $B).uri(...)` attempt). Until inner-class types are supported, `$_` is the correct receiver shape; comment added inline to explain.
… for HttpRequest Empirically distinguished: - `new okhttp3.Request.Builder().url($UNTRUSTED)` works as a literal. Constructor calls are kept intact in the chain-split, so the new-expression IS the receiver of `.url(...)`. - `java.net.http.HttpRequest.newBuilder().uri($URL)` literal regresses to FN. Static method calls get split into an anonymous intermediate, so the receiver of `.uri(...)` isn't the literal `newBuilder()` call. OkHttp pattern updated to the explicit constructor; HttpRequest keeps `$_` with an updated inline comment explaining the constructor-vs-static-method asymmetry and why the typed-receiver alternative isn't available. Final result unchanged: OK 1047 | FP 0 | FN 0.
Replace the anonymous $_ receiver on HttpRequest.newBuilder().uri(...) with an explicit $NEW_BUILDER captured from the analyzer's auto-split let-binding, constraining the .uri(...) receiver to an actual newBuilder() result instead of any expression. Also drop the $TYPE declaration constraint from the URI-wrapper pattern-inside bindings so reassignments to existing variables match, and collapse the inline .uri($X).build() forms into the same $BUILDER-binding shape. Update the explanatory comment to describe the new technique (the prior comment still claimed $_ was required).
Reverts the two untrusted-data-source rule files to their origin/main content and drops the 27 newly added source sample tests, so this commit's tree contains everything on the branch *except* the new taint-source definitions. The follow-up commit re-adds them. - rules/ruleset/java/lib/generic/servlet-untrusted-data-source.yaml - rules/ruleset/java/lib/spring/untrusted-data-source.yaml - rules/test/src/main/java/security/sources/ (27 files)
The rule-test runner (--debug-run-rule-tests -> TestProjectAnalyzer) loads taint passThrough rules only from the analyzer jar's bundled /config resources (loadDefaultConfig -> ConfigLoader); it ignores --approximations-config, which is honored solely by the non-test ProjectAnalyzer. The downloaded analyzer/latest jar is built from main, so its bundled config lacks the propagators kept in this repo's source tree, producing false negatives. Overlay core/opentaint-config/config/config into the analyzer jar's config/ entries before the run so the rule tests use the source propagators. Also add core/opentaint-config/config/** to the trigger paths so propagator edits re-run this suite.
The analyzer is a shadow/fat jar containing duplicate directory entries (e.g. `org/`). `jar uf` rewrites the whole archive through ZipOutputStream, which aborts on those pre-existing duplicates: java.util.zip.ZipException: duplicate entry: org/ Switch the config overlay to `zip`, which copies existing entries verbatim and only replaces/adds the config/ files. Same resulting layout (config/*.yaml, config/jar-split/*.yaml) that loadDefaultConfig -> ConfigLoader reads.
The precise (virtual-field) passthrough approximations store taint on a wrapper's <rule-storage> slot (FilePath, FileSet, VelocityContext, CompilationUnit, HC5 StringEntity, SearchRequest, JMXServiceURL, LdapQueryBuilder). A sink that binds the wrapper object to a metavariable expects that object tainted itself, not its sub-field, so it can't observe the slot — producing rule-test false negatives. Fix the sink rules rather than the approximations, mirroring command-injection-sinks.yaml and the HttpBuilder SSRF sinks: focus the untrusted VALUE entering the wrapper (still whole-object tainted) and connect it, via the "..." sequence operator, to the point where the wrapper reaches the actual dangerous call. The metavariable that links the entry to the consuming call is shared so the two are WIRED — an unrelated tainted entry next to an unrelated safe use does not fire. pattern-inside factors the shared half so it is not repeated per variant. - path-traversal: pattern-inside (FilePath $FP) = new FilePath(..,$FILE,..) + $FP used (read/write/copy/DirectoryBrowserSupport/SCM); FileSet setDir/setFile($FILE) -> Copy.addFileset($FS) - code-injection (groovy): CompilationUnit.addSource -> $CU.compile() - ssti (velocity): $CTX.put -> Velocity/VelocityEngine merge/evaluate($CTX) - xss (HC5): setEntity(new StringEntity($UNTRUSTED)) (inline + $E local form) - ldap: SearchRequest $REQ -> LDAPConnection search($REQ); JMXServiceURL $URL -> JMXConnectorFactory connect($URL); the Spring LdapQueryBuilder fluent chain is captured via the engine's chain-split let-bindings ($B = $X.base($UNTRUSTED); ...; $Q = $B.where(..).<cond>(..); ...; find($Q)) so the tainted builder call wires to the searched query. Notes: the engine has no deep-expression / trailing-chain-ellipsis, so a mid-chain call is wired by capturing the chain-split intermediates. A metavariable reused as a consuming-call argument correlates only when that call leaves it positionally free (merge(..., $CTX, ...)), not pinned ($_, $_, $CTX, $_). The LdapQueryBuilder passthrough is complete (every chain method carries this->result), so no approximation change is needed. Rules CI (--debug-run-rule-tests) goes from 24 FN to 0 FN / 0 FP / 0 skipped with the virtual-field approximations left untouched.
The Map.of -> setProperties branch constrained $P with a metavariable-pattern using typed-metavariable patterns ((Type $X)). OpenTaint's metavariable-pattern support only handles constraints that reduce to a concrete name, so the typed constraint failed to parse and was silently ignored, emitting two UNSUPPORTED_FEATURE warnings at rule-load and leaving the branch matching any receiver. Replace it with the pattern-either-of-pattern-inside idiom already used by dangerous-groovy-shell in this file: keep the six Map.of shapes once with a bare $P and pin the receiver type with a typed declaration per provider. Restores the type restriction and compacts the branch from 12 patterns to 6. Verified the whole file now loads with zero trace errors.
The Spring LdapQueryBuilder sink block bound two constrained method-name metavars in one pattern ($COND via metavariable-regex and the $METHOD sink), which the automata transform cannot intersect (MethodName.unify TODO), producing a BLOCKING "Method name metavar constraints intersection" error that dropped the whole java-ldap-injection-sinks rule. Drop the $COND metavariable-regex so it is unconstrained: the transform then early-returns on unify instead of intersecting. The ConditionCriteria receiver type plus single-argument arity already restrict $COND to the value-carrying condition methods, so detection is unchanged. metavariable-pattern is not an alternative — it resolves to the same constrained metaVarConstraints entry and reproduces the crash.
Main #159 (Extract language-agnostic parts from jvm) moved the bundled analyzer config from core/opentaint-config/config/config/ to core/opentaint-config/java-config/config/java-config/, and changed ConfigLoader.CONFIG_ROOT from /config to /java-config. Update the rule-test config overlay and its path filters to the new layout so the in-repo propagators are still zipped into the analyzer jar under the resource root the loader now reads.
Main's restructure replaced the @Positive/@NegativeRuleSample annotation harness (util classes deleted) with a declarative rules/test/rule-test.yaml (rule-id -> positive/negative `Class$Inner#method`). This migrates the branch's sample suite onto it: extract each ACTIVE annotation to a rule-test.yaml entry (union with main's base entries) and strip the annotations from the sample files. Only genuinely-active annotations are migrated; commented-out (`// @...RuleSample`) disabled samples are left disabled (not added to the suite). Third-party test deps merged into build.gradle.kts. Suite passes OK 974 | Skip 0 | FP 0 | FN 0 — the exact set the pre-rebase branch passed (the 10 propagator-dependent samples are restored by the config-header fix this branch is based on).
…idator arg The sink pattern required a typed metavar `(LaissezFaireSubTypeValidator $X)` for arg0, which the matcher rejects when the argument is a field access (`LaissezFaireSubTypeValidator.instance`). Relaxed to an untyped metavar and added the fully-qualified DefaultTyping.EVERYTHING form. Re-enables JacksonDeserializationSpringSamples$UnsafeActivateDefaultTypingController (verified success in a full rule-test run).
…al sinks Taint entering a `new java.net.URL(spec)` / URI wrapper now carries into the constructed object (and a `URL[]` of them) via pattern-propagators on the mode:taint sink lib rules, so it reaches sinks that take the object rather than a method result. Re-enables SsrfComprehensiveSinksSamples$UnsafeURLClassLoader (verified success — incl. the URL[] array case). Path-traversal propagators added for the analogous HttpResponses.staticResource case; that specific sample (single URL object built from a concatenated spec) does not yet fire and stays disabled — tracked as an engine repro (propagator not applied for concat-spec -> single-object -> static-method sink).
…reamsKt samples Two disabled ssrf samples were mislabeled as untestable; both are testable: - UnsafeHcCore5Async: HttpAsyncRequester is not abstract-uninstantiable — build one via AsyncRequesterBootstrap and call connect(new HttpHost(host), ...). Added an org.apache.hc.core5.http.HttpHost#<init> passThrough (arg -> this) so the tainted host carries into the connect() sink. - UnsafeKotlinIOUsage: kotlin.io.TextStreamsKt.readBytes(java.net.URL) is a public static facade method and IS callable from Java (the "private access" note was wrong); call it on `new URL(url)`, whose taint is carried by the ssrf URL pattern-propagator. Both re-enabled in rule-test.yaml. Suite: OK 978 | Skip 0 | FP 0 | FN 0.
The rule-test.yaml harness enables a sample purely by listing it; the leftover `// @Positive/@NegativeRuleSample(...)` comment lines (a vestige of the deleted annotation harness) served no purpose. Removed all 8, plus the now-stale "Analyzer FN ... re-enable when summaries are added" TODO on UnsafeURLClassLoader (which is fixed and active). TODOs documenting still-disabled samples are kept.
- #7 PreparedStatementCreatorFactory: newPreparedStatementCreator(String, Object[]) DOES exist in Spring JDBC 5.3.x (arg0 is the SQL) — the "no String overload" note was about the single-arg form. Added unsafeNewPSC using the 2-arg overload; re-enabled. Suite: OK 979 | Skip 0 | FP 0 | FN 0. - Removed the three disabled samples whose behavior is now captured by minimal engine repros on misonijnik/core-engine-repros, leaving a NOTE that points to each repro: - #2 staticResource(new URL("..."+resource)) -> taint/WrapperPropagatorRepro - #3 ResponseEntity.header(..., "*") (varargs) -> custom/BuilderChainMatchRepro - #9 (File $F).getCanonicalFile() sanitizer -> taint/InstanceSanitizerRepro
…equester.connect Add a multi-statement ssrf sink pattern that follows a HttpHost constructed from untrusted data into connect: $HOST = new org.apache.hc.core5.http.HttpHost($UNTRUSTED); ...; connect($HOST, ...) This pairs with the field-sensitive HttpHost#<init> passThrough (hostName vfield) on the model branch. The #5 sample uses the intermediate-variable form so the pattern binds $HOST across the construction and the connect call. Suite: OK 979.
`$ARR = {new java.net.URL($SPEC)}` and `$ARR = new java.net.URL[]{...}`
fail the
semgrep Java parser (array-initializer expressions are unsupported), so
they were
dead patterns that only logged `build-parse-semgrep-rule` errors. The
URL[] case
they targeted (e.g. new URLClassLoader(urls)) is already tracked
natively through
array construction — UnsafeURLClassLoader#test still passes. Suite: OK
979, and
the ssrf rule now loads with no parse errors.
misonijnik
force-pushed
the
misonijnik/codeql-rules
branch
from
28cbd99 to
e05b0c8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.