add models for koa-route and koa-router

2026-04-30 03:05:15 +02:00 · 2021-03-17 13:08:42 +01:00
parent 05779ef7ee
commit 3995ff322d
3 changed files with 162 additions and 11 deletions
--- a/javascript/ql/src/semmle/javascript/frameworks/Koa.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Koa.qll
@@ -36,18 +36,11 @@ module Koa {
  /**
   * A Koa route handler.
   */
-  class RouteHandler extends HTTP::Servers::StandardRouteHandler, DataFlow::ValueNode {
-    Function function;
-
-    RouteHandler() {
-      function = astNode and
-      any(RouteSetup setup).getARouteHandler() = this
-    }
-
+  abstract class RouteHandler extends HTTP::Servers::StandardRouteHandler, DataFlow::SourceNode {
    /**
     * Gets the parameter of the route handler that contains the context object.
     */
-    Parameter getContextParameter() { result = function.getParameter(0) }
+    Parameter getContextParameter() { result = getAFunctionValue().getFunction().getParameter(0) }

    /**
     * Gets an expression that contains the "context" object of
@@ -70,6 +63,35 @@ module Koa {
     * object of a route handler invocation.
     */
    Expr getARequestOrContextExpr() { result = getARequestExpr() or result = getAContextExpr() }
+
+    /**
+     * Gets a reference to a request parameter defined by this route handler.
+     */
+    DataFlow::Node getARequestParameterAccess() {
+      none() // overriden in subclasses.
+    }
+
+    /**
+     * Gets the dataflow node that is given to a `RouteSetup` to register the handler.
+     */
+    abstract DataFlow::SourceNode getRouteHandlerRegistration();
+  }
+
+  /**
+   * A koa route handler registered directly with a route-setup.
+   * Like:
+   * ```JavaScript
+   * var route = require('koa-route');
+   * var app = new Koa();
+   * app.use((context, next) => {
+   *     ...
+   * });
+   * ```
+   */
+  private class StandardRouteHandler extends RouteHandler {
+    StandardRouteHandler() { any(RouteSetup setup).getARouteHandler() = this }
+
+    override DataFlow::SourceNode getRouteHandlerRegistration() { result = this }
  }

  /**
@@ -100,6 +122,77 @@ module Koa {
    }
  }

+  /**
+   * A Koa route handler registered using a routing library.
+   *
+   * Example of what that could look like:
+   * ```JavaScript
+   * const router = require('koa-router')();
+   * const Koa = require('koa');
+   * const app = new Koa();
+   * router.get('/', async (ctx, next) => {
+   *    // route handler stuff
+   * });
+   * app.use(router.routes());
+   * ```
+   */
+  private class RoutedRouteHandler extends RouteHandler, DataFlow::SourceNode {
+    DataFlow::InvokeNode router;
+    DataFlow::MethodCallNode call;
+
+    RoutedRouteHandler() {
+      router = DataFlow::moduleImport(["@koa/router", "koa-router"]).getAnInvocation() and
+      call = router.getAMethodCall*() and
+      call.getMethodName() =
+        [
+          "use", "get", "post", "put", "link", "unlink", "delete", "del", "head", "options",
+          "patch", "all"
+        ] and
+      this.flowsTo(call.getArgument(any(int i | i >= 1)))
+    }
+
+    override DataFlow::SourceNode getRouteHandlerRegistration() {
+      result = call
+      or
+      result = router.getAMethodCall("routes")
+    }
+  }
+
+  /**
+   * A route handler registered using the `koa-route` library.
+   *
+   * Example of how `koa-route` can be used:
+   * ```JavaScript
+   * var route = require('koa-route');
+   * var Koa = require('koa');
+   * var app = new Koa();
+   *
+   * app.use(route.get('/pets', (context, param1, param2, param3, ...params) => {
+   *    // route handler stuff
+   * }));
+   */
+  class KoaRouteHandler extends RouteHandler, DataFlow::SourceNode {
+    DataFlow::CallNode call;
+
+    KoaRouteHandler() {
+      call =
+        DataFlow::moduleMember("koa-route",
+          [
+            "all", "acl", "bind", "checkout", "connect", "copy", "delete", "del", "get", "head",
+            "link", "lock", "msearch", "merge", "mkactivity", "mkcalendar", "mkcol", "move",
+            "notify", "options", "patch", "post", "propfind", "proppatch", "purge", "put", "rebind",
+            "report", "search", "subscribe", "trace", "unbind", "unlink", "unlock", "unsubscribe"
+          ]).getACall() and
+      this.flowsTo(call.getArgument(1))
+    }
+
+    override DataFlow::Node getARequestParameterAccess() {
+      result = call.getABoundCallbackParameter(1, any(int i | i >= 1))
+    }
+
+    override DataFlow::SourceNode getRouteHandlerRegistration() { result = call }
+  }
+
  /**
   * A Koa request source, that is, an access to the `request` property
   * of a context object.
@@ -189,6 +282,9 @@ module Koa {
      kind = "parameter" and
      this = getAQueryParameterAccess(rh)
      or
+      kind = "parameter" and
+      this = rh.getARequestParameterAccess()
+      or
      exists(Expr e | rh.getARequestOrContextExpr() = e |
        // `ctx.request.url`, `ctx.request.originalUrl`, or `ctx.request.href`
        exists(string propName |
@@ -202,6 +298,10 @@ module Koa {
          propName = "href"
        )
        or
+        // params, when handler is registered by `koa-router` or similar.
+        kind = "parameter" and
+        this.asExpr().(PropAccess).accesses(e, "params")
+        or
        // `ctx.request.body`
        e instanceof RequestExpr and
        kind = "body" and
@@ -285,7 +385,13 @@ module Koa {
      getMethodName() = "use"
    }

-    override DataFlow::SourceNode getARouteHandler() { result.flowsToExpr(getArgument(0)) }
+    override DataFlow::SourceNode getARouteHandler() {
+      // `StandardRouteHandler` uses this predicate in it's charpred, so making this predicate return a `RouteHandler` would give an empty recursion.
+      result.flowsToExpr(getArgument(0))
+      or
+      // For the route-handlers that does not depend on this predicate in their charpred.
+      result.(RouteHandler).getRouteHandlerRegistration().flowsToExpr(getArgument(0))
+    }

    override Expr getServer() { result = server }
  }
--- a/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected
@@ -57,6 +57,18 @@ nodes
 | tst.js:74:29:74:35 | req.url |
 | tst.js:76:19:76:25 | tainted |
 | tst.js:76:19:76:25 | tainted |
+| tst.js:83:38:83:43 | param1 |
+| tst.js:83:38:83:43 | param1 |
+| tst.js:84:19:84:24 | param1 |
+| tst.js:84:19:84:24 | param1 |
+| tst.js:91:19:91:28 | ctx.params |
+| tst.js:91:19:91:28 | ctx.params |
+| tst.js:91:19:91:32 | ctx.params.foo |
+| tst.js:91:19:91:32 | ctx.params.foo |
+| tst.js:93:19:93:28 | ctx.params |
+| tst.js:93:19:93:28 | ctx.params |
+| tst.js:93:19:93:32 | ctx.params.foo |
+| tst.js:93:19:93:32 | ctx.params.foo |
 edges
 | tst.js:14:9:14:52 | tainted | tst.js:18:13:18:19 | tainted |
 | tst.js:14:9:14:52 | tainted | tst.js:18:13:18:19 | tainted |
@@ -113,6 +125,18 @@ edges
 | tst.js:74:19:74:52 | url.par ... ery.url | tst.js:74:9:74:52 | tainted |
 | tst.js:74:29:74:35 | req.url | tst.js:74:19:74:42 | url.par ... , true) |
 | tst.js:74:29:74:35 | req.url | tst.js:74:19:74:42 | url.par ... , true) |
+| tst.js:83:38:83:43 | param1 | tst.js:84:19:84:24 | param1 |
+| tst.js:83:38:83:43 | param1 | tst.js:84:19:84:24 | param1 |
+| tst.js:83:38:83:43 | param1 | tst.js:84:19:84:24 | param1 |
+| tst.js:83:38:83:43 | param1 | tst.js:84:19:84:24 | param1 |
+| tst.js:91:19:91:28 | ctx.params | tst.js:91:19:91:32 | ctx.params.foo |
+| tst.js:91:19:91:28 | ctx.params | tst.js:91:19:91:32 | ctx.params.foo |
+| tst.js:91:19:91:28 | ctx.params | tst.js:91:19:91:32 | ctx.params.foo |
+| tst.js:91:19:91:28 | ctx.params | tst.js:91:19:91:32 | ctx.params.foo |
+| tst.js:93:19:93:28 | ctx.params | tst.js:93:19:93:32 | ctx.params.foo |
+| tst.js:93:19:93:28 | ctx.params | tst.js:93:19:93:32 | ctx.params.foo |
+| tst.js:93:19:93:28 | ctx.params | tst.js:93:19:93:32 | ctx.params.foo |
+| tst.js:93:19:93:28 | ctx.params | tst.js:93:19:93:32 | ctx.params.foo |
 #select
 | tst.js:18:5:18:20 | request(tainted) | tst.js:14:29:14:35 | req.url | tst.js:18:13:18:19 | tainted | The $@ of this request depends on $@. | tst.js:18:13:18:19 | tainted | URL | tst.js:14:29:14:35 | req.url | a user-provided value |
 | tst.js:20:5:20:24 | request.get(tainted) | tst.js:14:29:14:35 | req.url | tst.js:20:17:20:23 | tainted | The $@ of this request depends on $@. | tst.js:20:17:20:23 | tainted | URL | tst.js:14:29:14:35 | req.url | a user-provided value |
@@ -130,3 +154,6 @@ edges
 | tst.js:64:3:64:38 | client. ... inted}) | tst.js:58:29:58:35 | req.url | tst.js:64:30:64:36 | tainted | The $@ of this request depends on $@. | tst.js:64:30:64:36 | tainted | URL | tst.js:58:29:58:35 | req.url | a user-provided value |
 | tst.js:68:3:68:38 | client. ... inted}) | tst.js:58:29:58:35 | req.url | tst.js:68:30:68:36 | tainted | The $@ of this request depends on $@. | tst.js:68:30:68:36 | tainted | URL | tst.js:58:29:58:35 | req.url | a user-provided value |
 | tst.js:76:5:76:26 | JSDOM.f ... ainted) | tst.js:74:29:74:35 | req.url | tst.js:76:19:76:25 | tainted | The $@ of this request depends on $@. | tst.js:76:19:76:25 | tainted | URL | tst.js:74:29:74:35 | req.url | a user-provided value |
+| tst.js:84:5:84:25 | JSDOM.f ... param1) | tst.js:83:38:83:43 | param1 | tst.js:84:19:84:24 | param1 | The $@ of this request depends on $@. | tst.js:84:19:84:24 | param1 | URL | tst.js:83:38:83:43 | param1 | a user-provided value |
+| tst.js:91:5:91:33 | JSDOM.f ... ms.foo) | tst.js:91:19:91:28 | ctx.params | tst.js:91:19:91:32 | ctx.params.foo | The $@ of this request depends on $@. | tst.js:91:19:91:32 | ctx.params.foo | URL | tst.js:91:19:91:28 | ctx.params | a user-provided value |
+| tst.js:93:5:93:33 | JSDOM.f ... ms.foo) | tst.js:93:19:93:28 | ctx.params | tst.js:93:19:93:32 | ctx.params.foo | The $@ of this request depends on $@. | tst.js:93:19:93:32 | ctx.params.foo | URL | tst.js:93:19:93:28 | ctx.params | a user-provided value |
--- a/javascript/ql/test/query-tests/Security/CWE-918/tst.js
+++ b/javascript/ql/test/query-tests/Security/CWE-918/tst.js
@@ -74,4 +74,22 @@ var server = http.createServer(async function(req, res) {
    var tainted = url.parse(req.url, true).query.url;

    JSDOM.fromURL(tainted); // NOT OK
-});
+});
+
+var route = require('koa-route');
+var Koa = require('koa');
+var app = new Koa();
+
+app.use(route.get('/pets', (context, param1, param2, param3) => { 
+    JSDOM.fromURL(param1); // NOT OK
+}));
+
+
+const router = require('koa-router')();
+const app = new Koa();
+router.get('/', async (ctx, next) => {
+    JSDOM.fromURL(ctx.params.foo); // NOT OK
+}).post('/', async (ctx, next) => {
+    JSDOM.fromURL(ctx.params.foo); // NOT OK
+});
+app.use(router.routes());