Skip to content

Commit 40032f2

Browse files
erik-krogherik-krogh
committed
treat arrays that gets executed with shell:true as a sink for js/shell-command-constructed-from-input 
1 parent bc5b745 commit 40032f2

File tree

3 files changed

+41
-8
lines changed

3 files changed

+41
-8
lines changed

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

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -156,21 +156,27 @@ module UnsafeShellCommandConstruction {
156156
}
157157

158158
/**
159-
* Gets a node that ends up in an array that is ultimately executed as a shell script by `sys`.
159+
* Holds if the arguments array given to `sys` is joined as a string because `shell` is set to true.
160160
*/
161-
private DataFlow::SourceNode endsInShellExecutedArray(
162-
DataFlow::TypeBackTracker t, SystemCommandExecution sys
163-
) {
164-
t.start() and
165-
result = sys.getArgumentList().getALocalSource() and
166-
// the array gets joined to a string when `shell` is set to true.
161+
predicate executesArrayAsShell(SystemCommandExecution sys) {
167162
sys.getOptionsArg()
168163
.getALocalSource()
169164
.getAPropertyWrite("shell")
170165
.getRhs()
171166
.asExpr()
172167
.(BooleanLiteral)
173168
.getValue() = "true"
169+
}
170+
171+
/**
172+
* Gets a node that ends up in an array that is ultimately executed as a shell script by `sys`.
173+
*/
174+
private DataFlow::SourceNode endsInShellExecutedArray(
175+
DataFlow::TypeBackTracker t, SystemCommandExecution sys
176+
) {
177+
t.start() and
178+
result = sys.getArgumentList().getALocalSource() and
179+
executesArrayAsShell(sys)
174180
or
175181
exists(DataFlow::TypeBackTracker t2 |
176182
result = endsInShellExecutedArray(t2, sys).backtrack(t2, t)
@@ -193,6 +199,10 @@ module UnsafeShellCommandConstruction {
193199
or
194200
this = arr.getAMethodCall(["push", "unshift"]).getAnArgument()
195201
)
202+
or
203+
this = sys.getArgumentList() and
204+
not this instanceof DataFlow::ArrayCreationNode and
205+
executesArrayAsShell(sys)
196206
}
197207

198208
override string getSinkType() { result = "shell argument" }

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -223,8 +223,14 @@ nodes
223223
| lib/lib.js:420:29:420:32 | name |
224224
| lib/lib.js:424:24:424:27 | name |
225225
| lib/lib.js:424:24:424:27 | name |
226+
| lib/lib.js:425:6:425:13 | arr |
227+
| lib/lib.js:425:12:425:13 | [] |
226228
| lib/lib.js:426:11:426:14 | name |
227229
| lib/lib.js:426:11:426:14 | name |
230+
| lib/lib.js:427:14:427:16 | arr |
231+
| lib/lib.js:427:14:427:16 | arr |
232+
| lib/lib.js:428:14:428:58 | build(" ... + '-') |
233+
| lib/lib.js:428:14:428:58 | build(" ... + '-') |
228234
| lib/lib.js:428:28:428:51 | (name ? ... ' : '') |
229235
| lib/lib.js:428:28:428:57 | (name ? ... ) + '-' |
230236
| lib/lib.js:428:29:428:50 | name ? ... :' : '' |
@@ -302,6 +308,10 @@ nodes
302308
| lib/subLib/index.js:7:32:7:35 | name |
303309
| lib/subLib/index.js:8:22:8:25 | name |
304310
| lib/subLib/index.js:8:22:8:25 | name |
311+
| lib/subLib/index.js:13:44:13:46 | arr |
312+
| lib/subLib/index.js:13:44:13:46 | arr |
313+
| lib/subLib/index.js:14:22:14:24 | arr |
314+
| lib/subLib/index.js:14:22:14:24 | arr |
305315
edges
306316
| lib/isImported.js:5:49:5:52 | name | lib/isImported.js:6:22:6:25 | name |
307317
| lib/isImported.js:5:49:5:52 | name | lib/isImported.js:6:22:6:25 | name |
@@ -575,7 +585,13 @@ edges
575585
| lib/lib.js:414:40:414:43 | name | lib/lib.js:426:11:426:14 | name |
576586
| lib/lib.js:414:40:414:43 | name | lib/lib.js:428:36:428:39 | name |
577587
| lib/lib.js:414:40:414:43 | name | lib/lib.js:428:36:428:39 | name |
588+
| lib/lib.js:425:6:425:13 | arr | lib/lib.js:427:14:427:16 | arr |
589+
| lib/lib.js:425:6:425:13 | arr | lib/lib.js:427:14:427:16 | arr |
590+
| lib/lib.js:425:12:425:13 | [] | lib/lib.js:425:6:425:13 | arr |
591+
| lib/lib.js:426:11:426:14 | name | lib/lib.js:425:12:425:13 | [] |
578592
| lib/lib.js:428:28:428:51 | (name ? ... ' : '') | lib/lib.js:428:28:428:57 | (name ? ... ) + '-' |
593+
| lib/lib.js:428:28:428:57 | (name ? ... ) + '-' | lib/lib.js:428:14:428:58 | build(" ... + '-') |
594+
| lib/lib.js:428:28:428:57 | (name ? ... ) + '-' | lib/lib.js:428:14:428:58 | build(" ... + '-') |
579595
| lib/lib.js:428:28:428:57 | (name ? ... ) + '-' | lib/lib.js:431:23:431:26 | last |
580596
| lib/lib.js:428:29:428:50 | name ? ... :' : '' | lib/lib.js:428:28:428:51 | (name ? ... ' : '') |
581597
| lib/lib.js:428:36:428:39 | name | lib/lib.js:428:36:428:45 | name + ':' |
@@ -663,6 +679,10 @@ edges
663679
| lib/subLib/index.js:7:32:7:35 | name | lib/subLib/index.js:8:22:8:25 | name |
664680
| lib/subLib/index.js:7:32:7:35 | name | lib/subLib/index.js:8:22:8:25 | name |
665681
| lib/subLib/index.js:7:32:7:35 | name | lib/subLib/index.js:8:22:8:25 | name |
682+
| lib/subLib/index.js:13:44:13:46 | arr | lib/subLib/index.js:14:22:14:24 | arr |
683+
| lib/subLib/index.js:13:44:13:46 | arr | lib/subLib/index.js:14:22:14:24 | arr |
684+
| lib/subLib/index.js:13:44:13:46 | arr | lib/subLib/index.js:14:22:14:24 | arr |
685+
| lib/subLib/index.js:13:44:13:46 | arr | lib/subLib/index.js:14:22:14:24 | arr |
666686
#select
667687
| lib/isImported.js:6:10:6:25 | "rm -rf " + name | lib/isImported.js:5:49:5:52 | name | lib/isImported.js:6:22:6:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/isImported.js:5:49:5:52 | name | library input | lib/isImported.js:6:2:6:26 | cp.exec ... + name) | shell command |
668688
| 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 | This string concatenation which depends on $@ is later used in a $@. | lib/lib2.js:3:28:3:31 | name | library input | lib/lib2.js:4:2:4:26 | cp.exec ... + name) | shell command |
@@ -729,6 +749,8 @@ edges
729749
| lib/lib.js:420:29:420:32 | name | lib/lib.js:414:40:414:43 | name | lib/lib.js:420:29:420:32 | name | This shell argument which depends on $@ is later used in a $@. | lib/lib.js:414:40:414:43 | name | library input | lib/lib.js:420:2:420:49 | cp.spaw ... true}) | shell command |
730750
| lib/lib.js:424:24:424:27 | name | lib/lib.js:414:40:414:43 | name | lib/lib.js:424:24:424:27 | name | This shell argument which depends on $@ is later used in a $@. | lib/lib.js:414:40:414:43 | name | library input | lib/lib.js:424:2:424:40 | spawn(" ... WN_OPT) | shell command |
731751
| lib/lib.js:426:11:426:14 | name | lib/lib.js:414:40:414:43 | name | lib/lib.js:426:11:426:14 | name | This shell argument which depends on $@ is later used in a $@. | lib/lib.js:414:40:414:43 | name | library input | lib/lib.js:427:2:427:28 | spawn(" ... WN_OPT) | shell command |
752+
| lib/lib.js:427:14:427:16 | arr | lib/lib.js:414:40:414:43 | name | lib/lib.js:427:14:427:16 | arr | This shell argument which depends on $@ is later used in a $@. | lib/lib.js:414:40:414:43 | name | library input | lib/lib.js:427:2:427:28 | spawn(" ... WN_OPT) | shell command |
753+
| lib/lib.js:428:14:428:58 | build(" ... + '-') | lib/lib.js:414:40:414:43 | name | lib/lib.js:428:14:428:58 | build(" ... + '-') | This shell argument which depends on $@ is later used in a $@. | lib/lib.js:414:40:414:43 | name | library input | lib/lib.js:428:2:428:70 | spawn(" ... WN_OPT) | shell command |
732754
| lib/lib.js:436:19:436:22 | last | lib/lib.js:414:40:414:43 | name | lib/lib.js:436:19:436:22 | last | This shell argument which depends on $@ is later used in a $@. | lib/lib.js:414:40:414:43 | name | library input | lib/lib.js:428:2:428:70 | spawn(" ... WN_OPT) | shell command |
733755
| 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 | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:441:39:441:42 | name | library input | lib/lib.js:442:2:442:28 | asyncEx ... + name) | shell command |
734756
| 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 | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:446:20:446:23 | name | library input | lib/lib.js:447:3:447:29 | asyncEx ... + name) | shell command |
@@ -750,3 +772,4 @@ edges
750772
| lib/subLib/amdSub.js:4:10:4:25 | "rm -rf " + name | lib/subLib/amdSub.js:3:28:3:31 | name | lib/subLib/amdSub.js:4:22:4:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/subLib/amdSub.js:3:28:3:31 | name | library input | lib/subLib/amdSub.js:4:2:4:26 | cp.exec ... + name) | shell command |
751773
| lib/subLib/index.js:4:10:4:25 | "rm -rf " + name | lib/subLib/index.js:3:28:3:31 | name | lib/subLib/index.js:4:22:4:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/subLib/index.js:3:28:3:31 | name | library input | lib/subLib/index.js:4:2:4:26 | cp.exec ... + name) | shell command |
752774
| lib/subLib/index.js:8:10:8:25 | "rm -rf " + name | lib/subLib/index.js:7:32:7:35 | name | lib/subLib/index.js:8:22:8:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/subLib/index.js:7:32:7:35 | name | library input | lib/subLib/index.js:8:2:8:26 | cp.exec ... + name) | shell command |
775+
| lib/subLib/index.js:14:22:14:24 | arr | lib/subLib/index.js:13:44:13:46 | arr | lib/subLib/index.js:14:22:14:24 | arr | This shell argument which depends on $@ is later used in a $@. | lib/subLib/index.js:13:44:13:46 | arr | library input | lib/subLib/index.js:14:5:14:40 | cp.spaw ... true}) | shell command |

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,5 +11,5 @@ module.exports.foo = function (name) {
1111
module.exports.amd = require("./amd.js");
1212

1313
module.exports.arrToShell = function (cmd, arr) {
14-
cp.spawn("echo", arr, {shell: true}); // NOT OK - but not flagged [INCONSISTENCY]
14+
cp.spawn("echo", arr, {shell: true}); // NOT OK
1515
}

0 commit comments

Comments
 (0)