From eaad2281247d46a9aa6bbb58067052837f68db0b Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Fri, 9 Oct 2026 16:18:29 +0000 Subject: [PATCH] fix(sed): fall back to direct write when hidden temp file is refused sed -i and yq -i stage through a .bashkit--.tmp sibling. Embedder policies that refuse dot paths (e.g. a read/write workspace policy) made sed exit 4 with the file unchanged. When the temporary cannot be checked or written, replace the target with one all-or-nothing write_file and restore its mode. Chmod/rename failures still fail with the source untouched. --- crates/bashkit/src/builtins/atomic_write.rs | 50 +++++- crates/bashkit/tests/integration/main.rs | 1 + .../integration/sed_inplace_policy_tests.rs | 170 ++++++++++++++++++ knowledge/security/threat-model.md | 2 +- 4 files changed, 213 insertions(+), 10 deletions(-) create mode 100644 crates/bashkit/tests/integration/sed_inplace_policy_tests.rs diff --git a/crates/bashkit/src/builtins/atomic_write.rs b/crates/bashkit/src/builtins/atomic_write.rs index 591085860..d21149fc5 100644 --- a/crates/bashkit/src/builtins/atomic_write.rs +++ b/crates/bashkit/src/builtins/atomic_write.rs @@ -8,6 +8,16 @@ //! finding E): the previous `sed -i` wrote straight through //! `FileSystem::write_file`, which recreates the entry with mode 0644 and loses //! the original content if the write fails part-way. +//! +//! Decision: when the hidden sibling temporary cannot be created or written +//! (a filesystem or embedder policy that refuses dot paths, a file-count +//! limit with no room for one extra entry), fall back to a direct +//! `write_file` of the target and restore its mode. Every in-tree backend +//! makes `write_file` all-or-nothing (TM-FS-014 for `RealFs`, in-memory +//! replacement elsewhere), so the fallback still never leaves a half-written +//! target; it only gives up the rename step. Chmod and rename failures do not +//! fall back: by then the backend has accepted the temporary, so the failure +//! is real and is reported with the source untouched. use std::path::Path; @@ -44,19 +54,20 @@ pub(crate) async fn atomic_replace( .map(|byte| format!("{byte:02x}")) .collect::(); let candidate = vfs_join(parent, format!(".bashkit-{tool}-{suffix}.tmp")); - if !fs - .exists(&candidate) - .await - .map_err(|error| error.to_string())? - { - temp = Some(candidate); - break; + match fs.exists(&candidate).await { + Ok(false) => { + temp = Some(candidate); + break; + } + Ok(true) => {} + // The backend refuses to even look at the hidden path. + Err(_) => return direct_replace(fs, target, content).await, } } let temp = temp.ok_or_else(|| "cannot allocate temporary file".to_string())?; - if let Err(error) = fs.write_file(&temp, content).await { + if fs.write_file(&temp, content).await.is_err() { let _ = fs.remove(&temp, false).await; - return Err(format!("cannot write temporary file: {error}")); + return direct_replace(fs, target, content).await; } #[cfg(feature = "failpoints")] if injected_failure(failpoints.chmod, "error") { @@ -86,6 +97,27 @@ pub(crate) async fn atomic_replace( Ok(()) } +/// Fallback when the hidden temporary is refused: replace the target with one +/// `write_file` (all-or-nothing on every in-tree backend) and restore its mode. +async fn direct_replace( + fs: &dyn crate::fs::FileSystem, + target: &Path, + content: &[u8], +) -> std::result::Result<(), String> { + let mode = fs.stat(target).await.ok().map(|metadata| metadata.mode); + fs.write_file(target, content) + .await + .map_err(|error| format!("cannot write '{}': {error}", target.display()))?; + if let Some(mode) = mode + && fs.stat(target).await.map(|m| m.mode).ok() != Some(mode) + { + fs.chmod(target, mode) + .await + .map_err(|error| format!("cannot preserve file mode: {error}"))?; + } + Ok(()) +} + #[cfg(feature = "failpoints")] fn injected_failure(name: &str, expected: &str) -> bool { fail::eval(name, |action| action.as_deref() == Some(expected)).unwrap_or(false) diff --git a/crates/bashkit/tests/integration/main.rs b/crates/bashkit/tests/integration/main.rs index 26110486b..9640c9f0f 100644 --- a/crates/bashkit/tests/integration/main.rs +++ b/crates/bashkit/tests/integration/main.rs @@ -129,6 +129,7 @@ pub mod script_analysis; pub mod script_execution_tests; pub mod security_audit_pocs; mod sed_fuzz_scaffold_tests; +mod sed_inplace_policy_tests; mod sed_resource_tests; pub mod set_e_and_or_tests; pub mod shlvl_tests; diff --git a/crates/bashkit/tests/integration/sed_inplace_policy_tests.rs b/crates/bashkit/tests/integration/sed_inplace_policy_tests.rs new file mode 100644 index 000000000..188e26ac7 --- /dev/null +++ b/crates/bashkit/tests/integration/sed_inplace_policy_tests.rs @@ -0,0 +1,170 @@ +//! `sed -i` / `yq -i` under a filesystem that refuses dot-path writes. +//! +//! Embedders (e.g. a read/write workspace policy) may deny any path segment +//! starting with `.`. The atomic-replace helper stages a hidden +//! `.bashkit--.tmp` sibling; when that is refused it must fall back +//! to a direct write of the target instead of failing with the file unchanged. + +use bashkit::{ + Bash, DirEntry, FileSystem, FileSystemExt, InMemoryFs, Metadata, Result, async_trait, +}; +use std::io::{Error as IoError, ErrorKind}; +use std::path::{Path, PathBuf}; +use std::sync::Arc; +use std::time::SystemTime; + +/// Refuses every operation whose final path component starts with `.`. +struct NoDotFs { + inner: Arc, +} + +fn guard(path: &Path) -> Result<()> { + let hidden = path + .file_name() + .and_then(|n| n.to_str()) + .is_some_and(|n| n.starts_with('.')); + if hidden { + return Err(IoError::new(ErrorKind::PermissionDenied, "dot paths are not allowed").into()); + } + Ok(()) +} + +#[async_trait] +impl FileSystemExt for NoDotFs {} + +#[async_trait] +impl FileSystem for NoDotFs { + async fn read_file(&self, path: &Path) -> Result> { + guard(path)?; + self.inner.read_file(path).await + } + async fn write_file(&self, path: &Path, content: &[u8]) -> Result<()> { + guard(path)?; + self.inner.write_file(path, content).await + } + async fn append_file(&self, path: &Path, content: &[u8]) -> Result<()> { + guard(path)?; + self.inner.append_file(path, content).await + } + async fn mkdir(&self, path: &Path, recursive: bool) -> Result<()> { + guard(path)?; + self.inner.mkdir(path, recursive).await + } + async fn remove(&self, path: &Path, recursive: bool) -> Result<()> { + guard(path)?; + self.inner.remove(path, recursive).await + } + async fn stat(&self, path: &Path) -> Result { + guard(path)?; + self.inner.stat(path).await + } + async fn read_dir(&self, path: &Path) -> Result> { + self.inner.read_dir(path).await + } + async fn exists(&self, path: &Path) -> Result { + self.inner.exists(path).await + } + async fn rename(&self, from: &Path, to: &Path) -> Result<()> { + guard(from)?; + guard(to)?; + self.inner.rename(from, to).await + } + async fn copy(&self, from: &Path, to: &Path) -> Result<()> { + guard(from)?; + guard(to)?; + self.inner.copy(from, to).await + } + async fn symlink(&self, target: &Path, link: &Path) -> Result<()> { + guard(link)?; + self.inner.symlink(target, link).await + } + async fn read_link(&self, path: &Path) -> Result { + guard(path)?; + self.inner.read_link(path).await + } + async fn chmod(&self, path: &Path, mode: u32) -> Result<()> { + guard(path)?; + self.inner.chmod(path, mode).await + } + async fn set_modified_time(&self, path: &Path, time: SystemTime) -> Result<()> { + guard(path)?; + self.inner.set_modified_time(path, time).await + } +} + +async fn setup(path: &str, content: &[u8], mode: u32) -> (Arc, Bash) { + let inner = Arc::new(InMemoryFs::new()); + inner.mkdir(Path::new("/workspace"), true).await.unwrap(); + inner.write_file(Path::new(path), content).await.unwrap(); + inner.chmod(Path::new(path), mode).await.unwrap(); + let fs = Arc::new(NoDotFs { + inner: inner.clone(), + }); + let bash = Bash::builder() + .fs(fs as Arc) + .cwd("/workspace") + .build(); + (inner, bash) +} + +async fn no_temp_left(inner: &InMemoryFs) -> bool { + inner + .read_dir(Path::new("/workspace")) + .await + .unwrap() + .iter() + .all(|e| !e.name.starts_with(".bashkit-")) +} + +#[tokio::test] +async fn sed_in_place_falls_back_when_dot_paths_are_refused() { + let (inner, mut bash) = setup("/workspace/notes.txt", b"version = 1\n", 0o640).await; + let r = bash + .exec("sed -i 's/1/2/' notes.txt && cat notes.txt") + .await + .unwrap(); + assert_eq!(r.exit_code, 0, "stderr: {}", r.stderr); + assert_eq!(r.stdout, "version = 2\n"); + let path = Path::new("/workspace/notes.txt"); + assert_eq!(inner.stat(path).await.unwrap().mode, 0o640); + assert!(no_temp_left(&inner).await); +} + +#[tokio::test] +async fn sed_in_place_with_backup_falls_back_when_dot_paths_are_refused() { + let (inner, mut bash) = setup("/workspace/a.txt", b"x\n", 0o644).await; + let r = bash.exec("sed -i.bak 's/x/y/' a.txt").await.unwrap(); + assert_eq!(r.exit_code, 0, "stderr: {}", r.stderr); + let read = |p: &'static str| { + let inner = inner.clone(); + async move { inner.read_file(Path::new(p)).await.unwrap() } + }; + assert_eq!(read("/workspace/a.txt").await, b"y\n"); + assert_eq!(read("/workspace/a.txt.bak").await, b"x\n"); +} + +#[tokio::test] +async fn sed_in_place_on_refused_target_still_fails_cleanly() { + // The fallback must not bypass the policy for the target itself. + let (inner, mut bash) = setup("/workspace/.env", b"K=1\n", 0o600).await; + let r = bash.exec("sed -i 's/1/2/' .env").await.unwrap(); + assert_ne!(r.exit_code, 0); + assert_eq!( + inner.read_file(Path::new("/workspace/.env")).await.unwrap(), + b"K=1\n" + ); +} + +#[cfg(feature = "jq")] +#[tokio::test] +async fn yq_in_place_falls_back_when_dot_paths_are_refused() { + let (inner, mut bash) = setup("/workspace/c.yml", b"name: old\n", 0o644).await; + let r = bash.exec("yq -i '.name = \"new\"' c.yml").await.unwrap(); + assert_eq!(r.exit_code, 0, "stderr: {}", r.stderr); + let out = inner + .read_file(Path::new("/workspace/c.yml")) + .await + .unwrap(); + assert!(String::from_utf8_lossy(&out).contains("new")); + assert!(no_temp_left(&inner).await); +} diff --git a/knowledge/security/threat-model.md b/knowledge/security/threat-model.md index 8fa859a6f..3a9baa4cd 100644 --- a/knowledge/security/threat-model.md +++ b/knowledge/security/threat-model.md @@ -346,7 +346,7 @@ panicked. Resolved with `wrapping_*` ops, masked shift amounts, clamped exponent | TM-FS-013 | Permissive RealFs mount default | `mount_real_readonly_at("/", …)` exposes whole host without `allowed_mount_paths` | Allowlist-first: `/`, `/etc`, `/root`, `/Users`, `/home`, `/dev`, `/proc`, `/sys`, `/run`, `/var/run`, `/boot`, `/private`, Windows drive/share-qualified `Users` trees, and any path component matching `.ssh`, `.aws`, `.kube`, `.docker`, `.gnupg`, `.gcloud` are refused unless explicitly allowlisted. Secret components match case-insensitively on Windows. The C ABI enforces the same denylist (`bashkit::is_sensitive_mount_path`) for config-time and runtime mounts, where a sensitive root additionally requires an exact allowlist entry — a broad parent entry (e.g. the home directory) is not consent to expose `.ssh` etc. | **MITIGATED** | | TM-FS-014 | Partial filesystem mutation | Failed write/copy or copy-delete move corrupts/replaces a destination, duplicates a source, or consumes retained quota | `FileSystem` failure-atomicity contract; locked in-memory rename; MountableFs restores cross-mount destinations while NamespaceFs rejects cross-mount rename; RealFs stages and flushes sibling files before rename; failpoint and conformance regressions | **MITIGATED** | | TM-FS-015 | Partial archive extraction | A late traversal, malformed header, or size failure leaves earlier attacker-controlled files behind | Tar validates the complete archive and per-file limits before its first VFS mutation; conformance regression uses a valid entry followed by traversal | **MITIGATED** | -| TM-FS-016 | yq in-place partial or destructive update | Parse, evaluation, serialization, or write failure truncates the source; predictable temporary names permit collisions | Complete evaluation and bounded serialization first; write a random sibling temporary file, preserve mode, and rename only after success; failpoint regressions cover allocation, all backend-write classes, chmod, rename, original-byte retention, and temporary cleanup | **MITIGATED** | +| TM-FS-016 | yq in-place partial or destructive update | Parse, evaluation, serialization, or write failure truncates the source; predictable temporary names permit collisions | Complete evaluation and bounded serialization first; write a random sibling temporary file, preserve mode, and rename only after success; if the backend refuses the hidden temporary (dot-path policies, file-count limits), fall back to one all-or-nothing `write_file` of the target plus mode restore (`sed_inplace_policy_tests`); failpoint regressions cover allocation, all backend-write classes, chmod, rename, original-byte retention, and temporary cleanup | **MITIGATED** | | TM-FS-017 | JS host filesystem widens the sandbox to embedder storage | `new Bash({ fs })` in the wasm bindings routes every VFS operation into embedder-supplied JS, so a script reaches whatever that object exposes | Paths are normalized by `PosixFs` before any host call, and `read_file` rejects host-reported symlinks before delegating to a potentially symlink-following host read. The host object remains the security boundary and is the embedder's to scope (mount root, allowlist, read-only). Reads and writes bypass the in-memory quotas, see the FS-quota scope note in §1 | **ACCEPTED** (embedder-scoped, opt-in) | **Current Risk**: MEDIUM - Two open escape vectors (TM-ESC-012, TM-ESC-013) need remediation