Refactor creating barriers with annotations

Author: owen-mc

java/ql/lib/semmle/code/java/Concepts.qll

@@ -25,6 +25,12 @@ private module Frameworks {
  *
  * These are either method calls, which return `true` when there is a match, or
  * annotations, which are considered to match if they are present.
+ *
+ * To use a `RegexMatch` as a barrier or sanitizer, instantiate
+ * `DataFlow::BarrierGuard`. Method-call matches act as ordinary control-flow
+ * barrier guards, while annotation matches are treated as direct barriers,
+ * since an annotation does not dominate the expression it constrains;
+ * `DataFlow::BarrierGuard` handles both, so callers need not distinguish the two.
  */
 class RegexMatch extends Expr instanceof RegexMatch::Range {
   /** Gets the expression for the regex being executed by this node. */

java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll

@@ -418,7 +418,17 @@ module BarrierGuard<guardChecksSig/3 guardChecks> {
   }
 
   /** Gets a node that is safely guarded by the given guard check. */
-  Node getABarrierNode() { result = BarrierGuardValue<guardChecks0/3>::getABarrierNode() }
+  Node getABarrierNode() {
+    result = BarrierGuardValue<guardChecks0/3>::getABarrierNode()
+    or
+    // An annotation doesn't dominate the expression it constrains, so the
+    // barrier-guard machinery above never yields a node for it; treat it as a
+    // direct barrier instead. Annotations are always present, so we only
+    // consider the `true` branch.
+    exists(Guard g, Expr e |
+      g instanceof Annotation and guardChecks(g, e, true) and result.asExpr() = e
+    )
+  }
 }
 
 bindingset[this]

java/ql/lib/semmle/code/java/security/LogInjection.qll

@@ -90,13 +90,6 @@ private predicate logInjectionSanitizer(Expr e) {
       target.getStringValue() = ["\n", "\r", "\\n", "\\r", "\\R"]
     )
   )
-  or
-  exists(RegexMatch rm, CompileTimeConstantExpr target |
-    rm instanceof Annotation and
-    e = rm.getASanitizedExpr() and
-    target = rm.getRegex() and
-    regexPreventsLogInjection(target.getStringValue(), true)
-  )
 }
 
 /**
@@ -113,7 +106,6 @@ private predicate logInjectionGuard(Guard g, Expr e, boolean branch) {
   or
   exists(RegexMatch rm, CompileTimeConstantExpr target |
     rm = g and
-    not rm instanceof Annotation and
     target = rm.getRegex() and
     e = rm.getASanitizedExpr()
   |

java/ql/lib/semmle/code/java/security/PathSanitizer.qll

@@ -10,6 +10,7 @@ private import semmle.code.java.dataflow.SSA
 private import semmle.code.java.frameworks.kotlin.IO
 private import semmle.code.java.frameworks.kotlin.Text
 private import semmle.code.java.dataflow.Nullness
+private import semmle.code.java.security.Sanitizers
 
 /** A sanitizer that protects against path injection vulnerabilities. */
 abstract class PathInjectionSanitizer extends DataFlow::Node { }
@@ -470,12 +471,7 @@ private class DirectoryCharactersGuard extends PathGuard {
   Expr checkedExpr;
   boolean 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)
-  }
+  DirectoryCharactersGuard() { isMatchesCall(this, checkedExpr, branch) }
 
   override Expr getCheckedExpr() { result = checkedExpr }
 
@@ -501,12 +497,5 @@ private class DirectoryCharactersSanitizer extends PathInjectionSanitizer {
     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/lib/semmle/code/java/security/Sanitizers.qll

@@ -37,11 +37,14 @@ class SimpleTypeSanitizer extends DataFlow::Node {
  *
  * This is overapproximate: we do not attempt to reason about the correctness of the regexp.
  *
- * Use this if you want to define a derived `DataFlow::BarrierGuard` without
- * make the type recursive. Otherwise use `RegexpCheckBarrier`.
+ * This holds for both method-call and annotation regular-expression matches.
+ * Method-call matches yield barrier nodes via ordinary control-flow dominance,
+ * while annotation matches are treated as direct barriers by
+ * `DataFlow::BarrierGuard`, since an annotation does not dominate the
+ * expression it constrains.
  */
 predicate regexpMatchGuardChecks(Guard guard, Expr e, boolean branch) {
-  exists(RegexMatch rm | not rm instanceof Annotation |
+  exists(RegexMatch rm |
     guard = rm and
     e = rm.getASanitizedExpr() and
     branch = true
@@ -56,11 +59,6 @@ predicate regexpMatchGuardChecks(Guard guard, Expr e, boolean branch) {
 class RegexpCheckBarrier extends DataFlow::Node {
   RegexpCheckBarrier() {
     this = DataFlow::BarrierGuard<regexpMatchGuardChecks/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 | this.asExpr() = rm.getString())
   }
 }