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.
This commit is contained in:
2026-09-15 23:30:28 +02:00
parent b34e86dcfe
commit 48f6e6ce4e
+167
View File
@@ -1,6 +1,7 @@
use std::cmp::min; use std::cmp::min;
use std::error::Error; use std::error::Error;
use std::os::unix::fs::symlink; use std::os::unix::fs::symlink;
use std::path::Component;
use std::path::Path; use std::path::Path;
use std::path::PathBuf; use std::path::PathBuf;
@@ -209,6 +210,28 @@ where
continue; 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); let dest_path = dest.join(&relative);
// Create parent directories if needed // Create parent directories if needed
@@ -834,4 +857,148 @@ mod tests {
async fn test_pull_paraview_ubuntu_end_to_end() { async fn test_pull_paraview_ubuntu_end_to_end() {
test_pull_package_end_to_end("paraview", Some("noble"), None, None).await; 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<u8> {
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())
);
}
} }