From b8a542b31085630fdac7f29ad50ec829b94d3c51 Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Wed, 14 Oct 2026 19:39:04 -0500 Subject: [PATCH] fix(parser): unify indirect subscript consumption --- crates/bashkit/docs/threat-model.md | 2 +- crates/bashkit/src/interpreter/expansion.rs | 45 +--- crates/bashkit/src/parser/mod.rs | 223 ++++++++++++++---- .../integration/indirect_reference_tests.rs | 69 ++++++ crates/bashkit/tests/integration/main.rs | 1 + knowledge/foundations/parser.md | 10 + knowledge/security/threat-model.md | 2 +- 7 files changed, 262 insertions(+), 90 deletions(-) create mode 100644 crates/bashkit/tests/integration/indirect_reference_tests.rs diff --git a/crates/bashkit/docs/threat-model.md b/crates/bashkit/docs/threat-model.md index 2388773f2..aa9bad741 100644 --- a/crates/bashkit/docs/threat-model.md +++ b/crates/bashkit/docs/threat-model.md @@ -83,7 +83,7 @@ through configurable limits. | History files (TM-DOS-131) | `history -a` loops, huge `$HISTSIZE`, giant `$HISTFILE` | History list bounded by `max_history_entries`/`max_history_bytes`; `$HISTFILE` reads refused past `max_input_bytes`; VFS only | MITIGATED | | Completion specs and bindings (TM-DOS-132) | `complete`/`bind` in a loop | 1024 specs (64 KiB each) and 1024 binding changes (4 KiB each) per interpreter; never shared between instances | MITIGATED | | Prompt recursion (TM-DOS-133) | `x='${x@P}'; echo "${x@P}"`, mutual cycles or substitution-mediated cycles | Cumulative prompt depth capped at `min(max_ast_depth, 8)`; shared work, aggregate input, cancellation and deadline checks on each prompt; descendant shells inherit depth | MITIGATED | -| Indirect operand recursion (TM-DOS-135) | `r='a[0]:-${!r}]'` re-entering `${!...}` through an unset-element default | A bracket target is refused unless the first complete `[...]` reference consumes the whole value (bash 5.x parity; nested subscripts resolve whole) | MITIGATED | +| Indirect operand recursion (TM-DOS-135) | Indirect target text reinterpreted as an operator/default operand | Admission and parameter parsing share an exact-reference scanner for nested brackets, braces, quotes, and escapes; indirect reparses inherit request work/cancellation and enclosing substitution depth | MITIGATED | **Stack Overflow / Recursion:** diff --git a/crates/bashkit/src/interpreter/expansion.rs b/crates/bashkit/src/interpreter/expansion.rs index 23fdb1208..6276d1388 100644 --- a/crates/bashkit/src/interpreter/expansion.rs +++ b/crates/bashkit/src/interpreter/expansion.rs @@ -2749,40 +2749,6 @@ impl Interpreter { }) } - // End index (exclusive) of the first complete subscript reference that - // opens at `open`: nested brackets nest, quoted or escaped text is - // skipped. `None` when the brackets never balance. All boundary bytes - // are ASCII, so the index is always a char boundary. - fn indirect_array_end(value: &str, open: usize) -> Option { - let bytes = value.as_bytes(); - let mut depth = 0usize; - let mut quote: Option = None; - let mut escaped = false; - let mut i = open; - while i < bytes.len() { - let b = bytes[i]; - // 0x27 is a single quote, 0x22 a double quote, 0x5c a backslash. - if escaped { - escaped = false; - } else if quote.is_some_and(|q| b == q) { - quote = None; - } else if quote.is_none() && (b == 0x27 || b == 0x22) { - quote = Some(b); - } else if quote.is_none() && b == 0x5c { - escaped = true; - } else if quote.is_none() && b == b'[' { - depth += 1; - } else if quote.is_none() && b == b']' && depth > 0 { - depth -= 1; - if depth == 0 { - return Some(i + 1); - } - } - i += 1; - } - None - } - /// The parameter `${!name...}` refers to: the value of `name` (an /// element when it carries a subscript). bash rejects an unset `name` /// ("invalid indirect expansion") and a value that is not a parameter @@ -2831,9 +2797,8 @@ impl Interpreter { // unbalanced shapes are refused. Plain names, `a[$i]`, and `a[@]` // have no trailing text and pass through unchanged. let valid_boundary = match value.find('[') { - Some(open) => { - Self::indirect_array_end(&value, open).is_some_and(|end| end == value.len()) - } + Some(open) => crate::parser::parameter_subscript_end(&value, open) + .is_some_and(|end| end == value.len()), None => true, }; let valid = valid_boundary @@ -2914,11 +2879,13 @@ impl Interpreter { suffix }; let text = format!("${{{target}{suffix}}}"); - let parsed = Parser::parse_word_string_with_limits( + let parsed = Parser::parse_indirect_word( &text, self.limits.max_ast_depth, self.limits.max_parser_operations, - ); + self.counters.subst_depth, + self.execution_budget.clone(), + )?; let [new_part] = <[WordPart; 1]>::try_from(parsed.parts).map_err(|_| { crate::error::Error::LineAbort(self.diag(format!("{text}: bad substitution\n"))) })?; diff --git a/crates/bashkit/src/parser/mod.rs b/crates/bashkit/src/parser/mod.rs index bb44d0e52..cb46a69af 100644 --- a/crates/bashkit/src/parser/mod.rs +++ b/crates/bashkit/src/parser/mod.rs @@ -290,6 +290,26 @@ impl<'a> Parser<'a> { parser.parse_word(input.to_string()) } + /// Indirect reparses share request work/cancellation and enclosing depth. + pub(crate) fn parse_indirect_word( + input: &str, + max_depth: usize, + max_fuel: usize, + current_depth: usize, + budget: crate::limits::ExecutionBudget, + ) -> Result { + budget.consume_work(input.len() as u64)?; + let mut parser = + Parser::with_limits(input, max_depth, max_fuel).with_execution_budget(budget.clone()); + parser.current_depth = current_depth; + let word = parser.parse_word(input.to_string()); + budget.check()?; + if let Some(error) = parser.deferred_error.take() { + return Err(error); + } + Ok(word) + } + /// Parse `input` as an unquoted here-document body: expansions apply, /// `"` is literal, and only `\$`, `` \` `` and `\\` are escapes. Used for /// prompt strings (`${x@P}`, PS1), which bash expands the same way. @@ -4270,35 +4290,7 @@ impl<'a> Parser<'a> { if chars.peek() == Some(&'[') { chars.next(); // consume '[' let mut index = String::new(); - // Track nesting so nested ${...} containing - // brackets (e.g. ${#arr[@]}) don't prematurely - // close the subscript. - let mut bracket_depth: i32 = 0; - let mut brace_depth: i32 = 0; - while let Some(&c) = chars.peek() { - if c == ']' && bracket_depth == 0 && brace_depth == 0 { - chars.next(); - break; - } - match c { - '[' => bracket_depth += 1, - ']' => bracket_depth -= 1, - '$' => { - index.push(chars.next().unwrap()); - if chars.peek() == Some(&'{') { - brace_depth += 1; - index.push(chars.next().unwrap()); - continue; - } - continue; - } - '{' => brace_depth += 1, - '}' if brace_depth > 0 => brace_depth -= 1, - '}' => {} - _ => {} - } - index.push(chars.next().unwrap()); - } + scan_parameter_subscript(chars, |c| index.push(c)); // Strip surrounding quotes from index (e.g. "foo" -> foo) if index.len() >= 2 && ((index.starts_with('"') && index.ends_with('"')) @@ -5135,6 +5127,47 @@ fn empty_background(span: Span) -> (ListOperator, Command) { ) } +/// Consume a subscript after `[`, preserving its source text in `emit`. +/// THREAT[TM-DOS-135]: admission and parameter parsing must agree on its end. +fn scan_parameter_subscript( + chars: &mut impl Iterator, + mut emit: impl FnMut(char), +) -> Option { + let mut brackets = 0usize; + let mut braces = 0usize; + let mut quote = None; + let mut escaped = false; + let mut consumed = 0usize; + for c in chars.by_ref() { + consumed += c.len_utf8(); + if escaped { + escaped = false; + } else { + match (quote, c) { + (Some('\''), '\'') => quote = None, + (Some('\''), _) => {} + (_, '\\' | '\x00') => escaped = true, + (Some('"'), '"') => quote = None, + (Some(_), _) => {} + (None, '\'' | '"') => quote = Some(c), + (None, '{') => braces += 1, + (None, '}') => braces = braces.saturating_sub(1), + (None, '[') => brackets += 1, + (None, ']') if brackets == 0 && braces == 0 => return Some(consumed), + (None, ']') => brackets = brackets.saturating_sub(1), + _ => {} + } + } + emit(c); + } + None +} + +pub(crate) fn parameter_subscript_end(value: &str, open: usize) -> Option { + let rest = value.get(open..)?.strip_prefix('[')?; + scan_parameter_subscript(&mut rest.chars(), |_| {}).map(|used| open + 1 + used) +} + /// Whether a raw `=~` operand holds an expansion. A `$` that cannot start /// one (`^a$`, `(x$|y$)`) is the regex end anchor; single quotes hide both. /// Text inside `${...}` when bash would reject it with "bad substitution" @@ -5171,26 +5204,8 @@ fn bad_brace_parameter(chars: &std::iter::Peekable>) -> Opti it.next(); } if it.peek() == Some(&'[') { - // Subscript: balanced brackets; unterminated ones are bad. - let mut depth = 0usize; - let mut closed = false; - for c in it.by_ref() { - match c { - _ if MARKERS.contains(&c) => return None, - '[' => depth += 1, - ']' => { - depth -= 1; - if depth == 0 { - closed = true; - break; - } - } - _ => {} - } - } - if !closed { - return None; - } + it.next(); + scan_parameter_subscript(&mut it, |_| {})?; } next_is_bad(it.peek().copied(), length_prefix) } @@ -5727,6 +5742,116 @@ mod tests { use super::*; use std::time::Duration; + #[test] + fn parameter_subscript_consumption_preserves_escaped_and_quoted_brackets() { + for (source, expected) in [ + (r"${items[left\]right]}", r"left\]right"), + (r#"${items["left]right"]}"#, "left]right"), + (r"${items['left]right']}", "left]right"), + (r"${items[nested[0]]}", "nested[0]"), + (r"${items[${key}]}", "${key}"), + ] { + let parsed = Parser::parse_word_string(source); + assert!( + matches!( + parsed.parts.as_slice(), + [WordPart::ArrayAccess { name, index }] + if name == "items" && index == expected + ), + "unexpected parts for {source}: {:?}", + parsed.parts + ); + } + } + + #[test] + fn parameter_subscript_consumption_keeps_explicit_operator_after_reference() { + let parsed = Parser::parse_word_string(r"${items[left\]right]:-fallback}"); + assert!(matches!( + parsed.parts.as_slice(), + [WordPart::ParameterExpansion { + name, + operator: ParameterOp::UseDefault, + operand, + colon_variant: true, + }] if name == r"items[left\]right]" && operand == "fallback" + )); + } + + #[test] + fn parameter_subscript_consumption_admits_exact_reference_only() { + for value in [ + r"items[left\]right]", + r#"items["left]right"]"#, + r#"items["left\"]right"]"#, + "items[nested[0]]", + "items[${key}]", + "items[caf\u{e9}]", + ] { + assert_eq!( + parameter_subscript_end(value, 5), + Some(value.len()), + "{value}" + ); + } + for value in ["items[0]:-fallback", "items[0][1]"] { + assert_ne!( + parameter_subscript_end(value, 5), + Some(value.len()), + "{value}" + ); + } + for value in [r"items[left\]", "items['left]", "items[${key]"] { + assert_eq!(parameter_subscript_end(value, 5), None, "{value}"); + } + } + + #[test] + fn indirect_reparse_admission_inherits_depth_and_request_failures() { + use crate::limits::{ExecutionBudget, ExecutionLimits}; + use std::sync::atomic::AtomicBool; + + let budget = || { + ExecutionBudget::new( + &ExecutionLimits::default(), + Arc::new(AtomicBool::new(false)), + ) + }; + let input = "$(printf ok)"; + assert!(Parser::parse_indirect_word(input, 1, 100, 0, budget()).is_ok()); + let error = Parser::parse_indirect_word(input, 1, 100, 1, budget()).unwrap_err(); + assert!( + error.to_string().contains("AST nesting too deep"), + "{error}" + ); + + let empty_work = ExecutionBudget::new( + &ExecutionLimits::default().max_work_units(0), + Arc::new(AtomicBool::new(false)), + ); + assert!(matches!( + Parser::parse_indirect_word("${items[0]}", 1, 100, 0, empty_work), + Err(Error::ResourceLimit(_)) + )); + let cancelled = + ExecutionBudget::new(&ExecutionLimits::default(), Arc::new(AtomicBool::new(true))); + assert!(matches!( + Parser::parse_indirect_word("${items[0]}", 1, 100, 0, cancelled), + Err(Error::Cancelled) + )); + + let text = "${items[0]}"; + let shared = ExecutionBudget::new( + &ExecutionLimits::default().max_work_units(text.len() as u64), + Arc::new(AtomicBool::new(false)), + ); + assert!(Parser::parse_indirect_word(text, 1, 100, 0, shared.clone()).is_ok()); + assert!(matches!( + Parser::parse_indirect_word(text, 1, 100, 0, shared), + Err(Error::ResourceLimit(_)) + )); + } + #[test] fn test_parse_simple_command() { let parser = Parser::new("echo hello"); diff --git a/crates/bashkit/tests/integration/indirect_reference_tests.rs b/crates/bashkit/tests/integration/indirect_reference_tests.rs new file mode 100644 index 000000000..368243919 --- /dev/null +++ b/crates/bashkit/tests/integration/indirect_reference_tests.rs @@ -0,0 +1,69 @@ +//! Finite, ordinary indirect-reference compatibility fixtures. + +use bashkit::Bash; +use std::process::Command; + +#[tokio::test] +async fn indirect_reference_nested_index_matches_bash() { + let script = r#"a=(zero one); b=(1); r='a[b[0]]'; printf '%s/%s\n' "${!r}" "${a[b[0]]}""#; + let result = Bash::new().exec(script).await.unwrap(); + let oracle = Command::new("bash") + .env_clear() + .args(["--noprofile", "--norc", "-c", script]) + .output() + .unwrap(); + assert_eq!(result.stdout, "one/one\n"); + assert_eq!(result.stderr, ""); + assert_eq!(result.exit_code, 0); + assert_eq!(result.stdout.as_bytes(), oracle.stdout); + assert_eq!(result.stderr.as_bytes(), oracle.stderr); + assert_eq!(Some(result.exit_code), oracle.status.code()); +} + +#[tokio::test] +async fn indirect_reference_quoted_and_escaped_bracket_keys() { + let script = r#" +declare -A items +items['left]right']=ok +quoted='items["left]right"]' +escaped='items[left\]right]' +printf '%s/%s/%s\n' "${!quoted}" "${!escaped}" "${items['left]right']}" +"#; + let result = Bash::new().exec(script).await.unwrap(); + assert_eq!(result.stdout, "ok/ok/ok\n"); + assert_eq!(result.stderr, ""); + assert_eq!(result.exit_code, 0); +} + +#[tokio::test] +async fn indirect_reference_explicit_default_and_slice_remain_valid() { + let script = r#"a=(zero one); r='a[1]'; printf '%s/%s\n' "${!r:-fallback}" "${!r:1}""#; + let result = Bash::new().exec(script).await.unwrap(); + let oracle = Command::new("bash") + .env_clear() + .args(["--noprofile", "--norc", "-c", script]) + .output() + .unwrap(); + assert_eq!(result.stdout, "one/ne\n"); + assert_eq!(result.stderr, ""); + assert_eq!(result.exit_code, 0); + assert_eq!(result.stdout.as_bytes(), oracle.stdout); + assert_eq!(result.stderr.as_bytes(), oracle.stderr); + assert_eq!(Some(result.exit_code), oracle.status.code()); +} + +#[tokio::test] +async fn indirect_reference_trailing_text_is_rejected_and_session_recovers() { + let mut bash = Bash::new(); + let result = bash + .exec(r#"r='items[0][1]'; printf '%s' "${!r}""#) + .await + .unwrap(); + assert_eq!(result.stdout, ""); + assert!(result.stderr.contains("invalid variable name")); + assert_eq!(result.exit_code, 1); + let recovered = bash.exec("printf ok").await.unwrap(); + assert_eq!(recovered.stdout, "ok"); + assert_eq!(recovered.stderr, ""); + assert_eq!(recovered.exit_code, 0); +} diff --git a/crates/bashkit/tests/integration/main.rs b/crates/bashkit/tests/integration/main.rs index a6a3f3f6a..ddb66bed3 100644 --- a/crates/bashkit/tests/integration/main.rs +++ b/crates/bashkit/tests/integration/main.rs @@ -98,6 +98,7 @@ pub mod headtail_resource_tests; pub mod history_tests; pub mod host_call_execution_tests; pub mod host_mounts_tests; +pub mod indirect_reference_tests; pub mod issue_1175_test; pub mod issue_1776_test; pub mod issue_1777_test; diff --git a/knowledge/foundations/parser.md b/knowledge/foundations/parser.md index b619abb74..61becb358 100644 --- a/knowledge/foundations/parser.md +++ b/knowledge/foundations/parser.md @@ -66,6 +66,16 @@ Bash/POSIX `-p` plus `--` and the practical GNU report flags `-f/--format`, ### Context-Aware Lexing +**Parameter subscript boundaries.** Parameter parsing, preliminary braced-name +validation, and indirect-target admission use one subscript scanner. Quotes, +ordinary and lexer-sentinel escapes, nested brackets, and braced expansions +cannot expose an internal closing bracket as an operator boundary. An indirect +target must consume exactly one complete reference before the caller's explicit +suffix is appended. Indirect reparses share request work/cancellation and inherit +the caller's command-substitution depth; deferred child-parser errors propagate. +Tiny parser consumption/configuration fixtures and `indirect_reference_tests` +cover these contracts without recursive scripts or exhaustion probes (TM-DOS-135). + Handles bash's context-sensitivity: - `$var` in double quotes: expand; in single quotes: literal - Word splitting after expansion diff --git a/knowledge/security/threat-model.md b/knowledge/security/threat-model.md index 604df625a..3182a07db 100644 --- a/knowledge/security/threat-model.md +++ b/knowledge/security/threat-model.md @@ -308,7 +308,7 @@ on default 2 MiB test threads (TM-DOS-044). | TM-DOS-136 | Syntax diagnostic amplification | A multiline unexpected token combined with a long attacker-controlled `bash -c` ARG0 repeats the shell name for every token line, allocating quadratically before stderr capture | Unexpected-token messages and complete syntax reports use a fixed-capacity UTF-8 writer capped at 1 KiB before any append, stop formatting on overflow, and include `[diagnostic truncated]`; short reports preserve Bash semantics. Regressions: `error::tests::syntax_report_*`, `parser_bounds_unexpected_token_before_copying`, and `partial_parse_tests::{child_syntax_report_is_bounded, small_child_syntax_reports_match_bash}` | **MITIGATED** | | TM-DOS-101 | yq structured-data amplification | YAML aliases can expand exponentially during deserialization; deep YAML/JSON, document floods, jaq generators, and YAML re-serialization can consume stack, CPU, or memory beyond the source size | Reject YAML alias tokens with a bounded lexical pass before deserialization; VFS/stdin and aggregate input budgets; serde_norway recursion cap 128 plus Bashkit depth 100; 4096-document cap; shared jaq work/deadline/output limits; post-serialization stdout cap; YAML+JSON depth/document/input/output regressions plus `yq_fuzz` and arbitrary-input proptest | **MITIGATED** | | TM-DOS-133 | Unbounded prompt expansion | `x='${x@P}'; echo "${x@P}"`, mutual variable cycles or cycles through command substitutions repeatedly create shallow parsers and recursively poll futures | Prompt reparses count cumulative runtime depth capped at `min(max_ast_depth, 8)` before recursive polling; descendants inherit depth. Each prompt charges the shared work and aggregate-input budget, checking cancellation and deadlines before decoding; child parsers share the request budget and fail closed on poison. Depth unwinds on returned errors and resets at host execution boundaries. Regressions: `prompt_resource_tests`, `prompt_checks_cancellation_before_decoding`, `prompt_checks_shared_deadline_before_decoding`, `prompt_error_unwinds_depth` | **MITIGATED** | -| TM-DOS-135 | Indirect array-target operand recursion | `r='a[0]:-${!r}]'; echo "${!r}"`: the bracket validator accepted trailing operator/nesting text, so desugar re-parsed `${a[0]:-${!r}]}` and the unset-element default re-entered indirect expansion without bound (stack overflow). Numbered to avoid collision with the adjacent tilde-budget entry | A bracket target is valid only when the first complete `[...]` reference (nesting- and quote-aware) consumes the whole value; any trailing text is refused with `invalid variable name`, like modern bash 5.x (older 3.2 ignored the tail). Refusal keeps a nested `${!...}` from ever reaching the desugar re-parse while valid nested subscripts (`a[b[0]]`) resolve whole; `$(`/backquote/process-substitution guards unchanged. Regressions: `indirect_array_boundary_rejects_trailing_operators`, `indirect_hostile_cycles_rejected_in_bounded_child`, `indirect_nested_indexed_matches_real_bash` | **MITIGATED** | +| TM-DOS-135 | Indirect array-target operand recursion | `r='a[0]:-${!r}]'; echo "${!r}"`: the bracket validator accepted trailing operator/nesting text, so desugar re-parsed `${a[0]:-${!r}]}` and the unset-element default re-entered indirect expansion without bound (stack overflow). Numbered to avoid collision with the adjacent tilde-budget entry | A bracket target is valid only when the first complete `[...]` reference consumes the whole value; admission, preliminary braced-name validation, and parameter parsing share one nesting/brace/quote/escape-aware scanner, so an internal escaped bracket cannot become an operator boundary. Trailing text is refused with `invalid variable name`, like modern bash 5.x (older 3.2 ignored the tail); valid nested subscripts resolve whole and substitution guards stay unchanged. Indirect reparses share request work/cancellation and enclosing command-substitution depth, propagating child-parser errors. Ordinary regressions: `parameter_subscript_consumption_*`, `indirect_reparse_admission_inherits_depth_and_request_failures`, `indirect_reference_tests`. Prior regressions remain: `indirect_array_boundary_rejects_trailing_operators`, `indirect_hostile_cycles_rejected_in_bounded_child`, `indirect_nested_indexed_matches_real_bash` | **MITIGATED** | | TM-DOS-137 | pr pagination amplification | A long `-h` title repeated once per empty line with `-l 11` grows gigabytes before a late output check; eager `-D` date copies amplify across retained inputs | Each row/output append checks the 16 MiB cap before growth; header parts append directly without a temporary title copy. Inputs retain date timestamps, formatting dates one nonempty stream at a time. Input lines, cells, rows and output lease capacity before allocation; cells borrow input text and pages borrow row slices. Input/row/page loops charge shared work, input bytes charge aggregate input, and budgeted growth polls cancellation/deadlines. Regressions: `pr_resource_tests`, cap/UTF-8/cancellation unit tests and `pr_allocation_tests` peak-heap bound | **MITIGATED** | | TM-DOS-107 | Static-analysis command validation amplification | A large comment before thousands of commands makes a whole-script ordered-subsequence scan per command | Build one source-character position index, then validate each analyzed command-name character with a logarithmic position lookup instead of rescanning source | **MITIGATED** | | TM-DOS-138 | Join matching-row output growth | Equal-key row groups multiply generated output and synchronous work before dispatcher capture | Parsed rows, fields, keys, decoded input and generated output own shared-budget capacity before growth. VFS reads charge aggregate input and admit metadata size; output fields append directly without temporary row storage, and the byte-stream copy is admitted. Parsing, grouping (including suppressed matches), comparisons and emissions charge work and poll cancellation/deadlines. Errors abort the request. Ordinary two-row fixtures cover memory/work rejection, cancellation/deadlines, session recovery, and complete pipeline/redirect output | **MITIGATED** |