Java: Fix File.getName() path sanitizer#22175
Conversation
owen-mc
changed the title
File.getName() path sanitizer
There was a problem hiding this comment.
Pull request overview
This PR fixes an unsound Java path-injection model by removing java.io.File.getName() as a full java/path-injection sanitizer, and adjusting the sanitizer logic/tests to treat it as safe only when paired with an explicit .. check—preventing false negatives like those reported in issue #22144.
Changes:
- Remove the
barrierModelentry that previously treatedFile.getName()’s return value as a completepath-injectionsanitizer. - Update the path-sanitizer guard logic to recognize sanitization when a
..-check occurs after data derived fromFile.getName(). - Update the CWE-022 test to add a BAD case (no
..check) and keep a GOOD case (with a..check), plus add a change note documenting new alerts.
Show a summary per file
| File | Description |
|---|---|
| java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.java | Adjusts regression test to reflect that getName() alone is not a complete path-injection sanitizer; adds BAD + guarded GOOD variants. |
| java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.expected | Updates expected query results to include the newly-detected path-injection flow for the BAD case. |
| java/ql/lib/semmle/code/java/security/PathSanitizer.qll | Updates sanitizer-guard recognition so getName() can contribute to sanitization only when followed by a .. check. |
| java/ql/lib/ext/java.io.model.yml | Removes the barrierModel row marking File.getName() as a path-injection barrier. |
| java/ql/lib/change-notes/2026-07-11-file-getname-path-sanitizer.md | Documents the modeling change and that it may produce new alerts. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Low
| private predicate dotDotCheckGuard(Guard g, Expr e, boolean branch) { | ||
| pathTraversalGuard(g, e, branch) and | ||
| exists(Guard previousGuard | | ||
| previousGuard.(AllowedPrefixGuard).controls(g.getBasicBlock(), true) | ||
| ( | ||
| exists(Guard previousGuard | | ||
| previousGuard.(AllowedPrefixGuard).controls(g.getBasicBlock(), true) | ||
| or | ||
| previousGuard.(BlockListGuard).controls(g.getBasicBlock(), false) | ||
| ) | ||
| or | ||
| previousGuard.(BlockListGuard).controls(g.getBasicBlock(), false) | ||
| // `File.getName` strips any directory prefix, returning only the final path | ||
| // component. The only remaining path traversal risk is when that component is | ||
| // itself `..`, so a check for `..` components completes the sanitization. | ||
| exists(FileGetNameCall getName | localTaintFlowToPathGuard(getName, g)) |
There was a problem hiding this comment.
This looks a bit odd: If g guards the expression e for branch and g also guards (any) call to getName (for any branch), then g guards e for branch?
Should the logic be more something like: g guards e, if the result of a call to getName is guarded by g and there is flow from the result of getName to e?
Something like (it should be its own top level disjunkt):
exists(FileGetNameCall getName |
pathTraversalGuard(g, getName, branch) and
TaintTracking::localExprTaint(getName, e)
)There was a problem hiding this comment.
You are right. My mistake for not checking code written by copilot well enough.
There was a problem hiding this comment.
(I'm not impressed that CCR didn't spot that.)
michaelnebel
left a comment
michaelnebel
left a comment
There was a problem hiding this comment.
LGTM!
We should probably run DCA before merging (will start a run now).
Reported in #22144. I have now downgraded it to a partial sanitizer. The return value has to be checked to confirm that it doesn't have any ".." components to become fully sanitized.