Skip to content

Commit 6d80445

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #3851 from erik-krogh/queryStuff
Approved by esbena
2 parents 13c3513 + 078b6a8 commit 6d80445

File tree

7 files changed

+96
-13
lines changed

7 files changed

+96
-13
lines changed

‎javascript/ql/src/semmle/javascript/frameworks/Express.qll‎

Lines changed: 30 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -463,6 +463,26 @@ module Express {
463463
override RequestSource src;
464464
}
465465

466+
/**
467+
* Gets a reference to the "query" object from a request-object originating from route-handler `rh`.
468+
*/
469+
DataFlow::SourceNode getAQueryObjectReference(DataFlow::TypeTracker t, RouteHandler rh) {
470+
t.startInProp("query") and
471+
result = rh.getARequestSource()
472+
or
473+
exists(DataFlow::TypeTracker t2 | result = getAQueryObjectReference(t2, rh).track(t2, t))
474+
}
475+
476+
/**
477+
* Gets a reference to the "params" object from a request-object originating from route-handler `rh`.
478+
*/
479+
DataFlow::SourceNode getAParamsObjectReference(DataFlow::TypeTracker t, RouteHandler rh) {
480+
t.startInProp("params") and
481+
result = rh.getARequestSource()
482+
or
483+
exists(DataFlow::TypeTracker t2 | result = getAParamsObjectReference(t2, rh).track(t2, t))
484+
}
485+
466486
/**
467487
* An access to a user-controlled Express request input.
468488
*/
@@ -471,13 +491,14 @@ module Express {
471491
string kind;
472492

473493
RequestInputAccess() {
494+
kind = "parameter" and
495+
this =
496+
[getAQueryObjectReference(DataFlow::TypeTracker::end(), rh),
497+
getAParamsObjectReference(DataFlow::TypeTracker::end(), rh)].getAPropertyRead()
498+
or
474499
exists(DataFlow::SourceNode request | request = rh.getARequestSource().ref() |
475500
kind = "parameter" and
476-
(
477-
this = request.getAMethodCall("param")
478-
or
479-
this = request.getAPropertyRead(["params", "query"]).getAPropertyRead()
480-
)
501+
this = request.getAMethodCall("param")
481502
or
482503
// `req.originalUrl`
483504
kind = "url" and
@@ -518,13 +539,11 @@ module Express {
518539
kind = "parameter" and
519540
exists(DataFlow::Node request | request = DataFlow::valueNode(rh.getARequestExpr()) |
520541
this.(DataFlow::MethodCallNode).calls(request, "param")
521-
or
522-
exists(DataFlow::PropRead base |
523-
// `req.query.name`
524-
base.accesses(request, "query") and
525-
this = base.getAPropertyReference(_)
526-
)
527542
)
543+
or
544+
// `req.query.name`
545+
kind = "parameter" and
546+
this = getAQueryObjectReference(DataFlow::TypeTracker::end(), rh).getAPropertyRead()
528547
}
529548
}
530549

‎javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,10 @@ nodes
33
| ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id |
44
| ReflectedXss.js:8:33:8:45 | req.params.id |
55
| ReflectedXss.js:8:33:8:45 | req.params.id |
6+
| ReflectedXss.js:17:12:17:39 | "Unknow ... rams.id |
7+
| ReflectedXss.js:17:12:17:39 | "Unknow ... rams.id |
8+
| ReflectedXss.js:17:31:17:39 | params.id |
9+
| ReflectedXss.js:17:31:17:39 | params.id |
610
| ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
711
| ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
812
| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id |
@@ -95,6 +99,10 @@ edges
9599
| ReflectedXss.js:8:33:8:45 | req.params.id | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id |
96100
| ReflectedXss.js:8:33:8:45 | req.params.id | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id |
97101
| ReflectedXss.js:8:33:8:45 | req.params.id | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id |
102+
| ReflectedXss.js:17:31:17:39 | params.id | ReflectedXss.js:17:12:17:39 | "Unknow ... rams.id |
103+
| ReflectedXss.js:17:31:17:39 | params.id | ReflectedXss.js:17:12:17:39 | "Unknow ... rams.id |
104+
| ReflectedXss.js:17:31:17:39 | params.id | ReflectedXss.js:17:12:17:39 | "Unknow ... rams.id |
105+
| ReflectedXss.js:17:31:17:39 | params.id | ReflectedXss.js:17:12:17:39 | "Unknow ... rams.id |
98106
| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
99107
| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
100108
| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
@@ -173,6 +181,7 @@ edges
173181
| tst2.js:14:9:14:9 | p | tst2.js:14:7:14:24 | p |
174182
#select
175183
| ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | ReflectedXss.js:8:33:8:45 | req.params.id | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:8:33:8:45 | req.params.id | user-provided value |
184+
| ReflectedXss.js:17:12:17:39 | "Unknow ... rams.id | ReflectedXss.js:17:31:17:39 | params.id | ReflectedXss.js:17:12:17:39 | "Unknow ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:17:31:17:39 | params.id | user-provided value |
176185
| ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id | ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | user-provided value |
177186
| ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id | ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | user-provided value |
178187
| ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id | ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss.js‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,16 @@ var express = require('express');
33
var app = express();
44

55
app.get('/user/:id', function(req, res) {
6-
if (!isValidUserId(req.params.id))
6+
if (!isValidUserId(req.params.id)) {
77
// BAD: a request parameter is incorporated without validation into the response
88
res.send("Unknown user: " + req.params.id);
9-
else
9+
moreBadStuff(req.params, res);
10+
} else {
1011
// TODO: do something exciting
1112
;
13+
}
1214
});
15+
16+
function moreBadStuff(params, res) {
17+
res.send("Unknown user: " + params.id); // NOT OK
18+
}

‎javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssWithCustomSanitizer.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
| ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:8:33:8:45 | req.params.id | user-provided value |
2+
| ReflectedXss.js:17:12:17:39 | "Unknow ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:17:31:17:39 | params.id | user-provided value |
23
| ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | user-provided value |
34
| ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | user-provided value |
45
| ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-089/untyped/DatabaseAccesses.expected‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@
1111
| mongodb.js:65:3:65:17 | doc.find(query) |
1212
| mongodb.js:73:5:77:27 | client\\n ... tag }) |
1313
| mongodb.js:81:3:85:25 | importe ... tag }) |
14+
| mongodb.js:98:5:98:19 | doc.find(query) |
15+
| mongodb.js:112:5:112:19 | doc.find(query) |
1416
| mongodb_bodySafe.js:18:7:18:21 | doc.find(query) |
1517
| mongodb_bodySafe.js:29:7:29:21 | doc.find(query) |
1618
| mongoose.js:63:2:63:34 | Documen ... then(X) |

