Refactor creating barriers from RegexMatch

It is better to have the logic in one place, to avoid encountering this
bug again.

Author: owen-mc

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

@@ -25,6 +25,13 @@ 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, use the `RegexMatchBarrier`
+ * module rather than instantiating `DataFlow::BarrierGuard` directly. An
+ * annotation match does not dominate the expression it constrains, so it
+ * cannot act as a control-flow barrier guard; `RegexMatchBarrier` handles
+ * method-call matches as barrier guards and annotation matches as direct
+ * barriers, 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. */
@@ -33,7 +40,13 @@ class RegexMatch extends Expr instanceof RegexMatch::Range {
   /** Gets an expression for the string to be searched or matched against. */
   Expr getString() { result = super.getString() }
 
-  /** Gets an expression to be sanitized. */
+  /**
+   * Gets an expression to be sanitized.
+   *
+   * To turn these into barrier nodes, use the `RegexMatchBarrier` module (in
+   * `semmle.code.java.security.Sanitizers`), which correctly handles both
+   * method-call and annotation matches.
+   */
   Expr getASanitizedExpr() { result = [this.getString(), super.getAdditionalSanitizedExpr()] }
 
   /**

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

@@ -40,7 +40,7 @@ private class LineBreaksLogInjectionSanitizer extends LogInjectionSanitizer {
   LineBreaksLogInjectionSanitizer() {
     logInjectionSanitizer(this.asExpr())
     or
-    this = DataFlow::BarrierGuard<logInjectionGuard/3>::getABarrierNode()
+    this = RegexMatchBarrier<logInjectionGuard/3>::getABarrierNode()
   }
 }
 
@@ -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 }
 
@@ -500,13 +496,6 @@ private class DirectoryCharactersSanitizer extends PathInjectionSanitizer {
   DirectoryCharactersSanitizer() {
     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()
-    )
+    this = RegexMatchBarrier<directoryCharactersGuard/3>::getABarrierNode()
   }
 }

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

@@ -37,31 +37,57 @@ 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.
+ * Used directly with `DataFlow::BarrierGuard`, only method-call matches yield
+ * barrier nodes, since an annotation does not dominate the expression it
+ * constrains. Use `RegexMatchBarrier` (as `RegexpCheckBarrier` does) to also
+ * treat annotation matches as barriers.
  */
 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
   )
 }
 
+/** A guard-check predicate over regular-expression matches, as used by `RegexMatchBarrier`. */
+signature predicate regexMatchGuardChecksSig(Guard g, Expr e, boolean branch);
+
+/**
+ * Given a guard-check predicate that includes regular-expression matches,
+ * this module provides the corresponding barrier nodes, transparently handling
+ * the two kinds of `RegexMatch`:
+ *  - method calls, which act as ordinary control-flow barrier guards, and
+ *  - annotations, which don't dominate the expressions they constrain and so
+ *    are instead treated as direct barriers.
+ *
+ * Callers do not need to distinguish between the two: include annotation
+ * matches in `guardChecks` (with `branch = true`, since an annotation is
+ * always present) and use `getABarrierNode` in place of a direct
+ * `DataFlow::BarrierGuard` instantiation.
+ */
+module RegexMatchBarrier<regexMatchGuardChecksSig/3 guardChecks> {
+  /** Gets a node that is sanitized by a regular-expression match. */
+  DataFlow::Node getABarrierNode() {
+    result = DataFlow::BarrierGuard<guardChecks/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.
+    exists(Guard g, Expr e |
+      g instanceof Annotation and guardChecks(g, e, true) and result.asExpr() = e
+    )
+  }
+}
+
 /**
  * A check against a regular expression, considered as a barrier guard.
  *
  * This is overapproximate: we do not attempt to reason about the correctness of the regexp.
  */
 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())
-  }
+  RegexpCheckBarrier() { this = RegexMatchBarrier<regexpMatchGuardChecks/3>::getABarrierNode() }
 }
 
 /**