From c50a6f4b0664a59fdd39f77adb121a7833d2d6ff Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Mar 2026 02:31:17 +0000 Subject: [PATCH 1/2] =?UTF-8?q?fix(security):=20batch=201=20=E2=80=94=20is?= =?UTF-8?q?sues=20#488-#492?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - #488: Add is_internal_variable() guard to declare/readonly/local/export builtins that previously bypassed the check via direct HashMap insert (TM-INJ-012 to TM-INJ-015) - #489: Add _ARRAY_READ_ and _EVAL_CMD to is_internal_variable() check, preventing arbitrary array creation via marker prefix injection (TM-INJ-016) - #490: Filter internal marker variables from set and declare -p output, preventing information disclosure of nameref/readonly/case markers (TM-INF-017) - #491: Replace native +/-/* operators with wrapping variants in compound assignment paths; clamp shift amounts to 0..63 in <<=/>>=; add i64::MIN/-1 overflow protection for / and % (TM-DOS-043) - #492: Add depth tracking to lexer read_command_subst_into() to prevent stack overflow from deeply nested $() in double-quoted strings (TM-DOS-044) Closes #488, closes #489, closes #490, closes #491, closes #492 --- crates/bashkit/src/builtins/export.rs | 7 +- crates/bashkit/src/builtins/vars.rs | 16 ++++- crates/bashkit/src/interpreter/mod.rs | 79 ++++++++++++++------- crates/bashkit/src/parser/lexer.rs | 40 ++++++++++- crates/bashkit/src/parser/mod.rs | 2 +- crates/bashkit/tests/security_audit_pocs.rs | 11 --- 6 files changed, 113 insertions(+), 42 deletions(-) diff --git a/crates/bashkit/src/builtins/export.rs b/crates/bashkit/src/builtins/export.rs index e0e92849..26e1da1f 100644 --- a/crates/bashkit/src/builtins/export.rs +++ b/crates/bashkit/src/builtins/export.rs @@ -4,7 +4,7 @@ use async_trait::async_trait; use super::{Builtin, Context}; use crate::error::Result; -use crate::interpreter::ExecResult; +use crate::interpreter::{is_internal_variable, ExecResult}; /// Check if a variable name is valid: [a-zA-Z_][a-zA-Z0-9_]* fn is_valid_var_name(name: &str) -> bool { @@ -38,7 +38,10 @@ impl Builtin for Export { 1, )); } - ctx.variables.insert(name.to_string(), value.to_string()); + // THREAT[TM-INJ-015]: Block internal variable prefix injection via export + if !is_internal_variable(name) { + ctx.variables.insert(name.to_string(), value.to_string()); + } } else { // Just marking for export - in our model this is a no-op // unless the variable exists, in which case we keep it diff --git a/crates/bashkit/src/builtins/vars.rs b/crates/bashkit/src/builtins/vars.rs index 5399d4dd..4d08ffa8 100644 --- a/crates/bashkit/src/builtins/vars.rs +++ b/crates/bashkit/src/builtins/vars.rs @@ -6,7 +6,7 @@ use async_trait::async_trait; use super::{Builtin, Context}; use crate::error::Result; -use crate::interpreter::ExecResult; +use crate::interpreter::{is_internal_variable, ExecResult}; /// Check if a variable name is valid: [a-zA-Z_][a-zA-Z0-9_]* fn is_valid_var_name(name: &str) -> bool { @@ -111,10 +111,12 @@ impl Set { impl Builtin for Set { async fn execute(&self, ctx: Context<'_>) -> Result { if ctx.args.is_empty() { - // Display all variables + // Display all variables, filtering internal markers (TM-INF-017) let mut output = String::new(); for (name, value) in ctx.variables.iter() { - output.push_str(&format!("{}={}\n", name, value)); + if !is_internal_variable(name) { + output.push_str(&format!("{}={}\n", name, value)); + } } return Ok(ExecResult::ok(output)); } @@ -261,12 +263,20 @@ impl Builtin for Readonly { if let Some(eq_pos) = arg.find('=') { let name = &arg[..eq_pos]; let value = &arg[eq_pos + 1..]; + // THREAT[TM-INJ-013]: Block internal variable prefix injection via readonly + if is_internal_variable(name) { + continue; + } // Set the variable ctx.variables.insert(name.to_string(), value.to_string()); // Mark as readonly ctx.variables .insert(format!("_READONLY_{}", name), "1".to_string()); } else { + // THREAT[TM-INJ-013]: Block internal variable prefix injection via readonly + if is_internal_variable(arg) { + continue; + } // Just mark existing variable as readonly ctx.variables .insert(format!("_READONLY_{}", arg), "1".to_string()); diff --git a/crates/bashkit/src/interpreter/mod.rs b/crates/bashkit/src/interpreter/mod.rs index 601eb89e..3028db50 100644 --- a/crates/bashkit/src/interpreter/mod.rs +++ b/crates/bashkit/src/interpreter/mod.rs @@ -184,6 +184,19 @@ fn is_dev_null(path: &Path) -> bool { normalized == Path::new(DEV_NULL) } +/// THREAT[TM-INJ-009,TM-INJ-016]: Check if a variable name is an internal marker. +/// Used by builtins and interpreter to block user assignment to internal prefixes. +pub(crate) fn is_internal_variable(name: &str) -> bool { + name.starts_with("_NAMEREF_") + || name.starts_with("_READONLY_") + || name.starts_with("_UPPER_") + || name.starts_with("_LOWER_") + || name.starts_with("_ARRAY_READ_") + || name == "_EVAL_CMD" + || name == "_SHIFT_COUNT" + || name == "_SET_POSITIONAL" +} + /// A frame in the call stack for local variable scoping #[derive(Debug, Clone)] struct CallFrame { @@ -1559,19 +1572,20 @@ impl Interpreter { let rhs_value = self.execute_arithmetic_with_side_effects(effective_rhs); let final_value = if let Some(op) = op { let current = self.evaluate_arithmetic(var_name); + // THREAT[TM-DOS-043]: wrapping to prevent overflow panic match op { - '+' => current + rhs_value, - '-' => current - rhs_value, - '*' => current * rhs_value, + '+' => current.wrapping_add(rhs_value), + '-' => current.wrapping_sub(rhs_value), + '*' => current.wrapping_mul(rhs_value), '/' => { - if rhs_value != 0 { + if rhs_value != 0 && !(current == i64::MIN && rhs_value == -1) { current / rhs_value } else { 0 } } '%' => { - if rhs_value != 0 { + if rhs_value != 0 && !(current == i64::MIN && rhs_value == -1) { current % rhs_value } else { 0 @@ -4566,12 +4580,16 @@ impl Interpreter { ); return self.apply_redirections(result, redirects).await; } + // THREAT[TM-INJ-014]: Block internal variable prefix injection via local + if is_internal_variable(var_name) { + continue; + } if is_nameref { frame.locals.insert(var_name.to_string(), String::new()); } else { frame.locals.insert(var_name.to_string(), value.to_string()); } - } else { + } else if !is_internal_variable(arg) { frame.locals.insert(arg.to_string(), String::new()); } } @@ -4581,8 +4599,10 @@ impl Interpreter { if let Some(eq_pos) = arg.find('=') { let var_name = &arg[..eq_pos]; let value = &arg[eq_pos + 1..]; - self.variables - .insert(format!("_NAMEREF_{}", var_name), value.to_string()); + if !is_internal_variable(var_name) { + self.variables + .insert(format!("_NAMEREF_{}", var_name), value.to_string()); + } } } } @@ -4592,6 +4612,10 @@ impl Interpreter { if let Some(eq_pos) = arg.find('=') { let var_name = &arg[..eq_pos]; let value = &arg[eq_pos + 1..]; + // THREAT[TM-INJ-014]: Block internal variable prefix injection via local + if is_internal_variable(var_name) { + continue; + } if is_nameref { self.variables .insert(format!("_NAMEREF_{}", var_name), value.to_string()); @@ -4599,7 +4623,7 @@ impl Interpreter { self.variables .insert(var_name.to_string(), value.to_string()); } - } else { + } else if !is_internal_variable(arg) { self.variables.insert(arg.to_string(), String::new()); } } @@ -5365,11 +5389,14 @@ impl Interpreter { redirects: &[Redirect], ) -> Result { if args.is_empty() { - // declare with no args: print all variables (like set) + // declare with no args: print all variables, filtering internal markers (TM-INF-017) let mut output = String::new(); let mut entries: Vec<_> = self.variables.iter().collect(); entries.sort_by_key(|(k, _)| (*k).clone()); for (name, value) in entries { + if is_internal_variable(name) { + continue; + } output.push_str(&format!("declare -- {}=\"{}\"\n", name, value)); } let mut result = ExecResult::ok(output); @@ -5421,10 +5448,13 @@ impl Interpreter { if print_mode { let mut output = String::new(); if names.is_empty() { - // Print all variables + // Print all variables, filtering internal markers (TM-INF-017) let mut entries: Vec<_> = self.variables.iter().collect(); entries.sort_by_key(|(k, _)| (*k).clone()); for (name, value) in entries { + if is_internal_variable(name) { + continue; + } output.push_str(&format!("declare -- {}=\"{}\"\n", name, value)); } } else { @@ -5504,6 +5534,11 @@ impl Interpreter { let var_name = &name[..eq_pos]; let value = &name[eq_pos + 1..]; + // THREAT[TM-INJ-012]: Block internal variable prefix injection via declare + if is_internal_variable(var_name) { + continue; + } + // Handle compound array assignment: declare -A m=([k]="v" ...) if (is_assoc || is_array) && value.starts_with('(') && value.ends_with(')') { let inner = &value[1..value.len() - 1]; @@ -7018,19 +7053,20 @@ impl Interpreter { rhs_val } else { let lhs_val = self.expand_variable(var_name).parse::().unwrap_or(0); + // THREAT[TM-DOS-043]: wrapping to prevent overflow panic match op { - "+" => lhs_val + rhs_val, - "-" => lhs_val - rhs_val, - "*" => lhs_val * rhs_val, + "+" => lhs_val.wrapping_add(rhs_val), + "-" => lhs_val.wrapping_sub(rhs_val), + "*" => lhs_val.wrapping_mul(rhs_val), "/" => { - if rhs_val != 0 { + if rhs_val != 0 && !(lhs_val == i64::MIN && rhs_val == -1) { lhs_val / rhs_val } else { 0 } } "%" => { - if rhs_val != 0 { + if rhs_val != 0 && !(lhs_val == i64::MIN && rhs_val == -1) { lhs_val % rhs_val } else { 0 @@ -7039,8 +7075,8 @@ impl Interpreter { "&" => lhs_val & rhs_val, "|" => lhs_val | rhs_val, "^" => lhs_val ^ rhs_val, - "<<" => lhs_val << rhs_val, - ">>" => lhs_val >> rhs_val, + "<<" => lhs_val.wrapping_shl((rhs_val & 63) as u32), + ">>" => lhs_val.wrapping_shr((rhs_val & 63) as u32), _ => rhs_val, } }; @@ -7632,12 +7668,7 @@ impl Interpreter { /// THREAT[TM-INJ-009]: Check if a variable name is an internal marker. fn is_internal_variable(name: &str) -> bool { - name.starts_with("_NAMEREF_") - || name.starts_with("_READONLY_") - || name.starts_with("_UPPER_") - || name.starts_with("_LOWER_") - || name == "_SHIFT_COUNT" - || name == "_SET_POSITIONAL" + is_internal_variable(name) } /// Set a variable, respecting dynamic scoping. diff --git a/crates/bashkit/src/parser/lexer.rs b/crates/bashkit/src/parser/lexer.rs index 8e7a3b74..7327b975 100644 --- a/crates/bashkit/src/parser/lexer.rs +++ b/crates/bashkit/src/parser/lexer.rs @@ -14,6 +14,10 @@ pub struct SpannedToken { pub span: Span, } +/// Maximum nesting depth for command substitution in the lexer. +/// THREAT[TM-DOS-044]: Prevents stack overflow from deeply nested $() patterns. +const DEFAULT_MAX_SUBST_DEPTH: usize = 50; + /// Lexer for bash scripts. pub struct Lexer<'a> { #[allow(dead_code)] // Stored for error reporting in future @@ -24,16 +28,25 @@ pub struct Lexer<'a> { /// Buffer for re-injected characters (e.g., rest-of-line after heredoc delimiter). /// Consumed before `chars`. reinject_buf: VecDeque, + /// Maximum allowed nesting depth for command substitution + max_subst_depth: usize, } impl<'a> Lexer<'a> { /// Create a new lexer for the given input. pub fn new(input: &'a str) -> Self { + Self::with_max_subst_depth(input, DEFAULT_MAX_SUBST_DEPTH) + } + + /// Create a new lexer with a custom max substitution nesting depth. + /// THREAT[TM-DOS-044]: Limits recursion in read_command_subst_into(). + pub fn with_max_subst_depth(input: &'a str, max_depth: usize) -> Self { Self { input, position: Position::new(), chars: input.chars().peekable(), reinject_buf: VecDeque::new(), + max_subst_depth: max_depth, } } @@ -1106,7 +1119,32 @@ impl<'a> Lexer<'a> { /// Read command substitution content after `$(`, handling nested parens and quotes. /// Appends chars to `content` and adds the closing `)`. + /// THREAT[TM-DOS-044]: `subst_depth` tracks nesting to prevent stack overflow. fn read_command_subst_into(&mut self, content: &mut String) { + self.read_command_subst_into_depth(content, 0); + } + + fn read_command_subst_into_depth(&mut self, content: &mut String, subst_depth: usize) { + if subst_depth >= self.max_subst_depth { + // Depth limit exceeded — consume until matching ')' and emit error token + let mut depth = 1; + while let Some(c) = self.peek_char() { + self.advance(); + match c { + '(' => depth += 1, + ')' => { + depth -= 1; + if depth == 0 { + content.push(')'); + return; + } + } + _ => {} + } + } + return; + } + let mut depth = 1; while let Some(c) = self.peek_char() { match c { @@ -1149,7 +1187,7 @@ impl<'a> Lexer<'a> { if self.peek_char() == Some('(') { content.push('('); self.advance(); - self.read_command_subst_into(content); + self.read_command_subst_into_depth(content, subst_depth + 1); } } _ => { diff --git a/crates/bashkit/src/parser/mod.rs b/crates/bashkit/src/parser/mod.rs index 48cd36ad..b28beae2 100644 --- a/crates/bashkit/src/parser/mod.rs +++ b/crates/bashkit/src/parser/mod.rs @@ -81,7 +81,7 @@ impl<'a> Parser<'a> { /// to prevent stack overflow from misconfiguration. Even if the caller passes /// `max_depth = 1_000_000`, the parser will cap it at 500. pub fn with_limits(input: &'a str, max_depth: usize, max_fuel: usize) -> Self { - let mut lexer = Lexer::new(input); + let mut lexer = Lexer::with_max_subst_depth(input, max_depth.min(HARD_MAX_AST_DEPTH)); let spanned = lexer.next_spanned_token(); let (current_token, current_span) = match spanned { Some(st) => (Some(st.token), st.span), diff --git a/crates/bashkit/tests/security_audit_pocs.rs b/crates/bashkit/tests/security_audit_pocs.rs index 09090315..633b2bbf 100644 --- a/crates/bashkit/tests/security_audit_pocs.rs +++ b/crates/bashkit/tests/security_audit_pocs.rs @@ -33,7 +33,6 @@ mod internal_variable_injection { /// TM-INJ-012: `declare` must not create namerefs via _NAMEREF_ prefix. #[tokio::test] - #[ignore = "TM-INJ-012: declare bypasses is_internal_variable()"] async fn security_audit_declare_blocks_nameref_prefix() { let mut bash = Bash::builder().build(); @@ -58,7 +57,6 @@ mod internal_variable_injection { /// TM-INJ-013: `readonly` must not create namerefs via _NAMEREF_ prefix. #[tokio::test] - #[ignore = "TM-INJ-013: readonly bypasses is_internal_variable()"] async fn security_audit_readonly_blocks_nameref_prefix() { let mut bash = Bash::builder().build(); @@ -82,7 +80,6 @@ mod internal_variable_injection { /// TM-INJ-012: `declare` must not inject _UPPER_ case conversion marker. #[tokio::test] - #[ignore = "TM-INJ-012: declare bypasses is_internal_variable()"] async fn security_audit_declare_blocks_upper_prefix() { let mut bash = Bash::builder().build(); @@ -107,7 +104,6 @@ mod internal_variable_injection { /// TM-INJ-012: `declare` must not inject _LOWER_ case conversion marker. #[tokio::test] - #[ignore = "TM-INJ-012: declare bypasses is_internal_variable()"] async fn security_audit_declare_blocks_lower_prefix() { let mut bash = Bash::builder().build(); @@ -131,7 +127,6 @@ mod internal_variable_injection { /// TM-INJ-016: _ARRAY_READ_ prefix must be rejected by is_internal_variable(). #[tokio::test] - #[ignore = "TM-INJ-016: _ARRAY_READ_ not in is_internal_variable()"] async fn security_audit_array_read_prefix_blocked() { let mut bash = Bash::builder().build(); @@ -152,7 +147,6 @@ mod internal_variable_injection { /// TM-INJ-015: `export` must not inject _READONLY_ marker prefix. #[tokio::test] - #[ignore = "TM-INJ-015: export bypasses is_internal_variable()"] async fn security_audit_export_blocks_readonly_prefix() { let mut bash = Bash::builder().build(); @@ -183,7 +177,6 @@ mod internal_variable_injection { /// frame.locals without calling is_internal_variable(). /// The marker ends up in the call frame, which set_variable traverses. #[tokio::test] - #[ignore = "TM-INJ-014: execute_local_builtin bypasses is_internal_variable()"] async fn security_audit_local_blocks_internal_prefixes() { let mut bash = Bash::builder().build(); @@ -222,7 +215,6 @@ mod internal_variable_leak { /// TM-INF-017: `set` must not expose internal _NAMEREF_/_READONLY_ markers. #[tokio::test] - #[ignore = "TM-INF-017: set leaks internal marker variables"] async fn security_audit_set_hides_internal_markers() { let mut bash = Bash::builder().build(); @@ -246,7 +238,6 @@ mod internal_variable_leak { /// TM-INF-017: `declare -p` must not expose internal markers. #[tokio::test] - #[ignore = "TM-INF-017: declare -p leaks internal marker variables"] async fn security_audit_declare_p_hides_internal_markers() { let mut bash = Bash::builder().build(); @@ -283,7 +274,6 @@ mod arithmetic_overflow { /// TM-DOS-043: i64::MAX + 1 in ((x+=1)) must not panic. /// Should use wrapping arithmetic like the non-compound path. #[tokio::test] - #[ignore = "TM-DOS-043: compound += panics on i64 overflow in debug mode"] async fn security_audit_compound_add_no_panic() { let limits = ExecutionLimits::new().timeout(Duration::from_secs(5)); let mut bash = Bash::builder().limits(limits).build(); @@ -300,7 +290,6 @@ mod arithmetic_overflow { /// TM-DOS-043: Compound <<= must clamp shift amount like non-compound path. #[tokio::test] - #[ignore = "TM-DOS-043: compound <<= uses unclamped shift"] async fn security_audit_compound_shift_clamped() { let limits = ExecutionLimits::new().timeout(Duration::from_secs(5)); let mut bash = Bash::builder().limits(limits).build(); From d584121d7049640db78120e817c36db78851b1b0 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Mar 2026 02:38:47 +0000 Subject: [PATCH 2/2] chore: update ipnet exemption from 2.11.0 to 2.12.0 Fixes cargo-vet audit failure for ipnet version bump. --- supply-chain/config.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/supply-chain/config.toml b/supply-chain/config.toml index 9ff1b7e6..2161bd3d 100644 --- a/supply-chain/config.toml +++ b/supply-chain/config.toml @@ -571,7 +571,7 @@ version = "0.5.0" criteria = "safe-to-deploy" [[exemptions.ipnet]] -version = "2.11.0" +version = "2.12.0" criteria = "safe-to-deploy" [[exemptions.iri-string]]