From 63b5c4fa4f64ca6b9db7192a4f4c392a70588566 Mon Sep 17 00:00:00 2001 From: Damian Reeves <957246+DamianReeves@users.noreply.github.com> Date: Sun, 1 Nov 2026 18:28:24 -0600 Subject: [PATCH 1/2] fix(deps): move zip to 9.2.1 and check ZIP headers (GHSA-m665-v7pm-7grj) zip <= 9.0.4 is affected by a critical parser differential: ZipArchive trusts the central directory and ZipStreamReader trusts the local file header. morphir-common and morphir-distribution extract with ZipArchive. The fixed release does not change ZipArchive by default. The check is opt-in through ZipArchive::validate_header_consistency, and a test shows that the version change alone still extracts a cloaked archive under its central directory name. Both extractors now call it, so they reject archives whose headers disagree, duplicate central names, overlapping entries and local headers missing from the central directory. zip 9 also changes two things that the distribution extractor sees: - name() returns a Result. A name that does not decode now fails closed. - A read past the declared size fails inside zip with InvalidData, before the extractor's own check. copy_zip_entry reports that as an unsafe archive, so the refusal keeps its error class. A checksum failure has the same kind and is now also reported as an unsafe archive. Add end-to-end tests of path enclosure, entry kinds, collisions, size limits and modes for both extractors. They were first run against 8.6.0. --- CHANGELOG.md | 1 + Cargo.lock | 68 +++-- crates/morphir-common/Cargo.toml | 2 +- crates/morphir-common/src/remote/http.rs | 137 +++++++++ crates/morphir-distribution/Cargo.toml | 2 +- .../morphir-distribution/src/tool_archive.rs | 21 +- .../src/tool_state/tests.rs | 1 + .../tool_state/tests/zip_extraction_safety.rs | 269 ++++++++++++++++++ 8 files changed, 479 insertions(+), 22 deletions(-) create mode 100644 crates/morphir-distribution/src/tool_state/tests/zip_extraction_safety.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 50971b43f..e8c540a81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1433,6 +1433,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Security - Extension resolution now rejects releases without a host-supported MEP version, and v2 exact locks authenticate launch arguments, capabilities, and MEP versions before activation; legacy v1 locks are rejected explicitly +- `morphir-common` and `morphir-distribution` move from `zip` 8.6.0 to 9.2.1 for [GHSA-m665-v7pm-7grj](https://github.com/advisories/GHSA-m665-v7pm-7grj). Both ZIP extractors now call `ZipArchive::validate_header_consistency`, which `zip` does not run by default, so they reject an archive whose local headers and central directory disagree, including duplicate central names, overlapping entries and local headers that the central directory does not list. Before this change those archives extracted under the central directory names ## [0.2.0] - 2026-01-24 diff --git a/Cargo.lock b/Cargo.lock index 18c516aa0..cae960f03 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -32,6 +32,7 @@ dependencies = [ "cipher", "cpubits", "cpufeatures 0.3.1", + "zeroize", ] [[package]] @@ -867,9 +868,12 @@ checksum = "a6ef517f0926dd24a1582492c791b6a4818a4d94e789a334894aa15b0d12f55c" [[package]] name = "constant_time_eq" -version = "0.4.2" +version = "0.6.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3d52eff69cd5e647efe296129160853a42795992097e8af39800e1060caeea9b" +checksum = "9eb6feb175df65d0a3ccd3e73484e940d0ac664e8034f0519fd49a93db3071d9" +dependencies = [ + "rustversion", +] [[package]] name = "convert_case" @@ -3220,9 +3224,9 @@ checksum = "112b39cec0b298b6c1999fee3e31427f74f676e4cb9879ed1a121b43661a4154" [[package]] name = "lzma-rust2" -version = "0.16.5" +version = "0.23.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ca93e534d1142d1d0dcca6d25fe302508a5dfb40b302802904577725ea0b695b" +checksum = "f1c46f3fcd2e888afdb01a07efc071cb21300e07c68d32c3e8b01b5c8cb1fcaa" dependencies = [ "sha2 0.11.0", ] @@ -3849,6 +3853,18 @@ dependencies = [ "uuid", ] +[[package]] +name = "nix" +version = "0.31.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cf20d2fde8ff38632c426f1165ed7436270b44f199fc55284c38276f9db47c3d" +dependencies = [ + "bitflags 2.13.1", + "cfg-if", + "cfg_aliases", + "libc", +] + [[package]] name = "nohash-hasher" version = "0.2.0" @@ -4341,9 +4357,9 @@ checksum = "439ee305def115ba05938db6eb1644ff94165c5ab5e9420d1c1bcedbba909391" [[package]] name = "ppmd-rust" -version = "1.4.0" +version = "1.5.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "efca4c95a19a79d1c98f791f10aebd5c1363b473244630bb7dbde1dc98455a24" +checksum = "cd4acc8d264ea385db64ccfdcdfd5909ee9eda0975666723d59f3bfec80b3611" [[package]] name = "ppv-lite86" @@ -6266,9 +6282,9 @@ checksum = "82205ffd44a9697e34fc145491aa47310f9871540bb7909eaa9365e0a9a46607" [[package]] name = "typed-path" -version = "0.12.3" +version = "0.13.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8e28f89b80c87b8fb0cf04ab448d5dd0dd0ade2f8891bae878de66a75a28600e" +checksum = "9ce898823d8cd0f028662d2f349694b1af0a5cc9be83f1d213a00d3147445e1e" [[package]] name = "typenum" @@ -6794,7 +6810,7 @@ dependencies = [ "toml 0.9.12+spec-1.1.0", "wasmtime-environ", "windows-sys 0.61.2", - "zstd", + "zstd 0.13.3", ] [[package]] @@ -7806,9 +7822,9 @@ dependencies = [ [[package]] name = "zip" -version = "8.6.0" +version = "9.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2d04a6b5381502aa6087c94c669499eb1602eb9c5e8198e534de571f7154809b" +checksum = "848dad21639597378c3be94c59fd4de683f0f6d8a5387575896e1245737d4f37" dependencies = [ "aes", "bzip2", @@ -7821,14 +7837,16 @@ dependencies = [ "indexmap", "lzma-rust2", "memchr", + "nix", "pbkdf2", "ppmd-rust", "sha1 0.11.0", "time", - "typed-path 0.12.3", + "typed-path 0.13.0", + "windows-sys 0.61.2", "zeroize", "zopfli", - "zstd", + "zstd 0.14.3", ] [[package]] @@ -7861,7 +7879,16 @@ version = "0.13.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e91ee311a569c327171651566e07972200e76fcfe2242a4fa446149a3881c08a" dependencies = [ - "zstd-safe", + "zstd-safe 7.2.4", +] + +[[package]] +name = "zstd" +version = "0.14.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "db3ae9bf90af6e40197bef767eeefc46ad6397420fe873830bd85e5fbbfb3c70" +dependencies = [ + "zstd-safe 8.2.0", ] [[package]] @@ -7873,11 +7900,20 @@ dependencies = [ "zstd-sys", ] +[[package]] +name = "zstd-safe" +version = "8.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ded520f16b29e1a9b771c722221b2b0a0398c2316d9bb3c42b458a5414d7434b" +dependencies = [ + "zstd-sys", +] + [[package]] name = "zstd-sys" -version = "2.0.16+zstd.1.5.7" +version = "2.1.2+zstd.1.5.7" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "91e19ebc2adc8f83e43039e79776e3fda8ca919132d68a1fed6a5faca2683748" +checksum = "481ada9c543b2323cbb7bbdbb5249177fbf0188ed959ed2c15e4b22421c84ed3" dependencies = [ "cc", "pkg-config", diff --git a/crates/morphir-common/Cargo.toml b/crates/morphir-common/Cargo.toml index 546f985fb..029f70f4f 100644 --- a/crates/morphir-common/Cargo.toml +++ b/crates/morphir-common/Cargo.toml @@ -46,7 +46,7 @@ reqwest = { version = "0.13", default-features = false, features = [ "json", ] } flate2 = "1.0" -zip = "8.0" +zip = "9.2.1" tar = "0.4" tempfile = "3" diff --git a/crates/morphir-common/src/remote/http.rs b/crates/morphir-common/src/remote/http.rs index a3c1135dd..3693351d5 100644 --- a/crates/morphir-common/src/remote/http.rs +++ b/crates/morphir-common/src/remote/http.rs @@ -160,6 +160,11 @@ impl HttpFetcher { let file = std::fs::File::open(archive_path)?; let mut archive = zip::ZipArchive::new(file) .map_err(|e| RemoteSourceError::ArchiveError(format!("Failed to open zip: {}", e)))?; + // `ZipArchive` reads names from the central directory only, and does not check them + // against the local headers unless asked (GHSA-m665-v7pm-7grj). + archive.validate_header_consistency().map_err(|e| { + RemoteSourceError::ArchiveError(format!("Inconsistent zip headers: {}", e)) + })?; for i in 0..archive.len() { let mut file = archive.by_index(i).map_err(|e| { @@ -311,4 +316,136 @@ mod tests { } // Note: Additional tests would require a mock HTTP server + + mod extract_zip { + use super::super::{HttpFetcher, RemoteSourceError}; + use std::fs; + use std::io::Write; + use std::path::Path; + use zip::write::SimpleFileOptions; + + const CENTRAL_DIRECTORY_HEADER: &[u8] = b"PK\x01\x02"; + const CENTRAL_NAME_OFFSET: usize = 46; + + fn extract(archive: &Path, destination: &Path) -> crate::remote::error::Result<()> { + HttpFetcher::with_defaults() + .unwrap() + .extract_zip(archive, destination) + } + + fn an_archive_with_entries(path: &Path, entries: &[(&str, &[u8])]) { + let mut writer = zip::ZipWriter::new(fs::File::create(path).unwrap()); + for (name, bytes) in entries { + writer + .start_file(*name, SimpleFileOptions::default()) + .unwrap(); + writer.write_all(bytes).unwrap(); + } + writer.finish().unwrap(); + } + + /// One entry whose local header says `local_name` and whose central directory says + /// `central_name`. The names must have the same length. + fn an_archive_whose_headers_disagree_on_the_name( + path: &Path, + local_name: &str, + central_name: &str, + ) { + assert_eq!(local_name.len(), central_name.len()); + an_archive_with_entries(path, &[(local_name, b"payload")]); + let mut bytes = fs::read(path).unwrap(); + let central = bytes + .windows(CENTRAL_DIRECTORY_HEADER.len()) + .position(|window| window == CENTRAL_DIRECTORY_HEADER) + .unwrap() + + CENTRAL_NAME_OFFSET; + bytes[central..central + central_name.len()].copy_from_slice(central_name.as_bytes()); + fs::write(path, bytes).unwrap(); + } + + #[test] + fn a_well_formed_archive_is_extracted() { + let root = tempfile::tempdir().unwrap(); + let archive = root.path().join("source.zip"); + let destination = root.path().join("out"); + fs::create_dir(&destination).unwrap(); + an_archive_with_entries(&archive, &[("dir/file.txt", b"contents")]); + + extract(&archive, &destination).unwrap(); + + assert_eq!( + fs::read(destination.join("dir/file.txt")).unwrap(), + b"contents" + ); + } + + #[test] + fn an_archive_whose_headers_disagree_on_a_name_is_rejected() { + // zip 8.6.0, and 9.2.1 without the header check, extract this under the central name. + let root = tempfile::tempdir().unwrap(); + let archive = root.path().join("cloaked.zip"); + let destination = root.path().join("out"); + fs::create_dir(&destination).unwrap(); + an_archive_whose_headers_disagree_on_the_name(&archive, "../a.txt", "safe.txt"); + + let error = extract(&archive, &destination).unwrap_err(); + + assert!(matches!(error, RemoteSourceError::ArchiveError(_))); + assert!(!destination.join("safe.txt").exists()); + assert!(!root.path().join("a.txt").exists()); + } + + #[test] + fn an_entry_that_escapes_the_root_is_skipped() { + let root = tempfile::tempdir().unwrap(); + let archive = root.path().join("escape.zip"); + let destination = root.path().join("out"); + fs::create_dir(&destination).unwrap(); + an_archive_with_entries( + &archive, + &[("../escape.txt", b"escape"), ("safe.txt", b"safe")], + ); + + extract(&archive, &destination).unwrap(); + + assert!(!root.path().join("escape.txt").exists()); + assert_eq!(fs::read(destination.join("safe.txt")).unwrap(), b"safe"); + } + + #[cfg(unix)] + #[test] + fn unix_modes_are_applied_and_a_symlink_entry_stays_a_regular_file() { + use std::os::unix::fs::PermissionsExt; + let root = tempfile::tempdir().unwrap(); + let archive = root.path().join("modes.zip"); + let destination = root.path().join("out"); + fs::create_dir(&destination).unwrap(); + let mut writer = zip::ZipWriter::new(fs::File::create(&archive).unwrap()); + writer + .start_file( + "run.sh", + SimpleFileOptions::default().unix_permissions(0o750), + ) + .unwrap(); + writer.write_all(b"#!/bin/sh\n").unwrap(); + writer + .add_symlink("link", "/etc/passwd", SimpleFileOptions::default()) + .unwrap(); + writer.finish().unwrap(); + + extract(&archive, &destination).unwrap(); + + assert_eq!( + fs::metadata(destination.join("run.sh")) + .unwrap() + .permissions() + .mode() + & 0o777, + 0o750 + ); + let link = fs::symlink_metadata(destination.join("link")).unwrap(); + assert!(!link.file_type().is_symlink()); + assert_eq!(fs::read(destination.join("link")).unwrap(), b"/etc/passwd"); + } + } } diff --git a/crates/morphir-distribution/Cargo.toml b/crates/morphir-distribution/Cargo.toml index b8072f45c..e86550bf6 100644 --- a/crates/morphir-distribution/Cargo.toml +++ b/crates/morphir-distribution/Cargo.toml @@ -29,4 +29,4 @@ tracing = "0.1" unicode-casefold = { workspace = true } unicode-normalization = { workspace = true } url = "2.5.8" -zip = "8.0" +zip = "9.2.1" diff --git a/crates/morphir-distribution/src/tool_archive.rs b/crates/morphir-distribution/src/tool_archive.rs index fcfd66359..a1a65206a 100644 --- a/crates/morphir-distribution/src/tool_archive.rs +++ b/crates/morphir-distribution/src/tool_archive.rs @@ -49,6 +49,11 @@ pub(crate) fn extract_zip( format!("archive exceeds {MAX_ARCHIVE_ENTRIES} entries"), )); } + // `ZipArchive` reads names from the central directory only, and does not check them against + // the local headers unless asked (GHSA-m665-v7pm-7grj). + archive + .validate_header_consistency() + .map_err(|source| unsafe_archive("", format!("inconsistent ZIP headers: {source}")))?; let mut names = PortableArchivePaths::default(); let mut unpacked = 0_u64; @@ -58,7 +63,10 @@ pub(crate) fn extract_zip( let mut entry = archive .by_index(index) .map_err(|source| unsafe_archive("", format!("invalid ZIP entry: {source}")))?; - let raw_name = entry.name().to_owned(); + let raw_name = entry + .name() + .map_err(|source| unsafe_archive("", format!("invalid ZIP entry name: {source}")))? + .into_owned(); let enclosed = entry.enclosed_name().ok_or_else(|| { unsafe_archive(&raw_name, "entry escapes the package root".to_owned()) })?; @@ -542,9 +550,14 @@ pub(crate) fn copy_zip_entry( )); } let actual = io::copy(&mut input.take(declared_size + 1), output).map_err(|source| { - DistributionError::Io { - path: output_path.to_path_buf(), - source, + // `zip` 9 fails the read itself when an entry expands past its declared size. + if source.kind() == io::ErrorKind::InvalidData { + unsafe_archive(entry_name, format!("invalid ZIP entry data: {source}")) + } else { + DistributionError::Io { + path: output_path.to_path_buf(), + source, + } } })?; if actual != declared_size { diff --git a/crates/morphir-distribution/src/tool_state/tests.rs b/crates/morphir-distribution/src/tool_state/tests.rs index e6cc67551..7a32e30e9 100644 --- a/crates/morphir-distribution/src/tool_state/tests.rs +++ b/crates/morphir-distribution/src/tool_state/tests.rs @@ -2,6 +2,7 @@ mod archive_portability; mod package_namespacing; mod rollback_history; mod support; +mod zip_extraction_safety; use super::*; use crate::{Channel, RelativeArtifactPath, Selection, Sha256Digest, ToolId}; diff --git a/crates/morphir-distribution/src/tool_state/tests/zip_extraction_safety.rs b/crates/morphir-distribution/src/tool_state/tests/zip_extraction_safety.rs new file mode 100644 index 000000000..5bf526ac4 --- /dev/null +++ b/crates/morphir-distribution/src/tool_state/tests/zip_extraction_safety.rs @@ -0,0 +1,269 @@ +//! End-to-end safety properties of `extract_zip`, independent of the `zip` crate version. + +use super::support::write_zip; +use crate::tool_archive::{ExtractedArchive, extract_zip}; +use crate::{DistributionError, RelativeArtifactPath}; +use std::fs; +use std::io::Write; +use std::path::Path; +use zip::write::SimpleFileOptions; + +const ENTRY_POINT: &str = "tool.exe"; +const CENTRAL_DIRECTORY_HEADER: &[u8] = b"PK\x01\x02"; +const LOCAL_FILE_HEADER: &[u8] = b"PK\x03\x04"; +const CENTRAL_NAME_OFFSET: usize = 46; +const CENTRAL_UNCOMPRESSED_SIZE_OFFSET: usize = 24; +const LOCAL_UNCOMPRESSED_SIZE_OFFSET: usize = 22; + +fn extract(archive: &Path, destination: &Path) -> crate::Result { + extract_zip( + archive, + destination, + &RelativeArtifactPath::parse(ENTRY_POINT).unwrap(), + ) +} + +fn assert_unsafe(result: crate::Result) { + match result { + Err(DistributionError::UnsafeToolArchive { .. }) => {} + Err(other) => panic!("expected UnsafeToolArchive, got {other:?}"), + Ok(_) => panic!("expected UnsafeToolArchive, but the archive was extracted"), + } +} + +fn header_offsets(bytes: &[u8], signature: &[u8]) -> Vec { + bytes + .windows(signature.len()) + .enumerate() + .filter_map(|(offset, window)| (window == signature).then_some(offset)) + .collect() +} + +/// An archive with one entry whose local header says `local_name` and whose central directory +/// says `central_name`. The names must have the same length. +fn an_archive_whose_headers_disagree_on_the_name( + path: &Path, + local_name: &str, + central_name: &str, +) { + assert_eq!(local_name.len(), central_name.len()); + write_zip(path, &[(local_name, b"payload")]); + let mut bytes = fs::read(path).unwrap(); + let central = header_offsets(&bytes, CENTRAL_DIRECTORY_HEADER)[0] + CENTRAL_NAME_OFFSET; + bytes[central..central + central_name.len()].copy_from_slice(central_name.as_bytes()); + fs::write(path, bytes).unwrap(); +} + +/// An archive with two entries that the central directory gives the same name. +fn an_archive_with_a_duplicate_central_name(path: &Path) { + write_zip(path, &[(ENTRY_POINT, b"first"), ("tool.bin", b"second")]); + let mut bytes = fs::read(path).unwrap(); + let central = header_offsets(&bytes, CENTRAL_DIRECTORY_HEADER)[1] + CENTRAL_NAME_OFFSET; + bytes[central..central + ENTRY_POINT.len()].copy_from_slice(ENTRY_POINT.as_bytes()); + fs::write(path, bytes).unwrap(); +} + +/// An archive whose headers both declare fewer uncompressed bytes than the entry expands to. +fn an_archive_that_understates_its_size(path: &Path) { + let mut writer = zip::ZipWriter::new(fs::File::create(path).unwrap()); + writer + .start_file( + ENTRY_POINT, + SimpleFileOptions::default().compression_method(zip::CompressionMethod::Stored), + ) + .unwrap(); + writer.write_all(b"understated payload").unwrap(); + writer.finish().unwrap(); + let mut bytes = fs::read(path).unwrap(); + let understated = 1_u32.to_le_bytes(); + let local = header_offsets(&bytes, LOCAL_FILE_HEADER)[0] + LOCAL_UNCOMPRESSED_SIZE_OFFSET; + bytes[local..local + 4].copy_from_slice(&understated); + let central = + header_offsets(&bytes, CENTRAL_DIRECTORY_HEADER)[0] + CENTRAL_UNCOMPRESSED_SIZE_OFFSET; + bytes[central..central + 4].copy_from_slice(&understated); + fs::write(path, bytes).unwrap(); +} + +fn an_archive_with_a_symlink(path: &Path) { + let mut writer = zip::ZipWriter::new(fs::File::create(path).unwrap()); + writer + .start_file(ENTRY_POINT, SimpleFileOptions::default()) + .unwrap(); + writer + .add_symlink("link", "/etc/passwd", SimpleFileOptions::default()) + .unwrap(); + writer.finish().unwrap(); +} + +fn an_archive_with_modes(path: &Path, entries: &[(&str, u32)]) { + let mut writer = zip::ZipWriter::new(fs::File::create(path).unwrap()); + for (name, mode) in entries { + writer + .start_file(*name, SimpleFileOptions::default().unix_permissions(*mode)) + .unwrap(); + writer.write_all(b"contents").unwrap(); + } + writer.finish().unwrap(); +} + +#[test] +fn a_well_formed_archive_is_extracted() { + let root = tempfile::tempdir().unwrap(); + let archive = root.path().join("tool.zip"); + let destination = root.path().join("out"); + fs::create_dir(&destination).unwrap(); + write_zip( + &archive, + &[(ENTRY_POINT, b"tool"), ("resources/data.txt", b"data")], + ); + + let extracted = extract(&archive, &destination).unwrap(); + + assert_eq!(extracted.files.len(), 2); + assert_eq!(fs::read(destination.join(ENTRY_POINT)).unwrap(), b"tool"); + assert_eq!( + fs::read(destination.join("resources/data.txt")).unwrap(), + b"data" + ); +} + +#[test] +fn an_archive_whose_headers_disagree_on_a_name_is_rejected() { + // zip 8.6.0, and 9.2.1 without the header check, extract this under the central name. + let root = tempfile::tempdir().unwrap(); + let archive = root.path().join("cloaked.zip"); + let destination = root.path().join("out"); + fs::create_dir(&destination).unwrap(); + an_archive_whose_headers_disagree_on_the_name(&archive, "../a.exe", ENTRY_POINT); + + assert_unsafe(extract(&archive, &destination)); + assert!(!destination.join(ENTRY_POINT).exists()); + assert!(!root.path().join("a.exe").exists()); +} + +#[test] +fn a_duplicate_central_name_is_rejected() { + // zip 8.6.0, and 9.2.1 without the header check, index entries by central name: the later + // entry replaces the earlier one, and the extractor's collision check never sees the duplicate. + let root = tempfile::tempdir().unwrap(); + let archive = root.path().join("duplicate.zip"); + let destination = root.path().join("out"); + fs::create_dir(&destination).unwrap(); + an_archive_with_a_duplicate_central_name(&archive); + + assert_unsafe(extract(&archive, &destination)); + assert!(!destination.join(ENTRY_POINT).exists()); +} + +#[test] +fn an_entry_that_escapes_the_root_is_rejected() { + let root = tempfile::tempdir().unwrap(); + let archive = root.path().join("escape.zip"); + let destination = root.path().join("out"); + fs::create_dir(&destination).unwrap(); + write_zip( + &archive, + &[(ENTRY_POINT, b"tool"), ("../escape.exe", b"escape")], + ); + + assert_unsafe(extract(&archive, &destination)); + assert!(!root.path().join("escape.exe").exists()); +} + +#[test] +fn an_absolute_entry_name_stays_inside_the_root() { + // `enclosed_name` drops a leading separator, so the entry lands under the destination. + let root = tempfile::tempdir().unwrap(); + let archive = root.path().join("absolute.zip"); + let destination = root.path().join("out"); + fs::create_dir(&destination).unwrap(); + let outside = root.path().join("outside.exe"); + let absolute = outside.to_str().unwrap(); + write_zip(&archive, &[(ENTRY_POINT, b"tool"), (absolute, b"outside")]); + + extract(&archive, &destination).unwrap(); + + assert!(!outside.exists()); + assert_eq!( + fs::read(destination.join(absolute.trim_start_matches('/'))).unwrap(), + b"outside" + ); +} + +#[test] +fn a_symlink_entry_is_rejected() { + let root = tempfile::tempdir().unwrap(); + let archive = root.path().join("symlink.zip"); + let destination = root.path().join("out"); + fs::create_dir(&destination).unwrap(); + an_archive_with_a_symlink(&archive); + + assert_unsafe(extract(&archive, &destination)); + assert!(fs::symlink_metadata(destination.join("link")).is_err()); +} + +#[test] +fn a_case_alias_of_another_entry_is_rejected() { + let root = tempfile::tempdir().unwrap(); + let archive = root.path().join("alias.zip"); + let destination = root.path().join("out"); + fs::create_dir(&destination).unwrap(); + write_zip(&archive, &[(ENTRY_POINT, b"tool"), ("TOOL.exe", b"alias")]); + + assert_unsafe(extract(&archive, &destination)); +} + +#[test] +fn an_understated_entry_size_stops_the_copy() { + // zip 8.6.0 lets the extractor's own one-byte-over check see this. zip 9 fails the read first, + // and the extractor must report that as the same unsafe archive. + let root = tempfile::tempdir().unwrap(); + let archive = root.path().join("understated.zip"); + let destination = root.path().join("out"); + fs::create_dir(&destination).unwrap(); + an_archive_that_understates_its_size(&archive); + + assert_unsafe(extract(&archive, &destination)); + assert!(fs::metadata(destination.join(ENTRY_POINT)).unwrap().len() <= 2); +} + +#[test] +fn modes_decide_which_files_are_executable() { + let root = tempfile::tempdir().unwrap(); + let archive = root.path().join("modes.zip"); + let destination = root.path().join("out"); + fs::create_dir(&destination).unwrap(); + an_archive_with_modes( + &archive, + &[(ENTRY_POINT, 0o644), ("helper", 0o755), ("data.txt", 0o644)], + ); + + let extracted = extract(&archive, &destination).unwrap(); + + let executable = |name: &str| { + extracted + .files + .iter() + .find(|file| file.path.as_str() == name) + .unwrap() + .executable + }; + assert!( + executable(ENTRY_POINT), + "the entry point is always executable" + ); + assert!(executable("helper")); + assert!(!executable("data.txt")); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = |name: &str| { + fs::metadata(destination.join(name)) + .unwrap() + .permissions() + .mode() + }; + assert_ne!(mode("helper") & 0o100, 0); + assert_eq!(mode("data.txt") & 0o111, 0); + } +} From 8af2805b6a1259bcff4175b0d010c0de72dccdfa Mon Sep 17 00:00:00 2001 From: Damian Reeves <957246+DamianReeves@users.noreply.github.com> Date: Sun, 1 Nov 2026 18:31:52 -0600 Subject: [PATCH 2/2] test(distribution): make the rooted entry name test portable A temporary directory path has a drive prefix on Windows, so trimming a leading slash did not make it relative, and the test read the path that it had just asserted does not exist. Use a fixed rooted entry name. --- .../tool_state/tests/zip_extraction_safety.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/crates/morphir-distribution/src/tool_state/tests/zip_extraction_safety.rs b/crates/morphir-distribution/src/tool_state/tests/zip_extraction_safety.rs index 5bf526ac4..6f13fa94d 100644 --- a/crates/morphir-distribution/src/tool_state/tests/zip_extraction_safety.rs +++ b/crates/morphir-distribution/src/tool_state/tests/zip_extraction_safety.rs @@ -177,16 +177,20 @@ fn an_absolute_entry_name_stays_inside_the_root() { let archive = root.path().join("absolute.zip"); let destination = root.path().join("out"); fs::create_dir(&destination).unwrap(); - let outside = root.path().join("outside.exe"); - let absolute = outside.to_str().unwrap(); - write_zip(&archive, &[(ENTRY_POINT, b"tool"), (absolute, b"outside")]); + write_zip( + &archive, + &[ + (ENTRY_POINT, b"tool"), + ("/morphir-zip-safety-absolute.exe", b"rooted"), + ], + ); extract(&archive, &destination).unwrap(); - assert!(!outside.exists()); + assert!(!Path::new("/morphir-zip-safety-absolute.exe").exists()); assert_eq!( - fs::read(destination.join(absolute.trim_start_matches('/'))).unwrap(), - b"outside" + fs::read(destination.join("morphir-zip-safety-absolute.exe")).unwrap(), + b"rooted" ); }