diff --git a/crates/bashkit/src/builtins/jq/mod.rs b/crates/bashkit/src/builtins/jq/mod.rs index c6143ea6..70bedf79 100644 --- a/crates/bashkit/src/builtins/jq/mod.rs +++ b/crates/bashkit/src/builtins/jq/mod.rs @@ -214,10 +214,30 @@ async fn run_jq(ctx: Context<'_>, parsed: JqArgs<'_>) -> Result { .into_iter() .map(|(name, arity, run)| (name, arity, jaq_core::Native::::new(run))) .collect(); + // SECURITY (TM-INF-023, #1571): replace the upstream `halt` native. + // jaq-std's impl calls `std::process::exit(...)`, which would tear + // down the entire embedding process — a sandbox escape via DoS for + // any caller hosting bashkit. Strip it from the `funs` chain and add + // a safe stub so the wrapper defs in jaq-std's `defs.jq` + // (`def halt: halt(0);`, `def halt_error(...): ..., halt(...);`) + // still resolve, but produce a normal jq runtime error instead of + // killing the host. + let safe_halt_run: jaq_core::RunPtr = |mut cv| { + let _ = cv.0.pop_var(); + jaq_core::native::bome(Err(jaq_core::Error::str( + "halt is disabled in the bashkit sandbox", + ))) + }; + let safe_halt: jaq_core::native::Fun = ( + "halt", + jaq_core::native::v(1), + jaq_core::Native::::new(safe_halt_run), + ); let native_funs = jaq_core::funs::() .chain(jaq_std::funs::().filter(|(name, _, _)| { - *name != "env" && !regex_compat::SHADOWED_NATIVE_NAMES.contains(name) + *name != "env" && *name != "halt" && !regex_compat::SHADOWED_NATIVE_NAMES.contains(name) })) + .chain(std::iter::once(safe_halt)) .chain(input_funs) .chain(regex_funs) .chain(jaq_json::funs::()); diff --git a/crates/bashkit/src/builtins/jq/tests.rs b/crates/bashkit/src/builtins/jq/tests.rs index 03ffa2a2..d2f7bf91 100644 --- a/crates/bashkit/src/builtins/jq/tests.rs +++ b/crates/bashkit/src/builtins/jq/tests.rs @@ -1237,6 +1237,52 @@ async fn double_dash_separator() { assert_eq!(result.trim(), "2"); } +// ========================================================================= +// Security: jq filters must not terminate the host process (#1571) +// ========================================================================= + +#[tokio::test] +async fn halt_does_not_terminate_host_process() { + // Regression for #1571: the upstream `halt` native calls + // `std::process::exit(...)`, which would tear down the embedding + // process. We strip it from the funs chain; calling `halt` therefore + // surfaces as an ordinary jq compile/runtime error rather than process + // termination. + // + // The test reaching the assertion at all proves the host wasn't killed. + let result = run_jq_result("halt", r#"{}"#).await.unwrap(); + assert_ne!( + result.exit_code, 0, + "halt should fail safely; stdout=<{}> stderr=<{}>", + result.stdout, result.stderr + ); +} + +#[tokio::test] +async fn halt_with_arg_does_not_terminate_host_process() { + // Regression for #1571: the explicit-arity form `halt(N)` must also + // be neutered, not just the wrapper-def `halt`. + let result = run_jq_result("halt(7)", r#"{}"#).await.unwrap(); + assert_ne!( + result.exit_code, 0, + "halt(7) should fail safely; stdout=<{}> stderr=<{}>", + result.stdout, result.stderr + ); +} + +#[tokio::test] +async fn halt_error_does_not_terminate_host_process() { + // Regression for #1571: `halt_error` is a jq-syntax def in jaq-std + // that ultimately calls `halt(...)`. Stripping the native makes the + // whole family fail closed. + let result = run_jq_result("halt_error", r#""boom""#).await.unwrap(); + assert_ne!( + result.exit_code, 0, + "halt_error should fail safely; stdout=<{}> stderr=<{}>", + result.stdout, result.stderr + ); +} + // ========================================================================= // Differential tests vs real jq binary (when present in $PATH) // ========================================================================= diff --git a/specs/threat-model.md b/specs/threat-model.md index bdf381fc..16724ee4 100644 --- a/specs/threat-model.md +++ b/specs/threat-model.md @@ -437,6 +437,7 @@ All execution stays within the virtual interpreter — no OS subprocess is spawn | TM-INF-020 | `template` exposes env vars via `{{var}}` | Template builtin looks up variables from env as fallback after shell vars and JSON data | Same as TM-INF-001 (caller controls env) | **CALLER RISK** | | TM-INF-021 | Stack backtrace information disclosure | Panics leak internal source paths, dependency versions, and function names via stderr | Custom panic hook suppresses backtraces in CLI | **MITIGATED** | | TM-INF-022 | Library Debug shapes leak via stderr | Builtins wrapping external libs (jaq, regex, serde_json, semver, chrono, …) format errors with `format!("{:?}", e)` and dump internal struct shapes into stderr — e.g. jq printed `(File { code: "<800 chars of prepended compat-defs source>", path: () }, [("@tsv", Filter(0))])` to LLM agents | Three-layer enforcement, all run by `cargo test`: (1) static — `builtins::tests::no_debug_fmt_in_builtin_source` walks every `crates/bashkit/src/builtins/*.rs` and forbids `{:?}` / `{:#?}` / `{name:?}` (per-line opt-out: `// debug-ok: `); (2) dynamic per-tool — each tool's `mod tests` calls `bashkit::testing::assert_no_leak` with malformed inputs; (3) fuzz — every cargo-fuzz target plus 5 proptest cases call `bashkit::testing::assert_fuzz_invariants` against random input and check the same invariants on stderr/stdout | **FIXED** | +| TM-INF-023 | jq `halt` / `halt_error` terminate the host process | jaq-std 3.0's `halt(N)` native calls `std::process::exit(exit_code)`. `defs.jq` wraps it with `def halt: halt(0);` and `def halt_error(...): ..., halt(...);`. Untrusted jq filters can invoke `halt` / `halt_error` to exit the entire embedding process, escaping the `ExecResult`-based sandbox boundary and causing process-wide DoS for any caller hosting bashkit. | Strip the upstream `halt` native from `jaq_std::funs::()` and chain in a safe replacement that pops the variable argument and returns `Error::str("halt is disabled in the bashkit sandbox")`. The wrapper defs (`halt`, `halt_error`) still resolve, so callers see a normal jq runtime error (exit 5) instead of the host being killed. Regression tests in `builtins::jq::tests`: `halt_does_not_terminate_host_process`, `halt_with_arg_does_not_terminate_host_process`, `halt_error_does_not_terminate_host_process`. | **FIXED** | **TM-INF-022**: Generalizes TM-INF-016 to cover the whole builtin surface. The originating bug was `builtins/jq/` formatting jaq's compile/parse