Skip to content

Commit 0511e72

Browse files
codeql-cicodeql-ci
authored
Merge pull request github#5458 from erik-krogh/shellTrue
Approved by asgerf
2 parents 9d52db3 + c146b27 commit 0511e72

File tree

4 files changed

+171
-18
lines changed

4 files changed

+171
-18
lines changed

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

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,53 @@ module UnsafeShellCommandConstruction {
143143
override DataFlow::Node getAlertLocation() { result = this }
144144
}
145145

146+
/**
147+
* Gets a node that ends up in an array that is ultimately executed as a shell script by `sys`.
148+
*/
149+
private DataFlow::SourceNode endsInShellExecutedArray(
150+
DataFlow::TypeBackTracker t, SystemCommandExecution sys
151+
) {
152+
t.start() and
153+
result = sys.getArgumentList().getALocalSource() and
154+
// the array gets joined to a string when `shell` is set to true.
155+
sys.getOptionsArg()
156+
.getALocalSource()
157+
.getAPropertyWrite("shell")
158+
.getRhs()
159+
.asExpr()
160+
.(BooleanLiteral)
161+
.getValue() = "true"
162+
or
163+
exists(DataFlow::TypeBackTracker t2 |
164+
result = endsInShellExecutedArray(t2, sys).backtrack(t2, t)
165+
)
166+
}
167+
168+
/**
169+
* An argument to a command invocation where the `shell` option is set to true.
170+
*/
171+
class ShellTrueCommandExecutionSink extends Sink {
172+
SystemCommandExecution sys;
173+
174+
ShellTrueCommandExecutionSink() {
175+
// `shell` is set to true. That means string-concatenation happens behind the scenes.
176+
// We just assume that a `shell` option in any library means the same thing as it does in NodeJS.
177+
exists(DataFlow::SourceNode arr |
178+
arr = endsInShellExecutedArray(DataFlow::TypeBackTracker::end(), sys)
179+
|
180+
this = arr.(DataFlow::ArrayCreationNode).getAnElement()
181+
or
182+
this = arr.getAMethodCall(["push", "unshift"]).getAnArgument()
183+
)
184+
}
185+
186+
override string getSinkType() { result = "Shell argument" }
187+
188+
override SystemCommandExecution getCommandExecution() { result = sys }
189+
190+
override DataFlow::Node getAlertLocation() { result = this }
191+
}
192+
146193
/**
147194
* A sanitizer like: "'"+name.replace(/'/g,"'\\''")+"'"
148195
* Which sanitizes on Unix.

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

Lines changed: 87 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -205,14 +205,38 @@ nodes
205205
| lib/lib.js:405:39:405:42 | name |
206206
| lib/lib.js:406:22:406:25 | name |
207207
| lib/lib.js:406:22:406:25 | name |
208-
| lib/lib.js:413:39:413:42 | name |
209-
| lib/lib.js:413:39:413:42 | name |
210-
| lib/lib.js:414:24:414:27 | name |
211-
| lib/lib.js:414:24:414:27 | name |
212-
| lib/lib.js:418:20:418:23 | name |
213-
| lib/lib.js:418:20:418:23 | name |
214-
| lib/lib.js:419:25:419:28 | name |
215-
| lib/lib.js:419:25:419:28 | name |
208+
| lib/lib.js:414:40:414:43 | name |
209+
| lib/lib.js:414:40:414:43 | name |
210+
| lib/lib.js:415:22:415:25 | name |
211+
| lib/lib.js:415:22:415:25 | name |
212+
| lib/lib.js:417:28:417:31 | name |
213+
| lib/lib.js:417:28:417:31 | name |
214+
| lib/lib.js:418:25:418:28 | name |
215+
| lib/lib.js:418:25:418:28 | name |
216+
| lib/lib.js:419:32:419:35 | name |
217+
| lib/lib.js:419:32:419:35 | name |
218+
| lib/lib.js:420:29:420:32 | name |
219+
| lib/lib.js:420:29:420:32 | name |
220+
| lib/lib.js:424:24:424:27 | name |
221+
| lib/lib.js:424:24:424:27 | name |
222+
| lib/lib.js:426:11:426:14 | name |
223+
| lib/lib.js:426:11:426:14 | name |
224+
| lib/lib.js:428:28:428:51 | (name ? ... ' : '') |
225+
| lib/lib.js:428:28:428:57 | (name ? ... ) + '-' |
226+
| lib/lib.js:428:29:428:50 | name ? ... :' : '' |
227+
| lib/lib.js:428:36:428:39 | name |
228+
| lib/lib.js:428:36:428:45 | name + ':' |
229+
| lib/lib.js:431:23:431:26 | last |
230+
| lib/lib.js:436:19:436:22 | last |
231+
| lib/lib.js:436:19:436:22 | last |
232+
| lib/lib.js:441:39:441:42 | name |
233+
| lib/lib.js:441:39:441:42 | name |
234+
| lib/lib.js:442:24:442:27 | name |
235+
| lib/lib.js:442:24:442:27 | name |
236+
| lib/lib.js:446:20:446:23 | name |
237+
| lib/lib.js:446:20:446:23 | name |
238+
| lib/lib.js:447:25:447:28 | name |
239+
| lib/lib.js:447:25:447:28 | name |
216240
edges
217241
| lib/lib2.js:3:28:3:31 | name | lib/lib2.js:4:22:4:25 | name |
218242
| lib/lib2.js:3:28:3:31 | name | lib/lib2.js:4:22:4:25 | name |
@@ -452,14 +476,51 @@ edges
452476
| lib/lib.js:405:39:405:42 | name | lib/lib.js:406:22:406:25 | name |
453477
| lib/lib.js:405:39:405:42 | name | lib/lib.js:406:22:406:25 | name |
454478
| lib/lib.js:405:39:405:42 | name | lib/lib.js:406:22:406:25 | name |
455-
| lib/lib.js:413:39:413:42 | name | lib/lib.js:414:24:414:27 | name |
456-
| lib/lib.js:413:39:413:42 | name | lib/lib.js:414:24:414:27 | name |
457-
| lib/lib.js:413:39:413:42 | name | lib/lib.js:414:24:414:27 | name |
458-
| lib/lib.js:413:39:413:42 | name | lib/lib.js:414:24:414:27 | name |
459-
| lib/lib.js:418:20:418:23 | name | lib/lib.js:419:25:419:28 | name |
460-
| lib/lib.js:418:20:418:23 | name | lib/lib.js:419:25:419:28 | name |
461-
| lib/lib.js:418:20:418:23 | name | lib/lib.js:419:25:419:28 | name |
462-
| lib/lib.js:418:20:418:23 | name | lib/lib.js:419:25:419:28 | name |
479+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:415:22:415:25 | name |
480+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:415:22:415:25 | name |
481+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:415:22:415:25 | name |
482+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:415:22:415:25 | name |
483+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:417:28:417:31 | name |
484+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:417:28:417:31 | name |
485+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:417:28:417:31 | name |
486+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:417:28:417:31 | name |
487+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:418:25:418:28 | name |
488+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:418:25:418:28 | name |
489+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:418:25:418:28 | name |
490+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:418:25:418:28 | name |
491+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:419:32:419:35 | name |
492+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:419:32:419:35 | name |
493+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:419:32:419:35 | name |
494+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:419:32:419:35 | name |
495+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:420:29:420:32 | name |
496+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:420:29:420:32 | name |
497+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:420:29:420:32 | name |
498+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:420:29:420:32 | name |
499+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:424:24:424:27 | name |
500+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:424:24:424:27 | name |
501+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:424:24:424:27 | name |
502+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:424:24:424:27 | name |
503+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:426:11:426:14 | name |
504+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:426:11:426:14 | name |
505+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:426:11:426:14 | name |
506+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:426:11:426:14 | name |
507+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:428:36:428:39 | name |
508+
| lib/lib.js:414:40:414:43 | name | lib/lib.js:428:36:428:39 | name |
509+
| lib/lib.js:428:28:428:51 | (name ? ... ' : '') | lib/lib.js:428:28:428:57 | (name ? ... ) + '-' |
510+
| lib/lib.js:428:28:428:57 | (name ? ... ) + '-' | lib/lib.js:431:23:431:26 | last |
511+
| lib/lib.js:428:29:428:50 | name ? ... :' : '' | lib/lib.js:428:28:428:51 | (name ? ... ' : '') |
512+
| lib/lib.js:428:36:428:39 | name | lib/lib.js:428:36:428:45 | name + ':' |
513+
| lib/lib.js:428:36:428:45 | name + ':' | lib/lib.js:428:29:428:50 | name ? ... :' : '' |
514+
| lib/lib.js:431:23:431:26 | last | lib/lib.js:436:19:436:22 | last |
515+
| lib/lib.js:431:23:431:26 | last | lib/lib.js:436:19:436:22 | last |
516+
| lib/lib.js:441:39:441:42 | name | lib/lib.js:442:24:442:27 | name |
517+
| lib/lib.js:441:39:441:42 | name | lib/lib.js:442:24:442:27 | name |
518+
| lib/lib.js:441:39:441:42 | name | lib/lib.js:442:24:442:27 | name |
519+
| lib/lib.js:441:39:441:42 | name | lib/lib.js:442:24:442:27 | name |
520+
| lib/lib.js:446:20:446:23 | name | lib/lib.js:447:25:447:28 | name |
521+
| lib/lib.js:446:20:446:23 | name | lib/lib.js:447:25:447:28 | name |
522+
| lib/lib.js:446:20:446:23 | name | lib/lib.js:447:25:447:28 | name |
523+
| lib/lib.js:446:20:446:23 | name | lib/lib.js:447:25:447:28 | name |
463524
#select
464525
| lib/lib2.js:4:10:4:25 | "rm -rf " + name | lib/lib2.js:3:28:3:31 | name | lib/lib2.js:4:22:4:25 | name | $@ based on library input is later used in $@. | lib/lib2.js:4:10:4:25 | "rm -rf " + name | String concatenation | lib/lib2.js:4:2:4:26 | cp.exec ... + name) | shell command |
465526
| lib/lib2.js:8:10:8:25 | "rm -rf " + name | lib/lib2.js:7:32:7:35 | name | lib/lib2.js:8:22:8:25 | name | $@ based on library input is later used in $@. | lib/lib2.js:8:10:8:25 | "rm -rf " + name | String concatenation | lib/lib2.js:8:2:8:26 | cp.exec ... + name) | shell command |
@@ -518,5 +579,13 @@ edges
518579
| lib/lib.js:351:10:351:27 | "rm -rf " + unsafe | lib/lib.js:349:29:349:34 | unsafe | lib/lib.js:351:22:351:27 | unsafe | $@ based on library input is later used in $@. | lib/lib.js:351:10:351:27 | "rm -rf " + unsafe | String concatenation | lib/lib.js:351:2:351:28 | cp.exec ... unsafe) | shell command |
519580
| lib/lib.js:366:17:366:56 | "learn ... + model | lib/lib.js:360:20:360:23 | opts | lib/lib.js:366:28:366:42 | this.learn_args | $@ based on library input is later used in $@. | lib/lib.js:366:17:366:56 | "learn ... + model | String concatenation | lib/lib.js:367:3:367:18 | cp.exec(command) | shell command |
520581
| lib/lib.js:406:10:406:25 | "rm -rf " + name | lib/lib.js:405:39:405:42 | name | lib/lib.js:406:22:406:25 | name | $@ based on library input is later used in $@. | lib/lib.js:406:10:406:25 | "rm -rf " + name | String concatenation | lib/lib.js:406:2:406:26 | cp.exec ... + name) | shell command |
521-
| lib/lib.js:414:12:414:27 | "rm -rf " + name | lib/lib.js:413:39:413:42 | name | lib/lib.js:414:24:414:27 | name | $@ based on library input is later used in $@. | lib/lib.js:414:12:414:27 | "rm -rf " + name | String concatenation | lib/lib.js:414:2:414:28 | asyncEx ... + name) | shell command |
522-
| lib/lib.js:419:13:419:28 | "rm -rf " + name | lib/lib.js:418:20:418:23 | name | lib/lib.js:419:25:419:28 | name | $@ based on library input is later used in $@. | lib/lib.js:419:13:419:28 | "rm -rf " + name | String concatenation | lib/lib.js:419:3:419:29 | asyncEx ... + name) | shell command |
582+
| lib/lib.js:415:10:415:25 | "rm -rf " + name | lib/lib.js:414:40:414:43 | name | lib/lib.js:415:22:415:25 | name | $@ based on library input is later used in $@. | lib/lib.js:415:10:415:25 | "rm -rf " + name | String concatenation | lib/lib.js:415:2:415:26 | cp.exec ... + name) | shell command |
583+
| lib/lib.js:417:28:417:31 | name | lib/lib.js:414:40:414:43 | name | lib/lib.js:417:28:417:31 | name | $@ based on library input is later used in $@. | lib/lib.js:417:28:417:31 | name | Shell argument | lib/lib.js:417:2:417:66 | cp.exec ... => {}) | shell command |
584+
| lib/lib.js:418:25:418:28 | name | lib/lib.js:414:40:414:43 | name | lib/lib.js:418:25:418:28 | name | $@ based on library input is later used in $@. | lib/lib.js:418:25:418:28 | name | Shell argument | lib/lib.js:418:2:418:45 | cp.spaw ... true}) | shell command |
585+
| lib/lib.js:419:32:419:35 | name | lib/lib.js:414:40:414:43 | name | lib/lib.js:419:32:419:35 | name | $@ based on library input is later used in $@. | lib/lib.js:419:32:419:35 | name | Shell argument | lib/lib.js:419:2:419:52 | cp.exec ... true}) | shell command |
586+
| lib/lib.js:420:29:420:32 | name | lib/lib.js:414:40:414:43 | name | lib/lib.js:420:29:420:32 | name | $@ based on library input is later used in $@. | lib/lib.js:420:29:420:32 | name | Shell argument | lib/lib.js:420:2:420:49 | cp.spaw ... true}) | shell command |
587+
| lib/lib.js:424:24:424:27 | name | lib/lib.js:414:40:414:43 | name | lib/lib.js:424:24:424:27 | name | $@ based on library input is later used in $@. | lib/lib.js:424:24:424:27 | name | Shell argument | lib/lib.js:424:2:424:40 | spawn(" ... WN_OPT) | shell command |
588+
| lib/lib.js:426:11:426:14 | name | lib/lib.js:414:40:414:43 | name | lib/lib.js:426:11:426:14 | name | $@ based on library input is later used in $@. | lib/lib.js:426:11:426:14 | name | Shell argument | lib/lib.js:427:2:427:28 | spawn(" ... WN_OPT) | shell command |
589+
| lib/lib.js:436:19:436:22 | last | lib/lib.js:414:40:414:43 | name | lib/lib.js:436:19:436:22 | last | $@ based on library input is later used in $@. | lib/lib.js:436:19:436:22 | last | Shell argument | lib/lib.js:428:2:428:70 | spawn(" ... WN_OPT) | shell command |
590+
| lib/lib.js:442:12:442:27 | "rm -rf " + name | lib/lib.js:441:39:441:42 | name | lib/lib.js:442:24:442:27 | name | $@ based on library input is later used in $@. | lib/lib.js:442:12:442:27 | "rm -rf " + name | String concatenation | lib/lib.js:442:2:442:28 | asyncEx ... + name) | shell command |
591+
| lib/lib.js:447:13:447:28 | "rm -rf " + name | lib/lib.js:446:20:446:23 | name | lib/lib.js:447:25:447:28 | name | $@ based on library input is later used in $@. | lib/lib.js:447:13:447:28 | "rm -rf " + name | String concatenation | lib/lib.js:447:3:447:29 | asyncEx ... + name) | shell command |

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,8 @@ syncCommand
5353
| command-line-parameter-command-injection.js:20:2:20:30 | cp.exec ... + arg0) |
5454
| command-line-parameter-command-injection.js:26:2:26:51 | cp.exec ... tion"`) |
5555
| command-line-parameter-command-injection.js:27:2:27:58 | cp.exec ... tion"`) |
56+
| lib/lib.js:419:2:419:52 | cp.exec ... true}) |
57+
| lib/lib.js:420:2:420:49 | cp.spaw ... true}) |
5658
| other.js:7:5:7:36 | require ... nc(cmd) |
5759
| other.js:9:5:9:35 | require ... nc(cmd) |
5860
| other.js:12:5:12:30 | require ... nc(cmd) |
@@ -100,6 +102,13 @@ options
100102
| lib/lib.js:152:2:152:23 | cp.spaw ... gs, cb) | lib/lib.js:152:21:152:22 | cb |
101103
| lib/lib.js:159:2:159:23 | cp.spaw ... gs, cb) | lib/lib.js:159:21:159:22 | cb |
102104
| lib/lib.js:163:2:167:2 | cp.spaw ... t' }\\n\\t) | lib/lib.js:166:3:166:22 | { stdio: 'inherit' } |
105+
| lib/lib.js:417:2:417:66 | cp.exec ... => {}) | lib/lib.js:417:35:417:47 | {shell: true} |
106+
| lib/lib.js:418:2:418:45 | cp.spaw ... true}) | lib/lib.js:418:32:418:44 | {shell: true} |
107+
| lib/lib.js:419:2:419:52 | cp.exec ... true}) | lib/lib.js:419:39:419:51 | {shell: true} |
108+
| lib/lib.js:420:2:420:49 | cp.spaw ... true}) | lib/lib.js:420:36:420:48 | {shell: true} |
109+
| lib/lib.js:424:2:424:40 | spawn(" ... WN_OPT) | lib/lib.js:424:31:424:39 | SPAWN_OPT |
110+
| lib/lib.js:427:2:427:28 | spawn(" ... WN_OPT) | lib/lib.js:427:19:427:27 | SPAWN_OPT |
111+
| lib/lib.js:428:2:428:70 | spawn(" ... WN_OPT) | lib/lib.js:428:61:428:69 | SPAWN_OPT |
103112
| uselesscat.js:28:1:28:39 | execSyn ... 1000}) | uselesscat.js:28:28:28:38 | {uid: 1000} |
104113
| uselesscat.js:30:1:30:64 | exec('c ... t) { }) | uselesscat.js:30:26:30:38 | { cwd: './' } |
105114
| uselesscat.js:34:1:34:54 | execSyn ... utf8'}) | uselesscat.js:34:36:34:53 | {encoding: 'utf8'} |

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -409,6 +409,34 @@ module.exports.sanitizer3 = function (name) {
409409
cp.exec("rm -rf " + sanitized); // OK
410410
}
411411

412+
const cp = require("child_process");
413+
const spawn = cp.spawn;
414+
module.exports.shellOption = function (name) {
415+
cp.exec("rm -rf " + name); // NOT OK
416+
417+
cp.execFile("rm", ["-rf", name], {shell: true}, (err, out) => {}); // NOT OK
418+
cp.spawn("rm", ["-rf", name], {shell: true}); // NOT OK
419+
cp.execFileSync("rm", ["-rf", name], {shell: true}); // NOT OK
420+
cp.spawnSync("rm", ["-rf", name], {shell: true}); // NOT OK
421+
422+
const SPAWN_OPT = {shell: true};
423+
424+
spawn("rm", ["first", name], SPAWN_OPT); // NOT OK
425+
var arr = [];
426+
arr.push(name); // NOT OK
427+
spawn("rm", arr, SPAWN_OPT);
428+
spawn("rm", build("node", (name ? name + ':' : '') + '-'), SPAWN_OPT); // This is bad, but the alert location is down in `build`.
429+
}
430+
431+
function build(first, last) {
432+
var arr = [];
433+
if (something() === 'gm')
434+
arr.push('convert');
435+
first && arr.push(first);
436+
last && arr.push(last); // NOT OK
437+
return arr;
438+
};
439+
412440
var asyncExec = require("async-execute");
413441
module.exports.asyncStuff = function (name) {
414442
asyncExec("rm -rf " + name); // NOT OK

0 commit comments

Comments
 (0)