Skip to content

Commit 8e397ad

Browse files
asger-semmleasger-semmle
committed
JS: Use type tracking in mongodb/mongoose model
1 parent e899250 commit 8e397ad

File tree

4 files changed

+92
-22
lines changed

4 files changed

+92
-22
lines changed

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

Lines changed: 57 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -21,42 +21,79 @@ private module MongoDB {
2121
/**
2222
* Gets an access to `mongodb.MongoClient`.
2323
*/
24-
DataFlow::SourceNode getAMongoClient() { result = mongodb().getAPropertyRead("MongoClient") }
24+
private DataFlow::SourceNode getAMongoClient(DataFlow::TypeTracker t) {
25+
t.start() and
26+
result = mongodb().getAPropertyRead("MongoClient")
27+
or
28+
exists(DataFlow::TypeTracker t2 | result = getAMongoClient(t2).track(t2, t))
29+
}
30+
31+
/**
32+
* Gets an access to `mongodb.MongoClient`.
33+
*/
34+
DataFlow::SourceNode getAMongoClient() { result = getAMongoClient(DataFlow::TypeTracker::end()) }
35+
36+
/** Gets a data flow node that leads to a `connect` callback. */
37+
private DataFlow::SourceNode getAMongoDbCallback(DataFlow::TypeBackTracker t) {
38+
t.start() and
39+
result = getAMongoClient().getAMemberCall("connect").getArgument(1).getALocalSource()
40+
or
41+
exists(DataFlow::TypeBackTracker t2 | result = getAMongoDbCallback(t2).backtrack(t2, t))
42+
}
43+
44+
/** Gets a data flow node that leads to a `connect` callback. */
45+
private DataFlow::FunctionNode getAMongoDbCallback() {
46+
result = getAMongoDbCallback(DataFlow::TypeBackTracker::end())
47+
}
2548

2649
/**
2750
* Gets an expression that may refer to a MongoDB database connection.
2851
*/
29-
DataFlow::SourceNode getAMongoDb() {
30-
result = getAMongoClient().getAMemberCall("connect").getCallback(1).getParameter(1)
52+
private DataFlow::SourceNode getAMongoDb(DataFlow::TypeTracker t) {
53+
t.start() and
54+
result = getAMongoDbCallback().getParameter(1)
55+
or
56+
exists(DataFlow::TypeTracker t2 | result = getAMongoDb(t2).track(t2, t))
3157
}
3258

3359
/**
34-
* An expression that may hold a MongoDB collection.
60+
* Gets an expression that may refer to a MongoDB database connection.
61+
*/
62+
DataFlow::SourceNode getAMongoDb() { result = getAMongoDb(DataFlow::TypeTracker::end()) }
63+
64+
/**
65+
* A data flow node that may hold a MongoDB collection.
3566
*/
36-
abstract class Collection extends Expr { }
67+
abstract class Collection extends DataFlow::SourceNode { }
3768

3869
/**
3970
* A collection resulting from calling `Db.collection(...)`.
4071
*/
4172
private class CollectionFromDb extends Collection {
4273
CollectionFromDb() {
43-
exists(DataFlow::CallNode collection |
44-
collection = getAMongoDb().getAMethodCall("collection")
45-
|
46-
collection.flowsToExpr(this)
47-
or
48-
collection.getCallback(1).getParameter(0).flowsToExpr(this)
49-
)
74+
this = getAMongoDb().getAMethodCall("collection")
75+
or
76+
this = getAMongoDb().getAMethodCall("collection").getCallback(1).getParameter(0)
5077
}
5178
}
5279

80+
/** Gets a data flow node referring to a MongoDB collection. */
81+
private DataFlow::SourceNode getACollection(DataFlow::TypeTracker t) {
82+
t.start() and
83+
result instanceof Collection
84+
or
85+
exists(DataFlow::TypeTracker t2 | result = getACollection(t2).track(t2, t))
86+
}
87+
88+
/** Gets a data flow node referring to a MongoDB collection. */
89+
DataFlow::SourceNode getACollection() { result = getACollection(DataFlow::TypeTracker::end()) }
90+
5391
/** A call to a MongoDB query method. */
54-
private class QueryCall extends DatabaseAccess, DataFlow::ValueNode {
55-
override MethodCallExpr astNode;
92+
private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode {
5693
int queryArgIdx;
5794

5895
QueryCall() {
59-
exists(string m | asExpr().(MethodCallExpr).calls(any(Collection c), m) |
96+
exists(string m | this = getACollection().getAMethodCall(m) |
6097
m = "aggregate" and queryArgIdx = 0
6198
or
6299
m = "count" and queryArgIdx = 0
@@ -91,9 +128,7 @@ private module MongoDB {
91128
)
92129
}
93130

94-
override DataFlow::Node getAQueryArgument() {
95-
result = DataFlow::valueNode(astNode.getArgument(queryArgIdx))
96-
}
131+
override DataFlow::Node getAQueryArgument() { result = getArgument(queryArgIdx) }
97132
}
98133

99134
/**
@@ -116,15 +151,15 @@ private module Mongoose {
116151
/**
117152
* Gets a call to `mongoose.createConnection`.
118153
*/
119-
MethodCallExpr createConnection() {
120-
result = getAMongooseInstance().getAMemberCall("createConnection").asExpr()
154+
DataFlow::CallNode createConnection() {
155+
result = getAMongooseInstance().getAMemberCall("createConnection")
121156
}
122157

123158
/**
124159
* A Mongoose collection object.
125160
*/
126161
class Model extends MongoDB::Collection {
127-
Model() { getAMongooseInstance().getAMemberCall("model").flowsToExpr(this) }
162+
Model() { this = getAMongooseInstance().getAMemberCall("model") }
128163
}
129164

130165
/**
@@ -134,7 +169,7 @@ private module Mongoose {
134169
string kind;
135170

136171
Credentials() {
137-
exists(string prop | createConnection().hasOptionArgument(3, prop, this) |
172+
exists(string prop | this = createConnection().getOptionArgument(3, prop).asExpr() |
138173
prop = "user" and kind = "user name"
139174
or
140175
prop = "pass" and kind = "password"

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,15 @@ nodes
4141
| mongooseJsonParse.js:20:19:20:50 | JSON.pa ... ).title |
4242
| mongooseJsonParse.js:20:30:20:43 | req.query.data |
4343
| mongooseJsonParse.js:23:19:23:23 | query |
44+
| mongooseModelClient.js:10:7:10:32 | v |
45+
| mongooseModelClient.js:10:11:10:32 | JSON.pa ... body.x) |
46+
| mongooseModelClient.js:10:22:10:29 | req.body |
47+
| mongooseModelClient.js:10:22:10:31 | req.body.x |
48+
| mongooseModelClient.js:11:16:11:24 | { id: v } |
49+
| mongooseModelClient.js:11:22:11:22 | v |
50+
| mongooseModelClient.js:12:16:12:34 | { id: req.body.id } |
51+
| mongooseModelClient.js:12:22:12:29 | req.body |
52+
| mongooseModelClient.js:12:22:12:32 | req.body.id |
4453
| socketio.js:10:25:10:30 | handle |
4554
| socketio.js:11:12:11:53 | `INSERT ... andle}` |
4655
| socketio.js:11:46:11:51 | handle |
@@ -124,6 +133,13 @@ edges
124133
| mongooseJsonParse.js:20:19:20:50 | JSON.pa ... ).title | mongooseJsonParse.js:19:19:19:20 | {} |
125134
| mongooseJsonParse.js:20:19:20:50 | JSON.pa ... ).title | mongooseJsonParse.js:23:19:23:23 | query |
126135
| mongooseJsonParse.js:20:30:20:43 | req.query.data | mongooseJsonParse.js:20:19:20:44 | JSON.pa ... y.data) |
136+
| mongooseModelClient.js:10:7:10:32 | v | mongooseModelClient.js:11:22:11:22 | v |
137+
| mongooseModelClient.js:10:11:10:32 | JSON.pa ... body.x) | mongooseModelClient.js:10:7:10:32 | v |
138+
| mongooseModelClient.js:10:22:10:29 | req.body | mongooseModelClient.js:10:22:10:31 | req.body.x |
139+
| mongooseModelClient.js:10:22:10:31 | req.body.x | mongooseModelClient.js:10:11:10:32 | JSON.pa ... body.x) |
140+
| mongooseModelClient.js:11:22:11:22 | v | mongooseModelClient.js:11:16:11:24 | { id: v } |
141+
| mongooseModelClient.js:12:22:12:29 | req.body | mongooseModelClient.js:12:22:12:32 | req.body.id |
142+
| mongooseModelClient.js:12:22:12:32 | req.body.id | mongooseModelClient.js:12:16:12:34 | { id: req.body.id } |
127143
| socketio.js:10:25:10:30 | handle | socketio.js:11:46:11:51 | handle |
128144
| socketio.js:11:46:11:51 | handle | socketio.js:11:12:11:53 | `INSERT ... andle}` |
129145
| tst2.js:9:27:9:78 | "select ... rams.id | tst2.js:9:27:9:84 | "select ... d + "'" |
@@ -159,6 +175,8 @@ edges
159175
| mongoose.js:60:25:60:29 | query | mongoose.js:21:19:21:26 | req.body | mongoose.js:60:25:60:29 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value |
160176
| mongoose.js:63:24:63:28 | query | mongoose.js:21:19:21:26 | req.body | mongoose.js:63:24:63:28 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value |
161177
| mongooseJsonParse.js:23:19:23:23 | query | mongooseJsonParse.js:20:30:20:43 | req.query.data | mongooseJsonParse.js:23:19:23:23 | query | This query depends on $@. | mongooseJsonParse.js:20:30:20:43 | req.query.data | a user-provided value |
178+
| mongooseModelClient.js:11:16:11:24 | { id: v } | mongooseModelClient.js:10:22:10:29 | req.body | mongooseModelClient.js:11:16:11:24 | { id: v } | This query depends on $@. | mongooseModelClient.js:10:22:10:29 | req.body | a user-provided value |
179+
| mongooseModelClient.js:12:16:12:34 | { id: req.body.id } | mongooseModelClient.js:12:22:12:29 | req.body | mongooseModelClient.js:12:16:12:34 | { id: req.body.id } | This query depends on $@. | mongooseModelClient.js:12:22:12:29 | req.body | a user-provided value |
162180
| socketio.js:11:12:11:53 | `INSERT ... andle}` | socketio.js:10:25:10:30 | handle | socketio.js:11:12:11:53 | `INSERT ... andle}` | This query depends on $@. | socketio.js:10:25:10:30 | handle | a user-provided value |
163181
| tst2.js:9:27:9:84 | "select ... d + "'" | tst2.js:9:66:9:78 | req.params.id | tst2.js:9:27:9:84 | "select ... d + "'" | This query depends on $@. | tst2.js:9:66:9:78 | req.params.id | a user-provided value |
164182
| tst3.js:10:14:10:19 | query1 | tst3.js:9:16:9:34 | req.params.category | tst3.js:10:14:10:19 | query1 | This query depends on $@. | tst3.js:9:16:9:34 | req.params.category | a user-provided value |

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
import mongoose from 'mongoose';
2+
3+
export const MyModel = mongoose.model('MyModel', getSchema());

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
import { MyModel } from './mongooseModel';
2+
import express from 'express';
3+
import bodyParser from 'body-parser';
4+
5+
let app = express();
6+
7+
app.use(bodyParser.json());
8+
9+
app.post('/find', (req, res) => {
10+
let v = JSON.parse(req.body.x);
11+
MyModel.find({ id: v }); // NOT OK
12+
MyModel.find({ id: req.body.id }); // NOT OK
13+
MyModel.find({ id: `${req.body.id}` }); // OK
14+
});

0 commit comments

Comments
 (0)