Merge pull request github#3824 from asger-semmle/js/static-regexp-capture-group-step

Approved by erik-krogh, esbena

Author: semmle-qlci

javascript/ql/src/semmle/javascript/StandardLibrary.qll

@@ -121,6 +121,11 @@ class StringReplaceCall extends DataFlow::MethodCallNode {
    */
   DataFlow::Node getRawReplacement() { result = getArgument(1) }
 
+  /**
+   * Gets a function flowing into the second argument of this call to `replace`.
+   */
+  DataFlow::FunctionNode getReplacementCallback() { result = getCallback(1) }
+
   /**
    * Holds if this is a global replacement, that is, the first argument is a regular expression
    * with the `g` flag.

javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll

@@ -705,6 +705,74 @@ module TaintTracking {
     }
   }
 
+  private module RegExpCaptureSteps {
+    /** Gets a reference to a string derived from the most recent RegExp match, such as `RegExp.$1`. */
+    private DataFlow::PropRead getAStaticCaptureRef() {
+      result =
+        DataFlow::globalVarRef("RegExp")
+            .getAPropertyRead(["$" + [1 .. 9], "input", "lastMatch", "leftContext", "rightContext",
+                  "$&", "$^", "$`"])
+    }
+
+    /**
+     * Gets a control-flow node where `input` is used in a RegExp match.
+     */
+    private ControlFlowNode getACaptureSetter(DataFlow::Node input) {
+      exists(DataFlow::MethodCallNode call | result = call.asExpr() |
+        call.getMethodName() = ["search", "replace", "match"] and input = call.getReceiver()
+        or
+        call.getMethodName() = ["test", "exec"] and input = call.getArgument(0)
+      )
+    }
+
+    /**
+     * Gets a control-flow node that can locally reach the given static capture reference
+     * without passing through a capture setter.
+     *
+     * This is essentially an intraprocedural def-use analysis that ignores potential
+     * side effects from calls.
+     */
+    private ControlFlowNode getANodeReachingCaptureRef(DataFlow::PropRead read) {
+      result = read.asExpr() and
+      read = getAStaticCaptureRef()
+      or
+      exists(ControlFlowNode mid |
+        result = getANodeReachingCaptureRefAux(read, mid) and
+        not mid = getACaptureSetter(_)
+      )
+    }
+
+    pragma[nomagic]
+    private ControlFlowNode getANodeReachingCaptureRefAux(
+      DataFlow::PropRead read, ControlFlowNode mid
+    ) {
+      mid = getANodeReachingCaptureRef(read) and
+      result = mid.getAPredecessor()
+    }
+
+    /**
+     * Holds if there is a step `pred -> succ` from the input of a RegExp match to
+     * a static property of `RegExp`.
+     */
+    private predicate staticRegExpCaptureStep(DataFlow::Node pred, DataFlow::Node succ) {
+      getACaptureSetter(pred) = getANodeReachingCaptureRef(succ)
+      or
+      exists(StringReplaceCall replace |
+        getANodeReachingCaptureRef(succ) = replace.getReplacementCallback().getFunction().getEntry() and
+        pred = replace.getReceiver()
+      )
+    }
+
+    private class StaticRegExpCaptureStep extends AdditionalTaintStep {
+      StaticRegExpCaptureStep() { staticRegExpCaptureStep(this, _) }
+
+      override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+        pred = this and
+        staticRegExpCaptureStep(this, succ)
+      }
+    }
+  }
+
   /**
    * A conditional checking a tainted string against a regular expression, which is
    * considered to be a sanitizer for all configurations.

javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected

@@ -103,6 +103,14 @@ typeInferenceMismatch
 | spread.js:2:15:2:22 | source() | spread.js:5:8:5:43 | { f: 'h ... orld' } |
 | spread.js:2:15:2:22 | source() | spread.js:7:8:7:19 | [ ...taint ] |
 | spread.js:2:15:2:22 | source() | spread.js:8:8:8:28 | [ 1, 2, ... nt, 3 ] |
+| static-capture-groups.js:2:17:2:24 | source() | static-capture-groups.js:5:14:5:22 | RegExp.$1 |
+| static-capture-groups.js:2:17:2:24 | source() | static-capture-groups.js:15:14:15:22 | RegExp.$1 |
+| static-capture-groups.js:2:17:2:24 | source() | static-capture-groups.js:17:14:17:22 | RegExp.$1 |
+| static-capture-groups.js:2:17:2:24 | source() | static-capture-groups.js:21:14:21:22 | RegExp.$1 |
+| static-capture-groups.js:2:17:2:24 | source() | static-capture-groups.js:24:14:24:22 | RegExp.$1 |
+| static-capture-groups.js:2:17:2:24 | source() | static-capture-groups.js:27:14:27:22 | RegExp.$1 |
+| static-capture-groups.js:32:17:32:24 | source() | static-capture-groups.js:38:10:38:18 | RegExp.$1 |
+| static-capture-groups.js:42:12:42:19 | source() | static-capture-groups.js:43:14:43:22 | RegExp.$1 |
 | thisAssignments.js:4:17:4:24 | source() | thisAssignments.js:5:10:5:18 | obj.field |
 | thisAssignments.js:7:19:7:26 | source() | thisAssignments.js:8:10:8:20 | this.field2 |
 | tst.js:2:13:2:20 | source() | tst.js:4:10:4:10 | x |

javascript/ql/test/library-tests/TaintTracking/static-capture-groups.js

@@ -0,0 +1,45 @@
+function test(x) {
+    let taint = source();
+
+    if (/Hello (.*)/.exec(taint)) {
+        sink(RegExp.$1); // NOT OK
+    }
+
+    if (/Foo (.*)/.exec(x)) {
+        sink(RegExp.$1); // OK
+    } else {
+        sink(RegExp.$1); // NOT OK [INCONSISTENCY] - previous capture group remains
+    }
+
+    if (/Hello ([a-zA-Z]+)/.exec(taint)) {
+        sink(RegExp.$1); // OK [INCONSISTENCY] - capture group is sanitized
+    } else {
+        sink(RegExp.$1); // NOT OK [found but for the wrong reason] - original capture group possibly remains
+    }
+
+    if (/Hello (.*)/.exec(taint) && something()) {
+        sink(RegExp.$1); // NOT OK
+    }
+    if (something() && /Hello (.*)/.exec(taint)) {
+        sink(RegExp.$1); // NOT OK
+    }
+    if (/First (.*)/.exec(taint) || /Second (.*)/.exec(taint)) {
+        sink(RegExp.$1); // NOT OK
+    }
+}
+
+function test2(x) {
+    var taint = source();
+    if (something()) {
+        if (/Hello (.*)/.exec(taint)) {
+            something();
+        }
+    }
+    sink(RegExp.$1); // NOT OK
+}
+
+function replaceCallback() {
+    return source().replace(/(\w+)/, () => {
+        sink(RegExp.$1); // NOT OK
+    });
+}