security: improve dangerous command detection for rm#25545
security: improve dangerous command detection for rm#25545cocosheng-g wants to merge 1 commit intomainfrom
Conversation
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 strengthens the security posture of the CLI by improving the detection of potentially destructive shell commands and ensuring that safety heuristics cannot be bypassed in YOLO mode. These changes prevent accidental mass deletions and provide clearer feedback to users when approving tool execution. Highlights
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
|
|
Size Change: -99 B (0%) Total Size: 33.6 MB
ℹ️ View Unchanged
|
cocosheng-g
force-pushed
the
fix/rm-safety
branch
from
fb276c3 to
75ba607
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
cocosheng-g
force-pushed
the
fix/rm-safety
branch
2 times, most recently
from
377a015 to
16748e1
Compare
cocosheng-g
changed the title
cocosheng-g
force-pushed
the
fix/rm-safety
branch
4 times, most recently
from
11b6bcf to
738fd27
Compare
cocosheng-g
force-pushed
the
fix/rm-safety
branch
5 times, most recently
from
f3173fc to
9f17bbe
Compare
cocosheng-g
changed the title
cocosheng-g
force-pushed
the
fix/rm-safety
branch
from
9f17bbe to
00bf360
Compare
cocosheng-g
force-pushed
the
fix/rm-safety
branch
from
00bf360 to
dbbed7f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves command safety by utilizing normalizeCommand in both POSIX and Windows implementations and expanding the detection of the rm command. New tests have been added to verify these improvements. The review identifies potential security bypasses, specifically regarding how sudo flags are handled in POSIX and the lack of coverage for certain file extensions and aliases on Windows.
| } | ||
|
|
||
| const cmd = args[0]; | ||
| const cmd = normalizeCommand(args[0]); |
There was a problem hiding this comment.
The isDangerousCommand function is vulnerable to bypass when sudo is used with flags (e.g., sudo -u root rm). The recursive call isDangerousCommand(args.slice(1)) in the sudo block (lines 438-440) incorrectly processes the arguments, causing normalizeCommand to check a flag like -u instead of the actual dangerous command. This allows dangerous commands to be executed without detection, circumventing safety logic like YOLO mode overrides or user prompts.
References
- Correct identification of dangerous commands is essential for the system to apply safety policies, such as YOLO mode's 'ALLOW' decision or the default 'ASK_USER' prompt.
| if (cmd.endsWith('.exe')) { | ||
| cmd = cmd.slice(0, -4); | ||
| } | ||
| const cmd = normalizeCommand(args[0]); |
There was a problem hiding this comment.
The isDangerousCommand function on Windows has multiple bypass vulnerabilities. Firstly, normalizeCommand does not account for trailing dots or other executable extensions (like .bat, .cmd). This allows an attacker to bypass dangerous command detection by appending a dot to a dangerous command name (e.g., powershell. resolves to powershell.exe). Secondly, the common PowerShell alias rm for Remove-Item is not explicitly flagged as dangerous, creating a security gap. Both issues allow dangerous commands to be executed without detection, preventing the system from correctly applying safety policies.
| const cmd = normalizeCommand(args[0]); | |
| const cmd = normalizeCommand(args[0]); | |
| if (cmd === 'rm') return true; |
References
- Correct identification of dangerous commands is essential for the system to apply safety policies, such as YOLO mode's 'ALLOW' decision or the default 'ASK_USER' prompt.
1 participant
Summary
Improve dangerous command detection for
rmto prevent accidental mass deletion.Details
rmDetection: UpdatedisDangerousCommandin POSIX environments to correctly identifyrmregardless of path qualification (e.g.,/bin/rm) or flag ordering.rmcommand is now always flagged as dangerous, removing the fragile check that only looked for specific flags like-rfin the first argument position.delanderaseremain Windows-specific dangerous commands, andrmis correctly scoped to POSIX).Related Issues
Closes #25543
How to Validate
npm test -w @google/gemini-cli-core -- src/sandbox/utils/commandSafety.test.tsto verify the improved POSIX safety heuristics.rm test.txtand verify it triggers a confirmation prompt.Pre-Merge Checklist