From 10b4b8395763e3e6278c09a0c308868201c9bac7 Mon Sep 17 00:00:00 2001 From: Kaya Gokalp Date: Fri, 7 Feb 2025 14:42:09 -0800 Subject: [PATCH 1/4] fix: remove unvalid unpacking logic for ipfs pinned pkg sources --- Cargo.lock | 2 + Cargo.toml | 1 + forc-pkg/Cargo.toml | 2 + forc-pkg/src/source/ipfs.rs | 143 +++++++++++++++++++++++++++++++----- 4 files changed, 129 insertions(+), 19 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4235889d565..fab0a768035 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2889,6 +2889,7 @@ dependencies = [ "anyhow", "byte-unit", "cid", + "flate2", "forc-tracing 0.66.6", "forc-util", "fuel-abi-types", @@ -2912,6 +2913,7 @@ dependencies = [ "sway-utils", "sysinfo", "tar", + "tempfile", "toml 0.8.19", "tracing", "url", diff --git a/Cargo.toml b/Cargo.toml index 9bcc4389947..3a7e2b7261d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -140,6 +140,7 @@ etk-ops = { package = "fuel-etk-ops", version = "0.3.1-dev" } extension-trait = "1.0" fd-lock = "4.0" filecheck = "0.5" +flate2 = "1.0" fs_extra = "1.2" futures = { version = "0.3", default-features = false } gag = "1.0" diff --git a/forc-pkg/Cargo.toml b/forc-pkg/Cargo.toml index 6e0e6431fac..fa29d63a68b 100644 --- a/forc-pkg/Cargo.toml +++ b/forc-pkg/Cargo.toml @@ -13,6 +13,7 @@ ansiterm.workspace = true anyhow.workspace = true byte-unit.workspace = true cid.workspace = true +flate2.workspace = true forc-tracing.workspace = true forc-util.workspace = true fuel-abi-types.workspace = true @@ -42,6 +43,7 @@ walkdir.workspace = true [dev-dependencies] regex = "^1.10.2" +tempfile.workspace = true [target.'cfg(not(target_os = "macos"))'.dependencies] sysinfo = "0.29" diff --git a/forc-pkg/src/source/ipfs.rs b/forc-pkg/src/source/ipfs.rs index 4d465ff23f6..09fde435ae3 100644 --- a/forc-pkg/src/source/ipfs.rs +++ b/forc-pkg/src/source/ipfs.rs @@ -4,6 +4,7 @@ use crate::{ source, }; use anyhow::Result; +use flate2::read::GzDecoder; use forc_tracing::println_action_green; use futures::TryStreamExt; use ipfs_api::IpfsApi; @@ -130,6 +131,24 @@ impl fmt::Display for Pinned { } impl Cid { + fn extract_archive(&self, reader: R, dst: &Path) -> Result<()> { + let dst_dir = dst.join(self.0.to_string()); + std::fs::create_dir_all(&dst_dir)?; + let mut archive = Archive::new(reader); + + for entry in archive.entries()? { + let mut entry = entry?; + let path = entry.path()?; + let new_path = dst_dir.join(path); + // Create parent directories if they don't exist + if let Some(parent) = new_path.parent() { + std::fs::create_dir_all(parent)?; + } + entry.unpack(&new_path)?; + } + + Ok(()) + } /// Using local node, fetches the content described by this cid. async fn fetch_with_client(&self, ipfs_client: &IpfsClient, dst: &Path) -> Result<()> { let cid_path = format!("/ipfs/{}", self.0); @@ -140,8 +159,7 @@ impl Cid { .try_concat() .await?; // After collecting bytes of the archive, we unpack it to the dst. - let mut archive = Archive::new(bytes.as_slice()); - archive.unpack(dst)?; + self.extract_archive(bytes.as_slice(), dst)?; Ok(()) } @@ -150,7 +168,7 @@ impl Cid { let client = reqwest::Client::new(); // We request the content to be served to us in tar format by the public gateway. let fetch_url = format!( - "{}/ipfs/{}?download=true&format=tar&filename={}.tar", + "{}/ipfs/{}?download=true&filename={}.tar.gz", gateway_url, self.0, self.0 ); let req = client.get(&fetch_url); @@ -158,11 +176,10 @@ impl Cid { if !res.status().is_success() { anyhow::bail!("Failed to fetch from {fetch_url:?}"); } - let bytes: Vec<_> = res.text().await?.bytes().collect(); - - // After collecting bytes of the archive, we unpack it to the dst. - let mut archive = Archive::new(bytes.as_slice()); - archive.unpack(dst)?; + let bytes: Vec<_> = res.bytes().await?.into_iter().collect(); + let tar = GzDecoder::new(bytes.as_slice()); + // After collecting and decoding bytes of the archive, we unpack it to the dst. + self.extract_archive(tar, dst)?; Ok(()) } } @@ -225,18 +242,106 @@ fn pkg_cache_dir(cid: &Cid) -> PathBuf { fn ipfs_client() -> IpfsClient { IpfsClient::default() } +#[cfg(test)] +mod tests { + use super::*; + use anyhow::Result; + use std::io::Cursor; + use tar::Header; + use tempfile::TempDir; + + fn create_header(path: &str, size: u64) -> Header { + let mut header = Header::new_gnu(); + header.set_path(path).unwrap(); + header.set_size(size); + header.set_mode(0o755); + header.set_cksum(); + header + } + + fn create_test_tar(files: &[(&str, &str)]) -> Vec { + let mut ar = tar::Builder::new(Vec::new()); + + // Add root project directory + let header = create_header("test-project/", 0); + ar.append(&header, &mut std::io::empty()).unwrap(); + + // Add files + for (path, content) in files { + let full_path = format!("test-project/{}", path); + let header = create_header(&full_path, content.len() as u64); + ar.append(&header, content.as_bytes()).unwrap(); + } + + ar.into_inner().unwrap() + } + + fn create_test_cid() -> Cid { + let cid = cid::Cid::from_str("QmYwAPJzv5CZsnA625s3Xf2nemtYgPpHdWEz79ojWnPbdG").unwrap(); + Cid(cid) + } + + #[test] + fn test_basic_extraction() -> Result<()> { + let temp_dir = TempDir::new()?; + let cid = create_test_cid(); -#[test] -fn test_source_ipfs_pinned_parsing() { - let string = "ipfs+QmYwAPJzv5CZsnA625s3Xf2nemtYgPpHdWEz79ojWnPbdG"; + let tar_content = create_test_tar(&[("test.txt", "hello world")]); - let expected = Pinned(Cid(cid::Cid::from_str( - "QmYwAPJzv5CZsnA625s3Xf2nemtYgPpHdWEz79ojWnPbdG", - ) - .unwrap())); + cid.extract_archive(Cursor::new(tar_content), temp_dir.path())?; - let parsed = Pinned::from_str(string).unwrap(); - assert_eq!(parsed, expected); - let serialized = expected.to_string(); - assert_eq!(&serialized, string); + let extracted_path = temp_dir + .path() + .join(cid.0.to_string()) + .join("test-project") + .join("test.txt"); + + assert!(extracted_path.exists()); + assert_eq!(std::fs::read_to_string(extracted_path)?, "hello world"); + + Ok(()) + } + + #[test] + fn test_nested_files() -> Result<()> { + let temp_dir = TempDir::new()?; + let cid = create_test_cid(); + + let tar_content = + create_test_tar(&[("src/main.sw", "contract {};"), ("README.md", "# Test")]); + + cid.extract_archive(Cursor::new(tar_content), temp_dir.path())?; + + let base = temp_dir.path().join(cid.0.to_string()).join("test-project"); + assert_eq!( + std::fs::read_to_string(base.join("src/main.sw"))?, + "contract {};" + ); + assert_eq!(std::fs::read_to_string(base.join("README.md"))?, "# Test"); + + Ok(()) + } + + #[test] + fn test_invalid_tar() { + let temp_dir = TempDir::new().unwrap(); + let cid = create_test_cid(); + + let result = cid.extract_archive(Cursor::new(b"not a tar file"), temp_dir.path()); + + assert!(result.is_err()); + } + + #[test] + fn test_source_ipfs_pinned_parsing() { + let string = "ipfs+QmYwAPJzv5CZsnA625s3Xf2nemtYgPpHdWEz79ojWnPbdG"; + let expected = Pinned(Cid(cid::Cid::from_str( + "QmYwAPJzv5CZsnA625s3Xf2nemtYgPpHdWEz79ojWnPbdG", + ) + .unwrap())); + let parsed = Pinned::from_str(string).unwrap(); + assert_eq!(parsed, expected); + let serialized = expected.to_string(); + assert_eq!(&serialized, string); + } } From 5cc83256f16e1d7d3e02d62d75e5034655e73494 Mon Sep 17 00:00:00 2001 From: Kaya Gokalp Date: Mon, 10 Feb 2025 14:36:56 -0800 Subject: [PATCH 2/4] test: demonstrate path traversal attack was possible --- Cargo.lock | 9 +++++- forc-pkg/Cargo.toml | 1 + forc-pkg/src/source/ipfs.rs | 60 +++++++++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index 322d7285f8c..7f7889a5d12 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2903,6 +2903,7 @@ dependencies = [ "gix-url", "hex", "ipfs-api-backend-hyper", + "pathdiff", "petgraph", "regex", "reqwest", @@ -5500,7 +5501,7 @@ version = "0.7.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "af1844ef2428cc3e1cb900be36181049ef3d3193c63e43026cfe202983b27a56" dependencies = [ - "proc-macro-crate 1.1.3", + "proc-macro-crate 3.2.0", "proc-macro2", "quote", "syn 2.0.87", @@ -5773,6 +5774,12 @@ version = "1.0.15" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "57c0d7b74b563b49d38dae00a0c37d4d6de9b432382b2892f0574ddcae73fd0a" +[[package]] +name = "pathdiff" +version = "0.2.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "df94ce210e5bc13cb6651479fa48d14f601d9858cfe0467f43ae157023b938d3" + [[package]] name = "pbkdf2" version = "0.11.0" diff --git a/forc-pkg/Cargo.toml b/forc-pkg/Cargo.toml index fa29d63a68b..b76de19d473 100644 --- a/forc-pkg/Cargo.toml +++ b/forc-pkg/Cargo.toml @@ -42,6 +42,7 @@ vec1.workspace = true walkdir.workspace = true [dev-dependencies] +pathdiff = "0.2.3" regex = "^1.10.2" tempfile.workspace = true diff --git a/forc-pkg/src/source/ipfs.rs b/forc-pkg/src/source/ipfs.rs index 09fde435ae3..ed83f82c215 100644 --- a/forc-pkg/src/source/ipfs.rs +++ b/forc-pkg/src/source/ipfs.rs @@ -132,6 +132,18 @@ impl fmt::Display for Pinned { impl Cid { fn extract_archive(&self, reader: R, dst: &Path) -> Result<()> { + /* + let dst_dir = dst.join(self.0.to_string()); + std::fs::create_dir_all(&dst_dir)?; + let mut archive = Archive::new(reader); + + for entry in archive.entries()? { + let mut entry = entry?; + entry.unpack_in(&dst_dir)?; + } + + Ok(()) + */ let dst_dir = dst.join(self.0.to_string()); std::fs::create_dir_all(&dst_dir)?; let mut archive = Archive::new(reader); @@ -344,4 +356,52 @@ mod tests { let serialized = expected.to_string(); assert_eq!(&serialized, string); } + + #[test] + fn test_path_traversal_prevention() -> Result<()> { + let temp_dir = TempDir::new()?; + let cid = create_test_cid(); + + // Create a known directory structure + let target_dir = temp_dir.path().join("target"); + std::fs::create_dir(&target_dir)?; + + // Create our canary file in a known location + let canary_content = "sensitive content"; + let canary_path = target_dir.join("canary.txt"); + std::fs::write(&canary_path, canary_content)?; + + // Create tar with malicious path targeting our specific canary file + let mut header = tar::Header::new_gnu(); + let malicious_path = b"../../target/canary.txt"; + header.as_gnu_mut().unwrap().name[..malicious_path.len()].copy_from_slice(malicious_path); + header.set_size(17); + header.set_mode(0o644); + header.set_cksum(); + + let mut ar = tar::Builder::new(Vec::new()); + ar.append(&header, b"malicious content".as_slice())?; + + // Add safe file + let mut safe_header = tar::Header::new_gnu(); + safe_header.set_path("safe.txt")?; + safe_header.set_size(12); + safe_header.set_mode(0o644); + safe_header.set_cksum(); + ar.append(&safe_header, b"safe content".as_slice())?; + + // Extract to a subdirectory of temp_dir + let tar_content = ar.into_inner()?; + let extract_dir = temp_dir.path().join("extract"); + std::fs::create_dir(&extract_dir)?; + cid.extract_archive(Cursor::new(tar_content), &extract_dir)?; + + // Verify canary file was not modified + assert_eq!( + std::fs::read_to_string(&canary_path)?, + canary_content, + "Canary file was modified - path traversal protection failed!" + ); + Ok(()) + } } From ed29a64fd7ad0237b9d052f319a4398e58d7099f Mon Sep 17 00:00:00 2001 From: Kaya Gokalp Date: Mon, 10 Feb 2025 14:37:52 -0800 Subject: [PATCH 3/4] fix: use unpack_in to prevent path traversal attacks during extraction --- forc-pkg/src/source/ipfs.rs | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/forc-pkg/src/source/ipfs.rs b/forc-pkg/src/source/ipfs.rs index ed83f82c215..adb5148f8f2 100644 --- a/forc-pkg/src/source/ipfs.rs +++ b/forc-pkg/src/source/ipfs.rs @@ -132,7 +132,6 @@ impl fmt::Display for Pinned { impl Cid { fn extract_archive(&self, reader: R, dst: &Path) -> Result<()> { - /* let dst_dir = dst.join(self.0.to_string()); std::fs::create_dir_all(&dst_dir)?; let mut archive = Archive::new(reader); @@ -142,23 +141,6 @@ impl Cid { entry.unpack_in(&dst_dir)?; } - Ok(()) - */ - let dst_dir = dst.join(self.0.to_string()); - std::fs::create_dir_all(&dst_dir)?; - let mut archive = Archive::new(reader); - - for entry in archive.entries()? { - let mut entry = entry?; - let path = entry.path()?; - let new_path = dst_dir.join(path); - // Create parent directories if they don't exist - if let Some(parent) = new_path.parent() { - std::fs::create_dir_all(parent)?; - } - entry.unpack(&new_path)?; - } - Ok(()) } /// Using local node, fetches the content described by this cid. From 64fe9c3baa0fb20a7042e738ab42718ce4afc559 Mon Sep 17 00:00:00 2001 From: Kaya Gokalp Date: Mon, 10 Feb 2025 15:45:38 -0800 Subject: [PATCH 4/4] remove unused dep --- Cargo.lock | 7 ------- forc-pkg/Cargo.toml | 1 - 2 files changed, 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7f7889a5d12..4884ba15f04 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2903,7 +2903,6 @@ dependencies = [ "gix-url", "hex", "ipfs-api-backend-hyper", - "pathdiff", "petgraph", "regex", "reqwest", @@ -5774,12 +5773,6 @@ version = "1.0.15" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "57c0d7b74b563b49d38dae00a0c37d4d6de9b432382b2892f0574ddcae73fd0a" -[[package]] -name = "pathdiff" -version = "0.2.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "df94ce210e5bc13cb6651479fa48d14f601d9858cfe0467f43ae157023b938d3" - [[package]] name = "pbkdf2" version = "0.11.0" diff --git a/forc-pkg/Cargo.toml b/forc-pkg/Cargo.toml index b76de19d473..fa29d63a68b 100644 --- a/forc-pkg/Cargo.toml +++ b/forc-pkg/Cargo.toml @@ -42,7 +42,6 @@ vec1.workspace = true walkdir.workspace = true [dev-dependencies] -pathdiff = "0.2.3" regex = "^1.10.2" tempfile.workspace = true