From 6a912d7cd14d2937d8c9b206f32a625e6ae8fccc Mon Sep 17 00:00:00 2001 From: Charlie Tonneslan Date: Wed, 20 May 2026 19:51:01 -0400 Subject: [PATCH 1/3] server: guard web contract unpack against path traversal WebApp::unpack called tar's unpack() straight on the archive, with no overwrite or path filtering. A contract author could publish an archive with a `../../etc/passwd` entry, or an absolute path, and escape the webapp_cache_dir() sandbox. Since #3942 this is reachable from any subresource URL, not just top-level contract navigation. Unpack entries one at a time instead: reject any entry whose path is absolute or contains a `..` component, disable overwrite, and extract each remaining entry with unpack_in into the canonicalized destination. Closes #3946 Signed-off-by: Charlie Tonneslan --- crates/core/src/server/app_packaging.rs | 81 +++++++++++++++++++++++-- 1 file changed, 76 insertions(+), 5 deletions(-) diff --git a/crates/core/src/server/app_packaging.rs b/crates/core/src/server/app_packaging.rs index a5730d4930..e0c720e0d0 100644 --- a/crates/core/src/server/app_packaging.rs +++ b/crates/core/src/server/app_packaging.rs @@ -1,7 +1,7 @@ //! Helper functions and types for dealing with HTTP client API compatible contracts. use std::{ io::{Cursor, Read}, - path::Path, + path::{Component, Path}, }; use tracing::{debug, instrument}; @@ -70,11 +70,35 @@ impl WebApp { #[instrument(level = "debug", skip(self, dst))] pub fn unpack(&mut self, dst: impl AsRef) -> Result<(), WebContractError> { - debug!("Unpacking web content to {:?}", dst.as_ref()); + let dst = dst.as_ref(); + debug!("Unpacking web content to {:?}", dst); + std::fs::create_dir_all(dst).map_err(WebContractError::StoringError)?; + let dst = dst.canonicalize().map_err(WebContractError::StoringError)?; + let mut decoded_web = self.decode_web(); - decoded_web - .unpack(dst) - .map_err(WebContractError::StoringError)?; + decoded_web.set_overwrite(false); + decoded_web.set_preserve_mtime(false); + for entry in decoded_web + .entries() + .map_err(WebContractError::StoringError)? + { + let mut entry = entry.map_err(WebContractError::StoringError)?; + // Reject entries that would escape `dst`: an absolute path, or + // one with a `..` component. A malicious contract could otherwise + // ship an archive that writes outside the cache directory. + let path = entry + .path() + .map_err(WebContractError::StoringError)? + .into_owned(); + if path.is_absolute() || path.components().any(|c| c == Component::ParentDir) { + return Err(WebContractError::UnpackingError(anyhow::anyhow!( + "archive entry escapes the destination directory: {path:?}" + ))); + } + entry + .unpack_in(&dst) + .map_err(WebContractError::StoringError)?; + } Ok(()) } @@ -168,3 +192,50 @@ impl<'a> TryFrom<&'a [u8]> for WebApp { Ok(Self { metadata, web }) } } + +#[cfg(test)] +mod tests { + use super::*; + use tar::Header; + + fn web_app(entries: &[(&str, &[u8])]) -> WebApp { + let mut builder = Builder::new(Cursor::new(Vec::new())); + for (name, data) in entries { + let mut header = Header::new_gnu(); + header.set_entry_type(tar::EntryType::Regular); + header.set_size(data.len() as u64); + header.set_mode(0o644); + // Write the name field directly: Builder::append_data rejects a + // `..` path, but a malicious or non-Rust tar will happily include + // one, which is exactly the input this guard defends against. + let name_bytes = name.as_bytes(); + header.as_gnu_mut().unwrap().name[..name_bytes.len()].copy_from_slice(name_bytes); + header.set_cksum(); + builder + .append(&header, *data) + .expect("append archive entry"); + } + WebApp::from_data(Vec::new(), builder).expect("build WebApp") + } + + #[test] + fn unpack_writes_a_benign_archive() { + let dir = tempfile::tempdir().unwrap(); + let mut app = web_app(&[("index.html", b"")]); + app.unpack(dir.path()) + .expect("benign archive should unpack"); + assert!(dir.path().join("index.html").exists()); + } + + #[test] + fn unpack_rejects_parent_dir_traversal() { + let dir = tempfile::tempdir().unwrap(); + let mut app = web_app(&[("../escape.txt", b"pwned")]); + let err = app + .unpack(dir.path().join("web")) + .expect_err("an archive entry with a `..` component must be rejected"); + assert!(matches!(err, WebContractError::UnpackingError(_))); + // The malicious entry must not have escaped into the parent directory. + assert!(!dir.path().join("escape.txt").exists()); + } +} From f5b92d223b130e4010374f31de01b3bb31f4aa07 Mon Sep 17 00:00:00 2001 From: Charlie Tonneslan Date: Wed, 20 May 2026 19:56:09 -0400 Subject: [PATCH 2/3] test(server): cover the absolute-path branch of the unpack guard The traversal guard rejects both absolute paths and `..` components, but only the `..` branch had a test. Add unpack_rejects_absolute_path_entry so the absolute-path error path is exercised too. Signed-off-by: Charlie Tonneslan --- crates/core/src/server/app_packaging.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/crates/core/src/server/app_packaging.rs b/crates/core/src/server/app_packaging.rs index e0c720e0d0..63574db9be 100644 --- a/crates/core/src/server/app_packaging.rs +++ b/crates/core/src/server/app_packaging.rs @@ -238,4 +238,14 @@ mod tests { // The malicious entry must not have escaped into the parent directory. assert!(!dir.path().join("escape.txt").exists()); } + + #[test] + fn unpack_rejects_absolute_path_entry() { + let dir = tempfile::tempdir().unwrap(); + let mut app = web_app(&[("/etc/passwd", b"pwned")]); + let err = app + .unpack(dir.path()) + .expect_err("an archive entry with an absolute path must be rejected"); + assert!(matches!(err, WebContractError::UnpackingError(_))); + } } From 3b838fc878d68a8025d89bb37acca065b40993e0 Mon Sep 17 00:00:00 2001 From: Charlie Tonneslan Date: Wed, 27 May 2026 05:53:27 -0400 Subject: [PATCH 3/3] fix(server): also reject symlink/hardlink entries in WebApp unpack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per @sanity's review of #4204, the path-traversal guards I added only look at entry.path() and not at entry.link_name(). A symlink entry with a benign archive path (e.g. "app.js") but an escaping linkname (e.g. "../../../etc/passwd") passes the absolute/parent-dir check and is then created verbatim by tar::Entry::unpack_in. variable_content's read_to_string then follows the symlink and serves the linked file to the browser — full path-traversal exploit through the serving path. Reject EntryType::Symlink and EntryType::HardLink outright in WebApp::unpack. Web contracts have no legitimate use for either, so the narrowest fix is to refuse them rather than try to validate linknames. Hardlinks were already validated by the upstream tar crate, but rejecting both keeps the policy uniform and the reasoning local. Added unpack_rejects_symlink_entry regression test that injects a symlink with linkname ../escape.txt and asserts (1) the unpack returns an UnpackingError, (2) the symlink was never created inside dst, and (3) the linkname's target above dst doesn't exist. The test helper writes the GNU header linkname slot directly, same technique the existing tests use for the path slot, since Builder::append_link validates targets. Signed-off-by: Charlie Tonneslan --- crates/core/src/server/app_packaging.rs | 48 +++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/crates/core/src/server/app_packaging.rs b/crates/core/src/server/app_packaging.rs index 63574db9be..1177080613 100644 --- a/crates/core/src/server/app_packaging.rs +++ b/crates/core/src/server/app_packaging.rs @@ -83,6 +83,18 @@ impl WebApp { .map_err(WebContractError::StoringError)? { let mut entry = entry.map_err(WebContractError::StoringError)?; + // Reject symlinks and hardlinks outright. Even if the archive + // path is benign, a symlink entry's link target (which path- + // component checks don't see) can point anywhere; the underlying + // tar crate then creates the symlink verbatim, allowing serving + // code that follows symlinks to read files outside the cache. + // Web contracts have no legitimate use for these entry types. + let entry_type = entry.header().entry_type(); + if entry_type.is_symlink() || entry_type.is_hard_link() { + return Err(WebContractError::UnpackingError(anyhow::anyhow!( + "archive contains a {entry_type:?} entry; symlinks and hardlinks are not supported for web contracts" + ))); + } // Reject entries that would escape `dst`: an absolute path, or // one with a `..` component. A malicious contract could otherwise // ship an archive that writes outside the cache directory. @@ -248,4 +260,40 @@ mod tests { .expect_err("an archive entry with an absolute path must be rejected"); assert!(matches!(err, WebContractError::UnpackingError(_))); } + + /// Build a `WebApp` whose archive contains a single symlink entry with the + /// given archive path and linkname. The path-traversal guards on + /// `entry.path()` don't see the linkname, so without an entry-type + /// rejection a benign archive path + escaping linkname slips through. + fn web_app_with_symlink(name: &str, link_target: &str) -> WebApp { + let mut builder = Builder::new(Cursor::new(Vec::new())); + let mut header = Header::new_gnu(); + header.set_entry_type(tar::EntryType::Symlink); + header.set_size(0); + header.set_mode(0o644); + let name_bytes = name.as_bytes(); + header.as_gnu_mut().unwrap().name[..name_bytes.len()].copy_from_slice(name_bytes); + let link_bytes = link_target.as_bytes(); + header.as_gnu_mut().unwrap().linkname[..link_bytes.len()].copy_from_slice(link_bytes); + header.set_cksum(); + builder + .append(&header, &[][..]) + .expect("append symlink entry"); + WebApp::from_data(Vec::new(), builder).expect("build WebApp") + } + + #[test] + fn unpack_rejects_symlink_entry() { + let dir = tempfile::tempdir().unwrap(); + let dst = dir.path().join("web"); + let mut app = web_app_with_symlink("app.js", "../escape.txt"); + let err = app + .unpack(&dst) + .expect_err("a symlink entry must be rejected"); + assert!(matches!(err, WebContractError::UnpackingError(_))); + // The symlink must not have been created — neither at the archive + // path inside dst, nor at the linkname target above dst. + assert!(std::fs::symlink_metadata(dst.join("app.js")).is_err()); + assert!(!dir.path().join("escape.txt").exists()); + } }