From e76e9ed19e218473b17af12f0528867dbfd269d2 Mon Sep 17 00:00:00 2001 From: Javier Parada Date: Thu, 27 Aug 2026 11:41:13 +0200 Subject: [PATCH] core: refuse an entry that would be written over the archive being read An archive holding an entry named after the archive, extracted into the archive's own directory, overwrote itself. All three formats; two of the three said "Extracted 1 file(s)" and exited 0 while doing it. [zip] 132 -> 12 bytes [tar] 2048 -> 12 bytes [7z] 115 -> 12 bytes The entry is written onto the file still being read, so the archive is truncated mid-read and what replaces it is whatever fraction the extractor had reached: the contents are lost from the output as much as from disk. What made this indefensible rather than merely unfortunate is that compression has always refused the mirror image, and refuses it even with `--force` (`OutputIsSource`, and an output inside the folder being archived). The same product held two opposite positions on the same question. The planning pass already reads the whole listing before a byte is written, so the check costs one comparison per entry and no extra pass over the file. Two details it would be easy to get wrong, both pinned by tests: - it compares **file identity**, not paths. A hardlink is a second name for one file and never resolves to the same string, which is exactly how `--force` was once able to overwrite its own source on the compression side. Verified against a real hardlink. - it follows the **planned** name, not the archive's spelling, since a rename can land an entry on the archive that the archive's own name does not match. Refused outright rather than made overridable. Nobody agrees to this by asking to extract something, so no flag should unlock it. `WouldOverwriteArchive` is a variant of its own rather than a `Failed` string: the two callers that match on `CompressionError` both have catch-all arms, so nothing breaks, and a caller that wants to offer "extract somewhere else" now has something to match on. Four tests, and all four mutations are caught: removing the guard, comparing paths instead of identity, ignoring the plan, and refusing too much. The last one matters most, since "refuse anything that resembles the archive" would pass the other three and break ordinary extractions. Closes #96 --- README.md | 4 +- apps/core/src/compression.rs | 60 ++++++++++++++++++- apps/core/tests/security.rs | 110 +++++++++++++++++++++++++++++++++++ docs/threat_model.md | 35 +++++++++++ 4 files changed, 204 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 4afce6d..fff003b 100644 --- a/README.md +++ b/README.md @@ -188,8 +188,8 @@ Requires **Rust 1.88+** (2021 edition). ```bash make build # build the Rust crates -make test # run every suite (604 Rust tests + 116 Vitest cases) -make test/rust # only the Rust tests that need no Node toolchain (489) +make test # run every suite (608 Rust tests + 116 Vitest cases) +make test/rust # only the Rust tests that need no Node toolchain (493) ``` `make test` includes the desktop app's own Rust suite, which compiles Tauri, so diff --git a/apps/core/src/compression.rs b/apps/core/src/compression.rs index 13da057..4137783 100644 --- a/apps/core/src/compression.rs +++ b/apps/core/src/compression.rs @@ -103,6 +103,24 @@ pub enum CompressionError { source: std::io::Error, }, + /// One of the archive's own entries would be written over the archive. + /// + /// Refused outright rather than made overridable. Writing an entry onto the + /// archive being read truncates it mid-read: the archive is gone and what + /// replaces it is whatever fraction the extractor had reached, so the + /// contents are lost from the output as much as from disk. Nobody agrees to + /// that by asking to extract something. + /// + /// Compression has always refused the mirror image of this, and by file + /// identity rather than by path, so that a hardlink cannot slip past + /// (`paths::same_file`). Extraction had no equivalent (issue #96). + #[error( + "the entry {entry:?} would be written over the archive itself ({}), so nothing was \ + extracted. Extract into a different directory.", + archive.display() + )] + WouldOverwriteArchive { archive: PathBuf, entry: String }, + /// An entry name this filesystem cannot hold, or an answer that does not /// resolve one. #[error(transparent)] @@ -502,7 +520,8 @@ pub fn extract_with( // Before the archive is even opened: an answer that is itself unwritable is // wrong whether or not any entry needs it. options.rules().check_replacements(options.replacements())?; - let plan = plan_for(archive, algorithm, options)?; + let (names, plan) = plan_for(archive, algorithm, options)?; + refuse_overwriting_the_archive(archive, output_dir, &names, &plan)?; match algorithm { Algorithm::SevenZ => self::sevenz::extract_7z_planned(archive, output_dir, &plan), @@ -547,7 +566,42 @@ fn plan_for( archive: &Path, algorithm: Algorithm, options: &ExtractOptions, -) -> Result { +) -> Result<(Vec, NamePlan), CompressionError> { let names = list_entries(archive, algorithm).map_err(unreadable_archive)?; - Ok(plan_names(&names, options.rules(), options.replacements())?) + let plan = plan_names(&names, options.rules(), options.replacements())?; + Ok((names, plan)) +} + +/// Refuse an extraction that would write one of the archive's own entries over +/// the archive. +/// +/// Free to run, in the sense that matters: the listing it needs has already +/// been read and paid for by the planning pass, so this adds one identity check +/// per entry and no extra pass over the file. +/// +/// By **file identity**, not by path. A hardlink is a second name for one file +/// and never resolves to the same string, which is exactly how `--force` used +/// to be able to overwrite its own source on the compression side before that +/// was fixed. Repeating the path comparison here would repeat the bug. +fn refuse_overwriting_the_archive( + archive: &Path, + output_dir: &Path, + names: &[String], + plan: &NamePlan, +) -> Result<(), CompressionError> { + for name in names { + let Some(natural) = sanitize_entry_path(name) else { + // Not containable at all. The backend rejects it as traversal, and + // that is the message worth keeping. + continue; + }; + let rel = plan.written_as(name).map_or(natural, Path::to_path_buf); + if crate::paths::same_file(&output_dir.join(&rel), archive) { + return Err(CompressionError::WouldOverwriteArchive { + archive: archive.to_path_buf(), + entry: name.clone(), + }); + } + } + Ok(()) } diff --git a/apps/core/tests/security.rs b/apps/core/tests/security.rs index 63851ff..28c433c 100644 --- a/apps/core/tests/security.rs +++ b/apps/core/tests/security.rs @@ -799,3 +799,113 @@ fn a_colon_in_a_later_component_cannot_clear_the_path_being_built() { } } } + +// -------------------------------------- writing over the archive being read -- + +/// Issue #96. An archive holding an entry with its own name, extracted into its +/// own directory, used to overwrite itself and report success. +/// +/// Measured before this guard, on all three formats: `Ok`, "Extracted 1 +/// file(s)", and the archive replaced by the 12 bytes it contained. +/// +/// The asymmetry is what made it indefensible rather than merely unfortunate: +/// compression has always refused to write an archive over its own source, and +/// refuses it even with `--force`, while extraction had no equivalent at all. +#[test] +fn no_format_writes_an_entry_over_the_archive_it_is_reading() { + for ext in ["zip", "7z", "tar"] { + let dir = tempfile::TempDir::new().unwrap(); + let archive = dir.path().join(format!("victim.{ext}")); + + // An archive whose single entry is named after the archive itself. + let entry = format!("victim.{ext}"); + match ext { + "zip" => malicious_zip(&archive, &entry), + "7z" => malicious_7z(&archive, &entry), + _ => malicious_tar(&archive, &entry), + } + let before = std::fs::read(&archive).unwrap(); + + let err = extract(&archive, dir.path()) + .expect_err("extracting into its own directory must be refused"); + + assert!( + err.to_string().contains("over the archive itself"), + "{ext}: {err}" + ); + assert_eq!( + std::fs::read(&archive).unwrap(), + before, + "{ext}: the archive was modified" + ); + } +} + +/// The same guard, reached through a second name for the same file. +/// +/// A hardlink never resolves to the same path, so a string comparison would +/// wave this through. That is not hypothetical: it is exactly how `--force` +/// used to be able to overwrite its own source on the compression side, which +/// is why `paths::same_file` exists and why this uses it. +#[cfg(unix)] +#[test] +fn a_hardlink_to_the_archive_is_not_a_way_around_it() { + let dir = tempfile::TempDir::new().unwrap(); + let archive = dir.path().join("real.zip"); + malicious_zip(&archive, "alias.zip"); + + std::fs::hard_link(&archive, dir.path().join("alias.zip")).unwrap(); + let before = std::fs::read(&archive).unwrap(); + + let err = extract(&archive, dir.path()).expect_err("a second name is still the same file"); + assert!(err.to_string().contains("over the archive itself"), "{err}"); + assert_eq!(std::fs::read(&archive).unwrap(), before); +} + +/// The guard must not cost anyone an extraction that was never dangerous. +/// +/// Same archive, same entry name, a different output directory: nothing to +/// refuse. Without this the fix could be "refuse everything that looks vaguely +/// like the archive" and still pass the two tests above. +#[test] +fn the_same_archive_extracts_normally_somewhere_else() { + for ext in ["zip", "7z", "tar"] { + let dir = tempfile::TempDir::new().unwrap(); + let archive = dir.path().join(format!("victim.{ext}")); + let entry = format!("victim.{ext}"); + match ext { + "zip" => malicious_zip(&archive, &entry), + "7z" => malicious_7z(&archive, &entry), + _ => malicious_tar(&archive, &entry), + } + + let out = dir.path().join("elsewhere"); + let files = extract(&archive, &out).unwrap_or_else(|e| panic!("{ext}: {e}")); + assert_eq!(listing(files), vec![entry.clone()], "{ext}"); + assert!(out.join(&entry).exists(), "{ext}"); + } +} + +/// A renamed entry must be checked at the name it will actually be written +/// under, not the one the archive spells. +/// +/// The archive is `v_.zip` and its entry is `v?.zip`, which is nothing special +/// on Unix; under Windows rules the `?` is answered with `_`, so the entry +/// lands exactly on the archive. Checking the archive's own spelling would +/// miss it. +#[test] +fn the_check_follows_the_renamed_name_not_the_archive_s() { + let dir = tempfile::TempDir::new().unwrap(); + let archive = dir.path().join("v_.zip"); + malicious_zip(&archive, "v?.zip"); + let before = std::fs::read(&archive).unwrap(); + + let options = ExtractOptions::new() + .with_rules(NameRules::windows()) + .with_replacements(Substitutions::new().with('?', "_")); + + let err = extract_with(&archive, dir.path(), &options) + .expect_err("the planned name lands on the archive"); + assert!(err.to_string().contains("over the archive itself"), "{err}"); + assert_eq!(std::fs::read(&archive).unwrap(), before); +} diff --git a/docs/threat_model.md b/docs/threat_model.md index 230e645..0b81fbb 100644 --- a/docs/threat_model.md +++ b/docs/threat_model.md @@ -115,6 +115,41 @@ materializes a symlink, so nothing is planted. --- +### 4a. An entry written over the archive being read + +**Attack.** Not an attack so much as a foot-gun the product armed itself with: +an archive holding an entry named after the archive, extracted into the +archive's own directory. The entry is written onto the file still being read, so +the archive is truncated mid-read and what replaces it is whatever fraction the +extractor had reached. The contents are lost from the output as much as from +disk. + +Measured before the guard, on all three formats: `Ok`, "Extracted 1 file(s)", +and a 132 byte archive replaced by the 12 bytes it contained. Two of the three +reported success while doing it. + +**Prevention.** The planning pass already reads the whole listing before a byte +is written, so it now also resolves each entry's destination and refuses one +that turns out to be the archive itself. By **file identity**, not by path: a +hardlink is a second name for one file and never resolves to the same string, +which is precisely how `--force` was once able to overwrite its own source on +the compression side. + +The check follows the **planned** name rather than the archive's spelling, since +a rename can land an entry on the archive that the archive's own name does not +match. + +This is the mirror of a guard compression has always had (`OutputIsSource`, and +an output inside the folder being archived, neither of which `--force` unlocks). +Extraction simply had no equivalent, so the same product held two opposite +positions on the same question (issue #96). + +**Covered by** `no_format_writes_an_entry_over_the_archive_it_is_reading`, +`a_hardlink_to_the_archive_is_not_a_way_around_it`, +`the_check_follows_the_renamed_name_not_the_archive_s`, and +`the_same_archive_extracts_normally_somewhere_else`, which is the one that stops +the guard from being fixed by refusing too much. + ### 4b. Entry names this host cannot write **Attack.** An entry name that a filesystem does not reject but *reinterprets*.