Merge pull request github#5396 from asgerf/js/shared-taint-step

Approved by erik-krogh, esbena

Author: codeql-ci

docs/codeql/codeql-language-guides/analyzing-data-flow-in-javascript-and-typescript.rst

@@ -439,23 +439,24 @@ additional taint step from the first argument of ``resolveSymlinks`` to its resu
   }
 
 We might even consider adding this as a default taint step to be used by all taint-tracking configurations. In order to do this, we need
-to wrap it in a new subclass of ``TaintTracking::AdditionalTaintStep`` like this:
+to wrap it in a new subclass of ``TaintTracking::SharedTaintStep`` like this:
 
 .. code-block:: ql
 
-  class StepThroughResolveSymlinks extends TaintTracking::AdditionalTaintStep, DataFlow::CallNode {
-    StepThroughResolveSymlinks() { this = DataFlow::moduleImport("resolve-symlinks").getACall() }
-
+  class StepThroughResolveSymlinks extends TaintTracking::SharedTaintStep {
     override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-      pred = this.getArgument(0) and
-      succ = this
+      exists(DataFlow::CallNode c |
+        c = DataFlow::moduleImport("resolve-symlinks").getACall() and
+        pred = c.getArgument(0) and
+        succ = c
+      )
     }
   }
 
 If we add this definition to the standard library, it will be picked up by all taint-tracking configurations. Obviously, one has to be
 careful when adding such new additional taint steps to ensure that they really make sense for `all` configurations.
 
-Analogous to ``TaintTracking::AdditionalTaintStep``, there is also a class ``DataFlow::AdditionalFlowStep`` that can be extended to add
+Analogous to ``TaintTracking::SharedTaintStep``, there is also a class ``DataFlow::SharedFlowStep`` that can be extended to add
 extra steps to all data-flow configurations, and hence also to all taint-tracking configurations.
 
 Exercises

javascript/ql/src/Security/CWE-094/ImproperCodeSanitization.ql

@@ -27,7 +27,7 @@ private DataFlow::Node remoteFlow(DataFlow::TypeTracker t) {
   exists(DataFlow::TypeTracker t2, DataFlow::Node prev | prev = remoteFlow(t2) |
     t2 = t.smallstep(prev, result)
     or
-    any(TaintTracking::AdditionalTaintStep dts).step(prev, result) and
+    TaintTracking::sharedTaintStep(prev, result) and
     t = t2
   )
 }

javascript/ql/src/experimental/semmle/javascript/security/dataflow/ResourceExhaustion.qll

@@ -37,7 +37,7 @@ module ResourceExhaustion {
   }
 
   predicate isRestrictedAdditionalTaintStep(DataFlow::Node src, DataFlow::Node dst) {
-    any(TaintTracking::AdditionalTaintStep dts).step(src, dst) and
+    TaintTracking::sharedTaintStep(src, dst) and
     not dst.asExpr() instanceof AddExpr and
     not dst.(DataFlow::MethodCallNode).calls(src, "toString")
   }

javascript/ql/src/meta/analysis-quality/TaintSteps.ql

