Skip to content

Commit 3995ff3

Browse files
committed
add models for koa-route and koa-router
1 parent 05779ef commit 3995ff3

File tree

3 files changed

+162
-11
lines changed

3 files changed

+162
-11
lines changed

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

Lines changed: 116 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -36,18 +36,11 @@ module Koa {
3636
/**
3737
* A Koa route handler.
3838
*/
39-
class RouteHandler extends HTTP::Servers::StandardRouteHandler, DataFlow::ValueNode {
40-
Function function;
41-
42-
RouteHandler() {
43-
function = astNode and
44-
any(RouteSetup setup).getARouteHandler() = this
45-
}
46-
39+
abstract class RouteHandler extends HTTP::Servers::StandardRouteHandler, DataFlow::SourceNode {
4740
/**
4841
* Gets the parameter of the route handler that contains the context object.
4942
*/
50-
Parameter getContextParameter() { result = function.getParameter(0) }
43+
Parameter getContextParameter() { result = getAFunctionValue().getFunction().getParameter(0) }
5144

5245
/**
5346
* Gets an expression that contains the "context" object of
@@ -70,6 +63,35 @@ module Koa {
7063
* object of a route handler invocation.
7164
*/
7265
Expr getARequestOrContextExpr() { result = getARequestExpr() or result = getAContextExpr() }
66+
67+
/**
68+
* Gets a reference to a request parameter defined by this route handler.
69+
*/
70+
DataFlow::Node getARequestParameterAccess() {
71+
none() // overriden in subclasses.
72+
}
73+
74+
/**
75+
* Gets the dataflow node that is given to a `RouteSetup` to register the handler.
76+
*/
77+
abstract DataFlow::SourceNode getRouteHandlerRegistration();
78+
}
79+
80+
/**
81+
* A koa route handler registered directly with a route-setup.
82+
* Like:
83+
* ```JavaScript
84+
* var route = require('koa-route');
85+
* var app = new Koa();
86+
* app.use((context, next) => {
87+
* ...
88+
* });
89+
* ```
90+
*/
91+
private class StandardRouteHandler extends RouteHandler {
92+
StandardRouteHandler() { any(RouteSetup setup).getARouteHandler() = this }
93+
94+
override DataFlow::SourceNode getRouteHandlerRegistration() { result = this }
7395
}
7496

7597
/**
@@ -100,6 +122,77 @@ module Koa {
100122
}
101123
}
102124

125+
/**
126+
* A Koa route handler registered using a routing library.
127+
*
128+
* Example of what that could look like:
129+
* ```JavaScript
130+
* const router = require('koa-router')();
131+
* const Koa = require('koa');
132+
* const app = new Koa();
133+
* router.get('/', async (ctx, next) => {
134+
* // route handler stuff
135+
* });
136+
* app.use(router.routes());
137+
* ```
138+
*/
139+
private class RoutedRouteHandler extends RouteHandler, DataFlow::SourceNode {
140+
DataFlow::InvokeNode router;
141+
DataFlow::MethodCallNode call;
142+
143+
RoutedRouteHandler() {
144+
router = DataFlow::moduleImport(["@koa/router", "koa-router"]).getAnInvocation() and
145+
call = router.getAMethodCall*() and
146+
call.getMethodName() =
147+
[
148+
"use", "get", "post", "put", "link", "unlink", "delete", "del", "head", "options",
149+
"patch", "all"
150+
] and
151+
this.flowsTo(call.getArgument(any(int i | i >= 1)))
152+
}
153+
154+
override DataFlow::SourceNode getRouteHandlerRegistration() {
155+
result = call
156+
or
157+
result = router.getAMethodCall("routes")
158+
}
159+
}
160+
161+
/**
162+
* A route handler registered using the `koa-route` library.
163+
*
164+
* Example of how `koa-route` can be used:
165+
* ```JavaScript
166+
* var route = require('koa-route');
167+
* var Koa = require('koa');
168+
* var app = new Koa();
169+
*
170+
* app.use(route.get('/pets', (context, param1, param2, param3, ...params) => {
171+
* // route handler stuff
172+
* }));
173+
*/
174+
class KoaRouteHandler extends RouteHandler, DataFlow::SourceNode {
175+
DataFlow::CallNode call;
176+
177+
KoaRouteHandler() {
178+
call =
179+
DataFlow::moduleMember("koa-route",
180+
[
181+
"all", "acl", "bind", "checkout", "connect", "copy", "delete", "del", "get", "head",
182+
"link", "lock", "msearch", "merge", "mkactivity", "mkcalendar", "mkcol", "move",
183+
"notify", "options", "patch", "post", "propfind", "proppatch", "purge", "put", "rebind",
184+
"report", "search", "subscribe", "trace", "unbind", "unlink", "unlock", "unsubscribe"
185+
]).getACall() and
186+
this.flowsTo(call.getArgument(1))
187+
}
188+
189+
override DataFlow::Node getARequestParameterAccess() {
190+
result = call.getABoundCallbackParameter(1, any(int i | i >= 1))
191+
}
192+
193+
override DataFlow::SourceNode getRouteHandlerRegistration() { result = call }
194+
}
195+
103196
/**
104197
* A Koa request source, that is, an access to the `request` property
105198
* of a context object.
@@ -189,6 +282,9 @@ module Koa {
189282
kind = "parameter" and
190283
this = getAQueryParameterAccess(rh)
191284
or
285+
kind = "parameter" and
286+
this = rh.getARequestParameterAccess()
287+
or
192288
exists(Expr e | rh.getARequestOrContextExpr() = e |
193289
// `ctx.request.url`, `ctx.request.originalUrl`, or `ctx.request.href`
194290
exists(string propName |
@@ -202,6 +298,10 @@ module Koa {
202298
propName = "href"
203299
)
204300
or
301+
// params, when handler is registered by `koa-router` or similar.
302+
kind = "parameter" and
303+
this.asExpr().(PropAccess).accesses(e, "params")
304+
or
205305
// `ctx.request.body`
206306
e instanceof RequestExpr and
207307
kind = "body" and
@@ -285,7 +385,13 @@ module Koa {
285385
getMethodName() = "use"
286386
}
287387

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

290396
override Expr getServer() { result = server }
291397
}

javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,18 @@ nodes
5757
| tst.js:74:29:74:35 | req.url |
5858
| tst.js:76:19:76:25 | tainted |
5959
| tst.js:76:19:76:25 | tainted |
60+
| tst.js:83:38:83:43 | param1 |
61+
| tst.js:83:38:83:43 | param1 |
62+
| tst.js:84:19:84:24 | param1 |
63+
| tst.js:84:19:84:24 | param1 |
64+
| tst.js:91:19:91:28 | ctx.params |
65+
| tst.js:91:19:91:28 | ctx.params |
66+
| tst.js:91:19:91:32 | ctx.params.foo |
67+
| tst.js:91:19:91:32 | ctx.params.foo |
68+
| tst.js:93:19:93:28 | ctx.params |
69+
| tst.js:93:19:93:28 | ctx.params |
70+
| tst.js:93:19:93:32 | ctx.params.foo |
71+
| tst.js:93:19:93:32 | ctx.params.foo |
6072
edges
6173
| tst.js:14:9:14:52 | tainted | tst.js:18:13:18:19 | tainted |
6274
| tst.js:14:9:14:52 | tainted | tst.js:18:13:18:19 | tainted |
@@ -113,6 +125,18 @@ edges
113125
| tst.js:74:19:74:52 | url.par ... ery.url | tst.js:74:9:74:52 | tainted |
114126
| tst.js:74:29:74:35 | req.url | tst.js:74:19:74:42 | url.par ... , true) |
115127
| tst.js:74:29:74:35 | req.url | tst.js:74:19:74:42 | url.par ... , true) |
128+
| tst.js:83:38:83:43 | param1 | tst.js:84:19:84:24 | param1 |
129+
| tst.js:83:38:83:43 | param1 | tst.js:84:19:84:24 | param1 |
130+
| tst.js:83:38:83:43 | param1 | tst.js:84:19:84:24 | param1 |
131+
| tst.js:83:38:83:43 | param1 | tst.js:84:19:84:24 | param1 |
132+
| tst.js:91:19:91:28 | ctx.params | tst.js:91:19:91:32 | ctx.params.foo |
133+
| tst.js:91:19:91:28 | ctx.params | tst.js:91:19:91:32 | ctx.params.foo |
134+
| tst.js:91:19:91:28 | ctx.params | tst.js:91:19:91:32 | ctx.params.foo |
135+
| tst.js:91:19:91:28 | ctx.params | tst.js:91:19:91:32 | ctx.params.foo |
136+
| tst.js:93:19:93:28 | ctx.params | tst.js:93:19:93:32 | ctx.params.foo |
137+
| tst.js:93:19:93:28 | ctx.params | tst.js:93:19:93:32 | ctx.params.foo |
138+
| tst.js:93:19:93:28 | ctx.params | tst.js:93:19:93:32 | ctx.params.foo |
139+
| tst.js:93:19:93:28 | ctx.params | tst.js:93:19:93:32 | ctx.params.foo |
116140
#select
117141
| 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 |
118142
| 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
130154
| 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 |
131155
| 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 |
132156
| 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 |
157+
| 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 |
158+
| 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 |
159+
| 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 |

javascript/ql/test/query-tests/Security/CWE-918/tst.js

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,4 +74,22 @@ var server = http.createServer(async function(req, res) {
7474
var tainted = url.parse(req.url, true).query.url;
7575

7676
JSDOM.fromURL(tainted); // NOT OK
77-
});
77+
});
78+
79+
var route = require('koa-route');
80+
var Koa = require('koa');
81+
var app = new Koa();
82+
83+
app.use(route.get('/pets', (context, param1, param2, param3) => {
84+
JSDOM.fromURL(param1); // NOT OK
85+
}));
86+
87+
88+
const router = require('koa-router')();
89+
const app = new Koa();
90+
router.get('/', async (ctx, next) => {
91+
JSDOM.fromURL(ctx.params.foo); // NOT OK
92+
}).post('/', async (ctx, next) => {
93+
JSDOM.fromURL(ctx.params.foo); // NOT OK
94+
});
95+
app.use(router.routes());

0 commit comments

Comments
 (0)