Skip to content

Commit 2a341d9

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request github#3458 from esbena/js/NoSQLCodeInjection
Approved by erik-krogh
2 parents f5e491c + 7305a87 commit 2a341d9

File tree

13 files changed

+315
-1
lines changed

13 files changed

+315
-1
lines changed

‎change-notes/1.25/analysis-javascript.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
- [fstream](https://www.npmjs.com/package/fstream)
77
- [jGrowl](https://github.com/stanlemon/jGrowl)
88
- [jQuery](https://jquery.com/)
9+
- [marsdb](https://www.npmjs.com/package/marsdb)
10+
- [minimongo](https://www.npmjs.com/package/minimongo/)
911

1012
## New queries
1113

@@ -23,6 +25,7 @@
2325
| Uncontrolled command line (`js/command-line-injection`) | More results | This query now recognizes additional command execution calls. |
2426
| Expression has no effect (`js/useless-expression`) | Less results | This query no longer flags an expression when that expression is the only content of the containing file. |
2527
| Unknown directive (`js/unknown-directive`) | Less results | This query no longer flags directives generated by the Babel compiler. |
28+
| Code injection (`js/code-injection`) | More results | More potential vulnerabilities involving NoSQL code operators are now recognized. |
2629

2730
## Changes to libraries
2831

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

Lines changed: 132 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,17 @@ import javascript
66

77
module NoSQL {
88
/** An expression that is interpreted as a NoSQL query. */
9-
abstract class Query extends Expr { }
9+
abstract class Query extends Expr {
10+
/** Gets an expression that is interpreted as a code operator in this query. */
11+
DataFlow::Node getACodeOperator() { none() }
12+
}
13+
}
14+
15+
/**
16+
* Gets the value of a `$where` property of an object that flows to `n`.
17+
*/
18+
private DataFlow::Node getADollarWherePropertyValue(DataFlow::Node n) {
19+
result = n.getALocalSource().getAPropertyWrite("$where").getRhs()
1020
}
1121

1222
/**
@@ -190,6 +200,10 @@ private module MongoDB {
190200
*/
191201
class Query extends NoSQL::Query {
192202
Query() { this = any(QueryCall qc).getAQueryArgument().asExpr() }
203+
204+
override DataFlow::Node getACodeOperator() {
205+
result = getADollarWherePropertyValue(this.flow())
206+
}
193207
}
194208
}
195209

@@ -678,6 +692,10 @@ private module Mongoose {
678692
*/
679693
class MongoDBQueryPart extends NoSQL::Query {
680694
MongoDBQueryPart() { any(InvokeNode call).interpretsArgumentAsQuery(this.flow()) }
695+
696+
override DataFlow::Node getACodeOperator() {
697+
result = getADollarWherePropertyValue(this.flow())
698+
}
681699
}
682700

683701
/**
@@ -713,3 +731,116 @@ private module Mongoose {
713731
}
714732
}
715733
}
734+
735+
/**
736+
* Provides classes modeling the Minimongo library.
737+
*/
738+
private module Minimongo {
739+
/**
740+
* Gets an expression that may refer to a Minimongo database.
741+
*/
742+
private DataFlow::SourceNode getADb(DataFlow::TypeTracker t) {
743+
t.start() and
744+
// new (require('minimongo')[DBKINDNAME])()
745+
result = DataFlow::moduleImport("minimongo").getAPropertyRead().getAnInvocation()
746+
or
747+
exists(DataFlow::TypeTracker t2 | result = getADb(t2).track(t2, t))
748+
}
749+
750+
/** Gets a data flow node referring to a Minimongo collection. */
751+
private DataFlow::SourceNode getACollection(DataFlow::TypeTracker t) {
752+
t.start() and
753+
// db[COLLECTIONNAME]
754+
result = getADb(DataFlow::TypeTracker::end()).getAPropertyRead()
755+
or
756+
exists(DataFlow::TypeTracker t2 | result = getACollection(t2).track(t2, t))
757+
}
758+
759+
/**
760+
* Provides signatures for the Collection methods.
761+
*/
762+
module CollectionMethodSignatures {
763+
/**
764+
* Holds if Collection method `name` interprets parameter `n` as a query.
765+
*/
766+
predicate interpretsArgumentAsQuery(string m, int queryArgIdx) {
767+
// implements most of the MongoDB interface
768+
MongoDB::CollectionMethodSignatures::interpretsArgumentAsQuery(m, queryArgIdx)
769+
}
770+
}
771+
772+
/** A call to a Minimongo query method. */
773+
private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode {
774+
int queryArgIdx;
775+
776+
QueryCall() {
777+
exists(string m | this = getACollection(DataFlow::TypeTracker::end()).getAMethodCall(m) |
778+
CollectionMethodSignatures::interpretsArgumentAsQuery(m, queryArgIdx)
779+
)
780+
}
781+
782+
override DataFlow::Node getAQueryArgument() { result = getArgument(queryArgIdx) }
783+
}
784+
785+
/**
786+
* An expression that is interpreted as a Minimongo query.
787+
*/
788+
class Query extends NoSQL::Query {
789+
Query() { this = any(QueryCall qc).getAQueryArgument().asExpr() }
790+
791+
override DataFlow::Node getACodeOperator() {
792+
result = getADollarWherePropertyValue(this.flow())
793+
}
794+
}
795+
}
796+
797+
/**
798+
* Provides classes modeling the MarsDB library.
799+
*/
800+
private module MarsDB {
801+
/**
802+
* Gets an expression that may refer to a MarsDB database.
803+
*/
804+
private DataFlow::SourceNode getADb(DataFlow::TypeTracker t) {
805+
t.start() and
806+
// Collection = require('marsdb')
807+
result = DataFlow::moduleImport("marsdb")
808+
or
809+
exists(DataFlow::TypeTracker t2 | result = getADb(t2).track(t2, t))
810+
}
811+
812+
/** Gets a data flow node referring to a MarsDB collection. */
813+
private DataFlow::SourceNode getACollection(DataFlow::TypeTracker t) {
814+
t.start() and
815+
// new Collection(...)
816+
result =
817+
getADb(DataFlow::TypeTracker::end()).getAPropertyRead("Collection").getAnInstantiation()
818+
or
819+
exists(DataFlow::TypeTracker t2 | result = getACollection(t2).track(t2, t))
820+
}
821+
822+
/** A call to a MarsDB query method. */
823+
private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode {
824+
int queryArgIdx;
825+
826+
QueryCall() {
827+
exists(string m | this = getACollection(DataFlow::TypeTracker::end()).getAMethodCall(m) |
828+
// implements parts of the Minimongo interface
829+
Minimongo::CollectionMethodSignatures::interpretsArgumentAsQuery(m, queryArgIdx)
830+
)
831+
}
832+
833+
override DataFlow::Node getAQueryArgument() { result = getArgument(queryArgIdx) }
834+
}
835+
836+
/**
837+
* An expression that is interpreted as a MarsDB query.
838+
*/
839+
class Query extends NoSQL::Query {
840+
Query() { this = any(QueryCall qc).getAQueryArgument().asExpr() }
841+
842+
override DataFlow::Node getACodeOperator() {
843+
result = getADollarWherePropertyValue(this.flow())
844+
}
845+
}
846+
}

‎javascript/ql/src/semmle/javascript/heuristics/AdditionalSinks.qll

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ abstract class HeuristicSink extends DataFlow::Node { }
2525

2626
private class HeuristicCodeInjectionSink extends HeuristicSink, CodeInjection::Sink {
2727
HeuristicCodeInjectionSink() {
28+
isAssignedTo(this, "$where")
29+
or
2830
isAssignedToOrConcatenatedWith(this, "(?i)(command|cmd|exec|code|script|program)")
2931
or
3032
isArgTo(this, "(?i)(eval|run)") // "exec" clashes too often with `RegExp.prototype.exec`

‎javascript/ql/src/semmle/javascript/security/dataflow/CodeInjectionCustomizations.qll

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,4 +115,11 @@ module CodeInjection {
115115
)
116116
}
117117
}
118+
119+
/**
120+
* A code operator of a NoSQL query as a code injection sink.
121+
*/
122+
class NoSQLCodeInjectionSink extends Sink {
123+
NoSQLCodeInjectionSink() { any(NoSQL::Query q).getACodeOperator() = this }
124+
}
118125
}

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
| marsdb-flow-to.js:14:3:14:22 | db.myDoc.find(query) |
2+
| marsdb.js:16:3:16:17 | doc.find(query) |
3+
| minimongo.js:18:3:18:17 | doc.find(query) |
14
| mongodb.js:18:7:18:21 | doc.find(query) |
25
| mongodb.js:21:7:21:48 | doc.fin ... itle }) |
36
| mongodb.js:24:7:24:53 | doc.fin ... r(1) }) |

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

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,25 @@
11
nodes
2+
| marsdb-flow-to.js:10:9:10:18 | query |
3+
| marsdb-flow-to.js:10:17:10:18 | {} |
4+
| marsdb-flow-to.js:11:17:11:24 | req.body |
5+
| marsdb-flow-to.js:11:17:11:24 | req.body |
6+
| marsdb-flow-to.js:11:17:11:30 | req.body.title |
7+
| marsdb-flow-to.js:14:17:14:21 | query |
8+
| marsdb-flow-to.js:14:17:14:21 | query |
9+
| marsdb.js:12:9:12:18 | query |
10+
| marsdb.js:12:17:12:18 | {} |
11+
| marsdb.js:13:17:13:24 | req.body |
12+
| marsdb.js:13:17:13:24 | req.body |
13+
| marsdb.js:13:17:13:30 | req.body.title |
14+
| marsdb.js:16:12:16:16 | query |
15+
| marsdb.js:16:12:16:16 | query |
16+
| minimongo.js:14:9:14:18 | query |
17+
| minimongo.js:14:17:14:18 | {} |
18+
| minimongo.js:15:17:15:24 | req.body |
19+
| minimongo.js:15:17:15:24 | req.body |
20+
| minimongo.js:15:17:15:30 | req.body.title |
21+
| minimongo.js:18:12:18:16 | query |
22+
| minimongo.js:18:12:18:16 | query |
223
| mongodb.js:12:11:12:20 | query |
324
| mongodb.js:12:19:12:20 | {} |
425
| mongodb.js:13:19:13:26 | req.body |
@@ -150,6 +171,33 @@ nodes
150171
| tst.js:10:46:10:58 | req.params.id |
151172
| tst.js:10:46:10:58 | req.params.id |
152173
edges
174+
| marsdb-flow-to.js:10:9:10:18 | query | marsdb-flow-to.js:14:17:14:21 | query |
175+
| marsdb-flow-to.js:10:9:10:18 | query | marsdb-flow-to.js:14:17:14:21 | query |
176+
| marsdb-flow-to.js:10:17:10:18 | {} | marsdb-flow-to.js:10:9:10:18 | query |
177+
| marsdb-flow-to.js:11:17:11:24 | req.body | marsdb-flow-to.js:11:17:11:30 | req.body.title |
178+
| marsdb-flow-to.js:11:17:11:24 | req.body | marsdb-flow-to.js:11:17:11:30 | req.body.title |
179+
| marsdb-flow-to.js:11:17:11:30 | req.body.title | marsdb-flow-to.js:10:9:10:18 | query |
180+
| marsdb-flow-to.js:11:17:11:30 | req.body.title | marsdb-flow-to.js:10:17:10:18 | {} |
181+
| marsdb-flow-to.js:11:17:11:30 | req.body.title | marsdb-flow-to.js:14:17:14:21 | query |
182+
| marsdb-flow-to.js:11:17:11:30 | req.body.title | marsdb-flow-to.js:14:17:14:21 | query |
183+
| marsdb.js:12:9:12:18 | query | marsdb.js:16:12:16:16 | query |
184+
| marsdb.js:12:9:12:18 | query | marsdb.js:16:12:16:16 | query |
185+
| marsdb.js:12:17:12:18 | {} | marsdb.js:12:9:12:18 | query |
186+
| marsdb.js:13:17:13:24 | req.body | marsdb.js:13:17:13:30 | req.body.title |
187+
| marsdb.js:13:17:13:24 | req.body | marsdb.js:13:17:13:30 | req.body.title |
188+
| marsdb.js:13:17:13:30 | req.body.title | marsdb.js:12:9:12:18 | query |
189+
| marsdb.js:13:17:13:30 | req.body.title | marsdb.js:12:17:12:18 | {} |
190+
| marsdb.js:13:17:13:30 | req.body.title | marsdb.js:16:12:16:16 | query |
191+
| marsdb.js:13:17:13:30 | req.body.title | marsdb.js:16:12:16:16 | query |
192+
| minimongo.js:14:9:14:18 | query | minimongo.js:18:12:18:16 | query |
193+
| minimongo.js:14:9:14:18 | query | minimongo.js:18:12:18:16 | query |
194+
| minimongo.js:14:17:14:18 | {} | minimongo.js:14:9:14:18 | query |
195+
| minimongo.js:15:17:15:24 | req.body | minimongo.js:15:17:15:30 | req.body.title |
196+
| minimongo.js:15:17:15:24 | req.body | minimongo.js:15:17:15:30 | req.body.title |
197+
| minimongo.js:15:17:15:30 | req.body.title | minimongo.js:14:9:14:18 | query |
198+
| minimongo.js:15:17:15:30 | req.body.title | minimongo.js:14:17:14:18 | {} |
199+
| minimongo.js:15:17:15:30 | req.body.title | minimongo.js:18:12:18:16 | query |
200+
| minimongo.js:15:17:15:30 | req.body.title | minimongo.js:18:12:18:16 | query |
153201
| mongodb.js:12:11:12:20 | query | mongodb.js:18:16:18:20 | query |
154202
| mongodb.js:12:11:12:20 | query | mongodb.js:18:16:18:20 | query |
155203
| mongodb.js:12:19:12:20 | {} | mongodb.js:12:11:12:20 | query |
@@ -371,6 +419,9 @@ edges
371419
| tst.js:10:46:10:58 | req.params.id | tst.js:10:10:10:64 | 'SELECT ... d + '"' |
372420
| tst.js:10:46:10:58 | req.params.id | tst.js:10:10:10:64 | 'SELECT ... d + '"' |
373421
#select
422+
| marsdb-flow-to.js:14:17:14:21 | query | marsdb-flow-to.js:11:17:11:24 | req.body | marsdb-flow-to.js:14:17:14:21 | query | This query depends on $@. | marsdb-flow-to.js:11:17:11:24 | req.body | a user-provided value |
423+
| marsdb.js:16:12:16:16 | query | marsdb.js:13:17:13:24 | req.body | marsdb.js:16:12:16:16 | query | This query depends on $@. | marsdb.js:13:17:13:24 | req.body | a user-provided value |
424+
| minimongo.js:18:12:18:16 | query | minimongo.js:15:17:15:24 | req.body | minimongo.js:18:12:18:16 | query | This query depends on $@. | minimongo.js:15:17:15:24 | req.body | a user-provided value |
374425
| mongodb.js:18:16:18:20 | query | mongodb.js:13:19:13:26 | req.body | mongodb.js:18:16:18:20 | query | This query depends on $@. | mongodb.js:13:19:13:26 | req.body | a user-provided value |
375426
| mongodb.js:32:18:32:45 | { title ... itle) } | mongodb.js:26:19:26:26 | req.body | mongodb.js:32:18:32:45 | { title ... itle) } | This query depends on $@. | mongodb.js:26:19:26:26 | req.body | a user-provided value |
376427
| mongodb.js:54:16:54:20 | query | mongodb.js:49:19:49:33 | req.query.title | mongodb.js:54:16:54:20 | query | This query depends on $@. | mongodb.js:49:19:49:33 | req.query.title | a user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-089/untyped/marsdb-flow-from.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
const MarsDB = require("marsdb");
2+
3+
const myDoc = new MarsDB.Collection("myDoc");
4+
5+
const db = {
6+
myDoc
7+
};
8+
9+
module.exports = db;

