From c4757fb62cf3ba02783a77d3ff00d97a414c2657 Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Thu, 7 May 2026 04:22:01 +0000 Subject: [PATCH] fix(snapshot): make vfs_restore fail closed and apply atomically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, restore_snapshot_inner restored shell state first and then called fs.vfs_restore(...) ignoring its boolean return. The FileSystemExt contract returned `false` for unsupported restore, and InMemoryFs::restore returned early without clearing entries on validation failure. A forged unkeyed snapshot whose VFS section failed path validation would therefore return Ok after mutating the shell state, while leaving previous-tenant files readable in the VFS — a half-restored, cross-tenant leak. - Change `FileSystemExt::vfs_restore` to return `crate::Result<()>`. The default impl returns Err(Unsupported); InMemoryFs/OverlayFs/ MountableFs propagate the inner restore result. - Change `InMemoryFs::restore` to return `Result<()>` with the underlying limit error and never clear on validation failure. - Reorder restore_snapshot_inner: validate shell limits, then apply VFS (which can fail), then apply shell state (cannot fail past validation). Mutation order guarantees no half-restored state. Add an integration regression test (snapshot_restore_fails_closed_on_malformed_vfs) that forges a poisoned VFS path inside an otherwise valid snapshot and verifies: - restore_snapshot returns Err - shell variable values are unchanged - previous-tenant VFS contents remain intact Closes #1576 --- crates/bashkit/src/fs/memory.rs | 42 +++++++++------ crates/bashkit/src/fs/mountable.rs | 2 +- crates/bashkit/src/fs/overlay.rs | 5 +- crates/bashkit/src/fs/traits.rs | 12 +++-- crates/bashkit/src/snapshot.rs | 9 +++- crates/bashkit/tests/snapshot_tests.rs | 72 +++++++++++++++++++++++--- 6 files changed, 110 insertions(+), 32 deletions(-) diff --git a/crates/bashkit/src/fs/memory.rs b/crates/bashkit/src/fs/memory.rs index 32ed9660..ca5fb164 100644 --- a/crates/bashkit/src/fs/memory.rs +++ b/crates/bashkit/src/fs/memory.rs @@ -690,7 +690,7 @@ impl InMemoryFs { /// fs.write_file(Path::new("/tmp/new.txt"), b"new file").await?; /// /// // Restore - /// fs.restore(&snapshot); + /// fs.restore(&snapshot)?; /// /// // Original state is back /// let content = fs.read_file(Path::new("/tmp/data.txt")).await?; @@ -701,32 +701,39 @@ impl InMemoryFs { /// # Ok(()) /// # } /// ``` + /// + /// # Errors + /// + /// Returns an error if any snapshot entry violates the configured VFS + /// limits (path validation, file size, total bytes, or file count). On + /// error the existing filesystem contents are left untouched (issue + /// #1576) so callers can refuse the restore atomically. // THREAT[TM-ESC-012]: Enforce VFS limits to prevent bypass via restore() - pub fn restore(&self, snapshot: &VfsSnapshot) { + pub fn restore(&self, snapshot: &VfsSnapshot) -> crate::error::Result<()> { // Validate ALL snapshot entries before clearing existing state. // If any validation fails, return early WITHOUT clearing. let mut total_bytes = 0u64; let mut file_count = 0u64; for entry in &snapshot.entries { - if self.limits.validate_path(&entry.path).is_err() { - return; - } + self.limits + .validate_path(&entry.path) + .map_err(|e| IoError::other(e.to_string()))?; if let VfsEntryKind::File { content } = &entry.kind { - if self.limits.check_file_size(content.len() as u64).is_err() { - return; - } + self.limits + .check_file_size(content.len() as u64) + .map_err(|e| IoError::other(e.to_string()))?; total_bytes += content.len() as u64; file_count += 1; } } if total_bytes > self.limits.max_total_bytes { - return; - } - if self.limits.check_file_count(file_count).is_err() { - return; + return Err(IoError::other("snapshot total bytes exceed limit").into()); } + self.limits + .check_file_count(file_count) + .map_err(|e| IoError::other(e.to_string()))?; let mut entries = self.entries.write().unwrap(); entries.clear(); @@ -796,6 +803,8 @@ impl InMemoryFs { } } } + + Ok(()) } fn normalize_path(path: &Path) -> PathBuf { @@ -1714,9 +1723,8 @@ impl FileSystemExt for InMemoryFs { Some(self.snapshot()) } - fn vfs_restore(&self, snapshot: &VfsSnapshot) -> bool { - self.restore(snapshot); - true + fn vfs_restore(&self, snapshot: &VfsSnapshot) -> Result<()> { + self.restore(snapshot) } } @@ -2134,7 +2142,9 @@ mod tests { max_file_size: 100, ..FsLimits::default() }); - limited.restore(&snapshot); + // Issue #1576: restore must fail closed and not silently no-op. + let err = limited.restore(&snapshot); + assert!(err.is_err(), "restore must report the limit violation"); assert!(!limited.exists(Path::new("/tmp/huge.bin")).await.unwrap()); } diff --git a/crates/bashkit/src/fs/mountable.rs b/crates/bashkit/src/fs/mountable.rs index 9a5355d6..bc22636c 100644 --- a/crates/bashkit/src/fs/mountable.rs +++ b/crates/bashkit/src/fs/mountable.rs @@ -545,7 +545,7 @@ impl FileSystemExt for MountableFs { self.root.vfs_snapshot() } - fn vfs_restore(&self, snapshot: &super::VfsSnapshot) -> bool { + fn vfs_restore(&self, snapshot: &super::VfsSnapshot) -> crate::Result<()> { // Delegate to root filesystem self.root.vfs_restore(snapshot) } diff --git a/crates/bashkit/src/fs/overlay.rs b/crates/bashkit/src/fs/overlay.rs index 43ffabd9..e40cd7c5 100644 --- a/crates/bashkit/src/fs/overlay.rs +++ b/crates/bashkit/src/fs/overlay.rs @@ -953,9 +953,8 @@ impl FileSystemExt for OverlayFs { Some(self.upper.snapshot()) } - fn vfs_restore(&self, snapshot: &super::VfsSnapshot) -> bool { - self.upper.restore(snapshot); - true + fn vfs_restore(&self, snapshot: &super::VfsSnapshot) -> crate::Result<()> { + self.upper.restore(snapshot) } } diff --git a/crates/bashkit/src/fs/traits.rs b/crates/bashkit/src/fs/traits.rs index bca7bfbe..25275af2 100644 --- a/crates/bashkit/src/fs/traits.rs +++ b/crates/bashkit/src/fs/traits.rs @@ -165,10 +165,14 @@ pub trait FileSystemExt: Send + Sync { /// Restore filesystem contents from a snapshot. /// - /// Returns `false` if this filesystem doesn't support restore. - /// The default implementation returns `false`. - fn vfs_restore(&self, _snapshot: &super::VfsSnapshot) -> bool { - false + /// Returns `Err(...)` if this filesystem doesn't support restore, or if + /// the snapshot fails validation (path, size, or count limits). On error + /// the filesystem MUST be left untouched so callers can refuse the + /// restore atomically (issue #1576). + /// + /// The default implementation returns `Err(Unsupported)`. + fn vfs_restore(&self, _snapshot: &super::VfsSnapshot) -> Result<()> { + Err(IoError::new(ErrorKind::Unsupported, "vfs_restore not supported").into()) } } diff --git a/crates/bashkit/src/snapshot.rs b/crates/bashkit/src/snapshot.rs index 49c7eb73..1c829af4 100644 --- a/crates/bashkit/src/snapshot.rs +++ b/crates/bashkit/src/snapshot.rs @@ -324,12 +324,17 @@ impl crate::Bash { } fn restore_snapshot_inner(&mut self, snap: &Snapshot) -> crate::Result<()> { + // Issue #1576: validate everything that can fail before mutating + // either the shell or the VFS, so a malformed/forged snapshot can't + // leave the instance in a half-restored state. self.interpreter .validate_shell_state_restore_limits(&snap.shell)?; - self.interpreter.restore_shell_state(&snap.shell); if let Some(ref vfs) = snap.vfs { - self.fs.vfs_restore(vfs); + self.fs.vfs_restore(vfs)?; } + // Shell state cannot fail past validation, and the VFS has already + // been restored atomically (or rejected) above. + self.interpreter.restore_shell_state(&snap.shell); Ok(()) } diff --git a/crates/bashkit/tests/snapshot_tests.rs b/crates/bashkit/tests/snapshot_tests.rs index b28afa3f..a61610ad 100644 --- a/crates/bashkit/tests/snapshot_tests.rs +++ b/crates/bashkit/tests/snapshot_tests.rs @@ -24,7 +24,7 @@ async fn vfs_snapshot_restores_file_content() { .unwrap(); // Restore - fs.restore(&snapshot); + fs.restore(&snapshot).unwrap(); let content = fs.read_file(Path::new("/tmp/test.txt")).await.unwrap(); assert_eq!(content, b"original"); @@ -42,7 +42,7 @@ async fn vfs_snapshot_removes_new_files() { assert!(fs.exists(Path::new("/tmp/new.txt")).await.unwrap()); // Restore - fs.restore(&snapshot); + fs.restore(&snapshot).unwrap(); assert!(!fs.exists(Path::new("/tmp/new.txt")).await.unwrap()); } @@ -60,7 +60,7 @@ async fn vfs_snapshot_restores_deleted_files() { assert!(!fs.exists(Path::new("/tmp/keep.txt")).await.unwrap()); // Restore - fs.restore(&snapshot); + fs.restore(&snapshot).unwrap(); let content = fs.read_file(Path::new("/tmp/keep.txt")).await.unwrap(); assert_eq!(content, b"keep me"); } @@ -79,7 +79,7 @@ async fn vfs_snapshot_preserves_directories() { fs.remove(Path::new("/data"), true).await.unwrap(); assert!(!fs.exists(Path::new("/data")).await.unwrap()); - fs.restore(&snapshot); + fs.restore(&snapshot).unwrap(); assert!(fs.exists(Path::new("/data/sub")).await.unwrap()); let content = fs.read_file(Path::new("/data/sub/file.txt")).await.unwrap(); assert_eq!(content, b"content"); @@ -97,7 +97,7 @@ async fn vfs_snapshot_serialization_roundtrip() { let restored: bashkit::VfsSnapshot = serde_json::from_str(&json).unwrap(); let fs2 = Arc::new(InMemoryFs::new()); - fs2.restore(&restored); + fs2.restore(&restored).unwrap(); let content = fs2.read_file(Path::new("/tmp/data.txt")).await.unwrap(); assert_eq!(content, b"serialize me"); @@ -187,7 +187,7 @@ async fn combined_snapshot_restore_multi_turn() { .unwrap(); // Rollback to turn 1 - fs.restore(&vfs_snap); + fs.restore(&vfs_snap).unwrap(); bash.restore_shell_state(&shell_snap); let result = bash @@ -593,6 +593,66 @@ async fn snapshot_restore_rejects_tampered_shell_state_that_exceeds_memory_limit ); } +// ==================== Atomic restore (Issue #1576) ==================== + +/// A malformed VFS snapshot (path validation failure) must cause +/// `restore_snapshot` to fail closed. The previous-tenant shell state +/// must not be replaced and the previous-tenant VFS must not be +/// observable, otherwise a forged unkeyed snapshot can leave a reused +/// instance in a half-restored, cross-tenant state. +#[tokio::test] +async fn snapshot_restore_fails_closed_on_malformed_vfs() { + // Tenant A: real, valid snapshot. + let mut tenant_a = Bash::new(); + tenant_a.exec("greeting=alpha").await.unwrap(); + tenant_a + .fs() + .write_file(Path::new("/tmp/secret-a.txt"), b"alpha-secret") + .await + .unwrap(); + let bytes = tenant_a.snapshot().unwrap(); + + // Patch the snapshot JSON so the VFS section contains a poisoned + // path (rejected by limits::validate_path) and the shell variable + // changes — both clearly distinguish "applied" from "not applied". + let mut json: serde_json::Value = serde_json::from_slice(&bytes[32..]).unwrap(); + json["shell"]["variables"]["greeting"] = serde_json::json!("BETA"); + json["vfs"] = serde_json::json!({ + "entries": [ + { + "path": "/poison\u{0}bad", + "kind": { "File": { "content": [120] } }, + "mode": 0o644, + } + ] + }); + let forged_snap: Snapshot = serde_json::from_value(json).unwrap(); + let forged = forged_snap.to_bytes().unwrap(); + + let result = tenant_a.restore_snapshot(&forged); + assert!( + result.is_err(), + "restore must fail closed when VFS validation fails (#1576)" + ); + + // Shell state must not be mutated. + let r = tenant_a.exec("echo $greeting").await.unwrap(); + assert_eq!( + r.stdout.trim(), + "alpha", + "shell state must remain unchanged after a failed restore" + ); + + // Pre-existing VFS must still match tenant A's contents (no clobber + // by partial restore, no tenant-B injection). + let secret = tenant_a + .fs() + .read_file(Path::new("/tmp/secret-a.txt")) + .await + .unwrap(); + assert_eq!(secret, b"alpha-secret"); +} + // ==================== Integrity verification (Issue #977) ==================== #[tokio::test]