Skip to content

Commit 3415b64

Browse files
codeql-cicodeql-ci
authored
Merge pull request github#5423 from erik-krogh/koa
Approved by asgerf, esbena
2 parents c6a69e1 + 84e9229 commit 3415b64

File tree

4 files changed

+182
-27
lines changed

4 files changed

+182
-27
lines changed

‎javascript/change-notes/2021-03-17-koa-route.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
lgtm,codescanning
2+
* Route handlers registered using koa routing libraries are recognized as a source of remote user input.
3+
Affected packages are
4+
[koa-route](https://www.npmjs.com/package/koa-route), and
5+
[koa-router](https://www.npmjs.com/package/koa-router)

‎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 a dataflow node that can be given to a `RouteSetup` to register the handler.
76+
*/
77+
abstract DataFlow::SourceNode getARouteHandlerRegistrationObject();
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 getARouteHandlerRegistrationObject() { 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 {
140+
DataFlow::InvokeNode router;
141+
DataFlow::MethodCallNode call;
142+
143+
RoutedRouteHandler() {
144+
router = DataFlow::moduleImport(["@koa/router", "koa-router"]).getAnInvocation() and
145+
call =
146+
router
147+
.getAChainedMethodCall([
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 getARouteHandlerRegistrationObject() {
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 {
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 getARouteHandlerRegistrationObject() { 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).getARouteHandlerRegistrationObject().flowsToExpr(getArgument(0))
394+
}
289395

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

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

Lines changed: 43 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -57,14 +57,26 @@ 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:81:9:81:52 | tainted |
61-
| tst.js:81:19:81:42 | url.par ... , true) |
62-
| tst.js:81:19:81:48 | url.par ... ).query |
63-
| tst.js:81:19:81:52 | url.par ... ery.url |
64-
| tst.js:81:29:81:35 | req.url |
65-
| tst.js:81:29:81:35 | req.url |
66-
| tst.js:83:19:83:25 | tainted |
67-
| tst.js:83:19:83: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:90:19:90:28 | ctx.params |
65+
| tst.js:90:19:90:28 | ctx.params |
66+
| tst.js:90:19:90:32 | ctx.params.foo |
67+
| tst.js:90:19:90:32 | ctx.params.foo |
68+
| tst.js:92:19:92:28 | ctx.params |
69+
| tst.js:92:19:92:28 | ctx.params |
70+
| tst.js:92:19:92:32 | ctx.params.foo |
71+
| tst.js:92:19:92:32 | ctx.params.foo |
72+
| tst.js:98:9:98:52 | tainted |
73+
| tst.js:98:19:98:42 | url.par ... , true) |
74+
| tst.js:98:19:98:48 | url.par ... ).query |
75+
| tst.js:98:19:98:52 | url.par ... ery.url |
76+
| tst.js:98:29:98:35 | req.url |
77+
| tst.js:98:29:98:35 | req.url |
78+
| tst.js:100:19:100:25 | tainted |
79+
| tst.js:100:19:100:25 | tainted |
6880
edges
6981
| tst.js:14:9:14:52 | tainted | tst.js:18:13:18:19 | tainted |
7082
| tst.js:14:9:14:52 | tainted | tst.js:18:13:18:19 | tainted |
@@ -121,13 +133,25 @@ edges
121133
| tst.js:74:19:74:52 | url.par ... ery.url | tst.js:74:9:74:52 | tainted |
122134
| tst.js:74:29:74:35 | req.url | tst.js:74:19:74:42 | url.par ... , true) |
123135
| tst.js:74:29:74:35 | req.url | tst.js:74:19:74:42 | url.par ... , true) |
124-
| tst.js:81:9:81:52 | tainted | tst.js:83:19:83:25 | tainted |
125-
| tst.js:81:9:81:52 | tainted | tst.js:83:19:83:25 | tainted |
126-
| tst.js:81:19:81:42 | url.par ... , true) | tst.js:81:19:81:48 | url.par ... ).query |
127-
| tst.js:81:19:81:48 | url.par ... ).query | tst.js:81:19:81:52 | url.par ... ery.url |
128-
| tst.js:81:19:81:52 | url.par ... ery.url | tst.js:81:9:81:52 | tainted |
129-
| tst.js:81:29:81:35 | req.url | tst.js:81:19:81:42 | url.par ... , true) |
130-
| tst.js:81:29:81:35 | req.url | tst.js:81:19:81:42 | url.par ... , true) |
136+
| tst.js:83:38:83:43 | param1 | tst.js:84:19:84:24 | param1 |
137+
| tst.js:83:38:83:43 | param1 | tst.js:84:19:84:24 | param1 |
138+
| tst.js:83:38:83:43 | param1 | tst.js:84:19:84:24 | param1 |
139+
| tst.js:83:38:83:43 | param1 | tst.js:84:19:84:24 | param1 |
140+
| tst.js:90:19:90:28 | ctx.params | tst.js:90:19:90:32 | ctx.params.foo |
141+
| tst.js:90:19:90:28 | ctx.params | tst.js:90:19:90:32 | ctx.params.foo |
142+
| tst.js:90:19:90:28 | ctx.params | tst.js:90:19:90:32 | ctx.params.foo |
143+
| tst.js:90:19:90:28 | ctx.params | tst.js:90:19:90:32 | ctx.params.foo |
144+
| tst.js:92:19:92:28 | ctx.params | tst.js:92:19:92:32 | ctx.params.foo |
145+
| tst.js:92:19:92:28 | ctx.params | tst.js:92:19:92:32 | ctx.params.foo |
146+
| tst.js:92:19:92:28 | ctx.params | tst.js:92:19:92:32 | ctx.params.foo |
147+
| tst.js:92:19:92:28 | ctx.params | tst.js:92:19:92:32 | ctx.params.foo |
148+
| tst.js:98:9:98:52 | tainted | tst.js:100:19:100:25 | tainted |
149+
| tst.js:98:9:98:52 | tainted | tst.js:100:19:100:25 | tainted |
150+
| tst.js:98:19:98:42 | url.par ... , true) | tst.js:98:19:98:48 | url.par ... ).query |
151+
| tst.js:98:19:98:48 | url.par ... ).query | tst.js:98:19:98:52 | url.par ... ery.url |
152+
| tst.js:98:19:98:52 | url.par ... ery.url | tst.js:98:9:98:52 | tainted |
153+
| tst.js:98:29:98:35 | req.url | tst.js:98:19:98:42 | url.par ... , true) |
154+
| tst.js:98:29:98:35 | req.url | tst.js:98:19:98:42 | url.par ... , true) |
131155
#select
132156
| 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 |
133157
| 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
145169
| 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 |
146170
| 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 |
147171
| 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 |
148-
| 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 |
172+
| 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 |
173+
| 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 |
174+
| 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 |
175+
| 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 |

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

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,9 +76,26 @@ var server = http.createServer(async function(req, res) {
7676
JSDOM.fromURL(tainted); // NOT OK
7777
});
7878

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+
const router = require('koa-router')();
88+
const app = new Koa();
89+
router.get('/', async (ctx, next) => {
90+
JSDOM.fromURL(ctx.params.foo); // NOT OK
91+
}).post('/', async (ctx, next) => {
92+
JSDOM.fromURL(ctx.params.foo); // NOT OK
93+
});
94+
app.use(router.routes());
95+
7996
import {JSDOM} from "jsdom";
8097
var server = http.createServer(async function(req, res) {
8198
var tainted = url.parse(req.url, true).query.url;
8299

83100
new WebSocket(tainted); // NOT OK
84-
});
101+
});

0 commit comments

Comments
 (0)