diff --git a/javascript/change-notes/2021-03-17-koa-route.md b/javascript/change-notes/2021-03-17-koa-route.md new file mode 100644 index 000000000000..15b1d862c9aa --- /dev/null +++ b/javascript/change-notes/2021-03-17-koa-route.md @@ -0,0 +1,5 @@ +lgtm,codescanning +* Route handlers registered using koa routing libraries are recognized as a source of remote user input. + Affected packages are + [koa-route](https://www.npmjs.com/package/koa-route), and + [koa-router](https://www.npmjs.com/package/koa-router) diff --git a/javascript/ql/src/semmle/javascript/frameworks/Koa.qll b/javascript/ql/src/semmle/javascript/frameworks/Koa.qll index 325afacea10c..59d256cdba2e 100644 --- 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 a dataflow node that can be given to a `RouteSetup` to register the handler. + */ + abstract DataFlow::SourceNode getARouteHandlerRegistrationObject(); + } + + /** + * 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 getARouteHandlerRegistrationObject() { 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::InvokeNode router; + DataFlow::MethodCallNode call; + + RoutedRouteHandler() { + router = DataFlow::moduleImport(["@koa/router", "koa-router"]).getAnInvocation() and + call = + router + .getAChainedMethodCall([ + "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 getARouteHandlerRegistrationObject() { + 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::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 getARouteHandlerRegistrationObject() { 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).getARouteHandlerRegistrationObject().flowsToExpr(getArgument(0)) + } override Expr getServer() { result = server } } diff --git a/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected b/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected index 4e24efd2a5d6..ebb15e74f042 100644 --- a/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected +++ b/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected @@ -57,14 +57,26 @@ 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:81:9:81:52 | tainted | -| tst.js:81:19:81:42 | url.par ... , true) | -| tst.js:81:19:81:48 | url.par ... ).query | -| tst.js:81:19:81:52 | url.par ... ery.url | -| tst.js:81:29:81:35 | req.url | -| tst.js:81:29:81:35 | req.url | -| tst.js:83:19:83:25 | tainted | -| tst.js:83:19:83: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:90:19:90:28 | ctx.params | +| tst.js:90:19:90:28 | ctx.params | +| tst.js:90:19:90:32 | ctx.params.foo | +| tst.js:90:19:90:32 | ctx.params.foo | +| tst.js:92:19:92:28 | ctx.params | +| tst.js:92:19:92:28 | ctx.params | +| tst.js:92:19:92:32 | ctx.params.foo | +| tst.js:92:19:92:32 | ctx.params.foo | +| tst.js:98:9:98:52 | tainted | +| tst.js:98:19:98:42 | url.par ... , true) | +| tst.js:98:19:98:48 | url.par ... ).query | +| tst.js:98:19:98:52 | url.par ... ery.url | +| tst.js:98:29:98:35 | req.url | +| tst.js:98:29:98:35 | req.url | +| tst.js:100:19:100:25 | tainted | +| tst.js:100:19:100:25 | tainted | 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 | @@ -121,13 +133,25 @@ 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:81:9:81:52 | tainted | tst.js:83:19:83:25 | tainted | -| tst.js:81:9:81:52 | tainted | tst.js:83:19:83:25 | tainted | -| tst.js:81:19:81:42 | url.par ... , true) | tst.js:81:19:81:48 | url.par ... ).query | -| tst.js:81:19:81:48 | url.par ... ).query | tst.js:81:19:81:52 | url.par ... ery.url | -| tst.js:81:19:81:52 | url.par ... ery.url | tst.js:81:9:81:52 | tainted | -| tst.js:81:29:81:35 | req.url | tst.js:81:19:81:42 | url.par ... , true) | -| tst.js:81:29:81:35 | req.url | tst.js:81:19:81: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:90:19:90:28 | ctx.params | tst.js:90:19:90:32 | ctx.params.foo | +| tst.js:90:19:90:28 | ctx.params | tst.js:90:19:90:32 | ctx.params.foo | +| tst.js:90:19:90:28 | ctx.params | tst.js:90:19:90:32 | ctx.params.foo | +| tst.js:90:19:90:28 | ctx.params | tst.js:90:19:90:32 | ctx.params.foo | +| tst.js:92:19:92:28 | ctx.params | tst.js:92:19:92:32 | ctx.params.foo | +| tst.js:92:19:92:28 | ctx.params | tst.js:92:19:92:32 | ctx.params.foo | +| tst.js:92:19:92:28 | ctx.params | tst.js:92:19:92:32 | ctx.params.foo | +| tst.js:92:19:92:28 | ctx.params | tst.js:92:19:92:32 | ctx.params.foo | +| tst.js:98:9:98:52 | tainted | tst.js:100:19:100:25 | tainted | +| tst.js:98:9:98:52 | tainted | tst.js:100:19:100:25 | tainted | +| tst.js:98:19:98:42 | url.par ... , true) | tst.js:98:19:98:48 | url.par ... ).query | +| tst.js:98:19:98:48 | url.par ... ).query | tst.js:98:19:98:52 | url.par ... ery.url | +| tst.js:98:19:98:52 | url.par ... ery.url | tst.js:98:9:98:52 | tainted | +| tst.js:98:29:98:35 | req.url | tst.js:98:19:98:42 | url.par ... , true) | +| tst.js:98:29:98:35 | req.url | tst.js:98:19:98:42 | url.par ... , true) | #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 | @@ -145,4 +169,7 @@ 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:83:5:83:26 | new Web ... ainted) | tst.js:81:29:81:35 | req.url | tst.js:83:19:83:25 | tainted | The $@ of this request depends on $@. | tst.js:83:19:83:25 | tainted | URL | tst.js:81:29:81: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:90:5:90:33 | JSDOM.f ... ms.foo) | tst.js:90:19:90:28 | ctx.params | tst.js:90:19:90:32 | ctx.params.foo | The $@ of this request depends on $@. | tst.js:90:19:90:32 | ctx.params.foo | URL | tst.js:90:19:90:28 | ctx.params | a user-provided value | +| tst.js:92:5:92:33 | JSDOM.f ... ms.foo) | tst.js:92:19:92:28 | ctx.params | tst.js:92:19:92:32 | ctx.params.foo | The $@ of this request depends on $@. | tst.js:92:19:92:32 | ctx.params.foo | URL | tst.js:92:19:92:28 | ctx.params | a user-provided value | +| tst.js:100:5:100:26 | new Web ... ainted) | tst.js:98:29:98:35 | req.url | tst.js:100:19:100:25 | tainted | The $@ of this request depends on $@. | tst.js:100:19:100:25 | tainted | URL | tst.js:98:29:98:35 | req.url | a user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-918/tst.js b/javascript/ql/test/query-tests/Security/CWE-918/tst.js index 5464375cb62f..3ce7feb531cd 100644 --- a/javascript/ql/test/query-tests/Security/CWE-918/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-918/tst.js @@ -76,9 +76,26 @@ var server = http.createServer(async function(req, res) { 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()); + import {JSDOM} from "jsdom"; var server = http.createServer(async function(req, res) { var tainted = url.parse(req.url, true).query.url; new WebSocket(tainted); // NOT OK -}); \ No newline at end of file +});