Merge pull request github#5748 from asgerf/js/rate-limiting-fixes

Approved by erik-krogh

Author: codeql-ci

javascript/change-notes/2021-04-21-rate-limiting-fixes.md

@@ -0,0 +1,3 @@
+lgtm,codescanning
+* Tracking of HTTP route handlers has improved, which may lead to additional
+  security results, and fewer false-positive results from the `js/missing-rate-limiting` query.

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

@@ -1682,61 +1682,7 @@ module DataFlow {
   import Configuration
   import TrackedNodes
   import TypeTracking
+  import internal.FunctionWrapperSteps
 
   predicate localTaintStep = TaintTracking::localTaintStep/2;
-
-  /**
-   * Holds if the function in `succ` forwards all its arguments to a call to `pred` and returns
-   * its result. This can thus be seen as a step `pred -> succ` used for tracking function values
-   * through "wrapper functions", since the `succ` function partially replicates behavior of `pred`.
-   *
-   * Examples:
-   * ```js
-   * function f(x) {
-   *   return g(x); // step: g -> f
-   * }
-   *
-   * function doExec(x) {
-   *   console.log(x);
-   *   return exec(x); // step: exec -> doExec
-   * }
-   *
-   * function doEither(x, y) {
-   *   if (x > y) {
-   *     return foo(x, y); // step: foo -> doEither
-   *   } else {
-   *     return bar(x, y); // step: bar -> doEither
-   *   }
-   * }
-   *
-   * function wrapWithLogging(f) {
-   *   return (x) => {
-   *     console.log(x);
-   *     return f(x); // step: f -> anonymous function
-   *   }
-   * }
-   * wrapWithLogging(g); // step: g -> wrapWithLogging(g)
-   * ```
-   */
-  predicate functionForwardingStep(DataFlow::Node pred, DataFlow::Node succ) {
-    exists(DataFlow::FunctionNode function, DataFlow::CallNode call |
-      call.flowsTo(function.getReturnNode()) and
-      forall(int i | exists([call.getArgument(i), function.getParameter(i)]) |
-        function.getParameter(i).flowsTo(call.getArgument(i))
-      ) and
-      pred = call.getCalleeNode() and
-      succ = function
-    )
-    or
-    // Given a generic wrapper function like,
-    //
-    //   function wrap(f) { return (x, y) => f(x, y) };
-    //
-    // add steps through calls to that function: `g -> wrap(g)`
-    exists(DataFlow::FunctionNode wrapperFunction, SourceNode param, Node paramUse |
-      FlowSteps::argumentPassing(succ, pred, wrapperFunction.getFunction(), param) and
-      param.flowsTo(paramUse) and
-      functionForwardingStep(paramUse, wrapperFunction.getReturnNode().getALocalSource())
-    )
-  }
 }

javascript/ql/src/semmle/javascript/dataflow/internal/FunctionWrapperSteps.qll