@@ -13,7 +13,7 @@ import CallGraphQuality
 
 predicate relevantStep(DataFlow::Node pred, DataFlow::Node succ) {
   (
-    any(TaintTracking::AdditionalTaintStep dts).step(pred, succ)
+    TaintTracking::sharedTaintStep(pred, succ)
     or
     any(DataFlow::AdditionalFlowStep cfg).step(pred, succ)
     or

javascript/ql/src/semmle/javascript/Arrays.qll

@@ -9,12 +9,9 @@ module ArrayTaintTracking {
   /**
    * A taint propagating data flow edge caused by the builtin array functions.
    */
-  private class ArrayFunctionTaintStep extends TaintTracking::AdditionalTaintStep,
-    DataFlow::CallNode {
-    ArrayFunctionTaintStep() { arrayFunctionTaintStep(_, _, this) }
-
-    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-      arrayFunctionTaintStep(pred, succ, this)
+  private class ArrayFunctionTaintStep extends TaintTracking::SharedTaintStep {
+    override predicate arrayStep(DataFlow::Node pred, DataFlow::Node succ) {
+      arrayFunctionTaintStep(pred, succ, _)
     }
   }
 

javascript/ql/src/semmle/javascript/Base64.qll

@@ -67,14 +67,12 @@ module Base64 {
    * Note that we currently do not model base64 encoding as a taint-propagating data flow edge
    * to avoid false positives.
    */
-  private class Base64DecodingStep extends TaintTracking::AdditionalTaintStep {
-    Decode dec;
-
-    Base64DecodingStep() { this = dec }
-
+  private class Base64DecodingStep extends TaintTracking::SharedTaintStep {
     override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-      pred = dec.getInput() and
-      succ = dec.getOutput()
+      exists(Decode dec |
+        pred = dec.getInput() and
+        succ = dec.getOutput()
+      )
     }
   }
 }

javascript/ql/src/semmle/javascript/Extend.qll

@@ -165,14 +165,12 @@ private class FunctionalExtendCallShallow extends ExtendCall {
  * A taint propagating data flow edge from the objects flowing into an extend call to its return value
  * and to the source of the destination object.
  */
-private class ExtendCallTaintStep extends TaintTracking::AdditionalTaintStep {
-  ExtendCall extend;
-
-  ExtendCallTaintStep() { this = extend }
-
+private class ExtendCallTaintStep extends TaintTracking::SharedTaintStep {
   override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-    pred = extend.getASourceOperand() and succ = extend.getDestinationOperand().getALocalSource()
-    or
-    pred = extend.getAnOperand() and succ = extend
+    exists(ExtendCall extend |
+      pred = extend.getASourceOperand() and succ = extend.getDestinationOperand().getALocalSource()
+      or
+      pred = extend.getAnOperand() and succ = extend
+    )
   }
 }

javascript/ql/src/semmle/javascript/Promises.qll

@@ -398,74 +398,65 @@ module PromiseFlow {
 }
 
 /**
- * Holds if taint propagates from `pred` to `succ` through promises.
+ * DEPRECATED. Use `TaintTracking::promiseStep` instead.
  */
-predicate promiseTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
-  // from `x` to `new Promise((res, rej) => res(x))`
-  pred = succ.(PromiseDefinition).getResolveParameter().getACall().getArgument(0)
-  or
-  // from `x` to `Promise.resolve(x)`
-  pred = succ.(PromiseCreationCall).getValue() and
-  not succ instanceof PromiseAllCreation
-  or
-  // from `arr` to `Promise.all(arr)`
-  pred = succ.(PromiseAllCreation).getArrayNode()
-  or
-  exists(DataFlow::MethodCallNode thn | thn.getMethodName() = "then" |
-    // from `p` to `x` in `p.then(x => ...)`
-    pred = thn.getReceiver() and
-    succ = thn.getCallback(0).getParameter(0)
+deprecated predicate promiseTaintStep = TaintTracking::promiseStep/2;
+
+private class PromiseTaintStep extends TaintTracking::SharedTaintStep {
+  override predicate promiseStep(DataFlow::Node pred, DataFlow::Node succ) {
+    // from `x` to `new Promise((res, rej) => res(x))`
+    pred = succ.(PromiseDefinition).getResolveParameter().getACall().getArgument(0)
     or
-    // from `v` to `p.then(x => return v)`
-    pred = thn.getCallback([0 .. 1]).getAReturn() and
-    succ = thn
-  )
-  or
-  exists(DataFlow::MethodCallNode catch | catch.getMethodName() = "catch" |
-    // from `p` to `p.catch(..)`
-    pred = catch.getReceiver() and
-    succ = catch
+    // from `x` to `Promise.resolve(x)`
+    pred = succ.(PromiseCreationCall).getValue() and
+    not succ instanceof PromiseAllCreation
     or
-    // from `v` to `p.catch(x => return v)`
-    pred = catch.getCallback(0).getAReturn() and
-    succ = catch
-  )
-  or
-  // from `p` to `p.finally(..)`
-  exists(DataFlow::MethodCallNode finally | finally.getMethodName() = "finally" |
-    pred = finally.getReceiver() and
-    succ = finally
-  )
-  or
-  // from `x` to `await x`
-  exists(AwaitExpr await |
-    pred.getEnclosingExpr() = await.getOperand() and
-    succ.getEnclosingExpr() = await
-  )
-  or
-  exists(DataFlow::CallNode mapSeries |
-    mapSeries = DataFlow::moduleMember("bluebird", "mapSeries").getACall()
-  |
-    // from `xs` to `x` in `require("bluebird").mapSeries(xs, (x) => {...})`.
-    pred = mapSeries.getArgument(0) and
-    succ = mapSeries.getABoundCallbackParameter(1, 0)
+    // from `arr` to `Promise.all(arr)`
+    pred = succ.(PromiseAllCreation).getArrayNode()
     or
-    // from `y` to `require("bluebird").mapSeries(x, x => y)`.
-    pred = mapSeries.getCallback(1).getAReturn() and
-    succ = mapSeries
-  )
-}
-
-/**
- * An additional taint step that involves promises.
- */
-private class PromiseTaintStep extends TaintTracking::AdditionalTaintStep {
-  DataFlow::Node source;
-
-  PromiseTaintStep() { promiseTaintStep(source, this) }
-
-  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-    pred = source and succ = this
+    exists(DataFlow::MethodCallNode thn | thn.getMethodName() = "then" |
+      // from `p` to `x` in `p.then(x => ...)`
+      pred = thn.getReceiver() and
+      succ = thn.getCallback(0).getParameter(0)
+      or
+      // from `v` to `p.then(x => return v)`
+      pred = thn.getCallback([0 .. 1]).getAReturn() and
+      succ = thn
+    )
+    or
+    exists(DataFlow::MethodCallNode catch | catch.getMethodName() = "catch" |
+      // from `p` to `p.catch(..)`
+      pred = catch.getReceiver() and
+      succ = catch
+      or
+      // from `v` to `p.catch(x => return v)`
+      pred = catch.getCallback(0).getAReturn() and
+      succ = catch
+    )
+    or
+    // from `p` to `p.finally(..)`
+    exists(DataFlow::MethodCallNode finally | finally.getMethodName() = "finally" |
+      pred = finally.getReceiver() and
+      succ = finally
+    )
+    or
+    // from `x` to `await x`
+    exists(AwaitExpr await |
+      pred.getEnclosingExpr() = await.getOperand() and
+      succ.getEnclosingExpr() = await
+    )
+    or
+    exists(DataFlow::CallNode mapSeries |
+      mapSeries = DataFlow::moduleMember("bluebird", "mapSeries").getACall()
+    |
+      // from `xs` to `x` in `require("bluebird").mapSeries(xs, (x) => {...})`.
+      pred = mapSeries.getArgument(0) and
+      succ = mapSeries.getABoundCallbackParameter(1, 0)
+      or
+      // from `y` to `require("bluebird").mapSeries(x, x => y)`.
+      pred = mapSeries.getCallback(1).getAReturn() and
+      succ = mapSeries
+    )
   }
 }
 
@@ -500,14 +491,13 @@ private module AsyncReturnSteps {
   /**
    * A data-flow step for ordinary return from an async function in a taint configuration.
    */
-  private class AsyncTaintReturn extends TaintTracking::AdditionalTaintStep, DataFlow::FunctionNode {
-    Function f;
-
-    AsyncTaintReturn() { this.getFunction() = f and f.isAsync() }
-
+  private class AsyncTaintReturn extends TaintTracking::SharedTaintStep {
     override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-      returnExpr(f, pred, _) and
-      succ.(DataFlow::FunctionReturnNode).getFunction() = f
+      exists(Function f |
+        f.isAsync() and
+        returnExpr(f, pred, _) and
+        succ.(DataFlow::FunctionReturnNode).getFunction() = f
+      )
     }
   }
 }
@@ -616,14 +606,12 @@ private module ClosurePromise {
   /**
    * Taint steps through closure promise methods.
    */
-  private class ClosurePromiseTaintStep extends TaintTracking::AdditionalTaintStep {
-    DataFlow::Node pred;
-
-    ClosurePromiseTaintStep() {
+  private class ClosurePromiseTaintStep extends TaintTracking::SharedTaintStep {
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
       // static methods in goog.Promise
       exists(DataFlow::CallNode call, string name |
         call = Closure::moduleImport("goog.Promise." + name).getACall() and
-        this = call and
+        succ = call and
         pred = call.getAnArgument()
       |
         name = "all" or
@@ -635,12 +623,10 @@ private module ClosurePromise {
       // promise created through goog.promise.withResolver()
       exists(DataFlow::CallNode resolver |
         resolver = Closure::moduleImport("goog.Promise.withResolver").getACall() and
-        this = resolver.getAPropertyRead("promise") and
+        succ = resolver.getAPropertyRead("promise") and
         pred = resolver.getAMethodCall("resolve").getArgument(0)
       )
     }
-
-    override predicate step(DataFlow::Node src, DataFlow::Node dst) { src = pred and dst = this }
   }
 }
 

javascript/ql/src/semmle/javascript/Regexp.qll

@@ -904,7 +904,7 @@ private DataFlow::Node regExpSource(DataFlow::Node re, DataFlow::TypeBackTracker
   exists(DataFlow::TypeBackTracker t2, DataFlow::Node succ | succ = regExpSource(re, t2) |
     t2 = t.smallstep(result, succ)
     or
-    any(TaintTracking::AdditionalTaintStep dts).step(result, succ) and
+    TaintTracking::sharedTaintStep(result, succ) and
     t = t2
   )
 }

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

@@ -72,6 +72,7 @@ private import javascript
 private import internal.FlowSteps
 private import internal.AccessPaths
 private import internal.CallGraphs
+private import internal.Unit
 private import semmle.javascript.internal.CachedStages
 
 /**
@@ -609,6 +610,57 @@ abstract class AdditionalFlowStep extends DataFlow::Node {
   }
 }
 
+/**
+ * A data flow edge that should be added to all data flow configurations in
+ * addition to standard data flow edges.
+ *
+ * This class is a singleton, and thus subclasses do not need to specify a characteristic predicate.
+ *
+ * Note: For performance reasons, all subclasses of this class should be part
+ * of the standard library. Override `Configuration::isAdditionalFlowStep`
+ * for analysis-specific flow steps.
+ */
+class SharedFlowStep extends Unit {
+  /**
+   * Holds if `pred` → `succ` should be considered a data flow edge.
+   */
+  predicate step(DataFlow::Node pred, DataFlow::Node succ) { none() }
+
+  /**
+   * Holds if `pred` → `succ` should be considered a data flow edge
+   * transforming values with label `predlbl` to have label `succlbl`.
+   */
+  predicate step(
+    DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel predlbl,
+    DataFlow::FlowLabel succlbl
+  ) {
+    none()
+  }
+}
+
+/**
+ * Contributes subclasses of `SharedFlowStep` to `AdditionalFlowStep`.
+ *
+ * This is a placeholder until we migrate to the `SharedFlowStep` class and deprecate `AdditionalFlowStep`.
+ */
+private class SharedStepAsAdditionalFlowStep extends AdditionalFlowStep {
+  SharedStepAsAdditionalFlowStep() {
+    any(SharedFlowStep st).step(_, this) or
+    any(SharedFlowStep st).step(_, this, _, _)
+  }
+
+  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+    any(SharedFlowStep st).step(pred, succ) and succ = this
+  }
+
+  override predicate step(
+    DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel predlbl,
+    DataFlow::FlowLabel succlbl
+  ) {
+    any(SharedFlowStep st).step(pred, succ, predlbl, succlbl) and succ = this
+  }
+}
+
 /**
  * A collection of pseudo-properties that are used in multiple files.
  *