From 76b4f4f22343747ebf934aafbf23d6f92c69c268 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Fri, 3 Jul 2026 11:08:54 +0100 Subject: [PATCH] Fix @Pattern sanitizer for TaintedPath --- .../semmle/code/java/security/PathSanitizer.qll | 17 +++++++++++++++-- .../semmle/tests/SanitizationTests2.java | 4 ++-- .../CWE-022/semmle/tests/TaintedPath.expected | 4 ---- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll index 1288569fa5b..1b833b7d916 100644 --- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll +++ b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll @@ -470,7 +470,12 @@ private class DirectoryCharactersGuard extends PathGuard { Expr checkedExpr; boolean branch; - DirectoryCharactersGuard() { isMatchesCall(this, checkedExpr, branch) } + DirectoryCharactersGuard() { + // Annotations are handled directly as barriers in `DirectoryCharactersSanitizer`, + // since they don't dominate the sanitized expression and so can't act as barrier guards. + not this instanceof Annotation and + isMatchesCall(this, checkedExpr, branch) + } override Expr getCheckedExpr() { result = checkedExpr } @@ -493,7 +498,15 @@ private predicate directoryCharactersGuard(Guard g, Expr e, boolean branch) { */ private class DirectoryCharactersSanitizer extends PathInjectionSanitizer { DirectoryCharactersSanitizer() { - this.asExpr() instanceof ReplaceDirectoryCharactersSanitizer or + this.asExpr() instanceof ReplaceDirectoryCharactersSanitizer + or this = DataFlow::BarrierGuard::getABarrierNode() + or + // Annotations don't fit into the model of barrier guards because the + // annotation doesn't dominate the sanitized expression, so we instead + // treat them as barriers directly. + exists(RegexMatch rm | rm instanceof Annotation and isMatchesCall(rm, _, true) | + this.asExpr() = rm.getString() + ) } } diff --git a/java/ql/test/query-tests/security/CWE-022/semmle/tests/SanitizationTests2.java b/java/ql/test/query-tests/security/CWE-022/semmle/tests/SanitizationTests2.java index 3dcdfff8cf6..19f4996f0cb 100644 --- a/java/ql/test/query-tests/security/CWE-022/semmle/tests/SanitizationTests2.java +++ b/java/ql/test/query-tests/security/CWE-022/semmle/tests/SanitizationTests2.java @@ -21,8 +21,8 @@ public class SanitizationTests2 { @GetMapping("/product/vuln/path-injection-fix") public ResponseEntity vulnerablePathInjectionFix(@RequestParam("path") - @javax.validation.constraints.Pattern(regexp = "[a-zA-Z0-9]*") String path) throws IOException { // $ SPURIOUS: Source - try (FileInputStream stream = new FileInputStream(path)) { // $ SPURIOUS: Alert + @javax.validation.constraints.Pattern(regexp = "[a-zA-Z0-9]*") String path) throws IOException { + try (FileInputStream stream = new FileInputStream(path)) { byte[] fileContents = stream.readNBytes(1024); return ResponseEntity.ok(new String(fileContents, StandardCharsets.UTF_8)); } diff --git a/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.expected b/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.expected index 297187e8ece..bfeb4d730c2 100644 --- a/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.expected +++ b/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.expected @@ -1,6 +1,5 @@ #select | SanitizationTests2.java:16:59:16:62 | path | SanitizationTests2.java:14:59:14:91 | path : String | SanitizationTests2.java:16:59:16:62 | path | This path depends on a $@. | SanitizationTests2.java:14:59:14:91 | path | user-provided value | -| SanitizationTests2.java:25:59:25:62 | path | SanitizationTests2.java:23:62:24:139 | path : String | SanitizationTests2.java:25:59:25:62 | path | This path depends on a $@. | SanitizationTests2.java:23:62:24:139 | path | user-provided value | | TaintedPath.java:16:71:16:78 | filename | TaintedPath.java:13:58:13:78 | getInputStream(...) : InputStream | TaintedPath.java:16:71:16:78 | filename | This path depends on a $@. | TaintedPath.java:13:58:13:78 | getInputStream(...) | user-provided value | | Test.java:37:52:37:68 | (...)... | Test.java:32:16:32:45 | getParameter(...) : String | Test.java:37:52:37:68 | (...)... | This path depends on a $@. | Test.java:32:16:32:45 | getParameter(...) | user-provided value | | Test.java:39:32:39:48 | (...)... | Test.java:32:16:32:45 | getParameter(...) : String | Test.java:39:32:39:48 | (...)... | This path depends on a $@. | Test.java:32:16:32:45 | getParameter(...) | user-provided value | @@ -80,7 +79,6 @@ | Test.java:204:20:204:36 | (...)... | Test.java:32:16:32:45 | getParameter(...) : String | Test.java:204:20:204:36 | (...)... | This path depends on a $@. | Test.java:32:16:32:45 | getParameter(...) | user-provided value | edges | SanitizationTests2.java:14:59:14:91 | path : String | SanitizationTests2.java:16:59:16:62 | path | provenance | Sink:MaD:23 | -| SanitizationTests2.java:23:62:24:139 | path : String | SanitizationTests2.java:25:59:25:62 | path | provenance | Sink:MaD:23 | | TaintedPath.java:13:17:13:89 | new BufferedReader(...) : BufferedReader | TaintedPath.java:14:27:14:40 | filenameReader : BufferedReader | provenance | | | TaintedPath.java:13:36:13:88 | new InputStreamReader(...) : InputStreamReader | TaintedPath.java:13:17:13:89 | new BufferedReader(...) : BufferedReader | provenance | MaD:74 | | TaintedPath.java:13:58:13:78 | getInputStream(...) : InputStream | TaintedPath.java:13:36:13:88 | new InputStreamReader(...) : InputStreamReader | provenance | Src:MaD:72 MaD:76 | @@ -318,8 +316,6 @@ models nodes | SanitizationTests2.java:14:59:14:91 | path : String | semmle.label | path : String | | SanitizationTests2.java:16:59:16:62 | path | semmle.label | path | -| SanitizationTests2.java:23:62:24:139 | path : String | semmle.label | path : String | -| SanitizationTests2.java:25:59:25:62 | path | semmle.label | path | | TaintedPath.java:13:17:13:89 | new BufferedReader(...) : BufferedReader | semmle.label | new BufferedReader(...) : BufferedReader | | TaintedPath.java:13:36:13:88 | new InputStreamReader(...) : InputStreamReader | semmle.label | new InputStreamReader(...) : InputStreamReader | | TaintedPath.java:13:58:13:78 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |