Merge pull request #185 from sauyon/open-redirect-fp1

OpenRedirect: treat assignments to Url.Path as a barrier

ql/src/semmle/go/controlflow/ControlFlowGraph.qll

@@ -57,6 +57,18 @@ module ControlFlow {
     /** Gets the basic block to which this node belongs. */
     BasicBlock getBasicBlock() { result.getANode() = this }
 
+    /** Holds if this node dominates `dominee` in the control-flow graph. */
+    pragma[inline]
+    predicate dominatesNode(ControlFlow::Node dominee) {
+      exists(ReachableBasicBlock thisbb, ReachableBasicBlock dbb, int i, int j |
+        this = thisbb.getNode(i) and dominee = dbb.getNode(j)
+      |
+        thisbb.strictlyDominates(dbb)
+        or
+        thisbb = dbb and i <= j
+      )
+    }
+
     /** Gets the innermost function or file to which this node belongs. */
     Root getRoot() { none() }
 

ql/src/semmle/go/dataflow/SSA.qll

@@ -288,3 +288,67 @@ class SsaPhiNode extends SsaPseudoDefinition, TPhi {
     getBasicBlock().hasLocationInfo(filepath, startline, startcolumn, _, _)
   }
 }
+
+/**
+ * An SSA variable, possibly with a chain of field reads on it.
+ */
+private newtype TSsaWithFields =
+  TRoot(SsaVariable v) or
+  TStep(SsaWithFields base, Field f) { exists(accessPathAux(base, f)) }
+
+/**
+ * Gets a representation of `nd` as an ssa-with-fields value if there is one.
+ */
+private TSsaWithFields accessPath(IR::Instruction insn) {
+  exists(SsaVariable v | insn = v.getAUse() | result = TRoot(v))
+  or
+  exists(SsaWithFields base, Field f | insn = accessPathAux(base, f) | result = TStep(base, f))
+}
+
+/**
+ * Gets a data-flow node that reads a field `f` from a node that is represented
+ * by ssa-with-fields value `base`.
+ */
+private IR::Instruction accessPathAux(TSsaWithFields base, Field f) {
+  exists(IR::FieldReadInstruction fr | fr = result |
+    base = accessPath(fr.getBase()) and
+    f = fr.getField()
+  )
+}
+
+class SsaWithFields extends TSsaWithFields {
+  /**
+   * Gets the SSA variable corresponding to the base of this SSA variable with fields.
+   *
+   * For example, the SSA variable corresponding to `a` for the SSA variable with fields
+   * corresponding to `a.b`.
+   */
+  SsaVariable getBaseVariable() {
+    this = TRoot(result)
+    or
+    exists(SsaWithFields base, Field f | this = TStep(base, f) | result = base.getBaseVariable())
+  }
+
+  /** Gets a use that refers to this SSA variable with fields. */
+  DataFlow::Node getAUse() { this = accessPath(result.asInstruction()) }
+
+  /** Gets a textual representation of this element. */
+  string toString() {
+    exists(SsaVariable var | this = TRoot(var) | result = "(" + var + ")")
+    or
+    exists(SsaWithFields base, Field f | this = TStep(base, f) | result = base + "." + f.getName())
+  }
+
+  /**
+   * Holds if this element is at the specified location.
+   * The location spans column `startcolumn` of line `startline` to
+   * column `endcolumn` of line `endline` in file `filepath`.
+   * For more information, see
+   * [Locations](https://help.semmle.com/QL/learn-ql/ql/locations.html).
+   */
+  predicate hasLocationInfo(
+    string filepath, int startline, int startcolumn, int endline, int endcolumn
+  ) {
+    this.getBaseVariable().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+  }
+}

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

@@ -733,33 +733,6 @@ predicate localFlowStep(Node nodeFrom, Node nodeTo) {
  */
 predicate localFlow(Node source, Node sink) { localFlowStep*(source, sink) }
 
-/**
- * An SSA variable, possibly with a chain of field reads on it.
- */
-private newtype SsaWithFields =
-  Root(SsaVariable v) or
-  Step(SsaWithFields base, Field f) { exists(accessPathAux(base, f)) }
-
-/**
- * Gets a representation of `nd` as an ssa-with-fields value if there is one.
- */
-private SsaWithFields accessPath(Node nd) {
-  exists(SsaVariable v | nd.asInstruction() = v.getAUse() | result = Root(v))
-  or
-  exists(SsaWithFields base, Field f | nd = accessPathAux(base, f) | result = Step(base, f))
-}
-
-/**
- * Gets a data-flow node that reads a field `f` from a node that is represented
- * by ssa-with-fields value `base`.
- */
-private Node accessPathAux(SsaWithFields base, Field f) {
-  exists(IR::FieldReadInstruction fr | fr = result.asInstruction() |
-    base = accessPath(instructionNode(fr.getBase())) and
-    f = fr.getField()
-  )
-}
-
 /**
  * A guard that validates some expression.
  *
@@ -775,8 +748,10 @@ abstract class BarrierGuard extends Node {
 
   /** Gets a node guarded by this guard. */
   final Node getAGuardedNode() {
-    exists(ControlFlow::ConditionGuardNode guard, Node nd |
-      guards(guard, nd, accessPath(result)) and
+    exists(ControlFlow::ConditionGuardNode guard, Node nd, SsaWithFields var |
+      result = var.getAUse()
+    |
+      guards(guard, nd, var) and
       guard.dominates(result.asInstruction().getBasicBlock())
     )
   }
@@ -789,8 +764,7 @@ abstract class BarrierGuard extends Node {
    */
   pragma[noinline]
   private predicate guards(ControlFlow::ConditionGuardNode guard, Node nd, SsaWithFields ap) {
-    guards(guard, nd) and
-    ap = accessPath(nd)
+    guards(guard, nd) and nd = ap.getAUse()
   }
 
   /**

ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll

@@ -50,6 +50,25 @@ module OpenUrlRedirect {
     }
   }
 
+  /**
+   * An access to a variable that is preceded by an assignment to its `Path` field.
+   *
+   * This is overapproximate; this will currently remove flow through all `Url.Path` assignments
+   * which contain a substring that could sanitize data.
+   */
+  class PathAssignmentBarrier extends Barrier, Read {
+    PathAssignmentBarrier() {
+      exists(Write w, Field f, SsaWithFields var |
+        f.getName() = "Path" and
+        hasHostnameSanitizingSubstring(w.getRhs()) and
+        this = var.getAUse()
+      |
+        w.writesField(var.getAUse(), f, _) and
+        w.dominatesNode(insn)
+      )
+    }
+  }
+
   /**
    * A call to a function called `isLocalUrl` or similar, which is
    * considered a barrier for purposes of URL redirection.

ql/src/semmle/go/security/UrlConcatenation.qll

@@ -55,7 +55,7 @@ predicate sanitizingPrefixEdge(DataFlow::Node source, DataFlow::Node sink) {
  * In the latter two cases, the additional check is necessary to avoid a `/` that could be interpreted as
  * the `//` separating the (optional) scheme from the hostname.
  */
-private predicate hasHostnameSanitizingSubstring(DataFlow::Node nd) {
+predicate hasHostnameSanitizingSubstring(DataFlow::Node nd) {
   nd.getStringValue().regexpMatch(".*([?#]|[^?#:/\\\\][/\\\\]).*|[/\\\\][^/\\\\].*")
   or
   hasHostnameSanitizingSubstring(StringConcatenation::getAnOperand(nd))