Skip to content

Commit c580ada

Browse files
erik-krogherik-krogh
authored
Merge pull request #3643 from erik-krogh/yargs
JS: extend support for yargs for js/indirect-command-line-injection
2 parents 1a7570e + b04d701 commit c580ada

3 files changed

Lines changed: 77 additions & 3 deletions

File tree

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

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -50,14 +50,42 @@ module IndirectCommandInjection {
5050
// `require('minimist')(...)` => `{ _: [], a: ... b: ... }`
5151
this = DataFlow::moduleImport("minimist").getACall()
5252
or
53-
// `require('yargs').argv` => `{ _: [], a: ... b: ... }`
54-
this = DataFlow::moduleMember("yargs", "argv")
55-
or
5653
// `require('optimist').argv` => `{ _: [], a: ... b: ... }`
5754
this = DataFlow::moduleMember("optimist", "argv")
5855
}
5956
}
6057

58+
/**
59+
* Gets an instance of `yargs`.
60+
* Either directly imported as a module, or through some chained method call.
61+
*/
62+
private DataFlow::SourceNode yargs() {
63+
result = DataFlow::moduleImport("yargs")
64+
or
65+
// script used to generate list of chained methods: https://gist.github.com/erik-krogh/f8afe952c0577f4b563a993e613269ba
66+
exists(string method |
67+
not method =
68+
// the methods that does not return a chained `yargs` object.
69+
["getContext", "getDemandedOptions", "getDemandedCommands", "getDeprecatedOptions",
70+
"_getParseContext", "getOptions", "getGroups", "getStrict", "getStrictCommands",
71+
"getExitProcess", "locale", "getUsageInstance", "getCommandInstance"]
72+
|
73+
result = yargs().getAMethodCall(method)
74+
)
75+
}
76+
77+
/**
78+
* An array of command line arguments (`argv`) parsed by the `yargs` libary.
79+
*/
80+
class YargsArgv extends Source {
81+
YargsArgv() {
82+
this = yargs().getAPropertyRead("argv")
83+
or
84+
this = yargs().getAMethodCall("parse") and
85+
this.(DataFlow::MethodCallNode).getNumArgument() = 0
86+
}
87+
}
88+
6189
/**
6290
* A command-line argument that effectively is system-controlled, and therefore not likely to be exploitable when used in the execution of another command.
6391
*/