@@ -0,0 +1,147 @@
+/** Provides predicates for tracking functions through wrapper functions. */
+
+private import javascript
+private import FlowSteps as FlowSteps
+private import semmle.javascript.internal.CachedStages
+
+cached
+private module Cached {
+  private predicate forwardsParameter(
+    DataFlow::FunctionNode function, int i, DataFlow::CallNode call
+  ) {
+    exists(DataFlow::ParameterNode parameter | parameter = function.getParameter(i) |
+      not parameter.isRestParameter() and
+      parameter.flowsTo(call.getArgument(i))
+      or
+      parameter.isRestParameter() and
+      parameter.flowsTo(call.getASpreadArgument())
+    )
+  }
+
+  cached
+  private module Stage {
+    // Forces the module to be computed as part of the type-tracking stage.
+    cached
+    predicate forceStage() { Stages::TypeTracking::ref() }
+  }
+
+  /**
+   * Holds if the function in `succ` forwards all its arguments to a call to `pred` and returns
+   * its result. This can thus be seen as a step `pred -> succ` used for tracking function values
+   * through "wrapper functions", since the `succ` function partially replicates behavior of `pred`.
+   *
+   * Examples:
+   * ```js
+   * function f(x) {
+   *   return g(x); // step: g -> f
+   * }
+   *
+   * function doExec(x) {
+   *   console.log(x);
+   *   return exec(x); // step: exec -> doExec
+   * }
+   *
+   * function doEither(x, y) {
+   *   if (x > y) {
+   *     return foo(x, y); // step: foo -> doEither
+   *   } else {
+   *     return bar(x, y); // step: bar -> doEither
+   *   }
+   * }
+   *
+   * function wrapWithLogging(f) {
+   *   return (x) => {
+   *     console.log(x);
+   *     return f(x); // step: f -> anonymous function
+   *   }
+   * }
+   * wrapWithLogging(g); // step: g -> wrapWithLogging(g)
+   * ```
+   */
+  cached
+  predicate functionForwardingStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(DataFlow::FunctionNode function, DataFlow::CallNode call |
+      call.flowsTo(function.getReturnNode()) and
+      forall(int i | exists([call.getArgument(i), function.getParameter(i)]) |
+        forwardsParameter(function, i, call)
+      ) and
+      pred = call.getCalleeNode() and
+      succ = function
+    )
+    or
+    // Given a generic wrapper function like,
+    //
+    //   function wrap(f) { return (x, y) => f(x, y) };
+    //
+    // add steps through calls to that function: `g -> wrap(g)`
+    exists(
+      DataFlow::FunctionNode wrapperFunction, DataFlow::SourceNode param, DataFlow::Node paramUse
+    |
+      FlowSteps::argumentPassing(succ, pred, wrapperFunction.getFunction(), param) and
+      param.flowsTo(paramUse) and
+      functionForwardingStep(paramUse, wrapperFunction.getReturnNode().getALocalSource())
+    )
+  }
+
+  /**
+   * Holds if the function in `succ` forwards all its arguments to a call to `pred`.
+   * This can thus be seen as a step `pred -> succ` used for tracking function values
+   * through "wrapper functions", since the `succ` function partially replicates behavior of `pred`.
+   *
+   * This is similar to `functionForwardingStep` except the innermost forwarding call does not
+   * need flow to the return value; this can be useful for tracking callback-style functions
+   * where the result tends to be unused.
+   *
+   * Examples:
+   * ```js
+   * function f(x, callback) {
+   *   g(x, callback); // step: g -> f
+   * }
+   *
+   * function doExec(x, callback) {
+   *   console.log(x);
+   *   exec(x, callback); // step: exec -> doExec
+   * }
+   *
+   * function doEither(x, y) {
+   *   if (x > y) {
+   *     return foo(x, y); // step: foo -> doEither
+   *   } else {
+   *     return bar(x, y); // step: bar -> doEither
+   *   }
+   * }
+   *
+   * function wrapWithLogging(f) {
+   *   return (x) => {
+   *     console.log(x);
+   *     return f(x); // step: f -> anonymous function
+   *   }
+   * }
+   * wrapWithLogging(g); // step: g -> wrapWithLogging(g)
+   * ```
+   */
+  cached
+  predicate functionOneWayForwardingStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(DataFlow::FunctionNode function, DataFlow::CallNode call |
+      call.getContainer() = function.getFunction() and
+      forall(int i | exists(function.getParameter(i)) | forwardsParameter(function, i, call)) and
+      pred = call.getCalleeNode() and
+      succ = function
+    )
+    or
+    // Given a generic wrapper function like,
+    //
+    //   function wrap(f) { return (x, y) => f(x, y) };
+    //
+    // add steps through calls to that function: `g -> wrap(g)`
+    exists(
+      DataFlow::FunctionNode wrapperFunction, DataFlow::SourceNode param, DataFlow::Node paramUse
+    |
+      FlowSteps::argumentPassing(succ, pred, wrapperFunction.getFunction(), param) and
+      param.flowsTo(paramUse) and
+      functionOneWayForwardingStep(paramUse, wrapperFunction.getReturnNode().getALocalSource())
+    )
+  }
+}
+
+import Cached

javascript/ql/src/semmle/javascript/frameworks/Express.qll

