Skip to content

Commit df1bf4a

Browse files
authored
Merge pull request #1907 from asger-semmle/mongoose-types
Approved by xiemaisi
2 parents 2f54437 + 194a1c3 commit df1bf4a

File tree

22 files changed

+148
-22
lines changed

22 files changed

+148
-22
lines changed

change-notes/1.23/analysis-javascript.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44

55
* Support for the following frameworks and libraries has been improved:
66
- [firebase](https://www.npmjs.com/package/firebase)
7+
- [mongodb](https://www.npmjs.com/package/mongodb)
8+
- [mongoose](https://www.npmjs.com/package/mongoose)
79

810
* The call graph has been improved to resolve method calls in more cases. This may produce more security alerts.
911

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

Lines changed: 69 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -21,42 +21,91 @@ 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+
/**
81+
* A collection based on the type `mongodb.Collection`.
82+
*
83+
* Note that this also covers `mongoose` models since they are subtypes
84+
* of `mongodb.Collection`.
85+
*/
86+
private class CollectionFromType extends Collection {
87+
CollectionFromType() {
88+
hasUnderlyingType("mongodb", "Collection")
89+
}
90+
}
91+
92+
/** Gets a data flow node referring to a MongoDB collection. */
93+
private DataFlow::SourceNode getACollection(DataFlow::TypeTracker t) {
94+
t.start() and
95+
result instanceof Collection
96+
or
97+
exists(DataFlow::TypeTracker t2 | result = getACollection(t2).track(t2, t))
98+
}
99+
100+
/** Gets a data flow node referring to a MongoDB collection. */
101+
DataFlow::SourceNode getACollection() { result = getACollection(DataFlow::TypeTracker::end()) }
102+
53103
/** A call to a MongoDB query method. */
54-
private class QueryCall extends DatabaseAccess, DataFlow::ValueNode {
55-
override MethodCallExpr astNode;
104+
private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode {
56105
int queryArgIdx;
57106

58107
QueryCall() {
59-
exists(string m | asExpr().(MethodCallExpr).calls(any(Collection c), m) |
108+
exists(string m | this = getACollection().getAMethodCall(m) |
60109
m = "aggregate" and queryArgIdx = 0
61110
or
62111
m = "count" and queryArgIdx = 0
@@ -91,9 +140,7 @@ private module MongoDB {
91140
)
92141
}
93142

94-
override DataFlow::Node getAQueryArgument() {
95-
result = DataFlow::valueNode(astNode.getArgument(queryArgIdx))
96-
}
143+
override DataFlow::Node getAQueryArgument() { result = getArgument(queryArgIdx) }
97144
}
98145

99146
/**
@@ -116,15 +163,15 @@ private module Mongoose {
116163
/**
117164
* Gets a call to `mongoose.createConnection`.
118165
*/
119-
MethodCallExpr createConnection() {
120-
result = getAMongooseInstance().getAMemberCall("createConnection").asExpr()
166+
DataFlow::CallNode createConnection() {
167+
result = getAMongooseInstance().getAMemberCall("createConnection")
121168
}
122169

123170
/**
124171
* A Mongoose collection object.
125172
*/
126173
class Model extends MongoDB::Collection {
127-
Model() { getAMongooseInstance().getAMemberCall("model").flowsToExpr(this) }
174+
Model() { this = getAMongooseInstance().getAMemberCall("model") }
128175
}
129176

130177
/**
@@ -134,7 +181,7 @@ private module Mongoose {
134181
string kind;
135182

136183
Credentials() {
137-
exists(string prop | createConnection().hasOptionArgument(3, prop, this) |
184+
exists(string prop | this = createConnection().getOptionArgument(3, prop).asExpr() |
138185
prop = "user" and kind = "user name"
139186
or
140187
prop = "pass" and kind = "password"
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
nodes
2+
| typedClient.ts:13:7:13:32 | v |
3+
| typedClient.ts:13:11:13:32 | JSON.pa ... body.x) |
4+
| typedClient.ts:13:22:13:29 | req.body |
5+
| typedClient.ts:13:22:13:31 | req.body.x |
6+
| typedClient.ts:14:24:14:32 | { id: v } |
7+
| typedClient.ts:14:30:14:30 | v |
8+
edges
9+
| typedClient.ts:13:7:13:32 | v | typedClient.ts:14:30:14:30 | v |
10+
| typedClient.ts:13:11:13:32 | JSON.pa ... body.x) | typedClient.ts:13:7:13:32 | v |
11+
| typedClient.ts:13:22:13:29 | req.body | typedClient.ts:13:22:13:31 | req.body.x |
12+
| typedClient.ts:13:22:13:31 | req.body.x | typedClient.ts:13:11:13:32 | JSON.pa ... body.x) |
13+
| typedClient.ts:14:30:14:30 | v | typedClient.ts:14:24:14:32 | { id: v } |
14+
#select
15+
| typedClient.ts:14:24:14:32 | { id: v } | typedClient.ts:13:22:13:29 | req.body | typedClient.ts:14:24:14:32 | { id: v } | This query depends on $@. | typedClient.ts:13:22:13:29 | req.body | a user-provided value |

javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.qlref renamed to javascript/ql/test/query-tests/Security/CWE-089/typed/SqlInjection.qlref

File renamed without changes.
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
declare module "mongodb" {
2+
interface Collection {
3+
find(query: any): any;
4+
}
5+
}
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"include": ["."],
3+
"compilerOptions": {
4+
"esModuleInterop": true
5+
}
6+
}
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
import * as mongodb from "mongodb";
2+
3+
const express = require('express') as any;
4+
const bodyParser = require('body-parser') as any;
5+
6+
declare function getCollection(): mongodb.Collection;
7+
8+
let app = express();
9+
10+
app.use(bodyParser.json());
11+
12+
app.post('/find', (req, res) => {
13+
let v = JSON.parse(req.body.x);
14+
getCollection().find({ id: v }); // NOT OK
15+
});

javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected renamed to 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
@@ -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 |
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Security/CWE-089/SqlInjection.ql

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

File renamed without changes.

0 commit comments

Comments
 (0)