Skip to content

Commit 6b9c38c

Browse files
authored
fix(interpreter): add max_subst_depth limit to prevent OOM from nested $() (#1107)
## Summary - Add dedicated `max_subst_depth` limit (default 32) to `ExecutionLimits` to prevent OOM from deeply nested command substitutions - Each `$(...)` level clones the full interpreter state — memory grows as depth × state_size - Previously shared `max_function_depth` counter (default 100) was too generous for expensive state-cloning operations - Harden `arithmetic_fuzz` target with tighter resource limits ## Changes - `limits.rs`: new `max_subst_depth` field, `subst_depth` counter, `push_subst`/`pop_subst` methods, `MaxSubstDepth` error variant - `interpreter/mod.rs`: switch both command substitution paths from `push_function`/`pop_function` to `push_subst`/`pop_subst` - `arithmetic_fuzz.rs`: add `max_function_depth(10)`, `max_subst_depth(5)`, `max_stdout_bytes(4096)`, `max_stderr_bytes(4096)` - `blackbox_security_tests.rs`: set `max_subst_depth` in `tight_bash()` and `dos_bash()` helpers - New `subst_depth_limit_tests.rs`: 3 regression tests including decoded fuzz crash input - `specs/006-threat-model.md`: add TM-DOS-088 ## Test plan - [x] `cargo test --test subst_depth_limit_tests` — 3 new tests pass - [x] `cargo test --test blackbox_security_tests` — 78 tests pass (previously stack-overflowed) - [x] `cargo test --all-features -- --skip ssh_supabase` — all pass - [x] `cargo clippy --all-targets --all-features -- -D warnings` — clean - [x] `cargo fmt --check` — clean Closes #1088
1 parent 34cf2c2 commit 6b9c38c

File tree

6 files changed

+158
-7
lines changed

6 files changed

+158
-7
lines changed

crates/bashkit/fuzz/fuzz_targets/arithmetic_fuzz.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,10 @@ fuzz_target!(|data: &[u8]| {
5252
.limits(
5353
bashkit::ExecutionLimits::new()
5454
.max_commands(100)
55+
.max_function_depth(10)
56+
.max_subst_depth(5)
57+
.max_stdout_bytes(4096)
58+
.max_stderr_bytes(4096)
5559
.timeout(std::time::Duration::from_millis(100)),
5660
)
5761
.build();

crates/bashkit/src/interpreter/mod.rs

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6212,10 +6212,13 @@ impl Interpreter {
62126212
}
62136213
}
62146214
WordPart::CommandSubstitution(commands) => {
6215-
// THREAT[TM-DOS-044]: Track substitution depth to prevent stack overflow
6216-
if self.counters.push_function(&self.limits).is_err() {
6215+
// THREAT[TM-DOS-088]: Track substitution depth separately from
6216+
// function depth. Each level clones the full interpreter state,
6217+
// so memory ≈ depth × state_size. A tighter limit than
6218+
// max_function_depth prevents OOM.
6219+
if self.counters.push_subst(&self.limits).is_err() {
62176220
return Err(crate::error::Error::Execution(
6218-
"maximum nesting depth exceeded in command substitution".to_string(),
6221+
"maximum command substitution depth exceeded".to_string(),
62196222
));
62206223
}
62216224
// Command substitution runs in a subshell: snapshot all
@@ -6260,7 +6263,7 @@ impl Interpreter {
62606263
self.aliases = saved_aliases;
62616264
self.cwd = saved_cwd;
62626265
self.memory_budget = saved_memory_budget;
6263-
self.counters.pop_function();
6266+
self.counters.pop_subst();
62646267
self.subst_generation += 1;
62656268
let trimmed = stdout.trim_end_matches('\n');
62666269
result.push_str(trimmed);
@@ -8436,7 +8439,7 @@ impl Interpreter {
84368439
);
84378440
match parser.parse() {
84388441
Ok(script) => {
8439-
if self.counters.push_function(&self.limits).is_err() {
8442+
if self.counters.push_subst(&self.limits).is_err() {
84408443
result.push('0');
84418444
} else {
84428445
let saved_vars = self.variables.clone();
@@ -8457,7 +8460,7 @@ impl Interpreter {
84578460
self.aliases = saved_aliases;
84588461
self.cwd = saved_cwd;
84598462
self.memory_budget = saved_memory_budget;
8460-
self.counters.pop_function();
8463+
self.counters.pop_subst();
84618464
let trimmed = cmd_result.stdout.trim_end_matches('\n');
84628465
if trimmed.is_empty() {
84638466
result.push('0');

crates/bashkit/src/limits.rs

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,14 @@ pub struct ExecutionLimits {
8484
/// Prevents unbounded error output accumulation.
8585
pub max_stderr_bytes: usize,
8686

87+
// THREAT[TM-DOS-088]: Command substitutions clone the entire interpreter
88+
// state (variables, arrays, functions, etc.) per nesting level. At depth N,
89+
// memory ≈ N × state_size. A separate, tighter limit than max_function_depth
90+
// prevents OOM from deeply nested $(...) chains.
91+
/// Maximum command substitution nesting depth.
92+
/// Default: 32
93+
pub max_subst_depth: usize,
94+
8795
/// Whether to capture the final environment state in ExecResult.
8896
/// Default: false (opt-in to avoid cloning cost when not needed)
8997
pub capture_final_env: bool,
@@ -103,6 +111,7 @@ impl Default for ExecutionLimits {
103111
max_parser_operations: 100_000,
104112
max_stdout_bytes: 1_048_576, // 1MB
105113
max_stderr_bytes: 1_048_576, // 1MB
114+
max_subst_depth: 32,
106115
capture_final_env: false,
107116
}
108117
}
@@ -201,6 +210,12 @@ impl ExecutionLimits {
201210
self
202211
}
203212

213+
/// Set maximum command substitution nesting depth.
214+
pub fn max_subst_depth(mut self, depth: usize) -> Self {
215+
self.max_subst_depth = depth;
216+
self
217+
}
218+
204219
/// Enable capturing final environment state in ExecResult
205220
pub fn capture_final_env(mut self, capture: bool) -> Self {
206221
self.capture_final_env = capture;
@@ -287,6 +302,9 @@ pub struct ExecutionCounters {
287302
/// Total loop iterations across all loops (never reset)
288303
pub total_loop_iterations: usize,
289304

305+
/// Current command substitution nesting depth.
306+
pub subst_depth: usize,
307+
290308
// THREAT[TM-DOS-059]: Session-level cumulative counters.
291309
// These persist across exec() calls (never reset by reset_for_execution).
292310
/// Total commands across all exec() calls in this session.
@@ -309,9 +327,10 @@ impl ExecutionCounters {
309327
self.commands = 0;
310328
self.loop_iterations = 0;
311329
self.total_loop_iterations = 0;
312-
// function_depth should already be 0 between exec() calls,
330+
// function_depth and subst_depth should already be 0 between exec() calls,
313331
// but reset defensively to avoid stuck state
314332
self.function_depth = 0;
333+
self.subst_depth = 0;
315334
}
316335

317336
/// Increment command counter, returns error if limit exceeded
@@ -444,6 +463,22 @@ impl ExecutionCounters {
444463
self.function_depth -= 1;
445464
}
446465
}
466+
467+
/// Push command substitution, returns error if depth exceeded.
468+
/// THREAT[TM-DOS-088]: Command substitutions clone interpreter state,
469+
/// so their nesting depth must be bounded more tightly than functions.
470+
pub fn push_subst(&mut self, limits: &ExecutionLimits) -> Result<(), LimitExceeded> {
471+
if self.subst_depth >= limits.max_subst_depth {
472+
return Err(LimitExceeded::MaxSubstDepth(limits.max_subst_depth));
473+
}
474+
self.subst_depth += 1;
475+
Ok(())
476+
}
477+
478+
/// Pop command substitution
479+
pub fn pop_subst(&mut self) {
480+
self.subst_depth = self.subst_depth.saturating_sub(1);
481+
}
447482
}
448483

449484
/// Error returned when a resource limit is exceeded
@@ -461,6 +496,9 @@ pub enum LimitExceeded {
461496
#[error("maximum function depth exceeded ({0})")]
462497
MaxFunctionDepth(usize),
463498

499+
#[error("maximum command substitution depth exceeded ({0})")]
500+
MaxSubstDepth(usize),
501+
464502
#[error("execution timeout ({0:?})")]
465503
Timeout(Duration),
466504

crates/bashkit/tests/blackbox_security_tests.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ fn tight_bash() -> Bash {
2424
.max_loop_iterations(100)
2525
.max_total_loop_iterations(500)
2626
.max_function_depth(20)
27+
.max_subst_depth(15)
2728
.timeout(Duration::from_secs(5)),
2829
)
2930
.build()
@@ -38,6 +39,7 @@ fn dos_bash() -> Bash {
3839
.max_loop_iterations(10)
3940
.max_total_loop_iterations(50)
4041
.max_function_depth(5)
42+
.max_subst_depth(3)
4143
.timeout(Duration::from_secs(3)),
4244
)
4345
.build()
Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
// THREAT[TM-DOS-088]: Command substitution depth limit tests.
2+
// Each $(...) level clones the full interpreter state, so memory ≈ depth × state_size.
3+
// A dedicated max_subst_depth limit (default 32) prevents OOM from deeply nested
4+
// command substitution chains.
5+
6+
use bashkit::{Bash, ExecutionLimits};
7+
8+
/// Deeply nested command substitution must be capped by max_subst_depth.
9+
/// Regression test for issue #1088.
10+
#[tokio::test]
11+
async fn subst_depth_limit_prevents_oom() {
12+
// Build a deeply nested $(...) expression: $(echo $(echo $(echo ...)))
13+
let depth = 40; // exceeds default max_subst_depth of 32
14+
let mut script = "echo hi".to_string();
15+
for _ in 0..depth {
16+
script = format!("echo $({})", script);
17+
}
18+
19+
let mut bash = Bash::builder()
20+
.limits(
21+
ExecutionLimits::new()
22+
.max_commands(500)
23+
.max_subst_depth(5)
24+
.timeout(std::time::Duration::from_secs(5)),
25+
)
26+
.build();
27+
28+
// Must not OOM — should return an error or truncated result
29+
let result = bash.exec(&script).await;
30+
// The execution should either error or succeed with limited depth,
31+
// but must NOT panic or OOM.
32+
assert!(result.is_ok() || result.is_err());
33+
}
34+
35+
/// With a low subst depth limit, nested substitutions produce an error.
36+
#[tokio::test]
37+
async fn subst_depth_limit_returns_error() {
38+
let mut script = "echo hi".to_string();
39+
for _ in 0..10 {
40+
script = format!("echo $({})", script);
41+
}
42+
43+
let mut bash = Bash::builder()
44+
.limits(
45+
ExecutionLimits::new()
46+
.max_commands(500)
47+
.max_subst_depth(3)
48+
.timeout(std::time::Duration::from_secs(5)),
49+
)
50+
.build();
51+
52+
let result = bash.exec(&script).await;
53+
match result {
54+
Ok(r) => {
55+
// Execution succeeded but depth was limited (error message in stderr
56+
// or result was truncated)
57+
assert!(
58+
r.exit_code != 0 || r.stderr.contains("substitution depth"),
59+
"expected error from deep nesting, got exit_code={} stderr={:?}",
60+
r.exit_code,
61+
r.stderr
62+
);
63+
}
64+
Err(e) => {
65+
let msg = e.to_string();
66+
assert!(
67+
msg.contains("substitution depth") || msg.contains("nesting"),
68+
"unexpected error: {}",
69+
msg
70+
);
71+
}
72+
}
73+
}
74+
75+
/// Fuzz crash input from issue #1088 (decoded from base64).
76+
/// Original: agsfXzpfX19fX19nX19fJChbXTBfBQUFBQUfXzpfX19fX19nX18FBQUFBQQFBQUFBQUFBQUFBQUFBQUFBQUFBQU=
77+
#[tokio::test]
78+
async fn fuzz_crash_1088_oom_input() {
79+
use base64::Engine;
80+
let b64 =
81+
"agsfXzpfX19fX19nX19fJChbXTBfBQUFBQUfXzpfX19fX19nX18FBQUFBQQFBQUFBQUFBQUFBQUFBQUFBQUFBQU=";
82+
let decoded = base64::engine::general_purpose::STANDARD
83+
.decode(b64)
84+
.unwrap_or_default();
85+
let input = String::from_utf8_lossy(&decoded);
86+
let script = format!("echo $(({})) 2>/dev/null", input);
87+
88+
let mut bash = Bash::builder()
89+
.limits(
90+
ExecutionLimits::new()
91+
.max_commands(100)
92+
.max_function_depth(10)
93+
.max_subst_depth(5)
94+
.max_stdout_bytes(4096)
95+
.max_stderr_bytes(4096)
96+
.timeout(std::time::Duration::from_millis(500)),
97+
)
98+
.build();
99+
100+
// Must not panic or OOM
101+
let _ = bash.exec(&script).await;
102+
}

specs/006-threat-model.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1228,6 +1228,7 @@ This section maps former vulnerability IDs to the new threat ID scheme and track
12281228
| TM-ISO-024 | `$?` leaks into VFS subprocess | Parent `last_exit_code` visible inside VFS script subprocess, causing false `set -e` failures | Reset `last_exit_code = 0` and `nounset_error = None` in `execute_script_content` subprocess isolation |
12291229
| TM-INT-007 | `/dev/urandom` empty with `head -c` | Weak randomness — `head -c 16 /dev/urandom` returns empty string | Fix virtual device pipe handling in head builtin |
12301230
| TM-DOS-044 | Nested `$()` stack overflow (regression) | Process crash (SIGABRT) at depth ~50 despite #492 fix | Interpreter execution path may need separate depth tracking from lexer fix |
1231+
| TM-DOS-088 | Command substitution OOM via state cloning | OOM at depth N (memory ≈ N × state_size) | Dedicated `max_subst_depth` limit (default 32), separate from `max_function_depth`**FIXED** via #1088 |
12311232

12321233
### Accepted (Low Priority)
12331234

@@ -1322,6 +1323,7 @@ This section maps former vulnerability IDs to the new threat ID scheme and track
13221323
| Brace expansion DoS | TM-DOS-041, TM-DOS-042 | Cap range size and total expansion count | **NEEDED** |
13231324
| Arithmetic overflow in compound assignment | TM-DOS-043 | Use `wrapping_*` ops in `execute_arithmetic_with_side_effects` | **NEEDED** |
13241325
| Lexer stack overflow | TM-DOS-044 | Depth tracking in `read_command_subst_into` | **NEEDED** |
1326+
| Cmd subst OOM via state cloning | TM-DOS-088 | `max_subst_depth` limit in `ExecutionLimits` | **DONE** |
13251327
| OverlayFs symlink limit bypass | TM-DOS-045 | `check_write_limits()` + `validate_path()` in `symlink()` | **NEEDED** |
13261328
| MountableFs path validation gap | TM-DOS-046 | `validate_path()` in all MountableFs methods | **NEEDED** |
13271329
| VFS copy/rename semantic bugs | TM-DOS-047, TM-DOS-048 | Fix limit check in copy(), type check in rename() | **NEEDED** |

0 commit comments

Comments
 (0)