Skip to content

JS: Add models for Koa routing libraries #5423

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 19, 2021
Merged

JS: Add models for Koa routing libraries #5423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3995ff3
add models for `koa-route` and `koa-router`
erik-krogh Mar 17, 2021
b2d6982
add change note
erik-krogh Mar 17, 2021
38a9c71
Apply suggestions from code review
erik-krogh Mar 18, 2021
f94f82a
use getAChainedMethodCall
erik-krogh Mar 18, 2021
84e9229
Merge branch 'main' into koa
erik-krogh Mar 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions javascript/change-notes/2021-03-17-koa-route.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)
126 changes: 116 additions & 10 deletions javascript/ql/src/semmle/javascript/frameworks/Koa.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 }
}

/**
Expand Down Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this model the mentioned app.use(router.routes()) bit above?
Shouldn't the registration be the app.use(...) call instead then? My intuition is that router.routes() simply is a getter.

}
}

/**
* 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.
Expand Down Expand Up @@ -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 |
Expand All @@ -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
Expand Down Expand Up @@ -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 }
}
Expand Down
59 changes: 43 additions & 16 deletions javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down Expand Up @@ -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 |
Expand All @@ -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 |
19 changes: 18 additions & 1 deletion javascript/ql/test/query-tests/Security/CWE-918/tst.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
});
});