fix: reject archive entries that would extract outside the destination

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"

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<PathBuf> {
+    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<R: Read>(archive: &mut tar::Archive<R>, path: &Path) -> Result<()> {
     let entries = archive
         .entries()
@@ -35,10 +66,37 @@ fn unpack_without_first_dir<R: Read>(archive: &mut tar::Archive<R>, 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 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(),
+            );
+        }
+        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 +146,12 @@ 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 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() {
@@ -158,3 +217,195 @@ 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> {
+        Dir(&'a str),
+        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::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();
+                }
+                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::Dir("pkg/"),
+            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_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
+        // 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());
+        }
+    }
+
+    #[test]
+    fn zip_rejects_file_at_destination() {
+        for name in ["pkg", "pkg/."] {
+            let (dir, result) = unpack_zip(&[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);
+        }
+    }
+
+    #[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);
+        }
+    }
+}