diff --git a/crates/bashkit/src/builtins/date.rs b/crates/bashkit/src/builtins/date.rs index c9c34851..8a9b0751 100644 --- a/crates/bashkit/src/builtins/date.rs +++ b/crates/bashkit/src/builtins/date.rs @@ -151,7 +151,9 @@ fn parse_base_date(s: &str, now: DateTime) -> std::result::Result) -> std::result::Result Option { regex::Regex::new(r"^(\d+)\s+(second|minute|hour|day|week|month|year)s?\s+ago$").ok()?; if let Some(caps) = re_ago.captures(s) { let n: i64 = caps[1].parse().ok()?; - return Some(unit_duration(&caps[2], -n)); + return unit_duration(&caps[2], n.checked_neg()?); } // "+N unit(s)" or "-N unit(s)" or "N unit(s)" @@ -267,32 +276,35 @@ fn parse_relative_date(s: &str) -> Option { if let Some(caps) = re_rel.captures(s) { let sign = if &caps[1] == "-" { -1i64 } else { 1i64 }; let n: i64 = caps[2].parse().ok()?; - return Some(unit_duration(&caps[3], sign * n)); + return unit_duration(&caps[3], sign.checked_mul(n)?); } // "next unit" / "last unit" if let Some(unit) = s.strip_prefix("next ") { let unit = unit.trim().trim_end_matches('s'); - return Some(unit_duration(unit, 1)); + return unit_duration(unit, 1); } if let Some(unit) = s.strip_prefix("last ") { let unit = unit.trim().trim_end_matches('s'); - return Some(unit_duration(unit, -1)); + return unit_duration(unit, -1); } None } -fn unit_duration(unit: &str, n: i64) -> Duration { +// Returns None when the requested offset overflows i64 (the multiply) or the +// chrono Duration range, so callers can report "date out of range" instead of +// panicking on inputs like `date -d "30000000000000000 years"`. +fn unit_duration(unit: &str, n: i64) -> Option { match unit { - "second" => Duration::seconds(n), - "minute" => Duration::minutes(n), - "hour" => Duration::hours(n), - "day" => Duration::days(n), - "week" => Duration::weeks(n), - "month" => Duration::days(n * 30), // Approximate - "year" => Duration::days(n * 365), // Approximate - _ => Duration::zero(), + "second" => Duration::try_seconds(n), + "minute" => Duration::try_minutes(n), + "hour" => Duration::try_hours(n), + "day" => Duration::try_days(n), + "week" => Duration::try_weeks(n), + "month" => n.checked_mul(30).and_then(Duration::try_days), // Approximate + "year" => n.checked_mul(365).and_then(Duration::try_days), // Approximate + _ => Some(Duration::zero()), } } @@ -777,6 +789,20 @@ mod tests { assert_eq!(date.len(), 10); } + /// Relative offsets that overflow i64 or the chrono range must report an + /// error, not panic (e.g. `n * 365` in unit_duration, then base + offset). + #[tokio::test] + async fn test_date_d_relative_overflow_no_panic() { + for spec in [ + "30000000000000000 years", + "+9000000000000000000 days", + "300000000000000000 months ago", + ] { + let result = run_date(&["-d", spec, "+%Y"]).await; + assert_ne!(result.exit_code, 0, "spec '{spec}' should fail cleanly"); + } + } + #[tokio::test] async fn test_date_d_tomorrow() { let result = run_date(&["-d", "tomorrow", "+%Y-%m-%d"]).await; diff --git a/crates/bashkit/src/builtins/expr.rs b/crates/bashkit/src/builtins/expr.rs index 86d81f19..852a3718 100644 --- a/crates/bashkit/src/builtins/expr.rs +++ b/crates/bashkit/src/builtins/expr.rs @@ -118,13 +118,14 @@ fn evaluate(args: &[&str]) -> std::result::Result { if b == 0 { return Err("division by zero".to_string()); } - a / b + // checked_div guards i64::MIN / -1 (overflow panic). + a.checked_div(b).ok_or("integer overflow")? } "%" => { if b == 0 { return Err("division by zero".to_string()); } - a % b + a.checked_rem(b).ok_or("integer overflow")? } _ => unreachable!(), }; @@ -545,6 +546,31 @@ mod tests { assert_eq!(result.stdout.trim(), "0"); } + // ==================== arithmetic overflow ==================== + + /// i64::MIN / -1 (and % -1) overflow; must return a clean error, not panic. + #[tokio::test] + async fn expr_div_overflow_no_panic() { + let (fs, mut cwd, mut variables) = setup().await; + let env = HashMap::new(); + for op in ["/", "%"] { + let args = vec![ + "-9223372036854775808".to_string(), + op.to_string(), + "-1".to_string(), + ]; + let ctx = + Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); + let result = Expr.execute(ctx).await.unwrap(); + assert_eq!(result.exit_code, 2, "op {op} should error"); + assert!( + result.stderr.contains("integer overflow"), + "op {op}: expected overflow error, got {:?}", // debug-ok: test assertion + result.stderr + ); + } + } + // ==================== pattern matching ==================== #[tokio::test] diff --git a/crates/bashkit/src/builtins/jq/mod.rs b/crates/bashkit/src/builtins/jq/mod.rs index 92da84ee..eb958cc2 100644 --- a/crates/bashkit/src/builtins/jq/mod.rs +++ b/crates/bashkit/src/builtins/jq/mod.rs @@ -24,9 +24,10 @@ use jaq_core::{Compiler, Ctx, Vars, data}; use jaq_json::Val; use jaq_std::input::{HasInputs, Inputs, RcIter}; -use super::{Builtin, Context, read_text_file, resolve_path}; +use super::{Builtin, Context, ExecutionDeadline, read_text_file, resolve_path}; use crate::error::Result; use crate::interpreter::ExecResult; +use crate::limits::ExecutionLimits; mod args; mod compat; @@ -338,6 +339,19 @@ async fn run_jq(ctx: Context<'_>, parsed: JqArgs<'_>) -> Result { let mut has_output = false; let mut all_null_or_false = true; + // THREAT[TM-DOS-093]: jaq evaluation is a synchronous iterator that the + // async execution timeout cannot preempt. Unbounded generators + // (`jq -n 'repeat(1)'`, `range(0;1e18)`) would grow `output` without limit + // (OOM) or spin forever producing no output (hang). Cap accumulated output + // bytes against the caller's stdout limit and check the wall-clock deadline + // periodically so a runaway filter aborts instead of wedging the host. + let max_output_bytes = ctx + .execution_extension::() + .map(|l| l.max_stdout_bytes) + .unwrap_or_else(|| ExecutionLimits::default().max_stdout_bytes); + let deadline = ctx.execution_extension::(); + let mut values_emitted: usize = 0; + // Drive the outer loop from the shared input iterator so `input`/`inputs` // inside the filter consume from the same source (matching real jq: // `[inputs]` on a 3-value stream returns the *remaining* values, not all). @@ -405,6 +419,19 @@ async fn run_jq(ctx: Context<'_>, parsed: JqArgs<'_>) -> Result { if !parsed.join_output { output.push('\n'); } + + if output.len() > max_output_bytes { + return Ok(ExecResult::err( + format!("jq: output limit exceeded ({max_output_bytes} bytes)\n"), + 5, + )); + } + values_emitted += 1; + if values_emitted.is_multiple_of(4096) + && deadline.is_some_and(ExecutionDeadline::is_expired) + { + return Ok(ExecResult::err("jq: execution timed out\n".to_string(), 5)); + } } Err(e) => { return Ok(ExecResult::err(format_runtime_error(&e), 5)); diff --git a/crates/bashkit/src/builtins/jq/tests.rs b/crates/bashkit/src/builtins/jq/tests.rs index 2ea89c6a..f96839e5 100644 --- a/crates/bashkit/src/builtins/jq/tests.rs +++ b/crates/bashkit/src/builtins/jq/tests.rs @@ -126,6 +126,29 @@ async fn keys_pretty_prints_array() { assert_eq!(result.trim(), "[\n \"a\",\n \"b\"\n]"); } +/// TM-DOS-093: an unbounded generator must not grow output without limit or +/// hang the host. jaq's iterator is synchronous so the execution timeout +/// cannot preempt it; the output-byte cap (here the 1 MB default, since the +/// test harness supplies no shell/limits) must abort with a clean error. +#[tokio::test] +async fn unbounded_generator_is_capped() { + let result = run_jq_result_with_args(&["-n", "repeat(1)"], "") + .await + .unwrap(); + assert_ne!(result.exit_code, 0, "runaway generator should fail"); + assert!( + result.stderr.contains("output limit exceeded"), + "expected output-limit error, got stderr={:?} stdout_len={}", // debug-ok: test assertion + result.stderr, + result.stdout.len() + ); + assert!( + result.stdout.len() <= 1_048_576 + 64, + "output must be bounded by the cap, got {} bytes", + result.stdout.len() + ); +} + #[tokio::test] async fn length_returns_number() { let result = run_jq("length", r#"[1,2,3,4,5]"#).await.unwrap(); diff --git a/crates/bashkit/src/builtins/mod.rs b/crates/bashkit/src/builtins/mod.rs index 7069a562..18f7b64a 100644 --- a/crates/bashkit/src/builtins/mod.rs +++ b/crates/bashkit/src/builtins/mod.rs @@ -417,6 +417,13 @@ impl ExecutionDeadline { remaining } } + + /// Whether the wall-clock budget is exhausted. Builtins with synchronous + /// loops the async timeout cannot preempt poll this to abort. + #[allow(dead_code)] + pub(crate) fn is_expired(&self) -> bool { + self.started_at.elapsed() >= self.timeout + } } impl ExecutionExtensions { diff --git a/crates/bashkit/src/builtins/python.rs b/crates/bashkit/src/builtins/python.rs index 90d56115..653f3737 100644 --- a/crates/bashkit/src/builtins/python.rs +++ b/crates/bashkit/src/builtins/python.rs @@ -32,7 +32,7 @@ use std::pin::Pin; use std::sync::Arc; use std::time::Duration; -use super::{Builtin, Context, resolve_path}; +use super::{Builtin, Context, ExecutionDeadline, resolve_path}; use crate::error::Result; use crate::fs::{FileSystem, FileType}; use crate::interpreter::ExecResult; @@ -445,13 +445,25 @@ impl Builtin for Python { .map(|(k, v)| (k.clone(), v.clone())), ); + // Clamp Monty's wall-clock budget to the caller's remaining execution + // deadline (like the TypeScript builtin) so a CPU-bound script cannot + // overrun a tighter bash timeout — the async timeout cannot preempt + // Monty's synchronous start/resume sections. + let mut limits = self.limits.clone(); + if let Some(remaining) = ctx + .execution_extension::() + .map(ExecutionDeadline::remaining) + { + limits.max_duration = limits.max_duration.min(remaining); + } + run_python( &code, &filename, ctx.fs.clone(), ctx.cwd, &merged_env, - &self.limits, + &limits, self.external_fns.as_ref(), ) .await diff --git a/crates/bashkit/src/builtins/sqlite/mod.rs b/crates/bashkit/src/builtins/sqlite/mod.rs index 0bcba350..32491737 100644 --- a/crates/bashkit/src/builtins/sqlite/mod.rs +++ b/crates/bashkit/src/builtins/sqlite/mod.rs @@ -427,7 +427,7 @@ impl Builtin for Sqlite { let engine = match SqliteEngine::open_pure_memory() { Ok(e) => e, Err(msg) => { - return Ok(ExecResult::err(format!("sqlite: {msg}\n"), 1)); + return Ok(ExecResult::err(format!("sqlite: {}\n", sanitize(&msg)), 1)); } }; let outcome = run_statements( @@ -443,7 +443,7 @@ impl Builtin for Sqlite { ) .await; if let Err(e) = outcome { - stderr.push_str(&format!("sqlite: {e}\n")); + stderr.push_str(&format!("sqlite: {}\n", sanitize(&e))); exit_code = 1; } } @@ -473,7 +473,9 @@ impl Builtin for Sqlite { if reopen { match open_file_engine(backend, path, &ctx.fs, &self.limits).await { Ok(e) => *guard = Some(e), - Err(msg) => return Ok(ExecResult::err(format!("sqlite: {msg}\n"), 1)), + Err(msg) => { + return Ok(ExecResult::err(format!("sqlite: {}\n", sanitize(&msg)), 1)); + } } } let engine = guard.as_ref().expect("engine populated above"); @@ -491,7 +493,7 @@ impl Builtin for Sqlite { ) .await; if let Err(e) = outcome { - stderr.push_str(&format!("sqlite: {e}\n")); + stderr.push_str(&format!("sqlite: {}\n", sanitize(&e))); exit_code = 1; } diff --git a/crates/bashkit/src/credential.rs b/crates/bashkit/src/credential.rs index c3382bc5..acc0df02 100644 --- a/crates/bashkit/src/credential.rs +++ b/crates/bashkit/src/credential.rs @@ -264,7 +264,21 @@ impl CredentialPolicy { .iter() .find(|(name, _)| name.eq_ignore_ascii_case(header_name)) { - *header_value = header_value.replace(placeholder_str, secret_value); + // Substitute only when the placeholder appears + // exactly once. A legitimate value carries one + // placeholder; multiple occurrences are a + // segmentation/fingerprinting attempt (a script + // splitting the value to read the secret back in + // chunks from a header-reflecting endpoint), so + // we refuse and fail closed — the request goes + // out with placeholders, never the real secret. + // (The single-echo risk where an approved host + // reflects the whole header remains a documented + // v1 limitation; see credential-injection.md.) + if header_value.matches(placeholder_str).count() == 1 { + *header_value = + header_value.replacen(placeholder_str, secret_value, 1); + } } } } @@ -458,6 +472,37 @@ mod tests { } } + #[test] + fn test_placeholder_multiple_occurrences_fail_closed() { + // A script repeating the placeholder is attempting to segment the + // secret across a header-reflecting endpoint. We must not substitute + // anything: the request goes out with placeholders, never the secret. + let mut policy = CredentialPolicy::new(); + let placeholder = + policy.add_placeholder("https://api.openai.com", Credential::bearer("sk-real-key")); + + let hook = policy.into_hook(); + let event = HttpRequestEvent { + method: "POST".into(), + url: "https://api.openai.com/v1/chat/completions".into(), + headers: vec![( + "Authorization".into(), + format!("Bearer {placeholder}.{placeholder}"), + )], + }; + + match hook(event) { + HookAction::Continue(e) => { + assert!( + !e.headers[0].1.contains("sk-real-key"), + "secret must not be injected when placeholder is repeated" + ); + assert!(e.headers[0].1.contains("bk_placeholder_")); + } + HookAction::Cancel(_) => panic!("should not cancel"), + } + } + #[test] fn test_placeholder_not_replaced_for_wrong_host() { let mut policy = CredentialPolicy::new(); diff --git a/crates/bashkit/src/interpreter/mod.rs b/crates/bashkit/src/interpreter/mod.rs index 13fcaa39..456cf74a 100644 --- a/crates/bashkit/src/interpreter/mod.rs +++ b/crates/bashkit/src/interpreter/mod.rs @@ -4926,7 +4926,10 @@ impl Interpreter { 1, )) } else { - self.execute(&s).await + // Alias expansion runs in the current shell: use + // execute_script_body (like source/eval), NOT execute(), + // so an aliased command does not fire the EXIT trap. + self.execute_script_body(&s, false, true).await } } Err(e) => Ok(ExecResult::err( @@ -6292,7 +6295,12 @@ impl Interpreter { self.pipeline_stdin = stdin; } - let mut result = self.execute(&script).await?; + // eval runs in the current shell: use execute_script_body (like source), + // NOT execute(), so it does not fire the EXIT trap. execute() runs the + // EXIT trap, which is wrong for eval and — because the top-level EXIT + // trap has no re-entrancy guard — lets `trap 'eval :' EXIT` recurse one + // command per level until the budget aborts, risking stack overflow. + let mut result = self.execute_script_body(&script, false, true).await?; self.pipeline_stdin = prev_pipeline_stdin; @@ -11892,11 +11900,32 @@ impl Interpreter { fn expand_braces(&self, s: &str) -> Vec { const MAX_BRACE_EXPANSION_TOTAL: usize = 100_000; let mut count = 0; - self.expand_braces_capped(s, &mut count, MAX_BRACE_EXPANSION_TOTAL) - } - - fn expand_braces_capped(&self, s: &str, count: &mut usize, max: usize) -> Vec { - if *count >= max { + let mut bytes = 0; + self.expand_braces_capped(s, &mut count, &mut bytes, MAX_BRACE_EXPANSION_TOTAL, 0) + } + + /// THREAT[TM-DOS-042]: The combinatorial `count` cap alone is insufficient + /// because it is only incremented *after* each recursive call returns, so + /// the first DFS path descends to the full nesting/sequence depth (one + /// level per brace group) with `count` still zero. An input like + /// `{a,b}{a,b}...` repeated tens of thousands of times (well under + /// `max_input_bytes`) therefore stack-overflows the worker thread, and + /// allocates O(depth * suffix) memory, before the count cap or execution + /// timeout can fire. The `depth` cap bounds the descent up front; + /// legitimate scripts never approach this many nested/sequential groups. + fn expand_braces_capped( + &self, + s: &str, + count: &mut usize, + bytes: &mut usize, + max: usize, + recursion_depth: usize, + ) -> Vec { + const MAX_BRACE_EXPANSION_DEPTH: usize = 100; + if *count >= max + || *bytes >= Self::MAX_EXPANSION_RESULT_BYTES + || recursion_depth >= MAX_BRACE_EXPANSION_DEPTH + { return vec![s.to_string()]; } @@ -11955,12 +11984,14 @@ impl Interpreter { if let Some(range_result) = self.try_expand_range(&brace_content) { let mut results = Vec::new(); for item in range_result { - if *count >= max { + if *count >= max || *bytes >= Self::MAX_EXPANSION_RESULT_BYTES { break; } let expanded = format!("{}{}{}", prefix, item, suffix); - let sub = self.expand_braces_capped(&expanded, count, max); + let sub = + self.expand_braces_capped(&expanded, count, bytes, max, recursion_depth + 1); *count += sub.len(); + *bytes += sub.iter().map(String::len).sum::(); results.extend(sub); } return results; @@ -11976,12 +12007,13 @@ impl Interpreter { let mut results = Vec::new(); for item in items { - if *count >= max { + if *count >= max || *bytes >= Self::MAX_EXPANSION_RESULT_BYTES { break; } let expanded = format!("{}{}{}", prefix, item, suffix); - let sub = self.expand_braces_capped(&expanded, count, max); + let sub = self.expand_braces_capped(&expanded, count, bytes, max, recursion_depth + 1); *count += sub.len(); + *bytes += sub.iter().map(String::len).sum::(); results.extend(sub); } @@ -12136,6 +12168,24 @@ mod tests { use crate::fs::InMemoryFs; use crate::parser::Parser; + /// TM-DOS-042: comma-list brace expansion must not recurse one frame per + /// brace group (stack overflow) nor accumulate unbounded memory. A long + /// `{a,b}{a,b}...` sequence — far under the input cap — used to descend to + /// full depth before any cap engaged. + #[test] + fn brace_expansion_comma_sequence_is_bounded() { + let fs: Arc = Arc::new(InMemoryFs::new()); + let interp = Interpreter::new(Arc::clone(&fs)); + let s = "{a,b}".repeat(50_000); + let out = interp.expand_braces(&s); + // Must terminate without panic/overflow and stay bounded. + let total: usize = out.iter().map(String::len).sum(); + assert!( + total <= Interpreter::MAX_EXPANSION_RESULT_BYTES + 1024, + "brace expansion produced {total} bytes — should be byte-capped" + ); + } + #[test] fn test_empty_anchored_replacement_respects_expansion_limit() { let fs: Arc = Arc::new(InMemoryFs::new()); diff --git a/crates/bashkit/src/limits.rs b/crates/bashkit/src/limits.rs index aa7b8204..ae060947 100644 --- a/crates/bashkit/src/limits.rs +++ b/crates/bashkit/src/limits.rs @@ -810,7 +810,11 @@ impl MemoryBudget { limits.max_function_count ))); } - let new_bytes = self.function_body_bytes + body_bytes - old_body_bytes; + // saturating_sub mirrors record_function_insert: if accounting ever + // drifts so old_body_bytes exceeds the running total (e.g. after a + // snapshot restore recomputes sizes differently), the check must not + // underflow-panic. + let new_bytes = (self.function_body_bytes + body_bytes).saturating_sub(old_body_bytes); if new_bytes > limits.max_function_body_bytes { return Err(LimitExceeded::Memory(format!( "function body byte limit ({}) exceeded", diff --git a/crates/bashkit/src/parser/lexer.rs b/crates/bashkit/src/parser/lexer.rs index fd21dcc5..1680251b 100644 --- a/crates/bashkit/src/parser/lexer.rs +++ b/crates/bashkit/src/parser/lexer.rs @@ -322,8 +322,11 @@ impl<'a> Lexer<'a> { } // Check if it's a single digit followed by > or < - // We need to peek further without consuming - let input_remaining: String = self.chars.clone().collect(); + // We need to peek further without consuming. Only the digit plus a + // 2-char redirect operator (e.g. ">>", "<&", "<<") matter, so bound the + // lookahead — collecting all remaining input here made every + // digit-initial word O(n) and the whole lex O(n^2) (TM-DOS-024). + let input_remaining: String = self.chars.clone().take(4).collect(); // Check patterns: "N>" "N>>" "N>&" "N<" "N<&" if fd_str.len() == 1 @@ -1833,13 +1836,17 @@ impl<'a> Lexer<'a> { /// compound assignment like `([key]=val ...)`. Returns true when the /// first non-whitespace char after `(` is `[`. fn looks_like_assoc_assign(&self) -> bool { + // Cap the lookahead like looks_like_brace_expansion: an uncapped scan + // over leading whitespace made `x=(` followed by megabytes of spaces + // O(n) per call (TM-DOS-024). + const MAX_LOOKAHEAD: usize = 10_000; let mut chars = self.chars.clone(); // Skip the `(` we haven't consumed yet if chars.next() != Some('(') { return false; } // Skip optional whitespace - for ch in chars { + for ch in chars.take(MAX_LOOKAHEAD) { match ch { ' ' | '\t' => continue, '[' => return true, diff --git a/crates/bashkit/tests/integration/blackbox_security_tests.rs b/crates/bashkit/tests/integration/blackbox_security_tests.rs index a000ea95..595e8085 100644 --- a/crates/bashkit/tests/integration/blackbox_security_tests.rs +++ b/crates/bashkit/tests/integration/blackbox_security_tests.rs @@ -1224,6 +1224,37 @@ mod command_injection_passing { assert!(result.stdout.contains("normal")); } + /// eval runs in the current shell and must NOT fire the EXIT trap when the + /// eval'd code finishes — the trap fires once, at actual shell exit. + #[tokio::test] + async fn eval_does_not_fire_exit_trap_early() { + let mut bash = tight_bash(); + let result = bash + .exec("trap 'echo BYE' EXIT\neval 'echo hi'\necho done") + .await + .unwrap(); + assert_eq!( + result.stdout.matches("BYE").count(), + 1, + "EXIT trap should fire exactly once, got: {:?}", + result.stdout + ); + // BYE must come after `done` (end of script), not after `hi`. + let bye = result.stdout.find("BYE").unwrap(); + let done = result.stdout.find("done").unwrap(); + assert!(done < bye, "EXIT trap fired early: {:?}", result.stdout); + } + + /// An EXIT trap that runs `eval` must not recurse unboundedly. Before the + /// fix, eval re-ran the (unguarded) EXIT trap, recursing one command per + /// level until the budget aborted — risking a stack overflow first. + #[tokio::test] + async fn exit_trap_eval_recursion_terminates() { + let mut bash = tight_bash(); + // Must return (Ok or budget Err), never hang or stack-overflow. + let _ = bash.exec("trap 'eval :' EXIT\necho go").await; + } + /// Array subscript command substitution stays sandboxed #[tokio::test] async fn array_subscript_cmd_subst_sandboxed() { diff --git a/crates/bashkit/tests/integration/python_security_tests.rs b/crates/bashkit/tests/integration/python_security_tests.rs index af999129..4627568f 100644 --- a/crates/bashkit/tests/integration/python_security_tests.rs +++ b/crates/bashkit/tests/integration/python_security_tests.rs @@ -321,6 +321,36 @@ mod whitebox_resource_limits { assert_ne!(r.exit_code, 0, "100ms limit should block long loop"); } + /// The bash-level execution timeout must bound a CPU-bound Python loop even + /// when PythonLimits.max_duration is much larger: the async timeout cannot + /// preempt Monty's synchronous run, so the builtin clamps Monty's duration + /// to the remaining deadline. Without the clamp this would run for the full + /// 60s Python limit. Use a generous (2s) bash timeout so the clamp normally + /// stops Monty cleanly first; either a non-zero `Ok` exit or a `Timeout` + /// `Err` is acceptable — both prove the loop was bounded, and the + /// elapsed-time assertion (well under the 60s Python limit) is the real + /// regression guard. Accepting both avoids flakiness on loaded runners. + #[tokio::test] + async fn bash_timeout_clamps_python_duration() { + let bash = Bash::builder() + .python_with_limits(PythonLimits::default().max_duration(Duration::from_secs(60))) + .env("BASHKIT_ALLOW_INPROCESS_PYTHON", "1") + .limits(ExecutionLimits::new().timeout(Duration::from_secs(2))); + let mut bash = bash.build(); + let start = std::time::Instant::now(); + let result = bash.exec("python3 -c \"while True:\n pass\"").await; + let elapsed = start.elapsed(); + // An Err here is the outer execution timeout firing, which is also an + // acceptable bound; the elapsed-time assertion below is the real guard. + if let Ok(r) = result { + assert_ne!(r.exit_code, 0, "infinite loop should be stopped"); + } + assert!( + elapsed < Duration::from_secs(15), + "bash timeout did not clamp Python duration: ran {elapsed:?}" + ); + } + #[tokio::test] async fn tight_recursion_blocks_deep_call() { let limits = PythonLimits::default().max_recursion(10); diff --git a/crates/bashkit/tests/integration/threat_model_tests.rs b/crates/bashkit/tests/integration/threat_model_tests.rs index 6ebfca24..3490fedc 100644 --- a/crates/bashkit/tests/integration/threat_model_tests.rs +++ b/crates/bashkit/tests/integration/threat_model_tests.rs @@ -4650,6 +4650,45 @@ echo ${#arr[@]} } } + /// TM-DOS-042: Comma-list brace expansion recursion bomb. + /// `{a,b}{a,b}...` repeated tens of thousands of times is far under + /// `max_input_bytes`, but the combinatorial count cap is only charged on + /// recursion *return*, so the first DFS path used to descend one frame per + /// group and stack-overflow the worker thread before any cap fired. The + /// depth cap must bound the descent: this completes without panic/OOM and + /// with bounded output. + #[tokio::test] + async fn brace_expansion_comma_recursion_bomb() { + let limits = ExecutionLimits::new() + .max_commands(1_000) + .max_stdout_bytes(1_000_000); + let mut bash = Bash::builder().limits(limits).build(); + + // 50k repetitions of `{a,b}` (~250 KB source, well under the input cap). + let script = format!("echo {}", "{a,b}".repeat(50_000)); + let result = bash.exec(&script).await; + match result { + Ok(r) => { + assert!( + r.stdout.len() <= 1_000_000, + "comma-list brace bomb produced {} bytes — should be capped", + r.stdout.len() + ); + } + Err(e) => { + let msg = e.to_string(); + assert!( + msg.contains("brace") + || msg.contains("exceeded") + || msg.contains("budget") + || msg.contains("too large"), + "Expected a limit error, got: {}", + msg + ); + } + } + } + /// TM-DOS-059: Parameter expansion replacement bomb. /// `${x//a/$(echo bbbbbbbb)}` replaces each 'a' with 'bbbbbbbb'. /// At scale (10K 'a's × 1K 'b's = 10MB), this must be caught by diff --git a/specs/threat-model.md b/specs/threat-model.md index c10c7fcd..35474cfe 100644 --- a/specs/threat-model.md +++ b/specs/threat-model.md @@ -136,6 +136,15 @@ the session-level backstop. | TM-DOS-039 | Missing validate_path in VFS methods | `remove`, `stat`, `read_dir`, `copy`, `rename`, `symlink`, `chmod` skip `validate_path()` | All `FileSystem` methods on `InMemoryFs` and `OverlayFs` call `validate_path()`; `MountableFs` validates via the root limits before delegating (TM-DOS-046) | **MITIGATED** | | TM-DOS-040 | Integer truncation on 32-bit | `u64 as usize` casts in network/Python extension silently truncate on 32-bit, bypassing size checks | All `u64`-to-`usize` size-check casts in `network/client.rs` (Content-Length checks) and `bashkit-python/src/lib.rs` (limits propagation) use `usize::try_from(...).unwrap_or(usize::MAX)` so over-`usize` values fail safe instead of wrapping | **MITIGATED** | +> **Scope of the FS quotas above (TM-DOS-005/006/010, `max_file_size` / +> `max_file_count` / `max_total_bytes`)**: these are enforced by the +> in-memory and overlay backends. They do **not** apply to a `RealFs` mount +> in `RealFsMode::ReadWrite` — `RealFs::limits()` returns `unlimited()` and +> writes go straight to the host with no byte/count quota. This is by design +> (`--mount-rw` is sandbox-breaking, see TM-ESC-030), but it means a script +> with a writable real-FS mount can exhaust host disk/inodes. Use +> `--mount-ro` for untrusted scripts. + **TM-DOS-034**: Fixed. `InMemoryFs::append_file()` now uses a single write lock for the entire read-check-write operation, preventing TOCTOU races. See `fs/memory.rs:940-942`. @@ -308,21 +317,25 @@ longer the primary defence. Regression tests: depth exceeded` error. Regression test: `tests/threat_model_tests.rs::yaml_template_depth::tm_dos_052_template_if_depth_bomb`. -**Current Risk**: MEDIUM - Three open DoS vectors (TM-DOS-029, TM-DOS-030, TM-DOS-031) need remediation +**Current Risk**: LOW — TM-DOS-029, TM-DOS-030, and TM-DOS-031 are all mitigated +(see the status table above). Their original write-ups are kept below for history. -**TM-DOS-029**: Arithmetic exponentiation casts `i64` to `u32` (`right as u32`), wrapping negatives. -`i64::pow()` with large exponent panics or hangs. Shift operators panic if `right >= 64` or `right < 0`. -`i64::MIN / -1` panics. All arithmetic panics in debug on overflow. -Fix: use `wrapping_*` operations and clamp shift/exponent ranges. +**TM-DOS-029** (mitigated): Arithmetic exponentiation cast `i64` to `u32` (`right as u32`), +wrapping negatives; `i64::pow()` with a large exponent panicked or hung; shift operators panicked +if `right >= 64` or `right < 0`; `i64::MIN / -1` panicked. Resolved: arithmetic ops use `wrapping_*`, +shift amounts are masked, and the exponent is clamped (`interpreter/mod.rs` arithmetic evaluator). +The same `i64::MIN / -1` guard is now applied in the `expr` builtin (`checked_div`/`checked_rem`). -**TM-DOS-030**: `eval` (line 4613), `source` (line 4548), trap handlers (lines 697, 7795), and alias -expansion (line 3627) all use `Parser::new()` which ignores configured `max_ast_depth` and -`max_parser_operations`. Previously fixed for command substitution (TM-DOS-021) but not these paths. -Fix: use `Parser::with_limits()` everywhere. +**TM-DOS-030** (fixed): `eval`, `source`, trap handlers, and alias expansion previously used +`Parser::new()`, ignoring configured `max_ast_depth` / `max_parser_operations`. Resolved: all of +these paths use `Parser::with_limits()`. Separately, `eval` and alias expansion now run their bodies +via `execute_script_body(.., run_exit_trap=false)` (like `source`) so they do not fire the EXIT trap +— previously they ran the unguarded EXIT trap, letting `trap 'eval :' EXIT` recurse one command per +level until the budget aborted. -**TM-DOS-031**: ExtGlob `+(...)` and `*(...)` handlers recurse without depth limit. Pattern -`+(a|aa)` against `"aaaa..."` creates exponential backtracking via nested `glob_match_impl` calls. -Fix: add depth parameter to `glob_match_impl`, bail when exceeded. +**TM-DOS-031** (mitigated): ExtGlob `+(...)` / `*(...)` handlers recursed without a depth limit. +Resolved: `glob_match_impl` carries a recursion-depth cap and bails when exceeded +(`interpreter/glob.rs`). **Implementation**: `limits.rs`, `builtins/awk.rs`, `builtins/jq/`, `builtins/diff.rs` ```rust @@ -1354,7 +1367,7 @@ This section maps former vulnerability IDs to the new threat ID scheme and track | TM-INF-017 | `set` and `declare -p` leak internal markers | Internal state disclosure (_NAMEREF_, _READONLY_, _UPPER_, _LOWER_) | Filter `is_internal_variable()` names from output | | TM-INF-018 | `date` builtin returns real host time | Timezone fingerprinting, timing correlation | `Bash::builder().fixed_epoch(N)` freezes the clock; `Bash::builder().epoch_offset(N)` shifts real-clock by N seconds (mutually exclusive, last call wins). Both implemented via `Date::with_fixed_epoch` / `Date::with_offset_seconds` in `builtins/date.rs`. Default behavior is real clock — embed callers opt in for sandboxing. Regression tests: `tm_inf_018_date::*` in `tests/threat_model_tests.rs`. | **MITIGATED** (opt-in) | | ~~TM-DOS-041~~ | ~~Brace expansion `{N..M}` unbounded range~~ | ~~OOM via `{1..999999999}` allocating billions of strings~~ | Static parser-time check (`MAX_STATIC_BRACE_RANGE = 100_000` in `parser/budget.rs`) rejects oversized literal ranges with `BraceRangeTooLarge`; runtime fallback in `try_expand_range` (`MAX_BRACE_RANGE = 10_000`) treats remaining oversized ranges as literals (**FIXED**) | -| ~~TM-DOS-042~~ | ~~Brace expansion combinatorial explosion~~ | ~~OOM via `{1..100}{1..100}{1..100}` = 1M strings~~ | `expand_braces` caps total emitted strings at `MAX_BRACE_EXPANSION_TOTAL = 100_000` and bails out of the recursion when the budget is hit (**FIXED**) | +| ~~TM-DOS-042~~ | ~~Brace expansion combinatorial explosion~~ | ~~OOM/stack overflow via `{1..100}{1..100}{1..100}` (1M strings) or `{a,b}{a,b}...` (deep recursion)~~ | `expand_braces` caps total emitted strings (`MAX_BRACE_EXPANSION_TOTAL = 100_000`). The count cap alone was insufficient for comma-lists: it is only charged on recursion *return*, so the first DFS path descended one frame per group with the count still zero and stack-overflowed. Now also bounds recursion depth and cumulative output bytes (`MAX_EXPANSION_RESULT_BYTES`) so it degrades to a bounded literal result (**FIXED**) | | ~~TM-DOS-043~~ | ~~Arithmetic overflow in `execute_arithmetic_with_side_effects`~~ | ~~Panic (DoS) in debug mode via `((x+=1))` with x=i64::MAX~~ | ~~Use `wrapping_add/sub/mul`~~ (**FIXED**) | | ~~TM-DOS-044~~ | ~~Lexer `read_command_subst_into` stack overflow~~ | ~~Process crash (SIGABRT) via ~50 nested `$()` in double-quotes~~ | ~~Add depth parameter to `read_command_subst_into()`~~ (**FIXED**) | | ~~TM-DOS-045~~ | ~~OverlayFs `symlink()` bypasses all limits~~ | ~~Unlimited symlink creation despite `max_file_count`~~ | ~~Add `check_write_limits()` + `validate_path()` to symlink~~ — overlay symlink path enforces limits (**FIXED**) | @@ -1387,6 +1400,7 @@ This section maps former vulnerability IDs to the new threat ID scheme and track | ~~TM-DOS-090~~ | ~~`shuf` unbounded range/repeat materialization~~ | ~~OOM/CPU exhaustion via huge `--input-range` or `--head-count` before stdout truncation~~ | ~~Sample numeric ranges without full collection and reject output that exceeds `ExecutionLimits` before allocation~~ — `shuf_resource_tests` cover huge range `-n 1` and repeat output caps (**FIXED**) | | TM-DOS-091 | SQLite `.dump` cumulative output bypass | `.dump` built the full schema+rows string before checking `max_output_bytes`; an attacker controlling many tables/rows could cause memory to grow far beyond the configured output cap | `bounded_append()` helper enforces the cap after each schema line and each INSERT row; `dispatch()` receives the *remaining* budget (`max_output_bytes - stdout.len()`) from `run_statements` — **FIXED** via #1869 | | TM-DOS-092 | Subshell snapshot amplification | Deeply nested `( ... )` keeps CoW state snapshots and call-stack clones alive | `max_subshell_depth` counter (default 32) bounds live explicit subshell snapshots | **MITIGATED** | +| TM-DOS-093 | jq unbounded generator OOM/hang | `jq -n 'repeat(1)'` / `range(0;1e18)` — the jaq result loop appended every value to an in-memory string with no byte/value/deadline cap; jaq's iterator is synchronous so the async execution timeout cannot preempt it. jq is a core builtin (no opt-in gate) | Cap accumulated output at the caller's `max_stdout_bytes` and poll `ExecutionDeadline::is_expired()` every 4096 values, aborting with a clean error (`builtins/jq/mod.rs`) — **FIXED** | ### Accepted (Low Priority) @@ -1457,7 +1471,7 @@ This section maps former vulnerability IDs to the new threat ID scheme and track | VFS limit enforcement on public API | TM-ESC-012 | `validate_path()` + `check_write_limits()` in `add_file()` / `restore()` | **MITIGATED** | | Custom jaq env function | TM-INF-013, TM-ISO-004 | Read from `ctx.env`/`ctx.variables`, not `std::env` | **DONE** | | Internal variable namespace isolation | TM-INJ-009 | `is_internal_variable()` rejection in every write path; `set` / `declare -p` filter | **MITIGATED** | -| Parser limit propagation | TM-DOS-030 | `Parser::with_limits()` in eval/source/trap/alias | **NEEDED** | +| Parser limit propagation | TM-DOS-030 | `Parser::with_limits()` in eval/source/trap/alias | **MITIGATED** | | ExtGlob depth limit | TM-DOS-031 | Depth parameter in `glob_match_impl` (`interpreter/glob.rs:162,324`) | **MITIGATED** | | Python wrapper input sanitization | TM-PY-023, TM-PY-024 | `shlex.quote()` on every interpolated path; per-call random heredoc delimiter via `secrets.token_hex(8)` | **MITIGATED** | | Tar path validation | TM-INJ-010 | Resolved-path check + `starts_with(&extract_base)` in `archive.rs` | **MITIGATED** |