From 48f6e6ce4e3b8f14923214c77cb127e05679537a Mon Sep 17 00:00:00 2001 From: Valentin Haudiquet Date: Tue, 15 Sep 2026 23:30:28 +0200 Subject: [PATCH] pull: confine tar extraction to the destination directory Entry::unpack performs no path sanitization, so a malicious or malformed tarball (PPA, flat repository) could write files outside the package directory via '..' components or absolute entry paths. Refuse such entries with an error naming the offending path. --- src/pull.rs | 167 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 167 insertions(+) diff --git a/src/pull.rs b/src/pull.rs index 1e59081..205c21d 100644 --- a/src/pull.rs +++ b/src/pull.rs @@ -1,6 +1,7 @@ use std::cmp::min; use std::error::Error; use std::os::unix::fs::symlink; +use std::path::Component; use std::path::Path; use std::path::PathBuf; @@ -209,6 +210,28 @@ where continue; } + // Security: `Entry::unpack` performs no path sanitization, so extract + // the entry only if its path is confined to the destination directory. + // An absolute path or a path containing '..' would let a malicious (or + // deeply malformed) tarball write files anywhere outside 'dest' (path + // traversal). Refuse such entries with an error rather than skipping + // them silently, so the problem is not hidden. + let escapes_dest = relative.components().any(|component| { + matches!( + component, + Component::Prefix(_) | Component::RootDir | Component::ParentDir + ) + }); + if escapes_dest { + return Err(format!( + "Refusing to extract '{}': archive entry path is absolute or \ + contains '..' and would escape the destination directory '{}'", + relative.display(), + dest.display() + ) + .into()); + } + let dest_path = dest.join(&relative); // Create parent directories if needed @@ -834,4 +857,148 @@ mod tests { async fn test_pull_paraview_ubuntu_end_to_end() { test_pull_package_end_to_end("paraview", Some("noble"), None, None).await; } + + /// Build a minimal uncompressed ustar archive from (name, data) entries. + /// + /// Raw header blocks are crafted instead of using `tar::Builder` because + /// the builder itself refuses entry names containing '..' or absolute + /// paths, which is exactly what the traversal tests need to exercise. + fn build_tar(entries: &[(&str, &[u8])]) -> Vec { + let mut out = Vec::new(); + for (name, data) in entries { + let mut block = [0u8; 512]; + block[..name.len()].copy_from_slice(name.as_bytes()); + block[100..108].copy_from_slice(b"0000644\0"); + block[124..136].copy_from_slice(format!("{:011o}\0", data.len()).as_bytes()); + block[136..148].copy_from_slice(b"00000000000\0"); + block[156] = b'0'; // regular file + block[257..263].copy_from_slice(b"ustar\0"); + block[263..265].copy_from_slice(b"00"); + + // Checksum: sum of the header bytes with the checksum field + // (bytes 148..156) taken as spaces + let mut checksum: u32 = 0; + for (i, byte) in block.iter().enumerate() { + checksum += if (148..156).contains(&i) { + u32::from(b' ') + } else { + u32::from(*byte) + }; + } + block[148..154].copy_from_slice(format!("{checksum:06o}").as_bytes()); + block[154] = 0; + block[155] = b' '; + + out.extend_from_slice(&block); + out.extend_from_slice(data); + let padding = (512 - (data.len() % 512)) % 512; + out.extend_from_slice(&vec![0u8; padding]); + } + // End-of-archive marker: two zero-filled blocks + out.extend_from_slice(&[0u8; 1024]); + out + } + + #[test] + fn test_extract_tar_rejects_parent_dir_traversal() { + let temp_dir = tempfile::tempdir().unwrap(); + let tar_path = temp_dir.path().join("malicious.orig.tar"); + std::fs::write( + &tar_path, + build_tar(&[("good.txt", b"ok"), ("../evil.txt", b"pwned")]), + ) + .unwrap(); + + let dest = temp_dir.path().join("dest"); + let result = extract_tar_archive(&tar_path, &dest, None, |f| f); + + let err = match result { + Ok(_) => panic!("extraction of a traversal archive should fail"), + Err(e) => e, + }; + assert!( + err.to_string().contains("../evil.txt"), + "error should name the offending entry, got: {err}" + ); + // Nothing may be written outside of the destination directory + assert!(!temp_dir.path().join("evil.txt").exists()); + // Legitimate entries preceding the malicious one are still extracted + assert_eq!( + std::fs::read_to_string(dest.join("good.txt")).unwrap(), + "ok" + ); + } + + #[test] + fn test_extract_tar_rejects_absolute_path() { + let temp_dir = tempfile::tempdir().unwrap(); + let tar_path = temp_dir.path().join("malicious.orig.tar"); + std::fs::write( + &tar_path, + build_tar(&[ + ("good.txt", b"ok"), + ("/pkh_test_absolute_escape.txt", b"pwned"), + ]), + ) + .unwrap(); + + let dest = temp_dir.path().join("dest"); + let result = extract_tar_archive(&tar_path, &dest, None, |f| f); + + let err = match result { + Ok(_) => panic!("extraction of an absolute-path archive should fail"), + Err(e) => e, + }; + assert!( + err.to_string().contains("pkh_test_absolute_escape.txt"), + "error should name the offending entry, got: {err}" + ); + // Nothing may be written at the filesystem root + assert!(!Path::new("/pkh_test_absolute_escape.txt").exists()); + assert_eq!( + std::fs::read_to_string(dest.join("good.txt")).unwrap(), + "ok" + ); + } + + #[test] + fn test_extract_tar_archive_normal_entries() { + let temp_dir = tempfile::tempdir().unwrap(); + let tar_path = temp_dir.path().join("normal.orig.tar"); + // Raw names: 'tar::Builder' would normalize away the './' prefix + std::fs::write( + &tar_path, + build_tar(&[ + ("hello-1.0/file.txt", b"hello"), + ("./debian/rules", b"#!/m"), + ]), + ) + .unwrap(); + + let dest = temp_dir.path().join("dest"); + let extracted = extract_tar_archive(&tar_path, &dest, None, |f| f).unwrap(); + + assert_eq!( + std::fs::read_to_string(dest.join("hello-1.0/file.txt")).unwrap(), + "hello" + ); + assert_eq!( + std::fs::read_to_string(dest.join("debian/rules")).unwrap(), + "#!/m" + ); + assert!(extracted.in_place); + assert!( + extracted.files.contains( + &dest + .join("hello-1.0/file.txt") + .to_string_lossy() + .to_string() + ) + ); + assert!( + extracted + .files + .contains(&dest.join("debian/rules").to_string_lossy().to_string()) + ); + } }