‎javascript/ql/test/query-tests/Security/CWE-089/untyped/SqlInjection.expected‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,12 @@ nodes
5656
| mongodb.js:85:12:85:24 | { tags: tag } |
5757
| mongodb.js:85:12:85:24 | { tags: tag } |
5858
| mongodb.js:85:20:85:22 | tag |
59+
| mongodb.js:106:9:106:18 | query |
60+
| mongodb.js:106:17:106:18 | {} |
61+
| mongodb.js:107:17:107:29 | queries.title |
62+
| mongodb.js:107:17:107:29 | queries.title |
63+
| mongodb.js:112:14:112:18 | query |
64+
| mongodb.js:112:14:112:18 | query |
5965
| mongodb_bodySafe.js:23:11:23:20 | query |
6066
| mongodb_bodySafe.js:23:19:23:20 | {} |
6167
| mongodb_bodySafe.js:24:19:24:33 | req.query.title |
@@ -244,6 +250,17 @@ edges
244250
| mongodb.js:77:22:77:24 | tag | mongodb.js:77:14:77:26 | { tags: tag } |
245251
| mongodb.js:85:20:85:22 | tag | mongodb.js:85:12:85:24 | { tags: tag } |
246252
| mongodb.js:85:20:85:22 | tag | mongodb.js:85:12:85:24 | { tags: tag } |
253+
| mongodb.js:106:9:106:18 | query | mongodb.js:112:14:112:18 | query |
254+
| mongodb.js:106:9:106:18 | query | mongodb.js:112:14:112:18 | query |
255+
| mongodb.js:106:17:106:18 | {} | mongodb.js:106:9:106:18 | query |
256+
| mongodb.js:107:17:107:29 | queries.title | mongodb.js:106:9:106:18 | query |
257+
| mongodb.js:107:17:107:29 | queries.title | mongodb.js:106:9:106:18 | query |
258+
| mongodb.js:107:17:107:29 | queries.title | mongodb.js:106:17:106:18 | {} |
259+
| mongodb.js:107:17:107:29 | queries.title | mongodb.js:106:17:106:18 | {} |
260+
| mongodb.js:107:17:107:29 | queries.title | mongodb.js:112:14:112:18 | query |
261+
| mongodb.js:107:17:107:29 | queries.title | mongodb.js:112:14:112:18 | query |
262+
| mongodb.js:107:17:107:29 | queries.title | mongodb.js:112:14:112:18 | query |
263+
| mongodb.js:107:17:107:29 | queries.title | mongodb.js:112:14:112:18 | query |
247264
| mongodb_bodySafe.js:23:11:23:20 | query | mongodb_bodySafe.js:29:16:29:20 | query |
248265
| mongodb_bodySafe.js:23:11:23:20 | query | mongodb_bodySafe.js:29:16:29:20 | query |
249266
| mongodb_bodySafe.js:23:19:23:20 | {} | mongodb_bodySafe.js:23:11:23:20 | query |
@@ -428,6 +445,7 @@ edges
428445
| mongodb.js:65:12:65:16 | query | mongodb.js:60:16:60:30 | req.query.title | mongodb.js:65:12:65:16 | query | This query depends on $@. | mongodb.js:60:16:60:30 | req.query.title | a user-provided value |
429446
| mongodb.js:77:14:77:26 | { tags: tag } | mongodb.js:70:13:70:25 | req.query.tag | mongodb.js:77:14:77:26 | { tags: tag } | This query depends on $@. | mongodb.js:70:13:70:25 | req.query.tag | a user-provided value |
430447
| mongodb.js:85:12:85:24 | { tags: tag } | mongodb.js:70:13:70:25 | req.query.tag | mongodb.js:85:12:85:24 | { tags: tag } | This query depends on $@. | mongodb.js:70:13:70:25 | req.query.tag | a user-provided value |
448+
| mongodb.js:112:14:112:18 | query | mongodb.js:107:17:107:29 | queries.title | mongodb.js:112:14:112:18 | query | This query depends on $@. | mongodb.js:107:17:107:29 | queries.title | a user-provided value |
431449
| mongodb_bodySafe.js:29:16:29:20 | query | mongodb_bodySafe.js:24:19:24:33 | req.query.title | mongodb_bodySafe.js:29:16:29:20 | query | This query depends on $@. | mongodb_bodySafe.js:24:19:24:33 | req.query.title | a user-provided value |
432450
| mongoose.js:24:24:24:30 | [query] | mongoose.js:21:19:21:26 | req.body | mongoose.js:24:24:24:30 | [query] | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value |
433451
| mongoose.js:27:20:27:24 | query | mongoose.js:21:19:21:26 | req.body | mongoose.js:27:20:27:24 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-089/untyped/mongodb.js‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,3 +84,31 @@ app.post("/logs/count-by-tag", (req, res) => {
8484
// NOT OK: query is tainted by user-provided object value
8585
.count({ tags: tag });
8686
});
87+
88+
89+
app.get('/:id', (req, res) => {
90+
useParams(req.param);
91+
});
92+
function useParams(params) {
93+
let query = { id: params.id };
94+
MongoClient.connect('mongodb://localhost:27017/test', (err, db) => {
95+
let doc = db.collection('doc');
96+
97+
// OK: query is tainted, but only by string value
98+
doc.find(query);
99+
});
100+
}
101+
102+
app.post('/documents/find', (req, res) => {
103+
useQuery(req.query);
104+
});
105+
function useQuery(queries) {
106+
const query = {};
107+
query.title = queries.title;
108+
MongoClient.connect('mongodb://localhost:27017/test', (err, db) => {
109+
let doc = db.collection('doc');
110+
111+
// NOT OK: query is tainted by user-provided object value
112+
doc.find(query);
113+
});
114+
}

0 commit comments

Comments
 (0)