From 4679d4813e7b18d2a47b7f92cd55844579ec75a7 Mon Sep 17 00:00:00 2001 From: Kim Morrison Date: Mon, 5 Oct 2026 05:41:23 +0000 Subject: [PATCH 1/3] fix: reject archive entries that would extract outside the destination Validate paths of tar and zip entries after stripping the top-level directory, accepting only plain file names. Reject hard links in tar archives and accept symbolic links only if their target stays within the link's own directory. Update tar to 0.4.46. Co-Authored-By: Claude Opus 5.5 (1M context) --- Cargo.lock | 5 +- src/elan-dist/Cargo.toml | 3 + src/elan-dist/src/component/package.rs | 217 +++++++++++++++++++++++-- 3 files changed, 213 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 613cce7..e57828a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -439,6 +439,7 @@ dependencies = [ "serde", "serde_derive", "tar", + "tempfile", "time", "toml", "url", @@ -1796,9 +1797,9 @@ dependencies = [ [[package]] name = "tar" -version = "0.4.43" +version = "0.4.46" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c65998313f8e17d0d553d28f91a0df93e4dbbbf770279c7bc21ca0f09ea1a1f6" +checksum = "3f6221d9a6003c78398e3b239969f352578258df48c8eb051caadae0015bc840" dependencies = [ "filetime", "libc", diff --git a/src/elan-dist/Cargo.toml b/src/elan-dist/Cargo.toml index 5077163..b017813 100644 --- a/src/elan-dist/Cargo.toml +++ b/src/elan-dist/Cargo.toml @@ -28,6 +28,9 @@ serde = "1.0.119" serde_derive = "1.0.119" fslock = "0.2.1" +[dev-dependencies] +tempfile = "3.2.0" + [target."cfg(windows)".dependencies] winapi = { version = "0.3.9", features = ["handleapi", "sysinfoapi", "tlhelp32", "winnt"] } winreg = "0.8.0" diff --git a/src/elan-dist/src/component/package.rs b/src/elan-dist/src/component/package.rs index 89ff5ce..15cfef0 100644 --- a/src/elan-dist/src/component/package.rs +++ b/src/elan-dist/src/component/package.rs @@ -6,7 +6,7 @@ use crate::errors::*; use std::fs::{self, File}; use std::io::{self, Read, Seek}; -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; use time::OffsetDateTime; use zip::ZipArchive; @@ -24,6 +24,37 @@ impl TarPackage { } } +/// Whether `component` is a plain file name that cannot take on another meaning +/// when combined with other components, also on Windows (e.g. `C:`, `.. `). +fn is_plain_name(component: Component<'_>) -> bool { + match component { + Component::Normal(name) => { + let mut parsed = Path::new(name).components(); + matches!(parsed.next(), Some(Component::Normal(_))) + && parsed.next().is_none() + && !name.to_string_lossy().ends_with(['.', ' ']) + } + _ => false, + } +} + +/// Returns `path` relative to the unpacking destination, i.e. without its first +/// component, or an error if it could refer to a location outside the destination. +fn strip_first_dir(path: &Path) -> Result { + let mut components = path.components(); + // Throw away the first path component + components.next(); + let mut relpath = PathBuf::new(); + for component in components { + match component { + Component::CurDir => {} + c if is_plain_name(c) => relpath.push(c), + _ => return Err(format!("invalid path in archive: '{}'", path.display()).into()), + } + } + Ok(relpath) +} + fn unpack_without_first_dir(archive: &mut tar::Archive, path: &Path) -> Result<()> { let entries = archive .entries() @@ -35,10 +66,29 @@ fn unpack_without_first_dir(archive: &mut tar::Archive, path: &Path) let path = path.chain_err(|| ErrorKind::ExtractingPackage)?; path.into_owned() }; - let mut components = relpath.components(); - // Throw away the first path component - components.next(); - let full_path = path.join(&components.as_path()); + let full_path = path.join(strip_first_dir(&relpath)?); + + let kind = entry.header().entry_type(); + if kind.is_hard_link() { + return Err( + format!("unsupported hard link in archive: '{}'", relpath.display()).into(), + ); + } + if kind.is_symlink() { + // Only allow links to the link's own directory or below it, so that no + // chain of links can lead outside the destination. + let target = entry + .link_name() + .chain_err(|| ErrorKind::ExtractingPackage)?; + let is_contained = target.as_ref().is_some_and(|target| { + target + .components() + .all(|c| c == Component::CurDir || is_plain_name(c)) + }); + if !is_contained { + return Err(format!("invalid symlink in archive: '{}'", relpath.display()).into()); + } + } // Create the full path to the entry if it does not exist already match full_path.parent() { @@ -88,11 +138,7 @@ impl ZipPackage { if entry.name().ends_with('/') { continue; // skip directories } - let relpath = PathBuf::from(entry.name()); - let mut components = relpath.components(); - // Throw away the first path component - components.next(); - let full_path = path.join(&components.as_path()); + let full_path = path.join(strip_first_dir(Path::new(entry.name()))?); // Create the full path to the entry if it does not exist already match full_path.parent() { @@ -158,3 +204,154 @@ impl TarZstdPackage { Self::unpack(file, into) } } + +#[cfg(test)] +mod tests { + use super::*; + use std::io::{Cursor, Write}; + + fn tar_header(kind: tar::EntryType, path: &str, size: u64) -> tar::Header { + let mut header = tar::Header::new_gnu(); + header.set_entry_type(kind); + // Bypass `set_path`, which refuses to write invalid paths. + header.as_old_mut().name[..path.len()].copy_from_slice(path.as_bytes()); + header.set_mode(0o644); + header.set_size(size); + header.set_cksum(); + header + } + + enum TarEntry<'a> { + File(&'a str), + Symlink(&'a str, &'a str), + Hardlink(&'a str, &'a str), + } + + fn unpack_tar(entries: &[TarEntry<'_>]) -> (tempfile::TempDir, Result<()>) { + let mut builder = tar::Builder::new(Vec::new()); + for entry in entries { + match *entry { + TarEntry::File(path) => { + let header = tar_header(tar::EntryType::Regular, path, 2); + builder.append(&header, &b"hi"[..]).unwrap(); + } + TarEntry::Symlink(path, target) | TarEntry::Hardlink(path, target) => { + let kind = match *entry { + TarEntry::Symlink(..) => tar::EntryType::Symlink, + _ => tar::EntryType::Link, + }; + let mut header = tar_header(kind, path, 0); + header.set_link_name(target).unwrap(); + header.set_cksum(); + builder.append(&header, io::empty()).unwrap(); + } + } + } + let data = builder.into_inner().unwrap(); + let dir = tempfile::tempdir().unwrap(); + let result = TarPackage::unpack(&data[..], &dir.path().join("dest")); + (dir, result) + } + + fn unpack_zip(names: &[&str]) -> (tempfile::TempDir, Result<()>) { + let mut zip = zip::ZipWriter::new(Cursor::new(Vec::new())); + for name in names { + zip.start_file(*name, zip::write::SimpleFileOptions::default()) + .unwrap(); + zip.write_all(b"hi").unwrap(); + } + let data = zip.finish().unwrap().into_inner(); + let dir = tempfile::tempdir().unwrap(); + let result = ZipPackage::unpack(Cursor::new(data), &dir.path().join("dest")); + (dir, result) + } + + #[test] + fn tar_unpacks_normal_archive() { + let (dir, result) = unpack_tar(&[ + TarEntry::File("pkg/bin/lean"), + TarEntry::File("pkg/lib/libfoo.so.1"), + TarEntry::Symlink("pkg/lib/libfoo.so", "libfoo.so.1"), + ]); + result.unwrap(); + let dest = dir.path().join("dest"); + assert_eq!(fs::read(dest.join("bin/lean")).unwrap(), b"hi"); + assert_eq!(fs::read(dest.join("lib/libfoo.so")).unwrap(), b"hi"); + } + + #[test] + fn tar_rejects_parent_dir() { + let (dir, result) = unpack_tar(&[TarEntry::File("pkg/../evil")]); + assert!(result.is_err()); + assert!(!dir.path().join("evil").exists()); + } + + #[test] + fn tar_rejects_trailing_dot_or_space() { + for path in ["pkg/.. /evil", "pkg/... /evil", "pkg/evil."] { + let (_dir, result) = unpack_tar(&[TarEntry::File(path)]); + assert!(result.is_err(), "{}", path); + } + } + + #[test] + fn tar_strips_root_dir() { + // The leading `/` is the stripped first component, so the entry stays inside. + let (dir, result) = unpack_tar(&[TarEntry::File("/evil")]); + result.unwrap(); + assert!(dir.path().join("dest/evil").exists()); + } + + #[test] + fn tar_rejects_symlink_outside() { + for target in ["..", "../evil", "/tmp", "sub/../..", ".. "] { + let (_dir, result) = unpack_tar(&[TarEntry::Symlink("pkg/link", target)]); + assert!(result.is_err(), "{}", target); + } + } + + #[test] + fn tar_rejects_file_through_symlink_outside() { + let (dir, result) = unpack_tar(&[ + TarEntry::Symlink("pkg/link", ".."), + TarEntry::File("pkg/link/evil"), + ]); + assert!(result.is_err()); + assert!(!dir.path().join("evil").exists()); + } + + #[test] + fn tar_rejects_hard_link() { + // Hard link targets are not relative to the destination but to the + // working directory, which for tests contains `Cargo.toml`. + let (_dir, result) = unpack_tar(&[TarEntry::Hardlink("pkg/b", "Cargo.toml")]); + assert!(result.is_err()); + } + + #[test] + fn zip_unpacks_normal_archive() { + let (dir, result) = unpack_zip(&["pkg/bin/lean.exe", "pkg/lib/foo.dll"]); + result.unwrap(); + let dest = dir.path().join("dest"); + assert_eq!(fs::read(dest.join("bin/lean.exe")).unwrap(), b"hi"); + assert_eq!(fs::read(dest.join("lib/foo.dll")).unwrap(), b"hi"); + } + + #[test] + fn zip_rejects_parent_dir() { + for name in ["pkg/../evil", "pkg/sub/../../evil", "pkg/.. /evil"] { + let (dir, result) = unpack_zip(&[name]); + assert!(result.is_err(), "{}", name); + assert!(!dir.path().join("evil").exists()); + } + } + + #[cfg(windows)] + #[test] + fn zip_rejects_drive_prefix() { + for name in ["pkg/C:/evil", "pkg\\C:\\evil", "pkg/C:evil"] { + let (_dir, result) = unpack_zip(&[name]); + assert!(result.is_err(), "{}", name); + } + } +} From e789f542a5ef9a06a53820d5431435618cbb2d18 Mon Sep 17 00:00:00 2001 From: Kim Morrison Date: Tue, 6 Oct 2026 12:51:16 +1100 Subject: [PATCH 2/3] fix: reject non-directory archive entries at the destination itself An entry such as `pkg` or `pkg/.` strips to an empty path and would be unpacked at the destination itself; a symlink there is resolved relative to the destination's parent. Allow only directories at that path, in both tar and zip archives. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/elan-dist/src/component/package.rs | 59 ++++++++++++++++++++++++-- 1 file changed, 56 insertions(+), 3 deletions(-) diff --git a/src/elan-dist/src/component/package.rs b/src/elan-dist/src/component/package.rs index 15cfef0..805fc72 100644 --- a/src/elan-dist/src/component/package.rs +++ b/src/elan-dist/src/component/package.rs @@ -66,9 +66,17 @@ fn unpack_without_first_dir(archive: &mut tar::Archive, path: &Path) let path = path.chain_err(|| ErrorKind::ExtractingPackage)?; path.into_owned() }; - let full_path = path.join(strip_first_dir(&relpath)?); - + let stripped = strip_first_dir(&relpath)?; let kind = entry.header().entry_type(); + // An entry like `pkg` or `pkg/.` refers to the destination itself. Only a + // directory may be unpacked there: anything else would be created at + // `path`, and a symlink there would be resolved relative to the parent + // of `path`, letting later entries escape through it. + if stripped.as_os_str().is_empty() && !kind.is_dir() { + return Err(format!("invalid path in archive: '{}'", relpath.display()).into()); + } + let full_path = path.join(stripped); + if kind.is_hard_link() { return Err( format!("unsupported hard link in archive: '{}'", relpath.display()).into(), @@ -138,7 +146,12 @@ impl ZipPackage { if entry.name().ends_with('/') { continue; // skip directories } - let full_path = path.join(strip_first_dir(Path::new(entry.name()))?); + let stripped = strip_first_dir(Path::new(entry.name()))?; + // A file entry like `pkg` would be written at the destination itself. + if stripped.as_os_str().is_empty() { + return Err(format!("invalid path in archive: '{}'", entry.name()).into()); + } + let full_path = path.join(stripped); // Create the full path to the entry if it does not exist already match full_path.parent() { @@ -222,6 +235,7 @@ mod tests { } enum TarEntry<'a> { + Dir(&'a str), File(&'a str), Symlink(&'a str, &'a str), Hardlink(&'a str, &'a str), @@ -231,6 +245,12 @@ mod tests { let mut builder = tar::Builder::new(Vec::new()); for entry in entries { match *entry { + TarEntry::Dir(path) => { + let mut header = tar_header(tar::EntryType::Directory, path, 0); + header.set_mode(0o755); + header.set_cksum(); + builder.append(&header, io::empty()).unwrap(); + } TarEntry::File(path) => { let header = tar_header(tar::EntryType::Regular, path, 2); builder.append(&header, &b"hi"[..]).unwrap(); @@ -269,6 +289,7 @@ mod tests { #[test] fn tar_unpacks_normal_archive() { let (dir, result) = unpack_tar(&[ + TarEntry::Dir("pkg/"), TarEntry::File("pkg/bin/lean"), TarEntry::File("pkg/lib/libfoo.so.1"), TarEntry::Symlink("pkg/lib/libfoo.so", "libfoo.so.1"), @@ -320,6 +341,29 @@ mod tests { assert!(!dir.path().join("evil").exists()); } + #[test] + fn tar_rejects_symlink_at_destination() { + for path in ["pkg", "pkg/."] { + let (dir, result) = + unpack_tar(&[TarEntry::Symlink(path, "."), TarEntry::File("pkg/evil")]); + // On Unix, `symlink` would also fail on the `dest/` path that results + // from joining the empty path, so check that our own check fires. + let err = result.unwrap_err().to_string(); + assert!(err.starts_with("invalid path in archive"), "{}: {}", path, err); + assert!(!dir.path().join("evil").exists(), "{}", path); + } + } + + #[test] + fn tar_rejects_file_at_destination() { + for path in ["pkg", "pkg/."] { + let (dir, result) = unpack_tar(&[TarEntry::File(path)]); + let err = result.unwrap_err().to_string(); + assert!(err.starts_with("invalid path in archive"), "{}: {}", path, err); + assert!(!dir.path().join("dest").is_file(), "{}", path); + } + } + #[test] fn tar_rejects_hard_link() { // Hard link targets are not relative to the destination but to the @@ -346,6 +390,15 @@ mod tests { } } + #[test] + fn zip_rejects_file_at_destination() { + for name in ["pkg", "pkg/."] { + let (dir, result) = unpack_zip(&[name]); + assert!(result.is_err(), "{}", name); + assert!(!dir.path().join("dest").is_file(), "{}", name); + } + } + #[cfg(windows)] #[test] fn zip_rejects_drive_prefix() { From 9a9c56309169296ca4d6f7939662c0e06f406525 Mon Sep 17 00:00:00 2001 From: Kim Morrison Date: Thu, 8 Oct 2026 22:32:22 +1100 Subject: [PATCH 3/3] test: check the zip destination test fails with our own error Co-Authored-By: Claude Opus 5.5 (1M context) --- src/elan-dist/src/component/package.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/elan-dist/src/component/package.rs b/src/elan-dist/src/component/package.rs index 805fc72..f764e1e 100644 --- a/src/elan-dist/src/component/package.rs +++ b/src/elan-dist/src/component/package.rs @@ -394,7 +394,8 @@ mod tests { fn zip_rejects_file_at_destination() { for name in ["pkg", "pkg/."] { let (dir, result) = unpack_zip(&[name]); - assert!(result.is_err(), "{}", name); + let err = result.unwrap_err().to_string(); + assert!(err.starts_with("invalid path in archive"), "{}: {}", name, err); assert!(!dir.path().join("dest").is_file(), "{}", name); } }