Skip to content

fix(security): batch 1 — issues #488-#492 #501

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
chaliy merged 2 commits into from
Mar 3, 2026
Merged

fix(security): batch 1 — issues #488-#492 #501

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c50a6f4
fix(security): batch 1 — issues #488-#492
claude Mar 3, 2026
d584121
chore: update ipnet exemption from 2.11.0 to 2.12.0
claude Mar 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions crates/bashkit/src/builtins/export.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
16 changes: 13 additions & 3 deletions crates/bashkit/src/builtins/vars.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -111,10 +111,12 @@ impl Set {
impl Builtin for Set {
async fn execute(&self, ctx: Context<'_>) -> Result<ExecResult> {
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));
}
Expand Down Expand Up @@ -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());
Expand Down
79 changes: 55 additions & 24 deletions crates/bashkit/src/interpreter/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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());
}
}
Expand All @@ -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());
}
}
}
}
Expand All @@ -4592,14 +4612,18 @@ 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());
} else {
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());
}
}
Expand Down Expand Up @@ -5365,11 +5389,14 @@ impl Interpreter {
redirects: &[Redirect],
) -> Result<ExecResult> {
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);
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -7018,19 +7053,20 @@ impl Interpreter {
rhs_val
} else {
let lhs_val = self.expand_variable(var_name).parse::<i64>().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
Expand All @@ -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,
}
};
Expand Down Expand Up @@ -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.
Expand Down
40 changes: 39 additions & 1 deletion crates/bashkit/src/parser/lexer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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<char>,
/// 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,
}
}

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
}
}
_ => {
Expand Down
2 changes: 1 addition & 1 deletion crates/bashkit/src/parser/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
Loading
Loading