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()) + ); + } }