Merge pull request #4275 from erik-krogh/CVE760-indirect

Approved by esbena

Author: codeql-ci

change-notes/1.26/analysis-javascript.md

@@ -4,8 +4,10 @@
 
 * Support for the following frameworks and libraries has been improved:
   - [bluebird](https://www.npmjs.com/package/bluebird)
+  - [express](https://www.npmjs.com/package/express)
   - [fast-json-stable-stringify](https://www.npmjs.com/package/fast-json-stable-stringify)
   - [fast-safe-stringify](https://www.npmjs.com/package/fast-safe-stringify)
+  - [http](https://nodejs.org/api/http.html)
   - [javascript-stringify](https://www.npmjs.com/package/javascript-stringify)
   - [js-stringify](https://www.npmjs.com/package/js-stringify)
   - [json-stable-stringify](https://www.npmjs.com/package/json-stable-stringify)

javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql

@@ -46,7 +46,12 @@ private DataFlow::SourceNode getARouteUsingCookies(DataFlow::TypeTracker t) {
   t.start() and
   isRouteHandlerUsingCookies(result)
   or
-  exists(DataFlow::TypeTracker t2 | result = getARouteUsingCookies(t2).track(t2, t))
+  exists(DataFlow::TypeTracker t2, DataFlow::SourceNode pred | pred = getARouteUsingCookies(t2) |
+    result = pred.track(t2, t)
+    or
+    t = t2 and
+    HTTP::routeHandlerStep(pred, result)
+  )
 }
 
 /** Gets a data flow node referring to a route handler that uses cookies. */

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

@@ -125,9 +125,7 @@ module Express {
       exists(DataFlow::TypeBackTracker t2, DataFlow::SourceNode succ | succ = getARouteHandler(t2) |
         result = succ.backtrack(t2, t)
         or
-        exists(HTTP::RouteHandlerCandidateContainer container |
-          result = container.getRouteHandler(succ)
-        ) and
+        HTTP::routeHandlerStep(result, succ) and
         t = t2
       )
     }

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

@@ -233,6 +233,53 @@ 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.
+   */
+  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, Function outer, Function inner |
+      decoratee = call.getArgument(i).getALocalSource() and
+      outer = call.getACallee() and
+      inner = outer.getAReturnedExpr() and
+      isAForwardingRouteHandlerCall(DataFlow::parameterNode(outer.getParameter(i)), inner.flow())
+    )
+  }
+
+  /**
+   * 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)
+    or
+    // a container containing route-handlers.
+    exists(HTTP::RouteHandlerCandidateContainer container | pred = container.getRouteHandler(succ))
+    or
+    // (function (req, res) {}).bind(this);
+    exists(DataFlow::PartialInvokeNode call |
+      succ = call.getBoundFunction(any(DataFlow::Node n | pred.flowsTo(n)), 0)
+    )
+  }
+
   /**
    * An expression that sets up a route on a server.
    */
@@ -555,26 +602,51 @@ module HTTP {
             create.getArgument(0).asExpr() instanceof NullLiteral
           )
         ) and
-        exists(RouteHandlerCandidate candidate | candidate.flowsTo(getAPropertyWrite().getRhs()))
+        exists(RouteHandlerCandidate candidate |
+          getAPossiblyDecoratedHandler(candidate).flowsTo(getAPropertyWrite().getRhs())
+        )
       }
 
       override DataFlow::SourceNode getRouteHandler(DataFlow::SourceNode access) {
         result instanceof RouteHandlerCandidate and
         exists(DataFlow::PropWrite write, DataFlow::PropRead read |
           access = read and
           ref(this).getAPropertyRead() = read and
-          result.flowsTo(write.getRhs()) and
+          getAPossiblyDecoratedHandler(result).flowsTo(write.getRhs()) and
           write = this.getAPropertyWrite()
         |
           write.getPropertyName() = read.getPropertyName()
           or
           exists(EnumeratedPropName prop | access = prop.getASourceProp())
           or
           read = DataFlow::lvalueNode(any(ForOfStmt stmt).getLValue())
+          or
+          // for forwarding calls to an element where the key is determined by the request.
+          getRequestParameterRead().flowsToExpr(read.getPropertyNameExpr())
         )
       }
     }
 
+    /**
+     * Gets a (chained) property-read/method-call on the request parameter of the route-handler `f`.
+     */
+    private DataFlow::SourceNode getRequestParameterRead() {
+      result = any(RouteHandlerCandidate f).getParameter(0)
+      or
+      result = getRequestParameterRead().getAPropertyRead()
+      or
+      result = getRequestParameterRead().getAMethodCall()
+    }
+
+    /**
+     * Gets a node that is either `candidate`, or a call that decorates `candidate`.
+     */
+    DataFlow::SourceNode getAPossiblyDecoratedHandler(RouteHandlerCandidate candidate) {
+      result = candidate
+      or
+      isDecoratedCall(result, candidate)
+    }
+
     /**
      * A collection that contains one or more route potential handlers.
      */
@@ -592,13 +664,14 @@ module HTTP {
       }
 
       override DataFlow::SourceNode getRouteHandler(DataFlow::SourceNode access) {
+        result instanceof RouteHandlerCandidate and
         exists(
           DataFlow::Node input, TypeTrackingPseudoProperty key, CollectionFlowStep store,
           CollectionFlowStep load, DataFlow::Node storeTo, DataFlow::Node loadFrom
         |
           this.flowsTo(storeTo) and
           store.store(input, storeTo, key) and
-          result.(RouteHandlerCandidate).flowsTo(input) and
+          getAPossiblyDecoratedHandler(result).flowsTo(input) and
           ref(this).flowsTo(loadFrom) and
           load.load(loadFrom, access, key)
         )

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

@@ -228,7 +228,12 @@ module NodeJSLib {
       t.start() and
       result = handler.flow().getALocalSource()
       or
-      exists(DataFlow::TypeBackTracker t2 | result = getARouteHandler(t2).backtrack(t2, t))
+      exists(DataFlow::TypeBackTracker t2, DataFlow::SourceNode succ | succ = getARouteHandler(t2) |
+        result = succ.backtrack(t2, t)
+        or
+        t = t2 and
+        HTTP::routeHandlerStep(result, succ)
+      )
     }
 
     override Expr getServer() { result = server }
@@ -721,16 +726,8 @@ module NodeJSLib {
         astNode.getParameter(0).getName() = request and
         astNode.getParameter(1).getName() = response
       |
-        not (
-          // heuristic: not a class method (Node.js invokes this with a function call)
-          astNode = any(MethodDefinition def).getBody()
-          or
-          // heuristic: does not return anything (Node.js will not use the return value)
-          exists(astNode.getAReturnStmt().getExpr())
-          or
-          // heuristic: is not invoked (Node.js invokes this at a call site we cannot reason precisely about)
-          exists(DataFlow::InvokeNode cs | cs.getACallee() = astNode)
-        )
+        // heuristic: not a class method (Node.js invokes this with a function call)
+        not astNode = any(MethodDefinition def).getBody()
       )
     }
   }