Skip to content

Commit 14664be

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request github#3468 from p0/imp/nodejs-vm-sinks
Approved by esbena
2 parents 6041d52 + 3cc13db commit 14664be

File tree

5 files changed

+62
-13
lines changed

5 files changed

+62
-13
lines changed

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

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -710,23 +710,25 @@ module NodeJSLib {
710710
}
711711

712712
/**
713-
* A call to a method from module `vm`
713+
* DEPRECATED Use `VmModuleMemberInvocation` instead.
714714
*/
715-
class VmModuleMethodCall extends DataFlow::CallNode {
716-
string methodName;
715+
deprecated class VmModuleMethodCall = VmModuleMemberInvocation;
716+
717+
/**
718+
* An invocation of a member from module `vm`
719+
*/
720+
class VmModuleMemberInvocation extends DataFlow::InvokeNode {
721+
string memberName;
717722

718-
VmModuleMethodCall() { this = DataFlow::moduleMember("vm", methodName).getACall() }
723+
VmModuleMemberInvocation() { this = DataFlow::moduleMember("vm", memberName).getAnInvocation() }
719724

720725
/**
721-
* Gets the code to be executed as part of this call.
726+
* Gets the code to be executed as part of this invocation.
722727
*/
723728
DataFlow::Node getACodeArgument() {
724-
(
725-
methodName = "runInContext" or
726-
methodName = "runInNewContext" or
727-
methodName = "runInThisContext"
728-
) and
729-
// all of the above methods take the command as their first argument
729+
memberName in ["Script", "SourceTextModule", "compileFunction", "runInContext",
730+
"runInNewContext", "runInThisContext"] and
731+
// all of the above methods/constructors take the command as their first argument
730732
result = getArgument(0)
731733
}
732734
}

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,9 @@ module CodeInjection {
4848
* `vm` module.
4949
*/
5050
class NodeJSVmSink extends Sink, DataFlow::ValueNode {
51-
NodeJSVmSink() { exists(NodeJSLib::VmModuleMethodCall call | this = call.getACodeArgument()) }
51+
NodeJSVmSink() {
52+
exists(NodeJSLib::VmModuleMemberInvocation inv | this = inv.getACodeArgument())
53+
}
5254
}
5355

5456
/**

‎javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.expected

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,18 @@ nodes
7676
| express.js:12:8:12:53 | "return ... + "];" |
7777
| express.js:12:28:12:46 | req.param("wobble") |
7878
| express.js:12:28:12:46 | req.param("wobble") |
79+
| express.js:15:22:15:54 | req.par ... ction") |
80+
| express.js:15:22:15:54 | req.par ... ction") |
81+
| express.js:15:22:15:54 | req.par ... ction") |
82+
| express.js:17:30:17:53 | req.par ... cript") |
83+
| express.js:17:30:17:53 | req.par ... cript") |
84+
| express.js:17:30:17:53 | req.par ... cript") |
85+
| express.js:19:37:19:70 | req.par ... odule") |
86+
| express.js:19:37:19:70 | req.par ... odule") |
87+
| express.js:19:37:19:70 | req.par ... odule") |
88+
| express.js:21:19:21:48 | req.par ... ntext") |
89+
| express.js:21:19:21:48 | req.par ... ntext") |
90+
| express.js:21:19:21:48 | req.par ... ntext") |
7991
| react-native.js:7:7:7:33 | tainted |
8092
| react-native.js:7:17:7:33 | req.param("code") |
8193
| react-native.js:7:17:7:33 | req.param("code") |
@@ -193,6 +205,10 @@ edges
193205
| express.js:12:28:12:46 | req.param("wobble") | express.js:12:8:12:53 | "return ... + "];" |
194206
| express.js:12:28:12:46 | req.param("wobble") | express.js:12:8:12:53 | "return ... + "];" |
195207
| express.js:12:28:12:46 | req.param("wobble") | express.js:12:8:12:53 | "return ... + "];" |
208+
| express.js:15:22:15:54 | req.par ... ction") | express.js:15:22:15:54 | req.par ... ction") |
209+
| express.js:17:30:17:53 | req.par ... cript") | express.js:17:30:17:53 | req.par ... cript") |
210+
| express.js:19:37:19:70 | req.par ... odule") | express.js:19:37:19:70 | req.par ... odule") |
211+
| express.js:21:19:21:48 | req.par ... ntext") | express.js:21:19:21:48 | req.par ... ntext") |
196212
| react-native.js:7:7:7:33 | tainted | react-native.js:8:32:8:38 | tainted |
197213
| react-native.js:7:7:7:33 | tainted | react-native.js:8:32:8:38 | tainted |
198214
| react-native.js:7:7:7:33 | tainted | react-native.js:10:23:10:29 | tainted |
@@ -248,6 +264,10 @@ edges
248264
| express.js:7:24:7:69 | "return ... + "];" | express.js:7:44:7:62 | req.param("wobble") | express.js:7:24:7:69 | "return ... + "];" | $@ flows to here and is interpreted as code. | express.js:7:44:7:62 | req.param("wobble") | User-provided value |
249265
| express.js:9:34:9:79 | "return ... + "];" | express.js:9:54:9:72 | req.param("wobble") | express.js:9:34:9:79 | "return ... + "];" | $@ flows to here and is interpreted as code. | express.js:9:54:9:72 | req.param("wobble") | User-provided value |
250266
| express.js:12:8:12:53 | "return ... + "];" | express.js:12:28:12:46 | req.param("wobble") | express.js:12:8:12:53 | "return ... + "];" | $@ flows to here and is interpreted as code. | express.js:12:28:12:46 | req.param("wobble") | User-provided value |
267+
| express.js:15:22:15:54 | req.par ... ction") | express.js:15:22:15:54 | req.par ... ction") | express.js:15:22:15:54 | req.par ... ction") | $@ flows to here and is interpreted as code. | express.js:15:22:15:54 | req.par ... ction") | User-provided value |
268+
| express.js:17:30:17:53 | req.par ... cript") | express.js:17:30:17:53 | req.par ... cript") | express.js:17:30:17:53 | req.par ... cript") | $@ flows to here and is interpreted as code. | express.js:17:30:17:53 | req.par ... cript") | User-provided value |
269+
| express.js:19:37:19:70 | req.par ... odule") | express.js:19:37:19:70 | req.par ... odule") | express.js:19:37:19:70 | req.par ... odule") | $@ flows to here and is interpreted as code. | express.js:19:37:19:70 | req.par ... odule") | User-provided value |
270+
| express.js:21:19:21:48 | req.par ... ntext") | express.js:21:19:21:48 | req.par ... ntext") | express.js:21:19:21:48 | req.par ... ntext") | $@ flows to here and is interpreted as code. | express.js:21:19:21:48 | req.par ... ntext") | User-provided value |
251271
| react-native.js:8:32:8:38 | tainted | react-native.js:7:17:7:33 | req.param("code") | react-native.js:8:32:8:38 | tainted | $@ flows to here and is interpreted as code. | react-native.js:7:17:7:33 | req.param("code") | User-provided value |
252272
| react-native.js:10:23:10:29 | tainted | react-native.js:7:17:7:33 | req.param("code") | react-native.js:10:23:10:29 | tainted | $@ flows to here and is interpreted as code. | react-native.js:7:17:7:33 | req.param("code") | User-provided value |
253273
| tst.js:2:6:2:83 | documen ... t=")+8) | tst.js:2:6:2:22 | document.location | tst.js:2:6:2:83 | documen ... t=")+8) | $@ flows to here and is interpreted as code. | tst.js:2:6:2:22 | document.location | User-provided value |

‎javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/HeuristicSourceCodeInjection.expected

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,18 @@ nodes
8080
| express.js:12:8:12:53 | "return ... + "];" |
8181
| express.js:12:28:12:46 | req.param("wobble") |
8282
| express.js:12:28:12:46 | req.param("wobble") |
83+
| express.js:15:22:15:54 | req.par ... ction") |
84+
| express.js:15:22:15:54 | req.par ... ction") |
85+
| express.js:15:22:15:54 | req.par ... ction") |
86+
| express.js:17:30:17:53 | req.par ... cript") |
87+
| express.js:17:30:17:53 | req.par ... cript") |
88+
| express.js:17:30:17:53 | req.par ... cript") |
89+
| express.js:19:37:19:70 | req.par ... odule") |
90+
| express.js:19:37:19:70 | req.par ... odule") |
91+
| express.js:19:37:19:70 | req.par ... odule") |
92+
| express.js:21:19:21:48 | req.par ... ntext") |
93+
| express.js:21:19:21:48 | req.par ... ntext") |
94+
| express.js:21:19:21:48 | req.par ... ntext") |
8395
| react-native.js:7:7:7:33 | tainted |
8496
| react-native.js:7:17:7:33 | req.param("code") |
8597
| react-native.js:7:17:7:33 | req.param("code") |
@@ -201,6 +213,10 @@ edges
201213
| express.js:12:28:12:46 | req.param("wobble") | express.js:12:8:12:53 | "return ... + "];" |
202214
| express.js:12:28:12:46 | req.param("wobble") | express.js:12:8:12:53 | "return ... + "];" |
203215
| express.js:12:28:12:46 | req.param("wobble") | express.js:12:8:12:53 | "return ... + "];" |
216+
| express.js:15:22:15:54 | req.par ... ction") | express.js:15:22:15:54 | req.par ... ction") |
217+
| express.js:17:30:17:53 | req.par ... cript") | express.js:17:30:17:53 | req.par ... cript") |
218+
| express.js:19:37:19:70 | req.par ... odule") | express.js:19:37:19:70 | req.par ... odule") |
219+
| express.js:21:19:21:48 | req.par ... ntext") | express.js:21:19:21:48 | req.par ... ntext") |
204220
| react-native.js:7:7:7:33 | tainted | react-native.js:8:32:8:38 | tainted |
205221
| react-native.js:7:7:7:33 | tainted | react-native.js:8:32:8:38 | tainted |
206222
| react-native.js:7:7:7:33 | tainted | react-native.js:10:23:10:29 | tainted |

‎javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/express.js

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,5 +9,14 @@ app.get('/some/path', function(req, res) {
99
require("vm").runInThisContext("return wibbles[" + req.param("wobble") + "];");
1010
var runC = require("vm").runInNewContext;
1111
// NOT OK
12-
runC("return wibbles[" + req.param("wobble") + "];");
12+
runC("return wibbles[" + req.param("wobble") + "];");
13+
var vm = require("vm");
14+
// NOT OK
15+
vm.compileFunction(req.param("code_compileFunction"));
16+
// NOT OK
17+
var script = new vm.Script(req.param("code_Script"));
18+
// NOT OK
19+
var mdl = new vm.SourceTextModule(req.param("code_SourceTextModule"));
20+
// NOT OK
21+
vm.runInContext(req.param("code_runInContext"), vm.createContext());
1322
});

0 commit comments

Comments
 (0)