[MEDIUM] Prevent shell injection through environment interpolation - #583
Open
OskarEichler wants to merge 1 commit into
Open
[MEDIUM] Prevent shell injection through environment interpolation#583OskarEichler wants to merge 1 commit into
OskarEichler wants to merge 1 commit into
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
$VARand${VAR}expansion in environment values.Security impact
Command#environment_stringcurrently places keys directly into anexportstatement and places values inside double quotes. A caller that passes attacker-influenced environment data can therefore execute commands through a crafted key,$(), or backticks. The command runs through both the Local shell backend and Net::SSH's remoteexecpath.This is rated medium urgency: successful exploitation executes commands with the deployment user's privileges, but it requires an application or deployment integration to pass attacker-controlled keys or values into SSHKit's environment map.
Reproduction
A bounded external model passed three environment payloads to
SSHKit::Command.new(:true, env: ...)and executed the resulting command locally:All three markers are created on v1.25.1 and current master (
28e04dc). None are created with this patch.Compatibility
The escaping deliberately retains the documented
$PATHbehavior and plain${PATH}references. Focused execution also verifies spaces, quotes, escaped dollars, and ordinary backslashes keep their values.Valid environment keys retain their current behavior. Invalid keys now raise
ArgumentError; those keys could not form valid shell assignments. Command substitution in values is now passed literally rather than evaluated.Verification
rake buildproducessshkit-1.25.1.gemsuccessfully.git diff --checkpasses.The Docker-backed remote functional suite was not run, and no real deployment or remote host was used. No test files, dependency metadata, or version files are changed.
Breaking changes
No change for valid environment names or ordinary values. Integrations intentionally relying on command substitution inside environment values must move that shell logic into an explicit command.