From 25b4a0194353980a3ae0660ab940a7bf32840bb0 Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Wed, 23 Sep 2026 09:37:27 +0000 Subject: [PATCH] fix(hextools): report a missing operand the way real od does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `od /nope` aborted the whole script with `internal error: /nope: io error: file not found`. Real `od` prints `od: /nope: No such file or directory`, exits 1, and lets the rest of the script run. Two Rust enum shapes reached agent-visible stderr on a path any typo hits, and `internal error:` is in `testing::UNIVERSAL_BANNED`, so nightly `glob_fuzz` run 228 went red on `\nod}.\x07` — that input runs `od` with the operand `}`. An unreadable operand is ordinary command failure, so `collect_input` now returns `Err(ExecResult)` (exit 1, the command's own stderr) instead of `Error::Internal`, for `od`, `xxd` and `hexdump` alike. The reason text comes from the errno mapper redirections already used, promoted out of `interpreter::redirection` to `error::io_error_reason` so one helper serves both. Behaviour is unchanged there: a message that only restates its errno becomes the `strerror` text real tools print, and a custom `FileSystem` backend's richer wording (`filesystem is read-only`) is kept verbatim. The pass-through `redirect_error_reason` is gone; its nine call sites call the shared helper directly. Covered by a `glob_fuzz` scaffold replay of the crashing input, a parity test across all three dumpers, a `no_leak_missing_operand` guard over the whole banned list, and four unit tests on the mapper. --- crates/bashkit/src/builtins/hextools.rs | 50 +++++++++-- crates/bashkit/src/error.rs | 82 +++++++++++++++++ crates/bashkit/src/interpreter/mod.rs | 2 +- crates/bashkit/src/interpreter/redirection.rs | 87 ++++--------------- crates/bashkit/src/testing.rs | 2 +- .../integration/glob_fuzz_scaffold_tests.rs | 49 +++++++++++ knowledge/security/threat-model.md | 15 ++++ 7 files changed, 208 insertions(+), 79 deletions(-) diff --git a/crates/bashkit/src/builtins/hextools.rs b/crates/bashkit/src/builtins/hextools.rs index 3a83f711a..c2661710c 100644 --- a/crates/bashkit/src/builtins/hextools.rs +++ b/crates/bashkit/src/builtins/hextools.rs @@ -256,7 +256,10 @@ impl Builtin for Od { Err(e) => return Ok(ExecResult::err(format!("{}\n", e), 1)), }; - let data = collect_input(ctx.stdin, &files, ctx.cwd, &ctx.fs).await?; + let data = match collect_input("od", ctx.stdin, &files, ctx.cwd, &ctx.fs).await { + Ok(data) => data, + Err(failure) => return Ok(failure), + }; let output = od_dump(&data, &opts); Ok(ExecResult::ok(output)) @@ -463,7 +466,10 @@ impl Builtin for Xxd { Err(e) => return Ok(ExecResult::err(format!("{}\n", e), 1)), }; - let data = collect_input(ctx.stdin, &files, ctx.cwd, &ctx.fs).await?; + let data = match collect_input("xxd", ctx.stdin, &files, ctx.cwd, &ctx.fs).await { + Ok(data) => data, + Err(failure) => return Ok(failure), + }; if opts.reverse { let bytes = xxd_reverse(&data, opts.plain); @@ -621,7 +627,10 @@ impl Builtin for Hexdump { Err(e) => return Ok(ExecResult::err(format!("{}\n", e), 1)), }; - let data = collect_input(ctx.stdin, &files, ctx.cwd, &ctx.fs).await?; + let data = match collect_input("hexdump", ctx.stdin, &files, ctx.cwd, &ctx.fs).await { + Ok(data) => data, + Err(failure) => return Ok(failure), + }; let output = hexdump_dump(&data, &opts); Ok(ExecResult::ok(output)) @@ -630,12 +639,23 @@ impl Builtin for Hexdump { // --- Shared helpers --- +/// Read the operand files for `od` / `xxd` / `hexdump`. +/// +/// An unreadable operand is an ordinary command failure, not an interpreter +/// error: real `od` prints `od: FILE: No such file or directory`, exits 1 and +/// lets the rest of the script run. Wrapping the VFS error in +/// `Error::Internal` instead aborted the whole script with +/// `internal error: FILE: io error: file not found` — two Rust enum shapes on +/// a path any typo reaches, which is the TM-INF-022 leak nightly `glob_fuzz` +/// run 228 caught. Hence `Err(ExecResult)`: the failure is the command's own +/// output, so the caller returns it as `Ok`. async fn collect_input( + name: &str, stdin: Option<&crate::StreamData>, files: &[String], cwd: &std::path::Path, fs: &std::sync::Arc, -) -> Result> { +) -> std::result::Result, ExecResult> { let mut data = Vec::new(); if files.is_empty() { @@ -655,10 +675,12 @@ async fn collect_input( vfs_join(cwd, file) }; - let content = fs - .read_file(&path) - .await - .map_err(|e| crate::error::Error::Internal(format!("{}: {}", file, e)))?; + let content = fs.read_file(&path).await.map_err(|e| { + ExecResult::err( + format!("{name}: {file}: {}\n", crate::error::io_error_reason(&e)), + 1, + ) + })?; data.extend_from_slice(&content); } } @@ -1020,4 +1042,16 @@ mod tests { assert_eq!(result.exit_code, 0); assert!(result.stdout.contains("00000002")); // final offset } + + /// TM-INF-022: a missing operand must not put a Rust enum shape on + /// stderr. Guards the whole `UNIVERSAL_BANNED` list, not just the two + /// shapes `Error::Internal(Error::Io)` used to produce, so a future + /// rewording cannot reintroduce a different leak. + #[tokio::test] + async fn no_leak_missing_operand() { + let result = run_od_with_fs(&["/nope"], &[]).await; + crate::testing::assert_no_leak(&result, "od", &[]); + assert_eq!(result.stderr, "od: /nope: No such file or directory\n"); + assert_eq!(result.exit_code, 1); + } } diff --git a/crates/bashkit/src/error.rs b/crates/bashkit/src/error.rs index 943fa85ae..8a06e44a3 100644 --- a/crates/bashkit/src/error.rs +++ b/crates/bashkit/src/error.rs @@ -152,6 +152,48 @@ impl Error { } } +/// VFS messages that only restate their `io::ErrorKind`. +/// +/// These carry nothing the errno does not, so diagnostics swap them for the +/// `strerror` text real tools print. Every other message — +/// `filesystem is read-only`, a custom [`crate::FileSystem`] backend's own +/// wording — tells the caller *why* in a way the bare errno cannot, and is +/// kept verbatim. +const ERRNO_RESTATING_MESSAGES: &[&str] = &["file not found", "parent directory not found"]; + +/// Render a filesystem error the way a real shell tool renders it. +/// +/// Never format a [`Error`] with `Display` into a diagnostic a script can +/// see: `io error: ` and `internal error: ` are Rust enum shapes no shell +/// ever prints, and the fuzz/proptest leak detector in [`crate::testing`] +/// bans them (TM-INF-022). `od /nope` must report +/// `od: /nope: No such file or directory`, not +/// `od: /nope: io error: file not found`. +pub(crate) fn io_error_reason(e: &Error) -> String { + let io = match e { + Error::Io(io) => io, + other => return other.to_string(), + }; + let message = io.to_string(); + // Errors straight from the OS stringify as ` (os error N)`; + // real tools print only the strerror half. + let restates_errno = + io.raw_os_error().is_some() || ERRNO_RESTATING_MESSAGES.contains(&message.as_str()); + if !restates_errno { + return message; + } + match io.kind() { + std::io::ErrorKind::NotFound => "No such file or directory", + std::io::ErrorKind::PermissionDenied => "Permission denied", + std::io::ErrorKind::IsADirectory => "Is a directory", + std::io::ErrorKind::NotADirectory => "Not a directory", + std::io::ErrorKind::AlreadyExists => "File exists", + std::io::ErrorKind::Unsupported => "Operation not supported", + _ => return message, + } + .to_string() +} + /// THREAT[TM-INF-016]: Sanitize error messages to prevent information leakage. /// Strips: /// - Host filesystem paths (anything starting with /) @@ -198,6 +240,46 @@ fn sanitize_error_message(msg: &str) -> String { #[cfg(test)] mod tests { + /// A VFS message that only restates its errno is swapped for the + /// `strerror` text real tools print — `od`, `cat` and redirection all + /// depend on this to avoid printing the `io error: ` enum shape. + #[test] + fn io_error_reason_maps_errno_restating_messages() { + let err = Error::Io(std::io::Error::new( + std::io::ErrorKind::NotFound, + "file not found", + )); + assert_eq!(io_error_reason(&err), "No such file or directory"); + } + + /// A backend reason richer than its errno survives: `read-only` is the + /// only actionable detail and collapsing it to `Permission denied` + /// would drop it. + #[test] + fn io_error_reason_keeps_specific_backend_reason() { + let err = Error::Io(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + "filesystem is read-only", + )); + assert_eq!(io_error_reason(&err), "filesystem is read-only"); + } + + /// An OS error stringifies as ` (os error N)`; only the + /// strerror half is shell-shaped. + #[test] + fn io_error_reason_drops_the_os_error_number() { + let err = Error::Io(std::io::Error::from_raw_os_error(2)); + assert_eq!(io_error_reason(&err), "No such file or directory"); + } + + /// Non-I/O variants are passed through: they have no errno to map, and + /// their own `Display` is what callers already report. + #[test] + fn io_error_reason_passes_through_non_io_variants() { + let err = Error::Execution("boom".into()); + assert_eq!(io_error_reason(&err), "execution error: boom"); + } + use super::*; #[test] diff --git a/crates/bashkit/src/interpreter/mod.rs b/crates/bashkit/src/interpreter/mod.rs index 724e039e5..151d2738c 100644 --- a/crates/bashkit/src/interpreter/mod.rs +++ b/crates/bashkit/src/interpreter/mod.rs @@ -6082,7 +6082,7 @@ impl Interpreter { return Ok(ExecResult::err( format!( "bash: {target_path}: {}\n", - redirection::redirect_error_reason(&e) + crate::error::io_error_reason(&e) ), 1, )); diff --git a/crates/bashkit/src/interpreter/redirection.rs b/crates/bashkit/src/interpreter/redirection.rs index cc3e26df3..ac309b990 100644 --- a/crates/bashkit/src/interpreter/redirection.rs +++ b/crates/bashkit/src/interpreter/redirection.rs @@ -6,7 +6,8 @@ //! by interpreter state fields). //! //! Redirection failures are reported as `bash: : ` via -//! [`redirect_error_reason`] — never as the `Display` of a [`crate::Error`], +//! [`crate::error::io_error_reason`] — never as the `Display` of a +//! [`crate::Error`], //! whose `io error: ` prefix is a Rust enum shape no shell ever prints. //! //! The reason text matters beyond cosmetics: the fuzz/proptest leak detector @@ -18,45 +19,7 @@ //! TM-INF-016 host-path leak instead of an echo. use super::*; - -/// VFS messages that only restate their `io::ErrorKind`. -/// -/// These carry nothing the errno does not, so redirection diagnostics swap -/// them for the `strerror` text real bash prints. Every other message — -/// `filesystem is read-only`, a custom [`crate::FileSystem`] backend's own -/// wording — tells the caller *why* in a way the bare errno cannot, and is -/// kept verbatim. -const ERRNO_RESTATING_MESSAGES: &[&str] = &["file not found", "parent directory not found"]; - -/// Render a filesystem error the way bash renders a redirection failure. -/// -/// `ls < /nope` reports `bash: /nope: No such file or directory`, matching -/// real bash, rather than the Rust error-enum `Display` -/// (`bash: /nope: io error: file not found`). -pub(super) fn redirect_error_reason(e: &crate::error::Error) -> String { - let io = match e { - crate::error::Error::Io(io) => io, - other => return other.to_string(), - }; - let message = io.to_string(); - // Errors straight from the OS stringify as ` (os error N)`; - // bash prints only the strerror half. - let restates_errno = - io.raw_os_error().is_some() || ERRNO_RESTATING_MESSAGES.contains(&message.as_str()); - if !restates_errno { - return message; - } - match io.kind() { - std::io::ErrorKind::NotFound => "No such file or directory", - std::io::ErrorKind::PermissionDenied => "Permission denied", - std::io::ErrorKind::IsADirectory => "Is a directory", - std::io::ErrorKind::NotADirectory => "Not a directory", - std::io::ErrorKind::AlreadyExists => "File exists", - std::io::ErrorKind::Unsupported => "Operation not supported", - _ => return message, - } - .to_string() -} +use crate::error::io_error_reason; impl Interpreter { /// Process input redirections (< file, <<< string) @@ -96,7 +59,7 @@ impl Interpreter { Err(e) => { return Err(crate::error::Error::CommandFailure(format!( "bash: {target_path}: {}\n", - redirect_error_reason(&e) + io_error_reason(&e) ))); } } @@ -193,12 +156,9 @@ impl Interpreter { if let Err(e) = self.fs.write_file(&path, result.stderr.as_bytes()).await { - result.stderr = format!( - "bash: {}: {}\n", - target_path, - redirect_error_reason(&e) - ) - .into(); + result.stderr = + format!("bash: {}: {}\n", target_path, io_error_reason(&e)) + .into(); result.exit_code = 1; return Ok(result); } @@ -209,12 +169,9 @@ impl Interpreter { self.fs.write_file(&path, result.stdout.as_bytes()).await { result.stdout = crate::StreamData::new(); - result.stderr = format!( - "bash: {}: {}\n", - target_path, - redirect_error_reason(&e) - ) - .into(); + result.stderr = + format!("bash: {}: {}\n", target_path, io_error_reason(&e)) + .into(); result.exit_code = 1; return Ok(result); } @@ -237,12 +194,9 @@ impl Interpreter { if let Err(e) = self.fs.append_file(&path, result.stderr.as_bytes()).await { - result.stderr = format!( - "bash: {}: {}\n", - target_path, - redirect_error_reason(&e) - ) - .into(); + result.stderr = + format!("bash: {}: {}\n", target_path, io_error_reason(&e)) + .into(); result.exit_code = 1; return Ok(result); } @@ -253,12 +207,9 @@ impl Interpreter { self.fs.append_file(&path, result.stdout.as_bytes()).await { result.stdout = crate::StreamData::new(); - result.stderr = format!( - "bash: {}: {}\n", - target_path, - redirect_error_reason(&e) - ) - .into(); + result.stderr = + format!("bash: {}: {}\n", target_path, io_error_reason(&e)) + .into(); result.exit_code = 1; return Ok(result); } @@ -278,8 +229,7 @@ impl Interpreter { combined.extend_from_slice(result.stderr.as_bytes()); if let Err(e) = self.fs.write_file(&path, &combined).await { result.stderr = - format!("bash: {}: {}\n", target_path, redirect_error_reason(&e)) - .into(); + format!("bash: {}: {}\n", target_path, io_error_reason(&e)).into(); result.exit_code = 1; return Ok(result); } @@ -499,8 +449,7 @@ impl Interpreter { self.fs.write_file(path, content.as_bytes()).await }; if let Err(e) = write_result { - new_stderr = - format!("bash: {}: {}\n", display_path, redirect_error_reason(&e)).into(); + new_stderr = format!("bash: {}: {}\n", display_path, io_error_reason(&e)).into(); result.exit_code = 1; result.stdout = new_stdout; result.stderr = new_stderr; diff --git a/crates/bashkit/src/testing.rs b/crates/bashkit/src/testing.rs index 2ffccd8bf..1aadcd953 100644 --- a/crates/bashkit/src/testing.rs +++ b/crates/bashkit/src/testing.rs @@ -213,7 +213,7 @@ const SHELL_ERROR_SUFFIXES: &[&str] = &[ ": Permission denied", ": cannot execute: required file not found", ": cannot execute binary file", - // Remaining errno templates that `redirect_error_reason` can emit + // Remaining errno templates that `io_error_reason` can emit // for `bash: : `. ": Not a directory", ": File exists", diff --git a/crates/bashkit/tests/integration/glob_fuzz_scaffold_tests.rs b/crates/bashkit/tests/integration/glob_fuzz_scaffold_tests.rs index e2537d90d..9a0a67906 100644 --- a/crates/bashkit/tests/integration/glob_fuzz_scaffold_tests.rs +++ b/crates/bashkit/tests/integration/glob_fuzz_scaffold_tests.rs @@ -151,3 +151,52 @@ async fn nul_byte_in_word_is_dropped() { let without_nul = bash.exec("ls /tmp/ < /nope").await.unwrap(); assert_eq!(with_nul.stderr.to_string(), without_nul.stderr.to_string()); } + +/// Regression: fuzz run 228 (`crash-9be3ed84f275159780cc08d447fbc341c3e88ea7`, +/// bytes `[10, 111, 100, 125, 46, 7]` = `\nod}.\x07`). +/// +/// `ls /tmp/` + newline + `od}.\x07` runs `od` with the operand `}`, which the +/// VFS has no file for. `od` reported that as `Error::Internal`, so the whole +/// script aborted with `internal error: }: io error: file not found` — two +/// Rust enum shapes on a path any script can reach with a typo. +#[tokio::test] +async fn glob_fuzz_crash_missing_od_operand() { + let input = "\nod}.\u{7}"; + let scripts = [ + format!("ls /tmp/{}", input), + format!( + "case \"test.txt\" in {}) echo match;; *) echo no;; esac", + input + ), + format!("if [[ \"hello.world\" == {} ]]; then echo y; fi", input), + ]; + let mut bash = fuzz_bash(); + for script in &scripts { + fuzz_exec(&mut bash, script, "glob_fuzz_crash_missing_od_operand", &[]).await; + } +} + +/// A missing operand is an ordinary command failure for the hex dumpers, the +/// way it is for real `od`: `od: FILE: No such file or directory`, exit 1, +/// and the rest of the script still runs. Before the fix each of these +/// aborted the script with an `Error::Internal` carrying `io error:`. +#[tokio::test] +async fn hex_dumpers_report_missing_operand_like_real_od() { + for name in ["od", "xxd", "hexdump"] { + let mut bash = fuzz_bash(); + let result = bash + .exec(&format!("{name} /nope; echo after=$?")) + .await + .unwrap_or_else(|e| panic!("{name} aborted the script: {e}")); + assert_eq!( + result.stderr.to_string(), + format!("{name}: /nope: No such file or directory\n"), + "{name} stderr" + ); + assert_eq!( + result.stdout.to_string(), + "after=1\n", + "{name} kept running" + ); + } +} diff --git a/knowledge/security/threat-model.md b/knowledge/security/threat-model.md index 40415bfc7..9562314c9 100644 --- a/knowledge/security/threat-model.md +++ b/knowledge/security/threat-model.md @@ -457,6 +457,21 @@ then generalized via the static + dynamic + fuzz guards in the table. New builti library errors must use Display (`{}`) or a domain formatter, reference shape: `format_compile_errors` in `builtins/jq/errors.rs`. +Display of a **Bashkit** error is not a safe formatter either: `Error`'s own +variants stringify as Rust enum shapes (`io error: `, `internal error: `) that +no shell prints, and `internal error:` is in `UNIVERSAL_BANNED`. Builtins +reporting a filesystem failure must render the reason with +`crate::error::io_error_reason`, which maps the `io::ErrorKind` to the +`strerror` text real tools print (`No such file or directory`) while keeping a +custom `FileSystem` backend's own richer wording (`filesystem is read-only`) +verbatim. Redirection diagnostics share the same helper. + +A related shape: a user-reachable failure classified as `Error::Internal` +aborts the whole script *and* prints `internal error:`. Nightly `glob_fuzz` +run 228 caught `od`/`xxd`/`hexdump` doing this for a missing operand, where +real `od` exits 1 and lets the script continue. An unreadable operand is +ordinary command failure, so it belongs in `ExecResult`, not in `Error`. + The fuzz layer also catches three sister threats with the same machinery (`bashkit::testing::assert_fuzz_invariants`): - **TM-INF-013 regression**: `fuzz_init()` seeds the host OS env with