security: sanitize user inputs in SandboxShellTool to prevent shell command injection#1339
Open
programming-pupil wants to merge 1 commit into
Open
security: sanitize user inputs in SandboxShellTool to prevent shell command injection#1339programming-pupil wants to merge 1 commit into
programming-pupil wants to merge 1 commit into
Conversation
Author
|
@appleboy @zhoupeng @Shellmode @cnJasonZ Hi, I’m a big fan of OpenManus and would love to see it keep evolving. Since it’s been a while since the last update, I wanted to offer my help. Beyond this PR, I’m very much open to helping with issue triaging or ongoing maintenance if you’re looking for a co-maintainer. Let me know your thoughts! |
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
SandboxShellTool._execute_command()constructs shell commands by directly interpolating user-controlled inputs (session_name,folder,command) into f-strings passed totmuxandcd. A malicious or malformed input likesession_name="foo; rm -rf /"would be executed verbatim by the shell.This PR adds
shlex.quote()sanitization to all user-controlled values before they are interpolated into shell command strings, preventing injection attacks.## Changes
app/tool/sandbox/sb_shell_tool.py:import shlexat file topsession_namewithshlex.quote()in alltmuxcommands within_execute_command()folder/cwdwithshlex.quote()incdcommand constructioncommandwithshlex.quote()is NOT applied to the user command itself (it's intentionally a shell command), but thecwdandsession_namewrapping it are protected_check_command_output()and_terminate_command()## Attack vector example
After this fix,
shlex.quote()wraps the session name so it becomes a safe literal string.## Why this matters
SandboxShellToolis designed to run in sandbox environments, but the tmux management commands run on the host processsession_nameorfoldercould contain shell metacharacters## Test plan
python -m py_compile app/tool/sandbox/sb_shell_tool.pypassesexecute(action="execute_command", command="ls", session_name="my_session")works as beforesession_name='foo"; echo pwned #'does NOT executeecho pwnedfolder="my folder/sub dir"works correctly