From f6fae7ad60ad011e60ba072d9b7e68ac42fbcd42 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Wed, 30 Apr 2025 13:33:31 +0200 Subject: [PATCH 1/8] Added test cases for `cmd`, `which` and `asyncExec` --- .../test/library-tests/frameworks/Shelljs/ShellJS.expected | 1 + javascript/ql/test/library-tests/frameworks/Shelljs/tst.js | 7 +++++++ 2 files changed, 8 insertions(+) diff --git a/javascript/ql/test/library-tests/frameworks/Shelljs/ShellJS.expected b/javascript/ql/test/library-tests/frameworks/Shelljs/ShellJS.expected index 003ae3f442f8..de70e7a1d0af 100644 --- a/javascript/ql/test/library-tests/frameworks/Shelljs/ShellJS.expected +++ b/javascript/ql/test/library-tests/frameworks/Shelljs/ShellJS.expected @@ -56,6 +56,7 @@ test_FileSystemAccess | tst.js:60:1:60:41 | shelljs ... cement) | | tst.js:61:1:61:17 | shelljs.cat(file) | test_MissingFileSystemAccess +| tst.js:65:15:65:18 | file | test_SystemCommandExecution | tst.js:14:1:14:27 | shelljs ... ts, cb) | | tst.js:60:1:60:51 | shelljs ... ec(cmd) | diff --git a/javascript/ql/test/library-tests/frameworks/Shelljs/tst.js b/javascript/ql/test/library-tests/frameworks/Shelljs/tst.js index 9e7905c98477..2bbcd51f5821 100644 --- a/javascript/ql/test/library-tests/frameworks/Shelljs/tst.js +++ b/javascript/ql/test/library-tests/frameworks/Shelljs/tst.js @@ -59,3 +59,10 @@ shelljs.uniq(opts, file1, file2); shelljs.cat(file).sed(regex, replacement).exec(cmd); shelljs.cat(file).exec(cmd); + +shelljs.cmd(cmd, arg1, arg2, options); +shelljs.cmd(cmd); +shelljs.which(file); + +const shelljssync = require("async-shelljs"); +shelljssync.asyncExec(cmd, opts, cb); From 25d04f1cdd11e5bf6372924363035ac2cdb4895a Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Wed, 30 Apr 2025 13:35:17 +0200 Subject: [PATCH 2/8] Added support for `shelljs.which` --- .../ql/lib/semmle/javascript/frameworks/ShellJS.qll | 9 ++++++--- .../library-tests/frameworks/Shelljs/ShellJS.expected | 3 ++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/ShellJS.qll b/javascript/ql/lib/semmle/javascript/frameworks/ShellJS.qll index 9f3eeb6e0dc9..91b18a1779d1 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/ShellJS.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/ShellJS.qll @@ -14,7 +14,8 @@ module ShellJS { shellJSMember() .getMember([ "exec", "cd", "cp", "touch", "chmod", "pushd", "find", "ls", "ln", "mkdir", "mv", - "rm", "cat", "head", "sort", "tail", "uniq", "grep", "sed", "to", "toEnd", "echo" + "rm", "cat", "head", "sort", "tail", "uniq", "grep", "sed", "to", "toEnd", "echo", + "which", ]) .getReturn() } @@ -99,7 +100,8 @@ module ShellJS { */ private class ShellJSGenericFileAccess extends FileSystemAccess, ShellJSCall { ShellJSGenericFileAccess() { - name = ["cd", "cp", "touch", "chmod", "pushd", "find", "ls", "ln", "mkdir", "mv", "rm"] + name = + ["cd", "cp", "touch", "chmod", "pushd", "find", "ls", "ln", "mkdir", "mv", "rm", "which"] } override DataFlow::Node getAPathArgument() { result = this.getAnArgument() } @@ -111,7 +113,8 @@ module ShellJS { private class ShellJSFilenameSource extends FileNameSource, ShellJSCall { ShellJSFilenameSource() { name = "find" or - name = "ls" + name = "ls" or + name = "which" } } diff --git a/javascript/ql/test/library-tests/frameworks/Shelljs/ShellJS.expected b/javascript/ql/test/library-tests/frameworks/Shelljs/ShellJS.expected index de70e7a1d0af..4ca9f088bf49 100644 --- a/javascript/ql/test/library-tests/frameworks/Shelljs/ShellJS.expected +++ b/javascript/ql/test/library-tests/frameworks/Shelljs/ShellJS.expected @@ -55,8 +55,8 @@ test_FileSystemAccess | tst.js:60:1:60:17 | shelljs.cat(file) | | tst.js:60:1:60:41 | shelljs ... cement) | | tst.js:61:1:61:17 | shelljs.cat(file) | +| tst.js:65:1:65:19 | shelljs.which(file) | test_MissingFileSystemAccess -| tst.js:65:15:65:18 | file | test_SystemCommandExecution | tst.js:14:1:14:27 | shelljs ... ts, cb) | | tst.js:60:1:60:51 | shelljs ... ec(cmd) | @@ -67,3 +67,4 @@ test_FileNameSource | tst.js:25:1:25:22 | shelljs ... , file) | | tst.js:26:1:26:30 | shelljs ... file2) | | tst.js:27:1:27:24 | shelljs ... file2) | +| tst.js:65:1:65:19 | shelljs.which(file) | From 18cea2d6a5dec92818986f34d5730f769926ead8 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Wed, 30 Apr 2025 13:37:02 +0200 Subject: [PATCH 3/8] Added support for `shelljs.cmd` and `async-shelljs.asyncExec` --- .../semmle/javascript/frameworks/ShellJS.qll | 23 ++++++++++++++----- .../frameworks/Shelljs/ShellJS.expected | 3 +++ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/ShellJS.qll b/javascript/ql/lib/semmle/javascript/frameworks/ShellJS.qll index 91b18a1779d1..ae0ba17941c2 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/ShellJS.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/ShellJS.qll @@ -15,7 +15,7 @@ module ShellJS { .getMember([ "exec", "cd", "cp", "touch", "chmod", "pushd", "find", "ls", "ln", "mkdir", "mv", "rm", "cat", "head", "sort", "tail", "uniq", "grep", "sed", "to", "toEnd", "echo", - "which", + "which", "cmd", "asyncExec" ]) .getReturn() } @@ -154,16 +154,27 @@ module ShellJS { } /** - * A call to `shelljs.exec()` modeled as command execution. + * A call to `shelljs.exec()`, `shelljs.cmd()`, or `async-shelljs.asyncExec()` modeled as command execution. */ private class ShellJSExec extends SystemCommandExecution, ShellJSCall { - ShellJSExec() { name = "exec" } - - override DataFlow::Node getACommandArgument() { result = this.getArgument(0) } + ShellJSExec() { name = ["exec", "cmd", "asyncExec"] } + + override DataFlow::Node getACommandArgument() { + if name = "cmd" + then + result = this.getArgument(_) and + not ( + result = this.getLastArgument() and + exists(this.getOptionsArg()) + ) + else + // For exec/asyncExec: only first argument is command + result = this.getArgument(0) + } override predicate isShellInterpreted(DataFlow::Node arg) { arg = this.getACommandArgument() } - override predicate isSync() { none() } + override predicate isSync() { name = "cmd" } override DataFlow::Node getOptionsArg() { result = this.getLastArgument() and diff --git a/javascript/ql/test/library-tests/frameworks/Shelljs/ShellJS.expected b/javascript/ql/test/library-tests/frameworks/Shelljs/ShellJS.expected index 4ca9f088bf49..5c3c922a84fc 100644 --- a/javascript/ql/test/library-tests/frameworks/Shelljs/ShellJS.expected +++ b/javascript/ql/test/library-tests/frameworks/Shelljs/ShellJS.expected @@ -61,6 +61,9 @@ test_SystemCommandExecution | tst.js:14:1:14:27 | shelljs ... ts, cb) | | tst.js:60:1:60:51 | shelljs ... ec(cmd) | | tst.js:61:1:61:27 | shelljs ... ec(cmd) | +| tst.js:63:1:63:37 | shelljs ... ptions) | +| tst.js:64:1:64:16 | shelljs.cmd(cmd) | +| tst.js:68:1:68:36 | shelljs ... ts, cb) | test_FileNameSource | tst.js:15:1:15:26 | shelljs ... file2) | | tst.js:24:1:24:16 | shelljs.ls(file) | From 40d176a7703fd01407179668f3921e1702e48da9 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Wed, 30 Apr 2025 13:45:50 +0200 Subject: [PATCH 4/8] Added model for shelljs.env --- javascript/ql/lib/ext/shelljs.model.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 javascript/ql/lib/ext/shelljs.model.yml diff --git a/javascript/ql/lib/ext/shelljs.model.yml b/javascript/ql/lib/ext/shelljs.model.yml new file mode 100644 index 000000000000..8d2c8db645d8 --- /dev/null +++ b/javascript/ql/lib/ext/shelljs.model.yml @@ -0,0 +1,6 @@ +extensions: + - addsTo: + pack: codeql/javascript-all + extensible: sourceModel + data: + - ["shelljs", "Member[env]", "environment"] From 602500e280797acb207b4d41020fe0b36090d583 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Wed, 30 Apr 2025 13:40:52 +0200 Subject: [PATCH 5/8] Added change note --- javascript/ql/lib/change-notes/2025-04-30-shelljs.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 javascript/ql/lib/change-notes/2025-04-30-shelljs.md diff --git a/javascript/ql/lib/change-notes/2025-04-30-shelljs.md b/javascript/ql/lib/change-notes/2025-04-30-shelljs.md new file mode 100644 index 000000000000..90a5f5a2a308 --- /dev/null +++ b/javascript/ql/lib/change-notes/2025-04-30-shelljs.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* 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`. From 33d8ffa83e2866eb6c9602cbd570999bf8e83c31 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Thu, 1 May 2025 11:11:29 +0200 Subject: [PATCH 6/8] Added test cases for shelljs.env --- .../Security/CWE-078/IndirectCommandInjection/actions.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/actions.js b/javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/actions.js index 021715395217..a92b7713491c 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/actions.js +++ b/javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/actions.js @@ -12,3 +12,10 @@ function test(e) { test(process.env); // $ Source exec(getInput('data')); // $ Alert + +function test2(e) { + const shelljs = require('shelljs'); + exec('rm -rf ' + shelljs.env['SOME']); // $ MISSING: Alert + exec('rm -rf ' + shelljs.env.SOME); // $ MISSING: Alert + exec('rm -rf ' + shelljs.env); // $ MISSING: Alert +} From d4b5ef6a6659258f2d837cdcc006fd3e60a49562 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Thu, 1 May 2025 11:14:15 +0200 Subject: [PATCH 7/8] Refactor process.env handling in CleartextLogging and IndirectCommandInjection modules to use ThreatModelSource --- .../dataflow/CleartextLoggingCustomizations.qll | 2 +- .../IndirectCommandInjectionCustomizations.qll | 4 ++-- .../CWE-295/DisablingCertificateValidation.ql | 2 +- .../IndirectCommandInjection.expected | 12 ++++++++++++ .../CWE-078/IndirectCommandInjection/actions.js | 6 +++--- 5 files changed, 19 insertions(+), 7 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/CleartextLoggingCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/CleartextLoggingCustomizations.qll index 5dca4cf1df28..dbb775f99b58 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/CleartextLoggingCustomizations.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/CleartextLoggingCustomizations.qll @@ -171,7 +171,7 @@ module CleartextLogging { /** An access to the sensitive object `process.env`. */ class ProcessEnvSource extends Source { - ProcessEnvSource() { this = NodeJSLib::process().getAPropertyRead("env") } + ProcessEnvSource() { this.(ThreatModelSource).getThreatModel() = "environment" } override string describe() { result = "process environment" } } diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/IndirectCommandInjectionCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/IndirectCommandInjectionCustomizations.qll index 9dd6ab4b4a91..84c4600fbc03 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/IndirectCommandInjectionCustomizations.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/IndirectCommandInjectionCustomizations.qll @@ -29,7 +29,7 @@ module IndirectCommandInjection { * A read of `process.env`, considered as a flow source for command injection. */ private class ProcessEnvAsSource extends Source { - ProcessEnvAsSource() { this = NodeJSLib::process().getAPropertyRead("env") } + ProcessEnvAsSource() { this.(ThreatModelSource).getThreatModel() = "environment" } override string describe() { result = "environment variable" } } @@ -37,7 +37,7 @@ module IndirectCommandInjection { /** Gets a data flow node referring to `process.env`. */ private DataFlow::SourceNode envObject(DataFlow::TypeTracker t) { t.start() and - result = NodeJSLib::process().getAPropertyRead("env") + result.(ThreatModelSource).getThreatModel() = "environment" or exists(DataFlow::TypeTracker t2 | result = envObject(t2).track(t2, t)) } diff --git a/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.ql b/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.ql index d9f7267d425a..b5e52d7c6c79 100644 --- a/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.ql +++ b/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.ql @@ -37,7 +37,7 @@ DataFlow::ObjectLiteralNode tlsOptions() { result.flowsTo(tlsInvocation().getAnA from DataFlow::PropWrite disable where exists(DataFlow::SourceNode env | - env = NodeJSLib::process().getAPropertyRead("env") and + env.(ThreatModelSource).getThreatModel() = "environment" and disable = env.getAPropertyWrite("NODE_TLS_REJECT_UNAUTHORIZED") and disable.getRhs().mayHaveStringValue("0") ) diff --git a/javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/IndirectCommandInjection.expected b/javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/IndirectCommandInjection.expected index 7c7321845776..9fc6f6b1bc4e 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/IndirectCommandInjection.expected +++ b/javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/IndirectCommandInjection.expected @@ -2,6 +2,9 @@ | 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 | | 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 | | 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 | +| 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 | +| 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 | +| 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 | | 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 | | 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 | | 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 | actions.js:7:15:7:15 | e | actions.js:8:10:8:10 | e | provenance | | | actions.js:8:10:8:10 | e | actions.js:8:10:8:23 | e['TEST_DATA'] | provenance | | | actions.js:12:6:12:16 | process.env | actions.js:7:15:7:15 | e | provenance | | +| actions.js:18:22:18:32 | shelljs.env | actions.js:18:10:18:40 | 'rm -rf ... 'SOME'] | provenance | | +| actions.js:19:22:19:32 | shelljs.env | actions.js:19:10:19:37 | 'rm -rf ... nv.SOME | provenance | | +| actions.js:20:22:20:32 | shelljs.env | actions.js:20:10:20:32 | 'rm -rf ... ljs.env | provenance | | | 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 | | | command-line-parameter-command-injection.js:10:6:10:33 | args | command-line-parameter-command-injection.js:11:14:11:17 | args | provenance | | | 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 | actions.js:8:10:8:23 | e['TEST_DATA'] | semmle.label | e['TEST_DATA'] | | actions.js:12:6:12:16 | process.env | semmle.label | process.env | | actions.js:14:6:14:21 | getInput('data') | semmle.label | getInput('data') | +| actions.js:18:10:18:40 | 'rm -rf ... 'SOME'] | semmle.label | 'rm -rf ... 'SOME'] | +| actions.js:18:22:18:32 | shelljs.env | semmle.label | shelljs.env | +| actions.js:19:10:19:37 | 'rm -rf ... nv.SOME | semmle.label | 'rm -rf ... nv.SOME | +| actions.js:19:22:19:32 | shelljs.env | semmle.label | shelljs.env | +| actions.js:20:10:20:32 | 'rm -rf ... ljs.env | semmle.label | 'rm -rf ... ljs.env | +| actions.js:20:22:20:32 | shelljs.env | semmle.label | shelljs.env | | command-line-parameter-command-injection.js:4:10:4:21 | process.argv | semmle.label | process.argv | | command-line-parameter-command-injection.js:8:10:8:36 | "cmd.sh ... argv[2] | semmle.label | "cmd.sh ... argv[2] | | command-line-parameter-command-injection.js:8:22:8:33 | process.argv | semmle.label | process.argv | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/actions.js b/javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/actions.js index a92b7713491c..8fb6994a597c 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/actions.js +++ b/javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/actions.js @@ -15,7 +15,7 @@ exec(getInput('data')); // $ Alert function test2(e) { const shelljs = require('shelljs'); - exec('rm -rf ' + shelljs.env['SOME']); // $ MISSING: Alert - exec('rm -rf ' + shelljs.env.SOME); // $ MISSING: Alert - exec('rm -rf ' + shelljs.env); // $ MISSING: Alert + exec('rm -rf ' + shelljs.env['SOME']); // $ Alert + exec('rm -rf ' + shelljs.env.SOME); // $ Alert + exec('rm -rf ' + shelljs.env); // $ Alert } From 871e93d9fe2309d757bc6d19484fb491c9b31b64 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Fri, 2 May 2025 13:39:46 +0200 Subject: [PATCH 8/8] Update javascript/ql/lib/semmle/javascript/frameworks/ShellJS.qll Co-authored-by: Asger F --- javascript/ql/lib/semmle/javascript/frameworks/ShellJS.qll | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/ShellJS.qll b/javascript/ql/lib/semmle/javascript/frameworks/ShellJS.qll index ae0ba17941c2..fc0e366df4b8 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/ShellJS.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/ShellJS.qll @@ -163,10 +163,7 @@ module ShellJS { if name = "cmd" then result = this.getArgument(_) and - not ( - result = this.getLastArgument() and - exists(this.getOptionsArg()) - ) + not result = this.getOptionsArg() else // For exec/asyncExec: only first argument is command result = this.getArgument(0)