diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll index 32eda1db8de3..ac5f8e2c56b3 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll @@ -143,6 +143,53 @@ module UnsafeShellCommandConstruction { override DataFlow::Node getAlertLocation() { result = this } } + /** + * Gets a node that ends up in an array that is ultimately executed as a shell script by `sys`. + */ + private DataFlow::SourceNode endsInShellExecutedArray( + DataFlow::TypeBackTracker t, SystemCommandExecution sys + ) { + t.start() and + result = sys.getArgumentList().getALocalSource() and + // the array gets joined to a string when `shell` is set to true. + sys.getOptionsArg() + .getALocalSource() + .getAPropertyWrite("shell") + .getRhs() + .asExpr() + .(BooleanLiteral) + .getValue() = "true" + or + exists(DataFlow::TypeBackTracker t2 | + result = endsInShellExecutedArray(t2, sys).backtrack(t2, t) + ) + } + + /** + * An argument to a command invocation where the `shell` option is set to true. + */ + class ShellTrueCommandExecutionSink extends Sink { + SystemCommandExecution sys; + + ShellTrueCommandExecutionSink() { + // `shell` is set to true. That means string-concatenation happens behind the scenes. + // We just assume that a `shell` option in any library means the same thing as it does in NodeJS. + exists(DataFlow::SourceNode arr | + arr = endsInShellExecutedArray(DataFlow::TypeBackTracker::end(), sys) + | + this = arr.(DataFlow::ArrayCreationNode).getAnElement() + or + this = arr.getAMethodCall(["push", "unshift"]).getAnArgument() + ) + } + + override string getSinkType() { result = "Shell argument" } + + override SystemCommandExecution getCommandExecution() { result = sys } + + override DataFlow::Node getAlertLocation() { result = this } + } + /** * A sanitizer like: "'"+name.replace(/'/g,"'\\''")+"'" * Which sanitizes on Unix. diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected index 29a83580ad1e..f5c45b531e15 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected +++ b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected @@ -205,14 +205,38 @@ nodes | lib/lib.js:405:39:405:42 | name | | lib/lib.js:406:22:406:25 | name | | lib/lib.js:406:22:406:25 | name | -| lib/lib.js:413:39:413:42 | name | -| lib/lib.js:413:39:413:42 | name | -| lib/lib.js:414:24:414:27 | name | -| lib/lib.js:414:24:414:27 | name | -| lib/lib.js:418:20:418:23 | name | -| lib/lib.js:418:20:418:23 | name | -| lib/lib.js:419:25:419:28 | name | -| lib/lib.js:419:25:419:28 | name | +| lib/lib.js:414:40:414:43 | name | +| lib/lib.js:414:40:414:43 | name | +| lib/lib.js:415:22:415:25 | name | +| lib/lib.js:415:22:415:25 | name | +| lib/lib.js:417:28:417:31 | name | +| lib/lib.js:417:28:417:31 | name | +| lib/lib.js:418:25:418:28 | name | +| lib/lib.js:418:25:418:28 | name | +| lib/lib.js:419:32:419:35 | name | +| lib/lib.js:419:32:419:35 | name | +| lib/lib.js:420:29:420:32 | name | +| lib/lib.js:420:29:420:32 | name | +| lib/lib.js:424:24:424:27 | name | +| lib/lib.js:424:24:424:27 | name | +| lib/lib.js:426:11:426:14 | name | +| lib/lib.js:426:11:426:14 | name | +| lib/lib.js:428:28:428:51 | (name ? ... ' : '') | +| lib/lib.js:428:28:428:57 | (name ? ... ) + '-' | +| lib/lib.js:428:29:428:50 | name ? ... :' : '' | +| lib/lib.js:428:36:428:39 | name | +| lib/lib.js:428:36:428:45 | name + ':' | +| lib/lib.js:431:23:431:26 | last | +| lib/lib.js:436:19:436:22 | last | +| lib/lib.js:436:19:436:22 | last | +| lib/lib.js:441:39:441:42 | name | +| lib/lib.js:441:39:441:42 | name | +| lib/lib.js:442:24:442:27 | name | +| lib/lib.js:442:24:442:27 | name | +| lib/lib.js:446:20:446:23 | name | +| lib/lib.js:446:20:446:23 | name | +| lib/lib.js:447:25:447:28 | name | +| lib/lib.js:447:25:447:28 | name | edges | lib/lib2.js:3:28:3:31 | name | lib/lib2.js:4:22:4:25 | name | | lib/lib2.js:3:28:3:31 | name | lib/lib2.js:4:22:4:25 | name | @@ -452,14 +476,51 @@ edges | lib/lib.js:405:39:405:42 | name | lib/lib.js:406:22:406:25 | name | | lib/lib.js:405:39:405:42 | name | lib/lib.js:406:22:406:25 | name | | lib/lib.js:405:39:405:42 | name | lib/lib.js:406:22:406:25 | name | -| lib/lib.js:413:39:413:42 | name | lib/lib.js:414:24:414:27 | name | -| lib/lib.js:413:39:413:42 | name | lib/lib.js:414:24:414:27 | name | -| lib/lib.js:413:39:413:42 | name | lib/lib.js:414:24:414:27 | name | -| lib/lib.js:413:39:413:42 | name | lib/lib.js:414:24:414:27 | name | -| lib/lib.js:418:20:418:23 | name | lib/lib.js:419:25:419:28 | name | -| lib/lib.js:418:20:418:23 | name | lib/lib.js:419:25:419:28 | name | -| lib/lib.js:418:20:418:23 | name | lib/lib.js:419:25:419:28 | name | -| lib/lib.js:418:20:418:23 | name | lib/lib.js:419:25:419:28 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:415:22:415:25 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:415:22:415:25 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:415:22:415:25 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:415:22:415:25 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:417:28:417:31 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:417:28:417:31 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:417:28:417:31 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:417:28:417:31 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:418:25:418:28 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:418:25:418:28 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:418:25:418:28 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:418:25:418:28 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:419:32:419:35 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:419:32:419:35 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:419:32:419:35 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:419:32:419:35 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:420:29:420:32 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:420:29:420:32 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:420:29:420:32 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:420:29:420:32 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:424:24:424:27 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:424:24:424:27 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:424:24:424:27 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:424:24:424:27 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:426:11:426:14 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:426:11:426:14 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:426:11:426:14 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:426:11:426:14 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:428:36:428:39 | name | +| lib/lib.js:414:40:414:43 | name | lib/lib.js:428:36:428:39 | name | +| lib/lib.js:428:28:428:51 | (name ? ... ' : '') | lib/lib.js:428:28:428:57 | (name ? ... ) + '-' | +| lib/lib.js:428:28:428:57 | (name ? ... ) + '-' | lib/lib.js:431:23:431:26 | last | +| lib/lib.js:428:29:428:50 | name ? ... :' : '' | lib/lib.js:428:28:428:51 | (name ? ... ' : '') | +| lib/lib.js:428:36:428:39 | name | lib/lib.js:428:36:428:45 | name + ':' | +| lib/lib.js:428:36:428:45 | name + ':' | lib/lib.js:428:29:428:50 | name ? ... :' : '' | +| lib/lib.js:431:23:431:26 | last | lib/lib.js:436:19:436:22 | last | +| lib/lib.js:431:23:431:26 | last | lib/lib.js:436:19:436:22 | last | +| lib/lib.js:441:39:441:42 | name | lib/lib.js:442:24:442:27 | name | +| lib/lib.js:441:39:441:42 | name | lib/lib.js:442:24:442:27 | name | +| lib/lib.js:441:39:441:42 | name | lib/lib.js:442:24:442:27 | name | +| lib/lib.js:441:39:441:42 | name | lib/lib.js:442:24:442:27 | name | +| lib/lib.js:446:20:446:23 | name | lib/lib.js:447:25:447:28 | name | +| lib/lib.js:446:20:446:23 | name | lib/lib.js:447:25:447:28 | name | +| lib/lib.js:446:20:446:23 | name | lib/lib.js:447:25:447:28 | name | +| lib/lib.js:446:20:446:23 | name | lib/lib.js:447:25:447:28 | name | #select | 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 | | 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 | 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 | | 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 | | 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 | -| 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 | -| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected index 7a7e22fb3328..4c7e10b658e1 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected +++ b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected @@ -53,6 +53,8 @@ syncCommand | command-line-parameter-command-injection.js:20:2:20:30 | cp.exec ... + arg0) | | command-line-parameter-command-injection.js:26:2:26:51 | cp.exec ... tion"`) | | command-line-parameter-command-injection.js:27:2:27:58 | cp.exec ... tion"`) | +| lib/lib.js:419:2:419:52 | cp.exec ... true}) | +| lib/lib.js:420:2:420:49 | cp.spaw ... true}) | | other.js:7:5:7:36 | require ... nc(cmd) | | other.js:9:5:9:35 | require ... nc(cmd) | | other.js:12:5:12:30 | require ... nc(cmd) | @@ -100,6 +102,13 @@ options | lib/lib.js:152:2:152:23 | cp.spaw ... gs, cb) | lib/lib.js:152:21:152:22 | cb | | lib/lib.js:159:2:159:23 | cp.spaw ... gs, cb) | lib/lib.js:159:21:159:22 | cb | | lib/lib.js:163:2:167:2 | cp.spaw ... t' }\\n\\t) | lib/lib.js:166:3:166:22 | { stdio: 'inherit' } | +| lib/lib.js:417:2:417:66 | cp.exec ... => {}) | lib/lib.js:417:35:417:47 | {shell: true} | +| lib/lib.js:418:2:418:45 | cp.spaw ... true}) | lib/lib.js:418:32:418:44 | {shell: true} | +| lib/lib.js:419:2:419:52 | cp.exec ... true}) | lib/lib.js:419:39:419:51 | {shell: true} | +| lib/lib.js:420:2:420:49 | cp.spaw ... true}) | lib/lib.js:420:36:420:48 | {shell: true} | +| lib/lib.js:424:2:424:40 | spawn(" ... WN_OPT) | lib/lib.js:424:31:424:39 | SPAWN_OPT | +| lib/lib.js:427:2:427:28 | spawn(" ... WN_OPT) | lib/lib.js:427:19:427:27 | SPAWN_OPT | +| lib/lib.js:428:2:428:70 | spawn(" ... WN_OPT) | lib/lib.js:428:61:428:69 | SPAWN_OPT | | uselesscat.js:28:1:28:39 | execSyn ... 1000}) | uselesscat.js:28:28:28:38 | {uid: 1000} | | uselesscat.js:30:1:30:64 | exec('c ... t) { }) | uselesscat.js:30:26:30:38 | { cwd: './' } | | uselesscat.js:34:1:34:54 | execSyn ... utf8'}) | uselesscat.js:34:36:34:53 | {encoding: 'utf8'} | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js b/javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js index a645788aa723..1fe3e6804596 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js +++ b/javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js @@ -409,6 +409,34 @@ module.exports.sanitizer3 = function (name) { cp.exec("rm -rf " + sanitized); // OK } +const cp = require("child_process"); +const spawn = cp.spawn; +module.exports.shellOption = function (name) { + cp.exec("rm -rf " + name); // NOT OK + + cp.execFile("rm", ["-rf", name], {shell: true}, (err, out) => {}); // NOT OK + cp.spawn("rm", ["-rf", name], {shell: true}); // NOT OK + cp.execFileSync("rm", ["-rf", name], {shell: true}); // NOT OK + cp.spawnSync("rm", ["-rf", name], {shell: true}); // NOT OK + + const SPAWN_OPT = {shell: true}; + + spawn("rm", ["first", name], SPAWN_OPT); // NOT OK + var arr = []; + arr.push(name); // NOT OK + spawn("rm", arr, SPAWN_OPT); + spawn("rm", build("node", (name ? name + ':' : '') + '-'), SPAWN_OPT); // This is bad, but the alert location is down in `build`. +} + +function build(first, last) { + var arr = []; + if (something() === 'gm') + arr.push('convert'); + first && arr.push(first); + last && arr.push(last); // NOT OK + return arr; +}; + var asyncExec = require("async-execute"); module.exports.asyncStuff = function (name) { asyncExec("rm -rf " + name); // NOT OK