-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Handle zsh process substitution correctly #7658
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
Conversation
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.
Thank you for your contribution! This PR adds important security protection against zsh process substitution. The implementation is clean and follows existing patterns well. I've left a few minor suggestions inline for your consideration.
|
|
||
| // Check for zsh process substitution =(...) which executes commands | ||
| // =(...) creates a temporary file containing the output of the command, but executes it | ||
| const zshProcessSubstitution = /=\([^)]+\)/.test(source) |
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 handling edge cases in the regex pattern. The current pattern /=\([^)]+\)/ requires at least one character inside the parentheses. While =() with empty parentheses is invalid zsh syntax, you might want to consider if the pattern should also catch this for completeness:
| const zshProcessSubstitution = /=\([^)]+\)/.test(source) | |
| const zshProcessSubstitution = /=\([^)]*\)/.test(source) |
This would use * instead of + to match zero or more characters.
| expect(containsDangerousSubstitution('result="${cmd=\\x60pwd\\x60}${cmd@P}"')).toBe(true) | ||
|
|
||
| // The new zsh process substitution exploit | ||
| expect(containsDangerousSubstitution("ls =(open -a Calculator)")).toBe(true) |
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.
Good comprehensive test coverage! Is it intentional that this test case duplicates the one at line 351? If so, perhaps add a comment noting that line 351 specifically tests the reported exploit, while this section tests various forms more generally.
| parameterAssignmentWithEscapes || | ||
| indirectExpansion || | ||
| hereStringWithSubstitution || | ||
| zshProcessSubstitution |
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 pattern will match =() anywhere in the string, including potentially in contexts where it might not be process substitution (e.g., within strings or comments). However, this approach is consistent with how other dangerous patterns are detected in this module, appropriately favoring security over precision. Just noting this for awareness.
Thanks @MaccariTA for reporting this issue.
Important
Add detection for zsh process substitution in command validation to flag as dangerous.
containsDangerousSubstitutionincommand-validation.tsnow detects zsh process substitution=()as dangerous.command-validation.spec.tsto verify detection of zsh process substitution patterns likels =(open -a Calculator).This description was created by
for e05098e. You can customize this summary. It will automatically update as commits are pushed.