@@ -224,7 +224,13 @@ module Express {
     /**
      * Gets the function body of this handler, if it is defined locally.
      */
-    RouteHandler getBody() { result.(DataFlow::SourceNode).flowsToExpr(this) }
+    RouteHandler getBody() {
+      exists(DataFlow::SourceNode source | source = flow().getALocalSource() |
+        result = source
+        or
+        DataFlow::functionOneWayForwardingStep(result.(DataFlow::SourceNode).getALocalUse(), source)
+      )
+    }
 
     /**
      * Holds if this is not followed by more handlers.

javascript/ql/src/semmle/javascript/frameworks/HTTP.qll

@@ -235,64 +235,12 @@ module HTTP {
     ResponseExpr getAResponseExpr() { result.getRouteHandler() = this }
   }
 
-  /**
-   * Holds if `call` decorates the function `pred`.
-   * This means that `call` returns a function that forwards its arguments to `pred`.
-   * Only holds when the decorator looks like it is decorating a route-handler.
-   *
-   * Below is a code example relating `call`, `decoratee`, `outer`, `inner`.
-   * ```
-   * function outer(method) {
-   *    return function inner(req, res) {
-   *      return method.call(this, req, res);
-   *    };
-   *  }
-   *  var route = outer(function decoratee(req, res) { // <- call
-   *    res.end("foo");
-   *  });
-   * ```
-   */
-  private predicate isDecoratedCall(DataFlow::CallNode call, DataFlow::FunctionNode decoratee) {
-    // indirect route-handler `result` is given to function `outer`, which returns function `inner` which calls the function `pred`.
-    exists(int i, DataFlow::FunctionNode outer, HTTP::RouteHandlerCandidate inner |
-      inner = outer.getAReturn().getALocalSource() and
-      decoratee = call.getArgument(i).getALocalSource() and
-      outer.getFunction() = call.getACallee() and
-      hasForwardingHandlerParameter(i, outer, inner)
-    )
-  }
-
-  /**
-   * Holds if the `i`th parameter of `outer` has a call that `inner` forwards its parameters to.
-   */
-  private predicate hasForwardingHandlerParameter(
-    int i, DataFlow::FunctionNode outer, HTTP::RouteHandlerCandidate inner
-  ) {
-    isAForwardingRouteHandlerCall(outer.getParameter(i), inner)
-  }
-
-  /**
-   * Holds if `f` looks like a route-handler and a call to `callee` inside `f` forwards all of the parameters from `f` to that call.
-   */
-  private predicate isAForwardingRouteHandlerCall(
-    DataFlow::SourceNode callee, HTTP::RouteHandlerCandidate f
-  ) {
-    exists(DataFlow::CallNode call | call = callee.getACall() |
-      forall(int arg | arg = [0 .. f.getNumParameter() - 1] |
-        f.getParameter(arg).flowsTo(call.getArgument(arg))
-      ) and
-      call.getContainer() = f.getFunction()
-    )
-  }
-
   /**
    * Holds if there exists a step from `pred` to `succ` for a RouteHandler - beyond the usual steps defined by TypeTracking.
    */
   predicate routeHandlerStep(DataFlow::SourceNode pred, DataFlow::SourceNode succ) {
-    isDecoratedCall(succ, pred)
-    or
     // A forwarding call
-    isAForwardingRouteHandlerCall(pred, succ)
+    DataFlow::functionOneWayForwardingStep(pred.getALocalUse(), succ)
     or
     // a container containing route-handlers.
     exists(HTTP::RouteHandlerCandidateContainer container | pred = container.getRouteHandler(succ))
@@ -687,7 +635,7 @@ module HTTP {
     DataFlow::SourceNode getAPossiblyDecoratedHandler(RouteHandlerCandidate candidate) {
       result = candidate
       or
-      isDecoratedCall(result, candidate)
+      DataFlow::functionOneWayForwardingStep(candidate, result)
     }
 
     private string mapValueProp() {

javascript/ql/src/semmle/javascript/internal/CachedStages.qll

@@ -196,6 +196,8 @@ module Stages {
       exists(any(DataFlow::TypeTracker t).append(_))
       or
       exists(any(DataFlow::TypeBackTracker t).prepend(_))
+      or
+      DataFlow::functionForwardingStep(_, _)
     }
   }
 

javascript/ql/src/semmle/javascript/security/dataflow/MissingRateLimiting.qll

@@ -113,29 +113,53 @@ class DatabaseAccessAsExpensiveAction extends ExpensiveAction {
  * A route handler expression that is rate-limited by a rate-limiting middleware.
  */
 class RouteHandlerExpressionWithRateLimiter extends RateLimitedRouteHandlerExpr {
-  RouteHandlerExpressionWithRateLimiter() { getAMatchingAncestor() instanceof RateLimiter }
+  RouteHandlerExpressionWithRateLimiter() {
+    any(RateLimitingMiddleware m).ref().flowsToExpr(getAMatchingAncestor())
+  }
 }
 
 /**
+ * DEPRECATED. Use `RateLimitingMiddleware` instead.
+ *
  * A middleware that acts as a rate limiter.
  */
-abstract class RateLimiter extends Express::RouteHandlerExpr { }
+deprecated class RateLimiter extends Express::RouteHandlerExpr {
+  RateLimiter() { any(RateLimitingMiddleware m).ref().flowsToExpr(this) }
+}
+
+/**
+ * Creation of a middleware function that acts as a rate limiter.
+ */
+abstract class RateLimitingMiddleware extends DataFlow::SourceNode {
+  /** Gets a data flow node referring to this middleware. */
+  private DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
+    t.start() and
+    result = this
+    or
+    DataFlow::functionOneWayForwardingStep(ref(t.continue()).getALocalUse(), result)
+    or
+    exists(DataFlow::TypeTracker t2 | result = ref(t2).track(t2, t))
+  }
+
+  /** Gets a data flow node referring to this middleware. */
+  DataFlow::SourceNode ref() { result = ref(DataFlow::TypeTracker::end()) }
+}
 
 /**
  * A rate limiter constructed using the `express-rate-limit` package.
  */
-class ExpressRateLimit extends RateLimiter {
+class ExpressRateLimit extends RateLimitingMiddleware {
   ExpressRateLimit() {
-    this = API::moduleImport("express-rate-limit").getReturn().getAUse().asExpr()
+    this = API::moduleImport("express-rate-limit").getReturn().getAnImmediateUse()
   }
 }
 
 /**
  * A rate limiter constructed using the `express-brute` package.
  */
-class BruteForceRateLimit extends RateLimiter {
+class BruteForceRateLimit extends RateLimitingMiddleware {
   BruteForceRateLimit() {
-    this = API::moduleImport("express-brute").getInstance().getMember("prevent").getAUse().asExpr()
+    this = API::moduleImport("express-brute").getInstance().getMember("prevent").getAnImmediateUse()
   }
 }
 
@@ -183,8 +207,6 @@ class RateLimiterFlexibleRateLimiter extends DataFlow::FunctionNode {
 /**
  * A route-handler expression that is rate-limited by the `rate-limiter-flexible` package.
  */
-class RouteHandlerLimitedByRateLimiterFlexible extends RateLimiter {
-  RouteHandlerLimitedByRateLimiterFlexible() {
-    any(RateLimiterFlexibleRateLimiter rl).flowsToExpr(this)
-  }
+class RouteHandlerLimitedByRateLimiterFlexible extends RateLimitingMiddleware {
+  RouteHandlerLimitedByRateLimiterFlexible() { this instanceof RateLimiterFlexibleRateLimiter }
 }

javascript/ql/test/query-tests/Security/CWE-770/MissingRateLimiting.expected

@@ -7,3 +7,4 @@
 | tst.js:37:20:37:36 | expensiveHandler3 | This route handler performs $@, but is not rate-limited. | tst.js:16:40:16:70 | child_p ... /true") | a system command |
 | tst.js:38:20:38:36 | expensiveHandler4 | This route handler performs $@, but is not rate-limited. | tst.js:17:40:17:83 | connect ... ution') | a database access |
 | tst.js:64:25:64:63 | functio ... req); } | This route handler performs $@, but is not rate-limited. | tst.js:64:46:64:60 | verifyUser(req) | authorization |
+| tst.js:76:25:76:53 | catchAs ... ndler1) | This route handler performs $@, but is not rate-limited. | tst.js:14:40:14:46 | login() | authorization |