-
Notifications
You must be signed in to change notification settings - Fork 1.9k
JS: Modeling of ShellJS functions
#19422
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.
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
Conversation
…Injection modules to use ThreatModelSource
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR extends CodeQL’s JavaScript analysis to model additional ShellJS and async-shelljs functionality, specifically command location (which), generic command execution (cmd), asynchronous execution (asyncExec), and environment-variable sources (env).
- Added new tests and expected results for
shelljs.which,shelljs.cmd,async-shelljs.asyncExec, andshelljs.envin security query tests and library-tests. - Updated ShellJS QL framework (
ShellJS.qllandshelljs.model.yml) to include support for the new functions and to tagshelljs.envas an environment source. - Refactored environment-source handling in dataflow customization modules to use
ThreatModelSourcetagging.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/actions.js | Added test2 using shelljs.env to trigger indirect command injection alerts |
| javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/IndirectCommandInjection.expected | Updated expected alerts for the new shelljs.env cases |
| javascript/ql/test/library-tests/frameworks/Shelljs/tst.js | Extended library tests with shelljs.cmd, shelljs.which, and asyncExec |
| javascript/ql/test/library-tests/frameworks/Shelljs/ShellJS.expected | Added expected flow entries for the new ShellJS calls |
| javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.ql | Switched to threat-model tagging for process.env source |
| javascript/ql/lib/semmle/javascript/security/dataflow/IndirectCommandInjectionCustomizations.qll | Refactored ProcessEnvAsSource and envObject to use ThreatModelSource |
| javascript/ql/lib/semmle/javascript/security/dataflow/CleartextLoggingCustomizations.qll | Updated ProcessEnvSource to apply threat-model tagging |
| javascript/ql/lib/semmle/javascript/frameworks/ShellJS.qll | Added which, cmd, asyncExec to ShellJS model and adjusted call handling |
| javascript/ql/lib/ext/shelljs.model.yml | Declared shelljs.Member[env] as an environment source |
| javascript/ql/lib/change-notes/2025-04-30-shelljs.md | Documented the minor-analysis changes for new ShellJS/async-shelljs support |
Comments suppressed due to low confidence (3)
javascript/ql/lib/semmle/javascript/security/dataflow/IndirectCommandInjectionCustomizations.qll:32
- The constructor for
ProcessEnvAsSourceno longer initializesthisto theprocess.envproperty read, so it won’t actually captureprocess.envas a source. You should restorethis = NodeJSLib::process().getAPropertyRead("env")and then apply the threat-model tag.
ProcessEnvAsSource() { this.(ThreatModelSource).getThreatModel() = "environment" }
javascript/ql/lib/semmle/javascript/security/dataflow/CleartextLoggingCustomizations.qll:174
- This constructor only applies the threat-model tag but never sets
thistoNodeJSLib::process().getAPropertyRead("env"). Without the property-read node, the source won’t be detected. Reintroduce the property-read assignment before tagging.
ProcessEnvSource() { this.(ThreatModelSource).getThreatModel() = "environment" }
javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.ql:40
- The original assignment
env = NodeJSLib::process().getAPropertyRead("env")was removed. Nowenvis never bound to the property-read node. You need to both assign the property-read and then tag it withThreatModelSource.
env.(ThreatModelSource).getThreatModel() = "environment" and
asgerf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question otherwise LGTM
Co-authored-by: Asger F <asgerf@github.com>
Added support for additional functions in the
shelljsandasync-shelljslibraries:which: Command location functionalitycmd: Command execution wrapperasyncExec: Asynchronous command executionenv: Environment variable handling