Go: Deprecate and replace BarrierGuard class

Author: aschackmull

go/ql/lib/semmle/go/dataflow/barrierguardutil/RedirectCheckBarrierGuard.qll

@@ -4,11 +4,30 @@
 
 import go
 
+private predicate redirectCheckGuard(DataFlow::Node g, Expr e, boolean outcome) {
+  g.(DataFlow::CallNode)
+      .getCalleeName()
+      .regexpMatch("(?i)(is_?)?(local_?url|valid_?redir(ect)?)(ur[li])?") and
+  // `isLocalUrl(e)` is a barrier for `e` if it evaluates to `true`
+  g.(DataFlow::CallNode).getAnArgument().asExpr() = e and
+  outcome = true
+}
+
+/**
+ * A call to a function called `isLocalUrl`, `isValidRedirect`, or similar, which is
+ * considered a barrier guard for sanitizing untrusted URLs.
+ */
+class RedirectCheckBarrier extends DataFlow::Node {
+  RedirectCheckBarrier() { this = DataFlow::BarrierGuard<redirectCheckGuard/3>::getABarrierNode() }
+}
+
 /**
+ * DEPRECATED: Use `RedirectCheckBarrier` instead.
+ *
  * A call to a function called `isLocalUrl`, `isValidRedirect`, or similar, which is
  * considered a barrier guard for sanitizing untrusted URLs.
  */
