Skip to content

Commit ab2d059

Browse files
pavgustpavgust
committed
JavaScript: Model extra sinks in vm module
1 parent bc7163a commit ab2d059

File tree

5 files changed

+54
-10
lines changed

5 files changed

+54
-10
lines changed

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

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -705,20 +705,17 @@ module NodeJSLib {
705705
/**
706706
* A call to a method from module `vm`
707707
*/
708-
class VmModuleMethodCall extends DataFlow::CallNode {
709-
string methodName;
708+
class VmModuleMemberInvocation extends DataFlow::InvokeNode {
709+
string memberName;
710710

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

713713
/**
714714
* Gets the code to be executed as part of this call.
715715
*/
716716
DataFlow::Node getACodeArgument() {
717-
(
718-
methodName = "runInContext" or
719-
methodName = "runInNewContext" or
720-
methodName = "runInThisContext"
721-
) and
717+
memberName in ["Script", "SourceTextModule", "compileFunction", "runInContext",
718+
"runInNewContext", "runInThisContext"] and
722719
// all of the above methods take the command as their first argument
723720
result = getArgument(0)
724721
}

‎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
@@ -67,6 +67,18 @@ nodes
6767
| express.js:12:8:12:53 | "return ... + "];" |
6868
| express.js:12:28:12:46 | req.param("wobble") |
6969
| express.js:12:28:12:46 | req.param("wobble") |
70+
| express.js:15:22:15:54 | req.par ... ction") |
71+
| express.js:15:22:15:54 | req.par ... ction") |
72+
| express.js:15:22:15:54 | req.par ... ction") |
73+
| express.js:17:30:17:53 | req.par ... cript") |
74+
| express.js:17:30:17:53 | req.par ... cript") |
75+
| express.js:17:30:17:53 | req.par ... cript") |
76+
| express.js:19:37:19:70 | req.par ... odule") |
77+
| express.js:19:37:19:70 | req.par ... odule") |
78+
| express.js:19:37:19:70 | req.par ... odule") |
79+
| express.js:21:19:21:48 | req.par ... ntext") |
80+
| express.js:21:19:21:48 | req.par ... ntext") |
81+
| express.js:21:19:21:48 | req.par ... ntext") |
7082
| react-native.js:7:7:7:33 | tainted |
7183
| react-native.js:7:17:7:33 | req.param("code") |
7284
| react-native.js:7:17:7:33 | req.param("code") |
@@ -176,6 +188,10 @@ edges
176188
| express.js:12:28:12:46 | req.param("wobble") | express.js:12:8:12:53 | "return ... + "];" |
177189
| express.js:12:28:12:46 | req.param("wobble") | express.js:12:8:12:53 | "return ... + "];" |
178190
| express.js:12:28:12:46 | req.param("wobble") | express.js:12:8:12:53 | "return ... + "];" |
191+
| express.js:15:22:15:54 | req.par ... ction") | express.js:15:22:15:54 | req.par ... ction") |
192+
| express.js:17:30:17:53 | req.par ... cript") | express.js:17:30:17:53 | req.par ... cript") |
193+
| express.js:19:37:19:70 | req.par ... odule") | express.js:19:37:19:70 | req.par ... odule") |
194+
| express.js:21:19:21:48 | req.par ... ntext") | express.js:21:19:21:48 | req.par ... ntext") |
179195
| react-native.js:7:7:7:33 | tainted | react-native.js:8:32:8:38 | tainted |
180196
| react-native.js:7:7:7:33 | tainted | react-native.js:8:32:8:38 | tainted |
181197
| react-native.js:7:7:7:33 | tainted | react-native.js:10:23:10:29 | tainted |
@@ -229,6 +245,10 @@ edges
229245
| 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 |
230246
| 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 |
231247
| 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 |
248+
| 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 |
249+
| 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 |
250+
| 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 |
251+
| 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 |
232252
| 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 |
233253
| 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 |
234254
| 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
@@ -71,6 +71,18 @@ nodes
7171
| express.js:12:8:12:53 | "return ... + "];" |
7272
| express.js:12:28:12:46 | req.param("wobble") |
7373
| express.js:12:28:12:46 | req.param("wobble") |
74+
| express.js:15:22:15:54 | req.par ... ction") |
75+
| express.js:15:22:15:54 | req.par ... ction") |
76+
| express.js:15:22:15:54 | req.par ... ction") |
77+
| express.js:17:30:17:53 | req.par ... cript") |
78+
| express.js:17:30:17:53 | req.par ... cript") |
79+
| express.js:17:30:17:53 | req.par ... cript") |
80+
| express.js:19:37:19:70 | req.par ... odule") |
81+
| express.js:19:37:19:70 | req.par ... odule") |
82+
| express.js:19:37:19:70 | req.par ... odule") |
83+
| express.js:21:19:21:48 | req.par ... ntext") |
84+
| express.js:21:19:21:48 | req.par ... ntext") |
85+
| express.js:21:19:21:48 | req.par ... ntext") |
7486
| react-native.js:7:7:7:33 | tainted |
7587
| react-native.js:7:17:7:33 | req.param("code") |
7688
| react-native.js:7:17:7:33 | req.param("code") |
@@ -184,6 +196,10 @@ edges
184196
| express.js:12:28:12:46 | req.param("wobble") | express.js:12:8:12:53 | "return ... + "];" |
185197
| express.js:12:28:12:46 | req.param("wobble") | express.js:12:8:12:53 | "return ... + "];" |
186198
| express.js:12:28:12:46 | req.param("wobble") | express.js:12:8:12:53 | "return ... + "];" |
199+
| express.js:15:22:15:54 | req.par ... ction") | express.js:15:22:15:54 | req.par ... ction") |
200+
| express.js:17:30:17:53 | req.par ... cript") | express.js:17:30:17:53 | req.par ... cript") |
201+
| express.js:19:37:19:70 | req.par ... odule") | express.js:19:37:19:70 | req.par ... odule") |
202+
| express.js:21:19:21:48 | req.par ... ntext") | express.js:21:19:21:48 | req.par ... ntext") |
187203
| react-native.js:7:7:7:33 | tainted | react-native.js:8:32:8:38 | tainted |
188204
| react-native.js:7:7:7:33 | tainted | react-native.js:8:32:8:38 | tainted |
189205
| 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)