-
Notifications
You must be signed in to change notification settings - Fork 13.2k
security: improve dangerous command detection for rm #25545
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| /** | ||
| * @license | ||
| * Copyright 2026 Google LLC | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| // Mock shell-utils to avoid relying on tree-sitter WASM | ||
| vi.mock('../../utils/shell-utils.js', () => ({ | ||
| initializeShellParsers: vi.fn().mockResolvedValue(undefined), | ||
| splitCommands: (cmd: string) => [cmd], | ||
| stripShellWrapper: (cmd: string) => cmd, | ||
| extractStringFromParseEntry: (entry: unknown) => { | ||
| if (typeof entry === 'string') return entry; | ||
| if (entry && typeof entry === 'object' && 'content' in entry) { | ||
| return (entry as { content: string }).content; | ||
| } | ||
| return ''; | ||
| }, | ||
| normalizeCommand: (cmd: string) => { | ||
| // Simple mock normalization: /bin/rm -> rm | ||
| if (cmd.startsWith('/')) { | ||
| const parts = cmd.split('/'); | ||
| return parts[parts.length - 1]; | ||
| } | ||
| return cmd; | ||
| }, | ||
| })); | ||
|
|
||
| import { isKnownSafeCommand, isDangerousCommand } from './commandSafety.js'; | ||
|
|
||
| describe('POSIX commandSafety', () => { | ||
| describe('isKnownSafeCommand', () => { | ||
| it('should identify known safe commands', () => { | ||
| expect(isKnownSafeCommand(['ls', '-la'])).toBe(true); | ||
| expect(isKnownSafeCommand(['cat', 'file.txt'])).toBe(true); | ||
| expect(isKnownSafeCommand(['pwd'])).toBe(true); | ||
| expect(isKnownSafeCommand(['echo', 'hello'])).toBe(true); | ||
| }); | ||
|
|
||
| it('should identify safe git commands', () => { | ||
| expect(isKnownSafeCommand(['git', 'status'])).toBe(true); | ||
| expect(isKnownSafeCommand(['git', 'log'])).toBe(true); | ||
| expect(isKnownSafeCommand(['git', 'diff'])).toBe(true); | ||
| }); | ||
|
|
||
| it('should reject unsafe git commands', () => { | ||
| expect(isKnownSafeCommand(['git', 'commit'])).toBe(false); | ||
| expect(isKnownSafeCommand(['git', 'push'])).toBe(false); | ||
| expect(isKnownSafeCommand(['git', 'checkout'])).toBe(false); | ||
| }); | ||
|
|
||
| it('should reject commands with redirection', () => { | ||
| // isKnownSafeCommand handles bash -c "..." which can have redirections | ||
| // but the simple check for atomic commands doesn't see redirection because it's already parsed | ||
| }); | ||
| }); | ||
|
|
||
| describe('isDangerousCommand', () => { | ||
| it('should identify destructive rm commands', () => { | ||
| expect(isDangerousCommand(['rm'])).toBe(true); | ||
| expect(isDangerousCommand(['rm', 'file.txt'])).toBe(true); | ||
| expect(isDangerousCommand(['rm', '-rf', '/'])).toBe(true); | ||
| expect(isDangerousCommand(['rm', '-f', 'file'])).toBe(true); | ||
| expect(isDangerousCommand(['rm', '-r', 'dir'])).toBe(true); | ||
| expect(isDangerousCommand(['/bin/rm', 'file'])).toBe(true); | ||
| }); | ||
|
|
||
| it('should flag rm help/version as dangerous (strict)', () => { | ||
| expect(isDangerousCommand(['rm', '--help'])).toBe(true); | ||
| expect(isDangerousCommand(['rm', '--version'])).toBe(true); | ||
| }); | ||
|
|
||
| it('should identify sudo as dangerous if command is dangerous', () => { | ||
| expect(isDangerousCommand(['sudo', 'rm', 'file'])).toBe(true); | ||
| }); | ||
|
|
||
| it('should identify find -exec as dangerous', () => { | ||
| expect(isDangerousCommand(['find', '.', '-exec', 'rm', '{}', '+'])).toBe( | ||
| true, | ||
| ); | ||
| }); | ||
|
|
||
| it('should identify dangerous rg flags', () => { | ||
| expect(isDangerousCommand(['rg', '--hostname-bin', 'something'])).toBe( | ||
| true, | ||
| ); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,6 +9,7 @@ import { | |||||||
| initializeShellParsers, | ||||||||
| splitCommands, | ||||||||
| stripShellWrapper, | ||||||||
| normalizeCommand, | ||||||||
| } from '../../utils/shell-utils.js'; | ||||||||
|
|
||||||||
| /** | ||||||||
|
|
@@ -119,10 +120,7 @@ export function isKnownSafeCommand(args: string[]): boolean { | |||||||
| */ | ||||||||
| export function isDangerousCommand(args: string[]): boolean { | ||||||||
| if (!args || args.length === 0) return false; | ||||||||
| let cmd = args[0].toLowerCase(); | ||||||||
| if (cmd.endsWith('.exe')) { | ||||||||
| cmd = cmd.slice(0, -4); | ||||||||
| } | ||||||||
| const cmd = normalizeCommand(args[0]); | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Suggested change
References
|
||||||||
|
|
||||||||
| const dangerous = new Set([ | ||||||||
| 'del', | ||||||||
|
|
||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
isDangerousCommandfunction is vulnerable to bypass whensudois used with flags (e.g.,sudo -u root rm). The recursive callisDangerousCommand(args.slice(1))in thesudoblock (lines 438-440) incorrectly processes the arguments, causingnormalizeCommandto check a flag like-uinstead 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