Skip to content

JS: add shell interpreted arrays as sinks for js/shell-command-constructed-from-input #5458

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 25, 2021
Merged

JS: add shell interpreted arrays as sinks for js/shell-command-constructed-from-input #5458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8949b9e
add shell interpreted arrays as sinks for js/shell-command-constructe…
erik-krogh Mar 19, 2021
c146b27
Merge branch 'main' into shellTrue
erik-krogh Mar 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions .../src/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use this to make the alert appear at the actual spawn call instead of where the array is constructed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we could.
But the alert message already includes a link to the spawn call.

I currently have the alert-location set to the location where the taint-tracking ends (or, close to in the case of string-concats).
I thought it could be confusing to have a path-query where the path doesn't end in the alert-location.

Changing the alert-location to the spawn call would just require changing sinkNode.getAlertLocation() to sinkNode.getCommandExecution() in the .ql file, and changing the message a bit.
But I think I prefer not doing that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok fair enough. It would have been nice for the path to end at the argument to spawn, but I can see that would require adding flow labels to do properly.

}

/**
* A sanitizer like: "'"+name.replace(/'/g,"'\\''")+"'"
* Which sanitizes on Unix.
Expand Down
105 changes: 87 additions & 18 deletions javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down Expand Up @@ -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 |
Expand Down Expand Up @@ -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 |
9 changes: 9 additions & 0 deletions javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) |
Expand Down Expand Up @@ -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'} |
Expand Down
28 changes: 28 additions & 0 deletions javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down