Merge remote-tracking branch 'upstream/master' into models5

Author: geoffw0

cpp/ql/src/semmle/code/cpp/controlflow/IRGuards.qll

@@ -13,6 +13,7 @@ import semmle.code.cpp.ir.IR
  * has the AST for the `Function` itself, which tends to confuse mapping between the AST `BasicBlock`
  * and the `IRBlock`.
  */
+pragma[noinline]
 private predicate isUnreachedBlock(IRBlock block) {
   block.getFirstInstruction() instanceof UnreachedInstruction
 }
@@ -304,13 +305,13 @@ class IRGuardCondition extends Instruction {
     pred.getASuccessor() = succ and
     controls(pred, testIsTrue)
     or
-    hasBranchEdge(succ, testIsTrue) and
+    succ = getBranchSuccessor(testIsTrue) and
     branch.getCondition() = this and
     branch.getBlock() = pred
   }
 
   /**
-   * Holds if `branch` jumps directly to `succ` when this condition is `testIsTrue`.
+   * Gets the block to which `branch` jumps directly when this condition is `testIsTrue`.
    *
    * This predicate is intended to help with situations in which an inference can only be made
    * based on an edge between a block with multiple successors and a block with multiple
@@ -324,14 +325,14 @@ class IRGuardCondition extends Instruction {
    * return x;
    * ```
    */
-  private predicate hasBranchEdge(IRBlock succ, boolean testIsTrue) {
+  private IRBlock getBranchSuccessor(boolean testIsTrue) {
     branch.getCondition() = this and
     (
       testIsTrue = true and
-      succ.getFirstInstruction() = branch.getTrueSuccessor()
+      result.getFirstInstruction() = branch.getTrueSuccessor()
       or
       testIsTrue = false and
-      succ.getFirstInstruction() = branch.getFalseSuccessor()
+      result.getFirstInstruction() = branch.getFalseSuccessor()
     )
   }
 
@@ -405,20 +406,78 @@ class IRGuardCondition extends Instruction {
    */
   private predicate controlsBlock(IRBlock controlled, boolean testIsTrue) {
     not isUnreachedBlock(controlled) and
-    exists(IRBlock branchBlock | branchBlock.getAnInstruction() = branch |
-      exists(IRBlock succ |
-        testIsTrue = true and succ.getFirstInstruction() = branch.getTrueSuccessor()
+    //
+    // For this block to control the block `controlled` with `testIsTrue` the
+    // following must hold: Execution must have passed through the test; that
+    // is, `this` must strictly dominate `controlled`. Execution must have
+    // passed through the `testIsTrue` edge leaving `this`.
+    //
+    // Although "passed through the true edge" implies that
+    // `getBranchSuccessor(true)` dominates `controlled`, the reverse is not
+    // true, as flow may have passed through another edge to get to
+    // `getBranchSuccessor(true)`, so we need to assert that
+    // `getBranchSuccessor(true)` dominates `controlled` *and* that all
+    // predecessors of `getBranchSuccessor(true)` are either `this` or
+    // dominated by `getBranchSuccessor(true)`.
+    //
+    // For example, in the following snippet:
+    //
+    //     if (x)
+    //       controlled;
+    //     false_successor;
+    //     uncontrolled;
+    //
+    // `false_successor` dominates `uncontrolled`, but not all of its
+    // predecessors are `this` (`if (x)`) or dominated by itself. Whereas in
+    // the following code:
+    //
+    //     if (x)
+    //       while (controlled)
+    //         also_controlled;
+    //     false_successor;
+    //     uncontrolled;
+    //
+    // the block `while (controlled)` is controlled because all of its
+    // predecessors are `this` (`if (x)`) or (in the case of `also_controlled`)
+    // dominated by itself.
+    //
+    // The additional constraint on the predecessors of the test successor implies
+    // that `this` strictly dominates `controlled` so that isn't necessary to check
+    // directly.
+    exists(IRBlock succ |
+      succ = this.getBranchSuccessor(testIsTrue) and
+      this.hasDominatingEdgeTo(succ) and
+      succ.dominates(controlled)
+    )
+  }
+
+  /**
+   * Holds if `(this, succ)` is an edge that dominates `succ`, that is, all other
+   * predecessors of `succ` are dominated by `succ`. This implies that `this` is the
+   * immediate dominator of `succ`.
+   *
+   * This is a necessary and sufficient condition for an edge to dominate anything,
+   * and in particular `bb1.hasDominatingEdgeTo(bb2) and bb2.dominates(bb3)` means
+   * that the edge `(bb1, bb2)` dominates `bb3`.
+   */
+  private predicate hasDominatingEdgeTo(IRBlock succ) {
+    exists(IRBlock branchBlock | branchBlock = this.getBranchBlock() |
+      branchBlock.immediatelyDominates(succ) and
+      branchBlock.getASuccessor() = succ and
+      forall(IRBlock pred | pred = succ.getAPredecessor() and pred != branchBlock |
+        succ.dominates(pred)
         or
-        testIsTrue = false and succ.getFirstInstruction() = branch.getFalseSuccessor()
-      |
-        branch.getCondition() = this and
-        succ.dominates(controlled) and
-        forall(IRBlock pred | pred.getASuccessor() = succ |
-          pred = branchBlock or succ.dominates(pred) or not pred.isReachableFromFunctionEntry()
-        )
+        // An unreachable `pred` is vacuously dominated by `succ` since all
+        // paths from the entry to `pred` go through `succ`. Such vacuous
+        // dominance is not included in the `dominates` predicate since that
+        // could cause quadratic blow-up.
+        not pred.isReachableFromFunctionEntry()
       )
     )
   }
+
+  pragma[noinline]
+  private IRBlock getBranchBlock() { result = branch.getBlock() }
 }
 
 private ConditionalBranchInstruction get_branch_for_condition(Instruction guard) {

python/ql/src/semmle/python/regex.qll

@@ -28,7 +28,8 @@ predicate used_as_regex(Expr s, string mode) {
     /* Call to re.xxx(regex, ... [mode]) */
     exists(CallNode call, string name |
         call.getArg(0).refersTo(_, _, s.getAFlowNode()) and
-        call.getFunction().pointsTo(Module::named("re").attr(name))
+        call.getFunction().pointsTo(Module::named("re").attr(name)) and
+        not name = "escape"
     |
         mode = "None"
         or

python/ql/test/library-tests/regex/test.py

@@ -62,3 +62,7 @@
 re.compile(r"\[(?P<txt>[^[]*)\]\((?P<uri>[^)]*)")
 
 re.compile("", re.M) # ODASA-8056
+
+# FP reported in https://github.com/github/codeql/issues/3712
+# This does not define a regex (but could be used by other code to do so)
+escaped = re.escape("https://www.humblebundle.com/home/library")

python/ql/test/query-tests/Security/CWE-020/hosttest.py

@@ -17,3 +17,7 @@ def safe(request):
     target = request.args.get('target', '')
     if SAFE_REGEX.match(target):
         return redirect(target)
+
+# FP reported in https://github.com/github/codeql/issues/3712
+# This does not define a regex (but could be used by other code to do so)
+escaped = re.escape("https://www.humblebundle.com/home/library")