-class RedirectCheckBarrierGuard extends DataFlow::BarrierGuard, DataFlow::CallNode {
+deprecated class RedirectCheckBarrierGuard extends DataFlow::BarrierGuard, DataFlow::CallNode {
   RedirectCheckBarrierGuard() {
     this.getCalleeName().regexpMatch("(?i)(is_?)?(local_?url|valid_?redir(ect)?)(ur[li])?")
   }

go/ql/lib/semmle/go/dataflow/barrierguardutil/RegexpCheck.qll

@@ -10,7 +10,7 @@ import go
  * 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 `RegexpCheck`.
+ * make the type recursive. Otherwise use `RegexpCheckBarrier`.
  */
 predicate regexpFunctionChecksExpr(DataFlow::Node resultNode, Expr checked, boolean branch) {
   exists(RegexpMatchFunction matchfn, DataFlow::CallNode call |
@@ -26,7 +26,20 @@ predicate regexpFunctionChecksExpr(DataFlow::Node resultNode, Expr checked, bool
  *
  * This is overapproximate: we do not attempt to reason about the correctness of the regexp.
  */
-class RegexpCheck extends DataFlow::BarrierGuard {
+class RegexpCheckBarrier extends DataFlow::Node {
+  RegexpCheckBarrier() {
+    this = DataFlow::BarrierGuard<regexpFunctionChecksExpr/3>::getABarrierNode()
+  }
+}
+
+/**
+ * DEPRECATED: Use `RegexpCheckBarrier` instead.
+ *
+ * A call to a regexp match function, considered as a barrier guard for sanitizing untrusted URLs.
+ *
+ * This is overapproximate: we do not attempt to reason about the correctness of the regexp.
+ */
+deprecated class RegexpCheck extends DataFlow::BarrierGuard {
   RegexpCheck() { regexpFunctionChecksExpr(this, _, _) }
 
   override predicate checks(Expr e, boolean branch) { regexpFunctionChecksExpr(this, e, branch) }

go/ql/lib/semmle/go/dataflow/barrierguardutil/UrlCheck.qll

@@ -4,14 +4,44 @@
 
 import go
 
+private predicate urlCheck(DataFlow::Node g, Expr e, boolean outcome) {
+  exists(DataFlow::Node url, DataFlow::EqualityTestNode eq |
+    g = eq and
+    exists(eq.getAnOperand().getStringValue()) and
+    (
+      url = eq.getAnOperand()
+      or
+      exists(DataFlow::MethodCallNode mc | mc = eq.getAnOperand() |
+        mc.getTarget().getName() = "Hostname" and
+        url = mc.getReceiver()
+      )
+    ) and
+    e = url.asExpr() and
+    outcome = eq.getPolarity()
+  )
+}
+
 /**
  * An equality check comparing a data-flow node against a constant string, considered as
  * a barrier guard for sanitizing untrusted URLs.
  *
  * Additionally, a check comparing `url.Hostname()` against a constant string is also
  * considered a barrier guard for `url`.
  */
-class UrlCheck extends DataFlow::BarrierGuard, DataFlow::EqualityTestNode {
+class UrlCheckBarrier extends DataFlow::Node {
+  UrlCheckBarrier() { this = DataFlow::BarrierGuard<urlCheck/3>::getABarrierNode() }
+}
+
+/**
+ * DEPRECATED: Use `UrlCheckBarrier` instead.
+ *
+ * An equality check comparing a data-flow node against a constant string, considered as
+ * a barrier guard for sanitizing untrusted URLs.
+ *
+ * Additionally, a check comparing `url.Hostname()` against a constant string is also
+ * considered a barrier guard for `url`.
+ */
+deprecated class UrlCheck extends DataFlow::BarrierGuard, DataFlow::EqualityTestNode {
   DataFlow::Node url;
 
   UrlCheck() {

go/ql/lib/semmle/go/dataflow/internal/DataFlowUtil.qll

@@ -231,27 +231,39 @@ class SyntheticFieldContent extends Content, TSyntheticFieldContent {
 }
 
 /**
- * A guard that validates some expression.
+ * Holds if the guard `g` validates the expression `e` upon evaluating to `branch`.
  *
- * To use this in a configuration, extend the class and provide a
- * characteristic predicate precisely specifying the guard, and override
- * `checks` to specify what is being validated and in which branch.
+ * The expression `e` is expected to be a syntactic part of the guard `g`.
+ * For example, the guard `g` might be a call `isSafe(x)` and the expression `e`
+ * the argument `x`.
+ */
+signature predicate guardChecksSig(Node g, Expr e, boolean branch);
+
+/**
+ * Provides a set of barrier nodes for a guard that validates an expression.
  *
- * When using a data-flow or taint-flow configuration `cfg`, it is important
- * that any classes extending BarrierGuard in scope which are not used in `cfg`
- * are disjoint from any classes extending BarrierGuard in scope which are used
- * in `cfg`.
+ * This is expected to be used in `isBarrier`/`isSanitizer` definitions
+ * in data flow and taint tracking.
  */
-abstract class BarrierGuard extends Node {
-  /** Holds if this guard validates `e` upon evaluating to `branch`. */
-  abstract predicate checks(Expr e, boolean branch);
+module BarrierGuard<guardChecksSig/3 guardChecks> {
+  /** Gets a node that is safely guarded by the given guard check. */
+  Node getABarrierNode() {
+    exists(Node g, ControlFlow::ConditionGuardNode guard, Node nd, SsaWithFields var |
+      result = var.getAUse()
+    |
+      guards(g, guard, nd, var) and
+      guard.dominates(result.getBasicBlock())
+    )
+  }
 
-  /** Gets a node guarded by this guard. */
-  final Node getAGuardedNode() {
+  /**
+   * Gets a node that is safely guarded by the given guard check.
+   */
+  Node getABarrierNodeForGuard(Node guardCheck) {
     exists(ControlFlow::ConditionGuardNode guard, Node nd, SsaWithFields var |
       result = var.getAUse()
     |
-      this.guards(guard, nd, var) and
+      guards(guardCheck, guard, nd, var) and
       guard.dominates(result.getBasicBlock())
     )
   }
@@ -263,36 +275,36 @@ abstract class BarrierGuard extends Node {
    * This predicate exists to enforce a good join order in `getAGuardedNode`.
    */
   pragma[noinline]
-  private predicate guards(ControlFlow::ConditionGuardNode guard, Node nd, SsaWithFields ap) {
-    this.guards(guard, nd) and nd = ap.getAUse()
+  private predicate guards(Node g, ControlFlow::ConditionGuardNode guard, Node nd, SsaWithFields ap) {
+    guards(g, guard, nd) and nd = ap.getAUse()
   }
 
   /**
    * Holds if `guard` markes a point in the control-flow graph where this node
    * is known to validate `nd`.
    */
-  private predicate guards(ControlFlow::ConditionGuardNode guard, Node nd) {
+  private predicate guards(Node g, ControlFlow::ConditionGuardNode guard, Node nd) {
     exists(boolean branch |
-      this.checks(nd.asExpr(), branch) and
-      guard.ensures(this, branch)
+      guardChecks(g, nd.asExpr(), branch) and
+      guard.ensures(g, branch)
     )
     or
     exists(
       Function f, FunctionInput inp, FunctionOutput outp, DataFlow::Property p, CallNode c,
       Node resNode, Node check, boolean outcome
     |
-      this.guardingCall(f, inp, outp, p, c, nd, resNode) and
+      guardingCall(g, f, inp, outp, p, c, nd, resNode) and
       p.checkOn(check, outcome, resNode) and
       guard.ensures(pragma[only_bind_into](check), outcome)
     )
   }
 
   pragma[noinline]
   private predicate guardingCall(
-    Function f, FunctionInput inp, FunctionOutput outp, DataFlow::Property p, CallNode c, Node nd,
-    Node resNode
+    Node g, Function f, FunctionInput inp, FunctionOutput outp, DataFlow::Property p, CallNode c,
+    Node nd, Node resNode
   ) {
-    this.guardingFunction(f, inp, outp, p) and
+    guardingFunction(g, f, inp, outp, p) and
     c = f.getACall() and
     nd = inp.getNode(c) and
     localFlow(pragma[only_bind_out](outp.getNode(c)), resNode)
@@ -308,7 +320,7 @@ abstract class BarrierGuard extends Node {
    * `false`, `nil` or a non-`nil` value.)
    */
   private predicate guardingFunction(
-    Function f, FunctionInput inp, FunctionOutput outp, DataFlow::Property p
+    Node g, Function f, FunctionInput inp, FunctionOutput outp, DataFlow::Property p
   ) {
     exists(FuncDecl fd, Node arg, Node ret |
       fd.getFunction() = f and
@@ -317,7 +329,7 @@ abstract class BarrierGuard extends Node {
       (
         // Case: a function like "if someBarrierGuard(arg) { return true } else { return false }"
         exists(ControlFlow::ConditionGuardNode guard |
-          this.guards(guard, arg) and
+          guards(g, guard, arg) and
           guard.dominates(ret.getBasicBlock())
         |
           exists(boolean b |
@@ -336,12 +348,12 @@ abstract class BarrierGuard extends Node {
         // or "return !someBarrierGuard(arg) && otherCond(...)"
         exists(boolean outcome |
           ret = getUniqueOutputNode(fd, outp) and
-          this.checks(arg.asExpr(), outcome) and
+          guardChecks(g, arg.asExpr(), outcome) and
           // This predicate's contract is (p holds of ret ==> arg is checked),
           // (and we have (this has outcome ==> arg is checked))
           // but p.checkOn(ret, outcome, this) gives us (ret has outcome ==> p holds of this),
           // so we need to swap outcome and (specifically boolean) p:
-          DataFlow::booleanProperty(outcome).checkOn(ret, p.asBoolean(), this)
+          DataFlow::booleanProperty(outcome).checkOn(ret, p.asBoolean(), g)
         )
         or
         // Case: a function like "return guardProxy(arg)"
@@ -351,7 +363,7 @@ abstract class BarrierGuard extends Node {
           DataFlow::Property outpProp
         |
           ret = getUniqueOutputNode(fd, outp) and
-          this.guardingFunction(f2, inp2, outp2, outpProp) and
+          guardingFunction(g, f2, inp2, outp2, outpProp) and
           c = f2.getACall() and
           arg = inp2.getNode(c) and
           (
@@ -368,6 +380,34 @@ abstract class BarrierGuard extends Node {
   }
 }
 
+/**
+ * DEPRECATED: Use `BarrierGuard` module instead.
+ *
+ * A guard that validates some expression.
+ *
+ * To use this in a configuration, extend the class and provide a
+ * characteristic predicate precisely specifying the guard, and override
+ * `checks` to specify what is being validated and in which branch.
+ *
+ * When using a data-flow or taint-flow configuration `cfg`, it is important
+ * that any classes extending BarrierGuard in scope which are not used in `cfg`
+ * are disjoint from any classes extending BarrierGuard in scope which are used
+ * in `cfg`.
+ */
+abstract deprecated class BarrierGuard extends Node {
+  /** Holds if this guard validates `e` upon evaluating to `branch`. */
+  abstract predicate checks(Expr e, boolean branch);
+
+  /** Gets a node guarded by this guard. */
+  final Node getAGuardedNode() {
+    result = BarrierGuard<barrierGuardChecks/3>::getABarrierNodeForGuard(this)
+  }
+}
+
+deprecated private predicate barrierGuardChecks(Node g, Expr e, boolean branch) {
+  g.(BarrierGuard).checks(e, branch)
+}
+
 DataFlow::Node getUniqueOutputNode(FuncDecl fd, FunctionOutput outp) {
   result = unique(DataFlow::Node n | n = outp.getEntryNode(fd) | n)
 }

go/ql/lib/semmle/go/dataflow/internal/TaintTrackingUtil.qll

@@ -228,16 +228,22 @@ abstract class DefaultTaintSanitizer extends DataFlow::Node { }
 predicate defaultTaintSanitizer(DataFlow::Node node) { node instanceof DefaultTaintSanitizer }
 
 /**
+ * DEPRECATED: Use `DefaultTaintSanitizer` instead.
+ *
  * A sanitizer guard in all global taint flow configurations but not in local taint.
  */
-abstract class DefaultTaintSanitizerGuard extends DataFlow::BarrierGuard { }
+abstract deprecated class DefaultTaintSanitizerGuard extends DataFlow::BarrierGuard { }
 
-/**
- * Holds if `guard` should be a sanitizer guard in all global taint flow configurations
- * but not in local taint.
- */
-predicate defaultTaintSanitizerGuard(DataFlow::BarrierGuard guard) {
-  guard instanceof DefaultTaintSanitizerGuard
+private predicate equalityTestGuard(DataFlow::Node g, Expr e, boolean outcome) {
+  exists(DataFlow::EqualityTestNode eq, DataFlow::Node nonConstNode |
+    eq = g and
+    eq.getAnOperand().isConst() and
+    nonConstNode = eq.getAnOperand() and
+    not nonConstNode.isConst() and
+    not eq.getAnOperand() = Builtin::nil().getARead() and
+    e = nonConstNode.asExpr() and
+    outcome = eq.getPolarity()
+  )
 }
 
 /**
@@ -247,20 +253,8 @@ predicate defaultTaintSanitizerGuard(DataFlow::BarrierGuard guard) {
  * Note that comparisons to `nil` are excluded. This is needed for performance
  * reasons.
  */
-class EqualityTestGuard extends DefaultTaintSanitizerGuard, DataFlow::EqualityTestNode {
-  DataFlow::Node nonConstNode;
-
-  EqualityTestGuard() {
-    this.getAnOperand().isConst() and
-    nonConstNode = this.getAnOperand() and
-    not nonConstNode.isConst() and
-    not this.getAnOperand() = Builtin::nil().getARead()
-  }
-
-  override predicate checks(Expr e, boolean outcome) {
-    e = nonConstNode.asExpr() and
-    outcome = this.getPolarity()
-  }
+class EqualityTestBarrier extends DefaultTaintSanitizer {
+  EqualityTestBarrier() { this = DataFlow::BarrierGuard<equalityTestGuard/3>::getABarrierNode() }
 }
 
 /**
@@ -398,6 +392,16 @@ predicate inputIsConstantIfOutputHasProperty(
   )
 }
 
+private predicate listOfConstantsComparisonSanitizerGuard(DataFlow::Node g, Expr e, boolean outcome) {
+  exists(DataFlow::Node guardedExpr |
+    exists(DataFlow::Node outputNode, DataFlow::Property p |
+      inputIsConstantIfOutputHasProperty(guardedExpr, outputNode, p) and
+      p.checkOn(g, outcome, outputNode)
+    ) and
+    e = guardedExpr.asExpr()
+  )
+}
+
 /**
  * A comparison against a list of constants, acting as a sanitizer guard for
  * `guardedExpr` by restricting it to a known value.
@@ -406,18 +410,8 @@ predicate inputIsConstantIfOutputHasProperty(
  * it could equally look for a check for membership of a constant map or
  * constant array, which does not need to be in its own function.
  */
-class ListOfConstantsComparisonSanitizerGuard extends TaintTracking::DefaultTaintSanitizerGuard {
-  DataFlow::Node guardedExpr;
-  boolean outcome;
-
+class ListOfConstantsComparisonSanitizerGuard extends TaintTracking::DefaultTaintSanitizer {
   ListOfConstantsComparisonSanitizerGuard() {
-    exists(DataFlow::Node outputNode, DataFlow::Property p |
-      inputIsConstantIfOutputHasProperty(guardedExpr, outputNode, p) and
-      p.checkOn(this, outcome, outputNode)
-    )
-  }
-
-  override predicate checks(Expr e, boolean branch) {
-    e = guardedExpr.asExpr() and branch = outcome
+    this = DataFlow::BarrierGuard<listOfConstantsComparisonSanitizerGuard/3>::getABarrierNode()
   }
 }

go/ql/lib/semmle/go/security/AllocationSizeOverflow.qll

@@ -23,7 +23,7 @@ module AllocationSizeOverflow {
 
     override predicate isSink(DataFlow::Node nd) { nd = Builtin::len().getACall().getArgument(0) }
 
-    override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+    deprecated override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
       guard instanceof SanitizerGuard
     }
 
@@ -64,7 +64,7 @@ module AllocationSizeOverflow {
       )
     }
 
-    override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+    deprecated override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
       guard instanceof SanitizerGuard
     }