Skip to content

Commit d1e769b

Browse files
NapalysNapalys
authored
Merge pull request #19422 from Napalys/js/shelljs
JS: Modeling of `ShellJS` functions
2 parents 74669cb + 871e93d commit d1e769b

File tree

10 files changed

+64
-12
lines changed

10 files changed

+64
-12
lines changed

‎javascript/ql/lib/change-notes/2025-04-30-shelljs.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Improved modeling of the [`shelljs`](https://www.npmjs.com/package/shelljs) and [`async-shelljs`](https://www.npmjs.com/package/async-shelljs) libraries by adding support for the `which`, `cmd`, `asyncExec` and `env`.

‎javascript/ql/lib/ext/shelljs.model.yml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
extensions:
2+
- addsTo:
3+
pack: codeql/javascript-all
4+
extensible: sourceModel
5+
data:
6+
- ["shelljs", "Member[env]", "environment"]

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

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,8 @@ module ShellJS {
1414
shellJSMember()
1515
.getMember([
1616
"exec", "cd", "cp", "touch", "chmod", "pushd", "find", "ls", "ln", "mkdir", "mv",
17-
"rm", "cat", "head", "sort", "tail", "uniq", "grep", "sed", "to", "toEnd", "echo"
17+
"rm", "cat", "head", "sort", "tail", "uniq", "grep", "sed", "to", "toEnd", "echo",
18+
"which", "cmd", "asyncExec"
1819
])
1920
.getReturn()
2021
}
@@ -99,7 +100,8 @@ module ShellJS {
99100
*/
100101
private class ShellJSGenericFileAccess extends FileSystemAccess, ShellJSCall {
101102
ShellJSGenericFileAccess() {
102-
name = ["cd", "cp", "touch", "chmod", "pushd", "find", "ls", "ln", "mkdir", "mv", "rm"]
103+
name =
104+
["cd", "cp", "touch", "chmod", "pushd", "find", "ls", "ln", "mkdir", "mv", "rm", "which"]
103105
}
104106

105107
override DataFlow::Node getAPathArgument() { result = this.getAnArgument() }
@@ -111,7 +113,8 @@ module ShellJS {
111113
private class ShellJSFilenameSource extends FileNameSource, ShellJSCall {
112114
ShellJSFilenameSource() {
113115
name = "find" or
114-
name = "ls"
116+
name = "ls" or
117+
name = "which"
115118
}
116119
}
117120

@@ -151,16 +154,24 @@ module ShellJS {
151154
}
152155

153156
/**
154-
* A call to `shelljs.exec()` modeled as command execution.
157+
* A call to `shelljs.exec()`, `shelljs.cmd()`, or `async-shelljs.asyncExec()` modeled as command execution.
155158
*/
156159
private class ShellJSExec extends SystemCommandExecution, ShellJSCall {
157-
ShellJSExec() { name = "exec" }
158-
159-
override DataFlow::Node getACommandArgument() { result = this.getArgument(0) }
160+
ShellJSExec() { name = ["exec", "cmd", "asyncExec"] }
161+
162+
override DataFlow::Node getACommandArgument() {
163+
if name = "cmd"
164+
then
165+
result = this.getArgument(_) and
166+
not result = this.getOptionsArg()
167+
else
168+
// For exec/asyncExec: only first argument is command
169+
result = this.getArgument(0)
170+
}
160171

161172
override predicate isShellInterpreted(DataFlow::Node arg) { arg = this.getACommandArgument() }
162173

163-
override predicate isSync() { none() }
174+
override predicate isSync() { name = "cmd" }
164175

165176
override DataFlow::Node getOptionsArg() {
166177
result = this.getLastArgument() and

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ module CleartextLogging {
171171

172172
/** An access to the sensitive object `process.env`. */
173173
class ProcessEnvSource extends Source {
174-
ProcessEnvSource() { this = NodeJSLib::process().getAPropertyRead("env") }
174+
ProcessEnvSource() { this.(ThreatModelSource).getThreatModel() = "environment" }
175175

176176
override string describe() { result = "process environment" }
177177
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,15 +29,15 @@ module IndirectCommandInjection {
2929
* A read of `process.env`, considered as a flow source for command injection.
3030
*/
3131
private class ProcessEnvAsSource extends Source {
32-
ProcessEnvAsSource() { this = NodeJSLib::process().getAPropertyRead("env") }
32+
ProcessEnvAsSource() { this.(ThreatModelSource).getThreatModel() = "environment" }
3333

3434
override string describe() { result = "environment variable" }
3535
}
3636

3737
/** Gets a data flow node referring to `process.env`. */
3838
private DataFlow::SourceNode envObject(DataFlow::TypeTracker t) {
3939
t.start() and
40-
result = NodeJSLib::process().getAPropertyRead("env")
40+
result.(ThreatModelSource).getThreatModel() = "environment"
4141
or
4242
exists(DataFlow::TypeTracker t2 | result = envObject(t2).track(t2, t))
4343
}

‎javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.ql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ DataFlow::ObjectLiteralNode tlsOptions() { result.flowsTo(tlsInvocation().getAnA
3737
from DataFlow::PropWrite disable
3838
where
3939
exists(DataFlow::SourceNode env |
40-
env = NodeJSLib::process().getAPropertyRead("env") and
40+
env.(ThreatModelSource).getThreatModel() = "environment" and
4141
disable = env.getAPropertyWrite("NODE_TLS_REJECT_UNAUTHORIZED") and
4242
disable.getRhs().mayHaveStringValue("0")
4343
)

‎javascript/ql/test/library-tests/frameworks/Shelljs/ShellJS.expected

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,14 +55,19 @@ test_FileSystemAccess
5555
| tst.js:60:1:60:17 | shelljs.cat(file) |
5656
| tst.js:60:1:60:41 | shelljs ... cement) |
5757
| tst.js:61:1:61:17 | shelljs.cat(file) |
58+
| tst.js:65:1:65:19 | shelljs.which(file) |
5859
test_MissingFileSystemAccess
5960
test_SystemCommandExecution
6061
| tst.js:14:1:14:27 | shelljs ... ts, cb) |
6162
| tst.js:60:1:60:51 | shelljs ... ec(cmd) |
6263
| tst.js:61:1:61:27 | shelljs ... ec(cmd) |
64+
| tst.js:63:1:63:37 | shelljs ... ptions) |
65+
| tst.js:64:1:64:16 | shelljs.cmd(cmd) |
66+
| tst.js:68:1:68:36 | shelljs ... ts, cb) |
6367
test_FileNameSource
6468
| tst.js:15:1:15:26 | shelljs ... file2) |
6569
| tst.js:24:1:24:16 | shelljs.ls(file) |
6670
| tst.js:25:1:25:22 | shelljs ... , file) |
6771
| tst.js:26:1:26:30 | shelljs ... file2) |
6872
| tst.js:27:1:27:24 | shelljs ... file2) |
73+
| tst.js:65:1:65:19 | shelljs.which(file) |

‎javascript/ql/test/library-tests/frameworks/Shelljs/tst.js

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,3 +59,10 @@ shelljs.uniq(opts, file1, file2);
5959

6060
shelljs.cat(file).sed(regex, replacement).exec(cmd);
6161
shelljs.cat(file).exec(cmd);
62+
63+
shelljs.cmd(cmd, arg1, arg2, options);
64+
shelljs.cmd(cmd);
65+
shelljs.which(file);
66+
67+
const shelljssync = require("async-shelljs");
68+
shelljssync.asyncExec(cmd, opts, cb);

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@
22
| actions.js:4:6:4:29 | process ... _DATA'] | actions.js:4:6:4:16 | process.env | actions.js:4:6:4:29 | process ... _DATA'] | This command depends on an unsanitized $@. | actions.js:4:6:4:16 | process.env | environment variable |
33
| actions.js:8:10:8:23 | e['TEST_DATA'] | actions.js:12:6:12:16 | process.env | actions.js:8:10:8:23 | e['TEST_DATA'] | This command depends on an unsanitized $@. | actions.js:12:6:12:16 | process.env | environment variable |
44
| actions.js:14:6:14:21 | getInput('data') | actions.js:14:6:14:21 | getInput('data') | actions.js:14:6:14:21 | getInput('data') | This command depends on an unsanitized $@. | actions.js:14:6:14:21 | getInput('data') | GitHub Actions user input |
5+
| actions.js:18:10:18:40 | 'rm -rf ... 'SOME'] | actions.js:18:22:18:32 | shelljs.env | actions.js:18:10:18:40 | 'rm -rf ... 'SOME'] | This command depends on an unsanitized $@. | actions.js:18:22:18:32 | shelljs.env | environment variable |
6+
| actions.js:19:10:19:37 | 'rm -rf ... nv.SOME | actions.js:19:22:19:32 | shelljs.env | actions.js:19:10:19:37 | 'rm -rf ... nv.SOME | This command depends on an unsanitized $@. | actions.js:19:22:19:32 | shelljs.env | environment variable |
7+
| actions.js:20:10:20:32 | 'rm -rf ... ljs.env | actions.js:20:22:20:32 | shelljs.env | actions.js:20:10:20:32 | 'rm -rf ... ljs.env | This command depends on an unsanitized $@. | actions.js:20:22:20:32 | shelljs.env | environment variable |
58
| command-line-parameter-command-injection.js:4:10:4:21 | process.argv | command-line-parameter-command-injection.js:4:10:4:21 | process.argv | command-line-parameter-command-injection.js:4:10:4:21 | process.argv | This command depends on an unsanitized $@. | command-line-parameter-command-injection.js:4:10:4:21 | process.argv | command-line argument |
69
| command-line-parameter-command-injection.js:8:10:8:36 | "cmd.sh ... argv[2] | command-line-parameter-command-injection.js:8:22:8:33 | process.argv | command-line-parameter-command-injection.js:8:10:8:36 | "cmd.sh ... argv[2] | This command depends on an unsanitized $@. | command-line-parameter-command-injection.js:8:22:8:33 | process.argv | command-line argument |
710
| command-line-parameter-command-injection.js:11:14:11:20 | args[0] | command-line-parameter-command-injection.js:10:13:10:24 | process.argv | command-line-parameter-command-injection.js:11:14:11:20 | args[0] | This command depends on an unsanitized $@. | command-line-parameter-command-injection.js:10:13:10:24 | process.argv | command-line argument |
@@ -44,6 +47,9 @@ edges
4447
| actions.js:7:15:7:15 | e | actions.js:8:10:8:10 | e | provenance | |
4548
| actions.js:8:10:8:10 | e | actions.js:8:10:8:23 | e['TEST_DATA'] | provenance | |
4649
| actions.js:12:6:12:16 | process.env | actions.js:7:15:7:15 | e | provenance | |
50+
| actions.js:18:22:18:32 | shelljs.env | actions.js:18:10:18:40 | 'rm -rf ... 'SOME'] | provenance | |
51+
| actions.js:19:22:19:32 | shelljs.env | actions.js:19:10:19:37 | 'rm -rf ... nv.SOME | provenance | |
52+
| actions.js:20:22:20:32 | shelljs.env | actions.js:20:10:20:32 | 'rm -rf ... ljs.env | provenance | |
4753
| command-line-parameter-command-injection.js:8:22:8:33 | process.argv | command-line-parameter-command-injection.js:8:10:8:36 | "cmd.sh ... argv[2] | provenance | |
4854
| command-line-parameter-command-injection.js:10:6:10:33 | args | command-line-parameter-command-injection.js:11:14:11:17 | args | provenance | |
4955
| command-line-parameter-command-injection.js:10:6:10:33 | args | command-line-parameter-command-injection.js:12:26:12:29 | args | provenance | |
@@ -181,6 +187,12 @@ nodes
181187
| actions.js:8:10:8:23 | e['TEST_DATA'] | semmle.label | e['TEST_DATA'] |
182188
| actions.js:12:6:12:16 | process.env | semmle.label | process.env |
183189
| actions.js:14:6:14:21 | getInput('data') | semmle.label | getInput('data') |
190+
| actions.js:18:10:18:40 | 'rm -rf ... 'SOME'] | semmle.label | 'rm -rf ... 'SOME'] |
191+
| actions.js:18:22:18:32 | shelljs.env | semmle.label | shelljs.env |
192+
| actions.js:19:10:19:37 | 'rm -rf ... nv.SOME | semmle.label | 'rm -rf ... nv.SOME |
193+
| actions.js:19:22:19:32 | shelljs.env | semmle.label | shelljs.env |
194+
| actions.js:20:10:20:32 | 'rm -rf ... ljs.env | semmle.label | 'rm -rf ... ljs.env |
195+
| actions.js:20:22:20:32 | shelljs.env | semmle.label | shelljs.env |
184196
| command-line-parameter-command-injection.js:4:10:4:21 | process.argv | semmle.label | process.argv |
185197
| command-line-parameter-command-injection.js:8:10:8:36 | "cmd.sh ... argv[2] | semmle.label | "cmd.sh ... argv[2] |
186198
| command-line-parameter-command-injection.js:8:22:8:33 | process.argv | semmle.label | process.argv |

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,3 +12,10 @@ function test(e) {
1212
test(process.env); // $ Source
1313

1414
exec(getInput('data')); // $ Alert
15+
16+
function test2(e) {
17+
const shelljs = require('shelljs');
18+
exec('rm -rf ' + shelljs.env['SOME']); // $ Alert
19+
exec('rm -rf ' + shelljs.env.SOME); // $ Alert
20+
exec('rm -rf ' + shelljs.env); // $ Alert
21+
}

0 commit comments

Comments
 (0)