JS: Enhance command injection detection for CLI argument parsing libraries #20151
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances command injection detection by improving the modeling of four popular command-line argument parsing libraries: arg, args, command-line-args, and commander. The changes enable better taint tracking from user input through these libraries to identify potential command injection vulnerabilities.
Key Changes
- Enhanced taint flow modeling for commander, yargs, arg, and command-line-args libraries
- Added comprehensive test cases covering various usage patterns of these libraries
- Updated expected test results to include new command injection detection alerts
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| command-line-libs.js | New test file demonstrating command injection vulnerabilities through various CLI parsing libraries |
| CommandInjection.expected | Updated expected results including new alerts for CLI library-based command injections |
| CommandLineArguments.qll | Enhanced taint tracking logic for arg, command-line-args, commander, and yargs libraries |
| 2025-08-01-cli-code-injection.md | Change notes documenting the improved CLI library modeling |
| @@ -87,11 +87,43 @@ private class ArgsParseStep extends TaintTracking::SharedTaintStep { | |||
| override predicate step(DataFlow::Node pred, DataFlow::Node succ) { | |||
| exists(DataFlow::CallNode call | | |||
| call = DataFlow::moduleMember("args", "parse").getACall() or | |||
| call = DataFlow::moduleImport(["yargs-parser", "minimist", "subarg"]).getACall() | |||
| call = | |||
| DataFlow::moduleImport(["yargs-parser", "minimist", "subarg", "yargs/yargs", "yargs"]) | |||
There was a problem hiding this comment.
The module import list includes both "yargs/yargs" and "yargs" which could cause duplicate taint tracking for the same library. The "yargs" entry should be removed here since yargs is handled separately in the yargs() function below.
| DataFlow::moduleImport(["yargs-parser", "minimist", "subarg", "yargs/yargs", "yargs"]) | |
| DataFlow::moduleImport(["yargs-parser", "minimist", "subarg", "yargs/yargs"]) |
Copilot uses AI. Check for mistakes.
| .getArgument(0) | ||
| .(DataFlow::FunctionNode) | ||
| .getAParameter() | ||
| ] |
There was a problem hiding this comment.
This logic assumes the action callback always receives parameters, but commander actions can be called without parameters. This could miss taint flow when the action function has no parameters but still processes command arguments through other means.
| ] | |
| // Propagate to both the function node itself and its parameters | |
| as DataFlow::Node | |
| ] |
Copilot uses AI. Check for mistakes.
Improved modeling of command-line argument parsing libraries: