diff --git a/crates/bashkit/src/builtins/export.rs b/crates/bashkit/src/builtins/export.rs index 518bbd50..76de2be3 100644 --- a/crates/bashkit/src/builtins/export.rs +++ b/crates/bashkit/src/builtins/export.rs @@ -27,6 +27,9 @@ impl Builtin for Export { return Ok(ExecResult::ok(output)); } + let mut stderr = String::new(); + let mut exit_code = 0; + for arg in ctx.args { // Handle NAME=VALUE format if let Some(eq_pos) = arg.find('=') { @@ -43,8 +46,11 @@ impl Builtin for Export { if is_internal_variable(name) { continue; } - // THREAT[TM-INJ-021]: Refuse to overwrite readonly variables + // THREAT[TM-INJ-021]: Refuse to overwrite readonly variables and + // surface the error so callers cannot mistake a silent skip for success. if ctx.variables.contains_key(&format!("_READONLY_{}", name)) { + stderr.push_str(&format!("bash: export: {name}: readonly variable\n")); + exit_code = 1; continue; } ctx.variables.insert(name.to_string(), value.to_string()); @@ -54,6 +60,10 @@ impl Builtin for Export { } } - Ok(ExecResult::ok(String::new())) + Ok(ExecResult { + stderr, + exit_code, + ..Default::default() + }) } } diff --git a/crates/bashkit/src/interpreter/mod.rs b/crates/bashkit/src/interpreter/mod.rs index 98eb2a7a..152c270e 100644 --- a/crates/bashkit/src/interpreter/mod.rs +++ b/crates/bashkit/src/interpreter/mod.rs @@ -5911,6 +5911,9 @@ impl Interpreter { } } + let mut stderr = String::new(); + let mut exit_code: i32 = 0; + for arg in &var_args { if unset_function { self.functions.remove(arg.as_str()); @@ -5938,13 +5941,22 @@ impl Interpreter { let resolved = self.resolve_nameref(arg).to_string(); // THREAT[TM-INJ-009]: Block unset of internal marker variables if is_internal_variable(&resolved) { + stderr.push_str(&format!( + "bash: unset: {resolved}: cannot unset: readonly variable\n" + )); + exit_code = 1; continue; } - // THREAT[TM-INJ-019]: Refuse to unset readonly variables + // THREAT[TM-INJ-019]: Refuse to unset readonly variables and surface + // the error so callers cannot mistake a silent skip for success. if self .variables .contains_key(&format!("_READONLY_{}", resolved)) { + stderr.push_str(&format!( + "bash: unset: {resolved}: cannot unset: readonly variable\n" + )); + exit_code = 1; continue; } self.variables.remove(&resolved); @@ -5956,7 +5968,11 @@ impl Interpreter { } } } - let result = ExecResult::ok(String::new()); + let result = ExecResult { + stderr, + exit_code, + ..Default::default() + }; self.apply_redirections(result, redirects).await } @@ -6429,6 +6445,9 @@ impl Interpreter { // Reconstruct compound assignments: declare -A m=([a]="1" [b]="2") let merged_names = merge_compound_assignments(&names); + let mut declare_stderr = String::new(); + let mut declare_exit_code: i32 = 0; + // Set variables for name in &merged_names { if let Some(eq_pos) = name.find('=') { @@ -6440,11 +6459,15 @@ impl Interpreter { continue; } - // THREAT[TM-INJ-020]: Refuse to overwrite readonly variables + // THREAT[TM-INJ-020]: Refuse to overwrite readonly variables and + // surface the error so callers cannot mistake a silent skip for success. if self .variables .contains_key(&format!("_READONLY_{}", var_name)) { + declare_stderr + .push_str(&format!("bash: declare: {var_name}: readonly variable\n")); + declare_exit_code = 1; continue; } @@ -6594,7 +6617,11 @@ impl Interpreter { } } - let mut result = ExecResult::ok(String::new()); + let mut result = ExecResult { + stderr: declare_stderr, + exit_code: declare_exit_code, + ..Default::default() + }; result = self.apply_redirections(result, redirects).await?; Ok(result) } diff --git a/crates/bashkit/tests/blackbox_security_tests.rs b/crates/bashkit/tests/blackbox_security_tests.rs index 8b84d710..744ed49b 100644 --- a/crates/bashkit/tests/blackbox_security_tests.rs +++ b/crates/bashkit/tests/blackbox_security_tests.rs @@ -339,6 +339,122 @@ mod finding_readonly_bypass { ); } + /// TM-INJ-019: unset on a readonly variable reports the error and exits 1. + /// Value preservation is already covered above; this guards against silent skips. + #[tokio::test] + async fn unset_readonly_reports_error_and_exit_1() { + let mut bash = tight_bash(); + let result = bash + .exec( + r#" + readonly LOCKED=v + unset LOCKED + echo "exit=$?" + "#, + ) + .await + .unwrap(); + assert!( + result.stderr.contains("LOCKED") && result.stderr.contains("readonly"), + "expected stderr to mention readonly, got: {:?}", + result.stderr + ); + assert!( + result.stdout.contains("exit=1"), + "expected unset to exit 1, got: {:?}", + result.stdout + ); + } + + /// TM-INJ-020: declare on a readonly variable reports the error and exits 1. + #[tokio::test] + async fn declare_readonly_reports_error_and_exit_1() { + let mut bash = tight_bash(); + let result = bash + .exec( + r#" + readonly LOCKED=original + declare LOCKED=overwritten + echo "exit=$?" + echo "value=$LOCKED" + "#, + ) + .await + .unwrap(); + assert!( + result.stderr.contains("declare") + && result.stderr.contains("LOCKED") + && result.stderr.contains("readonly"), + "expected stderr to mention declare/readonly, got: {:?}", + result.stderr + ); + assert!( + result.stdout.contains("exit=1"), + "expected declare to exit 1, got: {:?}", + result.stdout + ); + assert!( + result.stdout.contains("value=original"), + "value must be preserved, got: {:?}", + result.stdout + ); + } + + /// TM-INJ-021: export on a readonly variable reports the error and exits 1. + #[tokio::test] + async fn export_readonly_reports_error_and_exit_1() { + let mut bash = tight_bash(); + let result = bash + .exec( + r#" + readonly LOCKED=original + export LOCKED=overwritten + echo "exit=$?" + echo "value=$LOCKED" + "#, + ) + .await + .unwrap(); + assert!( + result.stderr.contains("export") + && result.stderr.contains("LOCKED") + && result.stderr.contains("readonly"), + "expected stderr to mention export/readonly, got: {:?}", + result.stderr + ); + assert!( + result.stdout.contains("exit=1"), + "expected export to exit 1, got: {:?}", + result.stdout + ); + assert!( + result.stdout.contains("value=original"), + "value must be preserved, got: {:?}", + result.stdout + ); + } + + /// declare/export must keep processing remaining args even after a readonly hit. + #[tokio::test] + async fn declare_continues_after_readonly_error() { + let mut bash = tight_bash(); + let result = bash + .exec( + r#" + readonly LOCKED=original + declare LOCKED=skip OTHER=ok + echo "exit=$?" + echo "locked=$LOCKED" + echo "other=$OTHER" + "#, + ) + .await + .unwrap(); + assert!(result.stdout.contains("locked=original")); + assert!(result.stdout.contains("other=ok")); + assert!(result.stdout.contains("exit=1")); + } + /// Non-finding: readonly via local in function is bash-compatible shadowing. #[tokio::test] async fn local_shadows_readonly_in_function() { diff --git a/specs/threat-model.md b/specs/threat-model.md index f40f12f7..077f7f1f 100644 --- a/specs/threat-model.md +++ b/specs/threat-model.md @@ -536,9 +536,9 @@ Bash::builder() | TM-INJ-011 | Cyclic nameref silent resolution | Cyclic namerefs (a→b→a) silently resolve after 10 iterations instead of erroring | — | **OPEN** | | TM-INJ-018 | `dotenv` internal variable prefix injection | `.env` file with `_NAMEREF_x=target` sets internal interpreter variables via `ctx.variables.insert()` | — | **OPEN** | -| TM-INJ-019 | `unset` removes readonly variables | `readonly X=v; unset X` removes the variable despite readonly attribute | — | **OPEN** | -| TM-INJ-020 | `declare` overwrites readonly variables | `readonly X=v; declare X=new` overwrites without error | — | **OPEN** | -| TM-INJ-021 | `export` overwrites readonly variables | `readonly X=v; export X=new` overwrites without error | — | **OPEN** | +| TM-INJ-019 | `unset` removes readonly variables | `readonly X=v; unset X` removes the variable despite readonly attribute | `execute_unset_builtin` and `Unset` builtin both check `_READONLY_` markers, emit `bash: unset: : cannot unset: readonly variable`, and return exit 1 | **MITIGATED** | +| TM-INJ-020 | `declare` overwrites readonly variables | `readonly X=v; declare X=new` overwrites without error | `declare` assignment path checks `_READONLY_`, emits `bash: declare: : readonly variable`, returns exit 1 | **MITIGATED** | +| TM-INJ-021 | `export` overwrites readonly variables | `readonly X=v; export X=new` overwrites without error | `export NAME=VALUE` checks `_READONLY_`, emits `bash: export: : readonly variable`, returns exit 1 | **MITIGATED** | **TM-INJ-011**: `interpreter/mod.rs:7547-7560` — cyclic namerefs silently resolve to whatever variable is current after 10 iterations. Real bash errors with `circular name reference`. Can @@ -1299,9 +1299,9 @@ This section maps former vulnerability IDs to the new threat ID scheme and track | TM-DOS-056 | `source` self-recursion stack overflow | Process crash (SIGABRT) via script that sources itself | Track source depth like function depth; apply `max_function_depth` limit | | TM-DOS-057 | `sleep` bypasses execution timeout | CPU/time exhaustion; `sleep`, subshell sleep, pipeline sleep, background sleep, `timeout` builtin all ignore `ExecutionLimits::timeout` | Implement timeout as tokio::time::timeout wrapper around exec(), not cooperative check | | TM-DOS-058 | Single-builtin unbounded output | OOM via `seq 1 1000000` producing 1M lines despite command limit of 50 | Add `max_stdout_bytes` / `max_stderr_bytes` to ExecutionLimits (see #648) | -| TM-INJ-019 | `unset` removes readonly variables | Integrity bypass — readonly protection defeated | Check readonly attribute in unset before removal | -| TM-INJ-020 | `declare` overwrites readonly variables | Integrity bypass — `declare X=new` overwrites `readonly X=old` | Check readonly attribute in declare assignment path | -| TM-INJ-021 | `export` overwrites readonly variables | Integrity bypass — `export X=new` overwrites `readonly X=old` | Check readonly attribute in export assignment path | +| ~~TM-INJ-019~~ | ~~`unset` removes readonly variables~~ | ~~Integrity bypass — readonly protection defeated~~ | ~~Check readonly attribute in unset before removal~~ (**FIXED**) | +| ~~TM-INJ-020~~ | ~~`declare` overwrites readonly variables~~ | ~~Integrity bypass — `declare X=new` overwrites `readonly X=old`~~ | ~~Check readonly attribute in declare assignment path~~ (**FIXED**) | +| ~~TM-INJ-021~~ | ~~`export` overwrites readonly variables~~ | ~~Integrity bypass — `export X=new` overwrites `readonly X=old`~~ | ~~Check readonly attribute in export assignment path~~ (**FIXED**) | | TM-ISO-021 | EXIT trap leaks across `exec()` calls | Cross-invocation interference — trap from exec N fires in exec N+1 | Reset traps in `reset_for_execution()` | | TM-ISO-022 | `$?` leaks across `exec()` calls | State pollution — exit code from previous exec visible to next exec | Reset `last_exit_code` in `reset_for_execution()` | | TM-ISO-023 | `set -e` leaks across `exec()` calls | Unexpected abort — `set` options from previous exec affect next exec | `SET_OPTION_VARS` cleared in `reset_transient_state()` (**FIXED**) |