🛡️ Sentinel: [CRITICAL] Fix PowerShell command injection vulnerability#292
🛡️ Sentinel: [CRITICAL] Fix PowerShell command injection vulnerability#292Dexploarer wants to merge 1 commit intodevelopfrom
Conversation
…e and computer/keypress
🚨 Severity: CRITICAL
💡 Vulnerability: Arbitrary user input passed to `powershell -Command` via `execSync` on Windows using standard single-quote escaping (e.g., `text.replace(/'/g, "''")`) can be bypassed, leading to command injection vulnerabilities.
🎯 Impact: An attacker could execute arbitrary PowerShell commands on a Windows host machine via the `/api/sandbox/computer/type` and `/api/sandbox/computer/keypress` endpoints.
🔧 Fix: Replaced the vulnerable string concatenation with a secure approach. The user input is now encoded as a Base64 string in Node and decoded natively inside the PowerShell script block using `[System.Text.Encoding]::UTF8.GetString([System.Convert]::FromBase64String('${base64}'))` before being used.
✅ Verification: Ran unit tests and e2e integration tests for the sandbox subsystem. Evaluated the implementation logic correctly mitigating all shell command escaping issues by removing control characters entirely during transport. Documented learning in `.jules/sentinel.md`.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical command injection vulnerability affecting the sandbox subsystem on Windows. The previous method of escaping user input for PowerShell commands was found to be insufficient, allowing for potential arbitrary code execution. The updated solution enhances security by employing Base64 encoding for user input, ensuring that data passed to PowerShell is safely handled and preventing malicious command injection. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| throw new Error("xdotool required for keyboard input on Linux."); | ||
| } | ||
| } else if (os === "win32") { | ||
| const escaped = text.replace(/'/g, "''"); | ||
| const base64 = Buffer.from(text).toString('base64'); | ||
| runCommand( | ||
| "powershell", | ||
| [ | ||
| "-Command", | ||
| `Add-Type -AssemblyName System.Windows.Forms; [System.Windows.Forms.SendKeys]::SendWait('${escaped}')`, | ||
| `$text = [System.Text.Encoding]::UTF8.GetString([System.Convert]::FromBase64String('${base64}')); Add-Type -AssemblyName System.Windows.Forms; [System.Windows.Forms.SendKeys]::SendWait($text)`, | ||
| ], | ||
| 10000, | ||
| ); |
There was a problem hiding this comment.
Lack of input validation for Windows PowerShell SendKeys in performType
The Windows branch accepts arbitrary text, encodes it in base64, and passes it to PowerShell's SendKeys. If the input contains unsupported or special characters, this may cause unpredictable behavior or errors. It is recommended to validate or sanitize the input string to ensure only safe characters are sent, or to handle/escape special characters appropriately.
Example improvement:
// Before encoding, validate or sanitize text
if (!SAFE_KEYPRESS_PATTERN.test(text)) {
throw new Error("Input contains unsupported characters for SendKeys.");
}| throw new Error("xdotool required for key input on Linux."); | ||
| } | ||
| } else if (os === "win32") { | ||
| const escaped = keys.replace(/'/g, "''"); | ||
| const base64 = Buffer.from(keys).toString('base64'); | ||
| runCommand( | ||
| "powershell", | ||
| [ | ||
| "-Command", | ||
| `Add-Type -AssemblyName System.Windows.Forms; [System.Windows.Forms.SendKeys]::SendWait('${escaped}')`, | ||
| `$keys = [System.Text.Encoding]::UTF8.GetString([System.Convert]::FromBase64String('${base64}')); Add-Type -AssemblyName System.Windows.Forms; [System.Windows.Forms.SendKeys]::SendWait($keys)`, | ||
| ], | ||
| 5000, | ||
| ); |
There was a problem hiding this comment.
No error handling for external command execution in Windows/Linux branches of performKeypress
The runCommand calls for PowerShell and xdotool are not wrapped in try-catch blocks. If the command fails (e.g., due to invalid input or missing dependencies), the error will propagate and may not be handled gracefully. It is recommended to wrap these calls in try-catch and provide meaningful error responses to improve robustness.
Example improvement:
try {
runCommand(...);
} catch (err) {
throw new Error(`Keypress failed: ${err instanceof Error ? err.message : String(err)}`);
}![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a critical command injection vulnerability in the PowerShell commands used for computer/type and computer/keypress endpoints. The fix correctly replaces the insecure string escaping with Base64 encoding and decoding, which is a robust solution to this class of vulnerability. The changes are effective and well-documented in the new sentinel.md file. I have a suggestion to improve maintainability by reducing code duplication in the newly added PowerShell command strings.
| [ | ||
| "-Command", | ||
| `Add-Type -AssemblyName System.Windows.Forms; [System.Windows.Forms.SendKeys]::SendWait('${escaped}')`, | ||
| `$text = [System.Text.Encoding]::UTF8.GetString([System.Convert]::FromBase64String('${base64}')); Add-Type -AssemblyName System.Windows.Forms; [System.Windows.Forms.SendKeys]::SendWait($text)`, |
There was a problem hiding this comment.
To improve maintainability and reduce code duplication with the performKeypress function, consider using a more generic variable name like $input in your PowerShell command. This would make the command string identical in both places, paving the way for a potential helper function to generate the command.
$input = [System.Text.Encoding]::UTF8.GetString([System.Convert]::FromBase64String('${base64}')); Add-Type -AssemblyName System.Windows.Forms; [System.Windows.Forms.SendKeys]::SendWait($input)| [ | ||
| "-Command", | ||
| `Add-Type -AssemblyName System.Windows.Forms; [System.Windows.Forms.SendKeys]::SendWait('${escaped}')`, | ||
| `$keys = [System.Text.Encoding]::UTF8.GetString([System.Convert]::FromBase64String('${base64}')); Add-Type -AssemblyName System.Windows.Forms; [System.Windows.Forms.SendKeys]::SendWait($keys)`, |
There was a problem hiding this comment.
To improve maintainability and reduce code duplication with the performType function, consider using a more generic variable name like $input in your PowerShell command. This makes the command string identical in both places, which is a good practice.
$input = [System.Text.Encoding]::UTF8.GetString([System.Convert]::FromBase64String('${base64}')); Add-Type -AssemblyName System.Windows.Forms; [System.Windows.Forms.SendKeys]::SendWait($input)1 participant
🛡️ Sentinel: [CRITICAL] Fix command injection in sandbox computer/type and computer/keypress
🚨 Severity: CRITICAL
💡 Vulnerability: Arbitrary user input passed to
powershell -CommandviaexecSyncon Windows using standard single-quote escaping (e.g.,text.replace(/'/g, "''")) can be bypassed, leading to command injection vulnerabilities.🎯 Impact: An attacker could execute arbitrary PowerShell commands on a Windows host machine via the
/api/sandbox/computer/typeand/api/sandbox/computer/keypressendpoints.🔧 Fix: Replaced the vulnerable string concatenation with a secure approach. The user input is now encoded as a Base64 string in Node and decoded natively inside the PowerShell script block using
[System.Text.Encoding]::UTF8.GetString([System.Convert]::FromBase64String('${base64}'))before being used.✅ Verification: Ran unit tests and e2e integration tests for the sandbox subsystem. Evaluated the implementation logic correctly mitigating all shell command escaping issues by removing control characters entirely during transport. Documented learning in
.jules/sentinel.md.PR created automatically by Jules for task 1556986540250368535 started by @Dexploarer