Fix @pattern sanitizer for TaintedPath

Author: owen-mc

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<directoryCharactersGuard/3>::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()
+    )
   }
 }

java/ql/test/query-tests/security/CWE-022/semmle/tests/SanitizationTests2.java

@@ -21,8 +21,8 @@ public ResponseEntity<String> vulnerablePathInjection(@RequestParam("path") Stri
 
     @GetMapping("/product/vuln/path-injection-fix")
     public ResponseEntity<String> 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));
         }

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 |