Skip to content

Commit eb2db24

Browse files
vishalshetagarvishalshetagar
committed
JS: Refine shell argument sinks and add regression tests
1 parent 07f8ce2 commit eb2db24

13 files changed

Lines changed: 799 additions & 55 deletions

File tree

‎javascript/ql/lib/change-notes/2026-11-02-shell-command-arguments.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
11
---
22
category: minorAnalysis
33
---
4-
* Command injection modeling now recognizes argument arrays passed to `spawn` and `execFile` when the `shell` option is enabled, including string-valued shell paths and calls to `execFile` without a callback.
4+
* Command injection modeling now recognizes argument arrays passed to `spawn` and `execFile` when the `shell` option is enabled, including string-valued shell paths and calls to `execFile` without a callback. The model distinguishes argument arrays, options objects, and callbacks, and accounts for local writes to the `shell` option before execution.

‎javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll‎

Lines changed: 27 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -797,13 +797,31 @@ module NodeJSLib {
797797
override DataFlow::Node getArgumentList() {
798798
methodName = ["execFile", "execFileSync", "fork", "spawn", "spawnSync"] and
799799
// all of the above methods take the argument list as their second argument
800-
result = this.getParameter(1).asSink()
800+
result = this.getParameter(1).asSink() and
801+
not this.isOptionsObject(result) and
802+
not result.getALocalSource() instanceof DataFlow::FunctionNode
801803
}
802804

803805
override predicate isSync() { methodName.matches("%Sync") }
804806

807+
/** Holds if `node` is known to be an options object rather than an argument array or callback. */
808+
private predicate isOptionsObject(DataFlow::Node node) {
809+
not node.getALocalSource() instanceof DataFlow::ArrayCreationNode and
810+
(
811+
node.getALocalSource() instanceof DataFlow::ObjectLiteralNode
812+
or
813+
exists(node.getALocalSource().getAPropertyWrite("shell"))
814+
or
815+
exists(API::Node api |
816+
api.asSink() = node and
817+
api.getAValueReachingSink().getALocalSource() instanceof DataFlow::ObjectLiteralNode
818+
)
819+
)
820+
}
821+
805822
override DataFlow::Node getOptionsArg() {
806823
not result.getALocalSource() instanceof DataFlow::FunctionNode and // looks like callback
824+
// Shell execution still takes separate arguments for the array and the options object.
807825
not result.getALocalSource() instanceof DataFlow::ArrayCreationNode and // looks like argumentlist
808826
not result = this.getParameter(0).asSink() and
809827
// fork/spawn and all sync methos always has options as the last argument
@@ -812,9 +830,14 @@ module NodeJSLib {
812830
methodName.matches("spawn%") or
813831
methodName.matches("%Sync")
814832
then result = this.getLastArgument()
815-
else
816-
// exec/execFile may have a callback after the options argument.
817-
result = this.getParameter([this.getNumArgument() - 2, this.getNumArgument() - 1]).asSink()
833+
else (
834+
result = this.getParameter(this.getNumArgument() - 2).asSink() and
835+
(methodName = "exec" or this.getNumArgument() = 4 or this.isOptionsObject(result))
836+
or
837+
// Without a callback, require evidence that the last argument is an options object.
838+
result = this.getLastArgument() and
839+
this.isOptionsObject(result)
840+
)
818841
}
819842
}
820843

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

Lines changed: 38 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -174,36 +174,52 @@ module UnsafeShellCommandConstruction {
174174
override DataFlow::Node getAlertLocation() { result = this }
175175
}
176176

177-
/**
178-
* Holds if the arguments array given to `sys` is joined as a string because the `shell` option is enabled.
179-
*/
180-
predicate executesArrayAsShell(SystemCommandExecution sys) {
181-
exists(DataFlow::Node shell |
182-
shell = sys.getOptionsArg().getALocalSource().getAPropertyWrite("shell").getRhs()
177+
/** Holds if `node` can reach `use` without another write to the `shell` property of `options`. */
178+
private predicate reachesShellOptionUse(
179+
DataFlow::SourceNode options, ControlFlowNode node, ControlFlowNode use
180+
) {
181+
exists(SystemCommandExecution sys |
182+
options = sys.getOptionsArg().getALocalSource() and use = sys.asExpr()
183+
) and
184+
(
185+
node = use
183186
or
184-
exists(API::Node node |
185-
node.asSink() = sys.getOptionsArg() and
186-
shell = node.getMember("shell").asSink()
187-
)
187+
not node = options.getAPropertyWrite("shell").getWriteNode() and
188+
reachesShellOptionUse(options, node.getASuccessor(), use)
189+
)
190+
}
191+
192+
/** Gets a value of the `shell` option that can reach the command execution. */
193+
private DataFlow::Node getShellOption(SystemCommandExecution sys) {
194+
exists(DataFlow::SourceNode options, DataFlow::PropWrite write, Expr use |
195+
options = sys.getOptionsArg().getALocalSource() and
196+
write = options.getAPropertyWrite("shell") and
197+
use = sys.asExpr() and
198+
result = write.getRhs()
188199
|
189-
shell.mayHaveBooleanValue(true)
200+
write.getWriteNode().getContainer() != use.getContainer()
190201
or
191-
exists(string path | shell.mayHaveStringValue(path) and path != "")
202+
reachesShellOptionUse(options, write.getWriteNode().getASuccessor(), use)
203+
)
204+
or
205+
exists(API::Node node, DataFlow::SourceNode options, Expr use |
206+
node.asSink() = sys.getOptionsArg() and
207+
result = node.getMember("shell").asSink() and
208+
options = sys.getOptionsArg().getALocalSource() and
209+
use = sys.asExpr() and
210+
not result = options.getAPropertyWrite("shell").getRhs() and
211+
reachesShellOptionUse(options, use.getContainer().getStart(), use)
192212
)
193213
}
194214

195215
/**
196-
* Gets a node that ends up in an array that is ultimately executed as a shell script by `sys`.
216+
* Holds if the arguments array given to `sys` is joined as a string because the `shell` option is enabled.
197217
*/
198-
private DataFlow::SourceNode endsInShellExecutedArray(
199-
DataFlow::TypeBackTracker t, SystemCommandExecution sys
200-
) {
201-
t.start() and
202-
result = sys.getArgumentList().getALocalSource() and
203-
executesArrayAsShell(sys)
204-
or
205-
exists(DataFlow::TypeBackTracker t2 |
206-
result = endsInShellExecutedArray(t2, sys).backtrack(t2, t)
218+
predicate executesArrayAsShell(SystemCommandExecution sys) {
219+
exists(DataFlow::Node shell | shell = getShellOption(sys) |
220+
shell.mayHaveBooleanValue(true)
221+
or
222+
exists(string path | shell.mayHaveStringValue(path) and path != "")
207223
)
208224
}
209225

@@ -216,16 +232,7 @@ module UnsafeShellCommandConstruction {
216232
ShellTrueCommandExecutionSink() {
217233
// `shell` is enabled. That means string-concatenation happens behind the scenes.
218234
// We just assume that a `shell` option in any library means the same thing as it does in NodeJS.
219-
exists(DataFlow::SourceNode arr |
220-
arr = endsInShellExecutedArray(DataFlow::TypeBackTracker::end(), sys)
221-
|
222-
this = arr.(DataFlow::ArrayCreationNode).getAnElement()
223-
or
224-
this = arr.getAMethodCall(["push", "unshift"]).getAnArgument()
225-
)
226-
or
227235
this = sys.getArgumentList() and
228-
not this instanceof DataFlow::ArrayCreationNode and
229236
executesArrayAsShell(sys)
230237
}
231238

‎javascript/ql/src/change-notes/2026-11-02-shell-command-arguments.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
11
---
22
category: minorAnalysis
33
---
4-
* The `js/command-line-injection` query now reports remote input that reaches argument arrays when the `shell` option is enabled. The `js/shell-command-constructed-from-input` query also recognizes string-valued shell paths. Both queries now handle `execFile` calls without a callback.
4+
* The `js/command-line-injection` query now reports remote input that reaches argument arrays when the `shell` option is enabled. The `js/shell-command-constructed-from-input` query also recognizes string-valued shell paths. Both queries now handle `execFile` calls without a callback and check argument-array input at the execution call, avoiding alerts for later array mutations.

‎javascript/ql/test/library-tests/frameworks/NodeJSLib/SystemCommandExecution.qll‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,3 +3,9 @@ import javascript
33
query predicate test_SystemCommandExecution(SystemCommandExecution cmd, DataFlow::Node res) {
44
res = cmd.getACommandArgument()
55
}
6+
7+
query predicate test_SystemCommandExecution_getOptionsArg(
8+
SystemCommandExecution cmd, DataFlow::Node res
9+
) {
10+
res = cmd.getOptionsArg()
11+
}

‎javascript/ql/test/library-tests/frameworks/NodeJSLib/exec.js‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,3 +5,17 @@ cp.execFileSync("sh", ["-c", "node --version"]);
55
cp.fork("foo", ["arg"]);
66
cp.spawn("echo", ["Hi"], cb);
77
cp.spawnSync("echo", ["Hi", "there"]);
8+
9+
cp.spawn("echo", { shell: true });
10+
cp.spawn("echo", ["Hi"], { shell: true });
11+
cp.execFile("echo", { shell: "/bin/sh" });
12+
cp.execFile("echo", ["Hi"], { shell: "/bin/sh" });
13+
14+
function overloads(args, callback) {
15+
cp.exec("echo", callback);
16+
cp.execFile("echo", args, callback);
17+
cp.spawn("echo", args, { shell: true });
18+
cp.execFile("echo", args, { shell: "/bin/sh" });
19+
cp.execFile("echo", args, { shell: true }, callback);
20+
cp.execFile("echo", { shell: true }, callback);
21+
}

‎javascript/ql/test/library-tests/frameworks/NodeJSLib/tests.expected‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,16 @@ test_SystemCommandExecution
230230
| exec.js:5:1:5:23 | cp.fork ... "arg"]) | exec.js:5:9:5:13 | "foo" |
231231
| exec.js:6:1:6:28 | cp.spaw ... "], cb) | exec.js:6:10:6:15 | "echo" |
232232
| exec.js:7:1:7:37 | cp.spaw ... here"]) | exec.js:7:14:7:19 | "echo" |
233+
| exec.js:9:1:9:33 | cp.spaw ... true }) | exec.js:9:10:9:15 | "echo" |
234+
| exec.js:10:1:10:41 | cp.spaw ... true }) | exec.js:10:10:10:15 | "echo" |
235+
| exec.js:11:1:11:41 | cp.exec ... /sh" }) | exec.js:11:13:11:18 | "echo" |
236+
| exec.js:12:1:12:49 | cp.exec ... /sh" }) | exec.js:12:13:12:18 | "echo" |
237+
| exec.js:15:5:15:29 | cp.exec ... llback) | exec.js:15:13:15:18 | "echo" |
238+
| exec.js:16:5:16:39 | cp.exec ... llback) | exec.js:16:17:16:22 | "echo" |
239+
| exec.js:17:5:17:43 | cp.spaw ... true }) | exec.js:17:14:17:19 | "echo" |
240+
| exec.js:18:5:18:51 | cp.exec ... /sh" }) | exec.js:18:17:18:22 | "echo" |
241+
| exec.js:19:5:19:56 | cp.exec ... llback) | exec.js:19:17:19:22 | "echo" |
242+
| exec.js:20:5:20:50 | cp.exec ... llback) | exec.js:20:17:20:22 | "echo" |
233243
test_HeaderDefinition_defines
234244
| src/http.js:13:3:13:44 | res.set ... /html') | content-type | text/html |
235245
| src/https.js:13:3:13:44 | res.set ... /html') | content-type | text/html |
@@ -417,3 +427,19 @@ test_SystemCommandExecution_getAnArgumentForCommand
417427
| exec.js:5:1:5:23 | cp.fork ... "arg"]) | exec.js:5:16:5:22 | ["arg"] |
418428
| exec.js:6:1:6:28 | cp.spaw ... "], cb) | exec.js:6:18:6:23 | ["Hi"] |
419429
| exec.js:7:1:7:37 | cp.spaw ... here"]) | exec.js:7:22:7:36 | ["Hi", "there"] |
430+
| exec.js:10:1:10:41 | cp.spaw ... true }) | exec.js:10:18:10:23 | ["Hi"] |
431+
| exec.js:12:1:12:49 | cp.exec ... /sh" }) | exec.js:12:21:12:26 | ["Hi"] |
432+
| exec.js:16:5:16:39 | cp.exec ... llback) | exec.js:16:25:16:28 | args |
433+
| exec.js:17:5:17:43 | cp.spaw ... true }) | exec.js:17:22:17:25 | args |
434+
| exec.js:18:5:18:51 | cp.exec ... /sh" }) | exec.js:18:25:18:28 | args |
435+
| exec.js:19:5:19:56 | cp.exec ... llback) | exec.js:19:25:19:28 | args |
436+
test_SystemCommandExecution_getOptionsArg
437+
| exec.js:6:1:6:28 | cp.spaw ... "], cb) | exec.js:6:26:6:27 | cb |
438+
| exec.js:9:1:9:33 | cp.spaw ... true }) | exec.js:9:18:9:32 | { shell: true } |
439+
| exec.js:10:1:10:41 | cp.spaw ... true }) | exec.js:10:26:10:40 | { shell: true } |
440+
| exec.js:11:1:11:41 | cp.exec ... /sh" }) | exec.js:11:21:11:40 | { shell: "/bin/sh" } |
441+
| exec.js:12:1:12:49 | cp.exec ... /sh" }) | exec.js:12:29:12:48 | { shell: "/bin/sh" } |
442+
| exec.js:17:5:17:43 | cp.spaw ... true }) | exec.js:17:28:17:42 | { shell: true } |
443+
| exec.js:18:5:18:51 | cp.exec ... /sh" }) | exec.js:18:31:18:50 | { shell: "/bin/sh" } |
444+
| exec.js:19:5:19:56 | cp.exec ... llback) | exec.js:19:31:19:45 | { shell: true } |
445+
| exec.js:20:5:20:50 | cp.exec ... llback) | exec.js:20:25:20:39 | { shell: true } |

0 commit comments

Comments
 (0)