‎javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection.expected‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,17 @@ nodes
6868
| command-line-parameter-command-injection.js:33:21:33:44 | require ... ").argv |
6969
| command-line-parameter-command-injection.js:33:21:33:44 | require ... ").argv |
7070
| command-line-parameter-command-injection.js:33:21:33:48 | require ... rgv.foo |
71+
| command-line-parameter-command-injection.js:36:6:39:7 | args |
72+
| command-line-parameter-command-injection.js:36:13:39:7 | require ... \\t\\t.argv |
73+
| command-line-parameter-command-injection.js:36:13:39:7 | require ... \\t\\t.argv |
74+
| command-line-parameter-command-injection.js:41:10:41:25 | "cmd.sh " + args |
75+
| command-line-parameter-command-injection.js:41:10:41:25 | "cmd.sh " + args |
76+
| command-line-parameter-command-injection.js:41:22:41:25 | args |
77+
| command-line-parameter-command-injection.js:43:10:43:62 | "cmd.sh ... e().foo |
78+
| command-line-parameter-command-injection.js:43:10:43:62 | "cmd.sh ... e().foo |
79+
| command-line-parameter-command-injection.js:43:22:43:58 | require ... parse() |
80+
| command-line-parameter-command-injection.js:43:22:43:58 | require ... parse() |
81+
| command-line-parameter-command-injection.js:43:22:43:62 | require ... e().foo |
7182
edges
7283
| command-line-parameter-command-injection.js:4:10:4:21 | process.argv | command-line-parameter-command-injection.js:4:10:4:21 | process.argv |
7384
| command-line-parameter-command-injection.js:8:22:8:33 | process.argv | command-line-parameter-command-injection.js:8:22:8:36 | process.argv[2] |
@@ -129,6 +140,15 @@ edges
129140
| command-line-parameter-command-injection.js:33:21:33:44 | require ... ").argv | command-line-parameter-command-injection.js:33:21:33:48 | require ... rgv.foo |
130141
| command-line-parameter-command-injection.js:33:21:33:48 | require ... rgv.foo | command-line-parameter-command-injection.js:33:9:33:48 | "cmd.sh ... rgv.foo |
131142
| command-line-parameter-command-injection.js:33:21:33:48 | require ... rgv.foo | command-line-parameter-command-injection.js:33:9:33:48 | "cmd.sh ... rgv.foo |
143+
| command-line-parameter-command-injection.js:36:6:39:7 | args | command-line-parameter-command-injection.js:41:22:41:25 | args |
144+
| command-line-parameter-command-injection.js:36:13:39:7 | require ... \\t\\t.argv | command-line-parameter-command-injection.js:36:6:39:7 | args |
145+
| command-line-parameter-command-injection.js:36:13:39:7 | require ... \\t\\t.argv | command-line-parameter-command-injection.js:36:6:39:7 | args |
146+
| command-line-parameter-command-injection.js:41:22:41:25 | args | command-line-parameter-command-injection.js:41:10:41:25 | "cmd.sh " + args |
147+
| command-line-parameter-command-injection.js:41:22:41:25 | args | command-line-parameter-command-injection.js:41:10:41:25 | "cmd.sh " + args |
148+
| command-line-parameter-command-injection.js:43:22:43:58 | require ... parse() | command-line-parameter-command-injection.js:43:22:43:62 | require ... e().foo |
149+
| command-line-parameter-command-injection.js:43:22:43:58 | require ... parse() | command-line-parameter-command-injection.js:43:22:43:62 | require ... e().foo |
150+
| command-line-parameter-command-injection.js:43:22:43:62 | require ... e().foo | command-line-parameter-command-injection.js:43:10:43:62 | "cmd.sh ... e().foo |
151+
| command-line-parameter-command-injection.js:43:22:43:62 | require ... e().foo | command-line-parameter-command-injection.js:43:10:43:62 | "cmd.sh ... e().foo |
132152
#select
133153
| command-line-parameter-command-injection.js:4:10:4:21 | process.argv | command-line-parameter-command-injection.js:4:10:4:21 | process.argv | command-line-parameter-command-injection.js:4:10:4:21 | process.argv | This command depends on an unsanitized $@. | command-line-parameter-command-injection.js:4:10:4:21 | process.argv | command-line argument |
134154
| command-line-parameter-command-injection.js:8:10:8:36 | "cmd.sh ... argv[2] | command-line-parameter-command-injection.js:8:22:8:33 | process.argv | command-line-parameter-command-injection.js:8:10:8:36 | "cmd.sh ... argv[2] | This command depends on an unsanitized $@. | command-line-parameter-command-injection.js:8:22:8:33 | process.argv | command-line argument |
@@ -144,3 +164,5 @@ edges
144164
| command-line-parameter-command-injection.js:31:9:31:45 | "cmd.sh ... )().foo | command-line-parameter-command-injection.js:31:21:31:41 | require ... ist")() | command-line-parameter-command-injection.js:31:9:31:45 | "cmd.sh ... )().foo | This command depends on an unsanitized $@. | command-line-parameter-command-injection.js:31:21:31:41 | require ... ist")() | command-line argument |
145165
| command-line-parameter-command-injection.js:32:9:32:45 | "cmd.sh ... rgv.foo | command-line-parameter-command-injection.js:32:21:32:41 | require ... ").argv | command-line-parameter-command-injection.js:32:9:32:45 | "cmd.sh ... rgv.foo | This command depends on an unsanitized $@. | command-line-parameter-command-injection.js:32:21:32:41 | require ... ").argv | command-line argument |
146166
| command-line-parameter-command-injection.js:33:9:33:48 | "cmd.sh ... rgv.foo | command-line-parameter-command-injection.js:33:21:33:44 | require ... ").argv | command-line-parameter-command-injection.js:33:9:33:48 | "cmd.sh ... rgv.foo | This command depends on an unsanitized $@. | command-line-parameter-command-injection.js:33:21:33:44 | require ... ").argv | command-line argument |
167+
| command-line-parameter-command-injection.js:41:10:41:25 | "cmd.sh " + args | command-line-parameter-command-injection.js:36:13:39:7 | require ... \\t\\t.argv | command-line-parameter-command-injection.js:41:10:41:25 | "cmd.sh " + args | This command depends on an unsanitized $@. | command-line-parameter-command-injection.js:36:13:39:7 | require ... \\t\\t.argv | command-line argument |
168+
| command-line-parameter-command-injection.js:43:10:43:62 | "cmd.sh ... e().foo | command-line-parameter-command-injection.js:43:22:43:58 | require ... parse() | command-line-parameter-command-injection.js:43:10:43:62 | "cmd.sh ... e().foo | This command depends on an unsanitized $@. | command-line-parameter-command-injection.js:43:22:43:58 | require ... parse() | command-line argument |

‎javascript/ql/test/query-tests/Security/CWE-078/command-line-parameter-command-injection.js‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,3 +31,27 @@ cp.exec("cmd.sh " + require("get-them-args")().foo); // NOT OK
3131
cp.exec("cmd.sh " + require("minimist")().foo); // NOT OK
3232
cp.exec("cmd.sh " + require("yargs").argv.foo); // NOT OK
3333
cp.exec("cmd.sh " + require("optimist").argv.foo); // NOT OK
34+
35+
(function () {
36+
var args = require('yargs') // eslint-disable-line
37+
.command('serve [port]', 'start the server', (yargs) => { })
38+
.option('verbose', { foo: "bar" })
39+
.argv
40+
41+
cp.exec("cmd.sh " + args); // NOT OK
42+
43+
cp.exec("cmd.sh " + require("yargs").array("foo").parse().foo); // NOT OK
44+
});
45+
46+
(function () {
47+
const {
48+
argv: {
49+
...args
50+
},
51+
} = require('yargs')
52+
.usage('Usage: foo bar')
53+
.command();
54+
55+
cp.exec("cmd.sh " + args); // NOT OK - but not flagged yet.
56+
});
57+

0 commit comments

Comments
 (0)