diff --git a/crates/bashkit/docs/threat-model.md b/crates/bashkit/docs/threat-model.md index fe85da51b..c7a17f912 100644 --- a/crates/bashkit/docs/threat-model.md +++ b/crates/bashkit/docs/threat-model.md @@ -82,7 +82,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-134) | `r='a[0]:-${!r}]'` re-entering `${!...}` through an unset-element default | Bracket targets truncate at the first `]` (bash parity: `a[0]` resolves, rest ignored), so trailing operands never reach the indirect re-parse | 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 | **Stack Overflow / Recursion:** diff --git a/crates/bashkit/src/interpreter/expansion.rs b/crates/bashkit/src/interpreter/expansion.rs index 42e683879..bb01309f4 100644 --- a/crates/bashkit/src/interpreter/expansion.rs +++ b/crates/bashkit/src/interpreter/expansion.rs @@ -2637,6 +2637,40 @@ 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 @@ -2673,33 +2707,36 @@ impl Interpreter { .is_some_and(|f| f.is_ascii_alphabetic() || f == '_') && c.all(|ch| ch.is_ascii_alphanumeric() || ch == '_') }; - // TM-RES-INDIRECT: an indirect array target ends at the first `]` - // after `[` (real bash resolves `a[0]` from `a[0]:-x]` and ignores - // the rest). Truncating before validation keeps trailing - // operator/nesting text — including a nested `${!...}` — from ever - // reaching the desugar re-parse below, which re-entered indirect - // expansion without bound (`r='a[0]:-${!r}]'` overflowed the stack). - // Targets without a sub-`]` (plain names, `a[$i]`, `a[@]`) pass - // through unchanged. - let value = match value.find('[') { - Some(open) => match value[open..].find(']') { - Some(rel) => value[..open + rel + 1].to_string(), - None => value, - }, - None => value, + // TM-RES-INDIRECT: a bracket indirect target is only valid when + // the first complete `[...]` reference consumes the entire value. + // Modern bash (5.x) rejects trailing operator/nesting text with + // `invalid variable name` (older 3.2 ignored it); rejection also + // keeps a nested `${!...}` from ever reaching the desugar re-parse + // below, which re-entered indirect expansion without bound + // (`r='a[0]:-${!r}]'` overflowed the stack). The boundary tracks + // nested brackets and quoted text, so valid nested subscripts + // (`a[b[0]]`) resolve whole, while `a[0]:-x]`, `a[0][1]`, and + // 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()) + } + None => true, }; - let valid = ident(&value) - || (!value.is_empty() && value.bytes().all(|b| b.is_ascii_digit())) - || matches!(value.as_str(), "@" | "*" | "#" | "?" | "$" | "!" | "-") - || value.find('[').is_some_and(|open| { - ident(&value[..open]) - && value.ends_with(']') - && value.len() > open + 2 - && !value.contains("$(") - && !value.contains('`') - && !value.contains("<(") - && !value.contains(">(") - }); + let valid = valid_boundary + && (ident(&value) + || (!value.is_empty() && value.bytes().all(|b| b.is_ascii_digit())) + || matches!(value.as_str(), "@" | "*" | "#" | "?" | "$" | "!" | "-") + || value.find('[').is_some_and(|open| { + ident(&value[..open]) + && value.ends_with(']') + && value.len() > open + 2 + && !value.contains("$(") + && !value.contains('`') + && !value.contains("<(") + && !value.contains(">(") + })); if !valid { // THREAT[TM-DOS-130]: echo a bounded prefix of the bad target. let shown: String = value.chars().take(256).collect(); diff --git a/crates/bashkit/tests/integration/threat_model_tests.rs b/crates/bashkit/tests/integration/threat_model_tests.rs index 35c9a2245..9cf3d67f7 100644 --- a/crates/bashkit/tests/integration/threat_model_tests.rs +++ b/crates/bashkit/tests/integration/threat_model_tests.rs @@ -10,8 +10,9 @@ use bashkit::{ OverlayFs, SessionLimits, TraceEventDetails, TraceEventKind, TraceMode, }; use std::path::Path; +use std::process::{Command, Stdio}; use std::sync::Arc; -use std::time::Duration; +use std::time::{Duration, Instant}; // ============================================================================= // 1. RESOURCE EXHAUSTION TESTS @@ -20,14 +21,105 @@ use std::time::Duration; mod resource_exhaustion { use super::*; - /// TM-RES-INDIRECT: an indirect array target ends at the first `]`. - /// Trailing operator/nesting text is never re-expanded as code, so a - /// default operand cannot re-enter indirect expansion and exhaust the - /// stack (`r='a[0]:-${!r}]'`). Matches real bash: element-or-empty. + /// TM-DOS-135: child-process runner for hostile scripts. + /// Hostile reproducers never run in the test process: pre-fix they + /// overflowed the stack and aborted the whole runner. The CLI binary + /// sits next to the test binary's `deps` directory. + fn bashkit_cli_path() -> std::path::PathBuf { + let exe = std::env::current_exe().expect("current test executable"); + let deps = exe.parent().expect("deps dir"); + let profile = deps.parent().expect("profile dir"); + profile.join(format!("bashkit{}", std::env::consts::EXE_SUFFIX)) + } + + struct BoundedRun { + timed_out: bool, + exit_code: Option, + stdout: String, + stderr: String, + } + + /// Runs a script in a child CLI process under a wall-clock watchdog. + /// Darwin note: Rust std exposes no portable per-child CPU, stack, or + /// memory caps, so the watchdog kill plus the 8 KiB output cap are the + /// enforced bounds here. Records the real producer exit status. + fn run_cli_bounded(script: &str, limit: Duration) -> BoundedRun { + let mut child = Command::new(bashkit_cli_path()) + .args(["-c", script]) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("spawn bashkit CLI"); + let start = Instant::now(); + let timed_out = loop { + match child.try_wait().expect("poll child") { + Some(_) => break false, + None => { + if start.elapsed() >= limit { + let _ = child.kill(); + break true; + } + std::thread::sleep(Duration::from_millis(10)); + } + } + }; + let output = child.wait_with_output().expect("collect child output"); + const CAP: usize = 8192; + let mut stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + let mut stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + stdout.truncate(CAP); + stderr.truncate(CAP); + BoundedRun { + timed_out, + exit_code: output.status.code(), + stdout, + stderr, + } + } + + /// TM-DOS-135: hostile indirect cycles are refused in a bounded child. + /// Exact finding reproducer plus the mutual variant. Both run in a + /// child CLI under a wall-clock watchdog (never in the test runner: + /// pre-fix they overflowed the stack and aborted the process) and are + /// refused with `invalid variable name`, like modern bash (5.x); the + /// real producer exit status is asserted, never relabeled. + #[test] + fn indirect_hostile_cycles_rejected_in_bounded_child() { + for script in [ + "unset a\nr='a[0]:-${!r}]'\necho \"${!r}\"", + "unset a\nr='a[0]:-${!s}]'\ns='a[0]:-${!r}]'\necho \"${!r}\"", + ] { + let run = run_cli_bounded(script, Duration::from_secs(20)); + assert!(!run.timed_out, "hostile script hung: {script}"); + assert_eq!( + run.exit_code, + Some(1), + "script: {script}, stdout: {}, stderr: {}", + run.stdout, + run.stderr + ); + assert!(run.stdout.is_empty(), "stdout: {}", run.stdout); + assert!( + run.stderr.contains("invalid variable name"), + "stderr: {}", + run.stderr + ); + assert!(run.stderr.len() < 1024, "stderr: {}", run.stderr); + } + } + + /// TM-DOS-135: a bracket indirect target is refused unless the first + /// complete `[...]` reference consumes the whole value. Modern bash + /// (5.x) rejects trailing operators with `invalid variable name`; + /// refusal also keeps a nested `${!...}` from ever reaching the desugar + /// re-parse, which re-entered indirect expansion without bound. + /// Valid nested subscripts resolve whole. #[tokio::test] - async fn indirect_array_target_ends_at_first_close_bracket() { - // Trailing operator/nesting text is ignored, like real bash - // (verified: bash prints `out=[]`, `st=0` for every target below). + async fn indirect_array_boundary_rejects_trailing_operators() { + // Trailing operator/nesting text is refused, like bash 5.x. Older + // bash 3.2 ignored the tail, so no real-bash oracle is asserted + // for these shapes; the rejection itself is the contract. for target in [ "a[0]:-injected]", "a[0]-injected]", @@ -36,64 +128,84 @@ mod resource_exhaustion { "a[0]/x/injected]", "a[0][1]", "a[0]}${x:-injected}[0]", + "a[b[0]]:-D", ] { let mut bash = Bash::new(); let result = bash - .exec(&format!( - "r='{target}'\necho \"out=[${{!r}}]\"\necho \"st=$?\"" - )) + .exec(&format!("a=(1 2)\nb=(0)\nr='{target}'\necho \"${{!r}}\"")) .await .unwrap(); - assert_eq!(result.stdout, "out=[]\nst=0\n", "target: {target}"); + assert_ne!(result.exit_code, 0, "target accepted: {target}"); + assert_eq!(result.exit_code, 1, "target: {target}"); + assert!(result.stdout.is_empty(), "target: {target}"); assert!( - result.stderr.is_empty(), + result.stderr.contains("invalid variable name"), "target: {target}: {}", result.stderr ); + assert!(result.stderr.len() < 1024, "target: {target}"); } - // Set element resolves through the truncated target (bash: `1`). - let mut bash = Bash::new(); - let result = bash - .exec("a=(1 2)\nr='a[0]:-injected]'\necho \"out=[${!r}]\"\necho \"st=$?\"") - .await - .unwrap(); - assert_eq!(result.stdout, "out=[1]\nst=0\n"); - assert!(result.stderr.is_empty(), "stderr: {}", result.stderr); - } - /// TM-RES-INDIRECT: self-referential default operand terminates. - /// Pre-fix this exact reproducer overflowed the stack (SIGABRT). - #[tokio::test] - async fn indirect_self_cycle_default_operand_terminates() { - // Exact finding reproducer: unset element + self-cycle in default. - // Real bash prints `out=[]` with status 0 and never crashes. + // Nested subscripts resolve whole (bash: `two`/`two`). let mut bash = Bash::new(); let result = bash - .exec("unset a\nr='a[0]:-${!r}]'\necho \"out=[${!r}]\"\necho \"st=$?\"") + .exec("a=(zero one two three)\nb=(2)\nr='a[b[0]]'\nprintf \"indirect=[%s]\\n\" \"${!r}\"\nprintf \"direct=[%s]\\n\" \"${a[b[0]]}\"") .await .unwrap(); - assert_eq!(result.stdout, "out=[]\nst=0\n"); + assert_eq!(result.stdout, "indirect=[two]\ndirect=[two]\n"); assert!(result.stderr.is_empty(), "stderr: {}", result.stderr); - // Set element: the cycle operand is discarded, element wins. + // Associative references agree between direct and indirect use. + // Portable note: indexed cases above also run against real bash + // below; assoc arrays need bash 4+, so agreement is asserted here. let mut bash = Bash::new(); let result = bash - .exec("a=(1 2)\nr='a[0]:-${!r}]'\necho \"out=[${!r}]\"\necho \"st=$?\"") + .exec("declare -A h=([k]=V [key]=W)\nkey=k\nr='h[$key]'\necho \"dollar=[${!r}] direct=[${h[$key]}]\"\nr='h[key]'\necho \"plain=[${!r}] direct=[${h[key]}]\"\nr='h[@]'\necho \"at=[${!r}] direct=[${h[@]}]\"") .await .unwrap(); - assert_eq!(result.stdout, "out=[1]\nst=0\n"); - assert!(result.stderr.is_empty(), "stderr: {}", result.stderr); - // Mutual cycle through two names terminates as well. - let mut bash = Bash::new(); - let result = bash - .exec( - "unset a\nr='a[0]:-${!s}]'\ns='a[0]:-${!r}]'\necho \"out=[${!r}]\"\necho \"st=$?\"", - ) - .await - .unwrap(); - assert_eq!(result.stdout, "out=[]\nst=0\n"); + assert_eq!( + result.stdout, + "dollar=[V] direct=[V]\nplain=[W] direct=[W]\nat=[V W] direct=[V W]\n" + ); assert!(result.stderr.is_empty(), "stderr: {}", result.stderr); } + // Out of scope: `]` inside an assoc key is unsupported by the + // direct subscript parser as well (`declare` accepts it, reads + // misparse), so no indirect agreement is asserted for that shape. + // The boundary scan still tracks quotes so a complete reference + // stays whole if the parser ever learns quoted brackets. + + /// TM-DOS-135: finite nested indexed indirects match real bash + /// byte-for-byte. Indexed arrays work on bash 3.2+, so this runs on + /// macOS and Linux CI alike. + #[tokio::test] + async fn indirect_nested_indexed_matches_real_bash() { + for script in [ + "a=(zero one two three); b=(2); r='a[b[0]]'; printf 'indirect=[%s]\n' \"${!r}\"; printf 'direct=[%s]\n' \"${a[b[0]]}\"", + "a=(1 2); r='a[0]'; echo \"out=[${!r}]\"", + "a=(x y); r='a[@]'; echo \"out=[${!r}]\"", + "a=(x y); i=1; r='a[$i]'; echo \"out=[${!r}]\"", + ] { + let expected = Command::new("bash") + .args(["--noprofile", "--norc", "-c", script]) + .output() + .expect("run real bash"); + assert!( + expected.status.success(), + "real bash failed: {script}: {}", + String::from_utf8_lossy(&expected.stderr) + ); + let mut harness = Bash::new(); + let result = harness.exec(script).await.unwrap(); + assert_eq!( + result.stdout, + String::from_utf8_lossy(&expected.stdout).into_owned(), + "script: {script}" + ); + assert!(result.stderr.is_empty(), "stderr: {}", result.stderr); + } + } + /// V1: Test that command limit prevents infinite execution #[tokio::test] async fn threat_infinite_commands_blocked() { diff --git a/knowledge/security/threat-model.md b/knowledge/security/threat-model.md index 856e8ccc9..035d9bcaf 100644 --- a/knowledge/security/threat-model.md +++ b/knowledge/security/threat-model.md @@ -302,7 +302,7 @@ runaway scripts without permanently breaking the session. | TM-DOS-132 | Completion spec and key binding growth | `complete -W "$big" nameN` or `bind -x '"\C-xN": cmd'` in a loop keeps per-shell state growing | At most 1024 completion specs of at most 64 KiB option text each, and at most 1024 `bind` changes of at most 4 KiB each, per interpreter; past a cap the builtin fails with status 1. The state is built on first use, lives in the interpreter (never global, so tenants never share it) and is copied into subshells like other shell state. Regression: `completion_specs_are_bounded`, `binding_changes_are_capped` | **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_yaml_ng 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-134 | 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) | The resolved bracket target is truncated at the first `]` after `[` before validation (real bash resolves `a[0]` and ignores the rest), so trailing operands — including a nested `${!...}` — never reach the desugar re-parse; `$(`/backquote/process-substitution guards unchanged. Regressions: `indirect_array_target_ends_at_first_close_bracket`, `indirect_self_cycle_default_operand_terminates` | **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-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-051** is historical. The custom parser was deleted with the `yaml` helper; the replacement `yq` parser/evaluator boundary is tracked by TM-DOS-101.