Skip to content

Commit 70a28f6

Browse files
erik-krogherik-krogh
committed
Merge branch 'master' of https://github.com/github/codeql into pr/erik-krogh/3478
2 parents c6276dd + 0da1e68 commit 70a28f6

File tree

9 files changed

+119
-20
lines changed

9 files changed

+119
-20
lines changed

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@
1111
- [marsdb](https://www.npmjs.com/package/marsdb)
1212
- [minimongo](https://www.npmjs.com/package/minimongo/)
1313
- [Promise](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise)
14+
- [ssh2](https://www.npmjs.com/package/ssh2)
15+
- [ssh2-streams](https://www.npmjs.com/package/ssh2-streams)
1416

1517
## New queries
1618

‎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/frameworks/SystemCommandExecutors.qll

Lines changed: 33 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -51,13 +51,9 @@ private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::I
5151
)
5252
or
5353
shell = true and
54-
(
55-
mod = "exec" and
56-
optionsArg = -2 and
57-
cmdArg = 0
58-
or
59-
mod = "remote-exec" and cmdArg = 1 and optionsArg = -1
60-
)
54+
mod = "exec" and
55+
optionsArg = -2 and
56+
cmdArg = 0
6157
) and
6258
callee = DataFlow::moduleImport(mod)
6359
|
@@ -97,3 +93,33 @@ private boolean getSync(string name) {
9793
then result = true
9894
else result = false
9995
}
96+
97+
private class RemoteCommandExecutor extends SystemCommandExecution, DataFlow::InvokeNode {
98+
int cmdArg;
99+
100+
RemoteCommandExecutor() {
101+
this = DataFlow::moduleImport("remote-exec").getACall() and
102+
cmdArg = 1
103+
or
104+
exists(DataFlow::SourceNode ssh2, DataFlow::SourceNode client |
105+
ssh2 = DataFlow::moduleImport("ssh2") and
106+
(client = ssh2 or client = ssh2.getAPropertyRead("Client")) and
107+
this = client.getAnInstantiation().getAMethodCall("exec") and
108+
cmdArg = 0
109+
)
110+
or
111+
exists(DataFlow::SourceNode ssh2stream |
112+
ssh2stream = DataFlow::moduleMember("ssh2-streams", "SSH2Stream") and
113+
this = ssh2stream.getAnInstantiation().getAMethodCall("exec") and
114+
cmdArg = 1
115+
)
116+
}
117+
118+
override DataFlow::Node getACommandArgument() { result = getArgument(cmdArg) }
119+
120+
override predicate isShellInterpreted(DataFlow::Node arg) { arg = getACommandArgument() }
121+
122+
override predicate isSync() { none() }
123+
124+
override DataFlow::Node getOptionsArg() { none() }
125+
}

‎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-078/CommandInjection.expected

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,12 @@ nodes
9393
| other.js:18:22:18:24 | cmd |
9494
| other.js:19:36:19:38 | cmd |
9595
| other.js:19:36:19:38 | cmd |
96+
| other.js:22:21:22:23 | cmd |
97+
| other.js:22:21:22:23 | cmd |
98+
| other.js:23:28:23:30 | cmd |
99+
| other.js:23:28:23:30 | cmd |
100+
| other.js:26:34:26:36 | cmd |
101+
| other.js:26:34:26:36 | cmd |
96102
| third-party-command-injection.js:5:20:5:26 | command |
97103
| third-party-command-injection.js:5:20:5:26 | command |
98104
| third-party-command-injection.js:6:21:6:27 | command |
@@ -184,6 +190,12 @@ edges
184190
| other.js:5:9:5:49 | cmd | other.js:18:22:18:24 | cmd |
185191
| other.js:5:9:5:49 | cmd | other.js:19:36:19:38 | cmd |
186192
| other.js:5:9:5:49 | cmd | other.js:19:36:19:38 | cmd |
193+
| other.js:5:9:5:49 | cmd | other.js:22:21:22:23 | cmd |
194+
| other.js:5:9:5:49 | cmd | other.js:22:21:22:23 | cmd |
195+
| other.js:5:9:5:49 | cmd | other.js:23:28:23:30 | cmd |
196+
| other.js:5:9:5:49 | cmd | other.js:23:28:23:30 | cmd |
197+
| other.js:5:9:5:49 | cmd | other.js:26:34:26:36 | cmd |
198+
| other.js:5:9:5:49 | cmd | other.js:26:34:26:36 | cmd |
187199
| other.js:5:15:5:38 | url.par ... , true) | other.js:5:15:5:44 | url.par ... ).query |
188200
| other.js:5:15:5:44 | url.par ... ).query | other.js:5:15:5:49 | url.par ... ry.path |
189201
| other.js:5:15:5:49 | url.par ... ry.path | other.js:5:9:5:49 | cmd |
@@ -226,4 +238,7 @@ edges
226238
| other.js:17:27:17:29 | cmd | other.js:5:25:5:31 | req.url | other.js:17:27:17:29 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
227239
| other.js:18:22:18:24 | cmd | other.js:5:25:5:31 | req.url | other.js:18:22:18:24 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
228240
| other.js:19:36:19:38 | cmd | other.js:5:25:5:31 | req.url | other.js:19:36:19:38 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
241+
| other.js:22:21:22:23 | cmd | other.js:5:25:5:31 | req.url | other.js:22:21:22:23 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
242+
| other.js:23:28:23:30 | cmd | other.js:5:25:5:31 | req.url | other.js:23:28:23:30 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
243+
| other.js:26:34:26:36 | cmd | other.js:5:25:5:31 | req.url | other.js:26:34:26:36 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
229244
| third-party-command-injection.js:6:21:6:27 | command | third-party-command-injection.js:5:20:5:26 | command | third-party-command-injection.js:6:21:6:27 | command | This command depends on $@. | third-party-command-injection.js:5:20:5:26 | command | a server-provided value |

‎javascript/ql/test/query-tests/Security/CWE-078/other.js

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,4 +17,11 @@ var server = http.createServer(function(req, res) {
1717
require("exec-async")(cmd); // NOT OK
1818
require("execa")(cmd); // NOT OK
1919
require("remote-exec")(target, cmd); // NOT OK
20+
21+
const ssh2 = require("ssh2");
22+
new ssh2().exec(cmd); // NOT OK
23+
new ssh2.Client().exec(cmd); // NOT OK
24+
25+
const SSH2Stream = require("ssh2-streams").SSH2Stream;
26+
new SSH2Stream().exec(false, cmd); // NOT OK
2027
});

‎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)