‎javascript/ql/test/query-tests/Security/CWE-089/untyped/marsdb-flow-to.js

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
const express = require("express"),
2+
bodyParser = require("body-parser"),
3+
db = require('./marsdb-flow-from');
4+
5+
const app = express();
6+
7+
app.use(bodyParser.urlencoded({ extended: true }));
8+
9+
app.post("/documents/find", (req, res) => {
10+
const query = {};
11+
query.title = req.body.title;
12+
13+
// NOT OK: query is tainted by user-provided object value
14+
db.myDoc.find(query);
15+
});

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
const express = require("express"),
2+
MarsDB = require("marsdb"),
3+
bodyParser = require("body-parser");
4+
5+
let doc = new MarsDB.Collection("myDoc");
6+
7+
const app = express();
8+
9+
app.use(bodyParser.urlencoded({ extended: true }));
10+
11+
app.post("/documents/find", (req, res) => {
12+
const query = {};
13+
query.title = req.body.title;
14+
15+
// NOT OK: query is tainted by user-provided object value
16+
doc.find(query);
17+
});

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
const express = require("express"),
2+
minimongo = require("minimongo"),
3+
bodyParser = require("body-parser");
4+
5+
var LocalDb = minimongo.MemoryDb,
6+
db = new LocalDb(),
7+
doc = db.myDocs;
8+
9+
const app = express();
10+
11+
app.use(bodyParser.urlencoded({ extended: true }));
12+
13+
app.post("/documents/find", (req, res) => {
14+
const query = {};
15+
query.title = req.body.title;
16+
17+
// NOT OK: query is tainted by user-provided object value
18+
doc.find(query);
19+
});

0 commit comments

Comments
 (0)