diff --git a/README.md b/README.md index b871af9..0b5f28d 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 (578 Rust tests + 112 Vitest cases) -make test/rust # only the Rust tests that need no Node toolchain (463) +make test # run every suite (585 Rust tests + 113 Vitest cases) +make test/rust # only the Rust tests that need no Node toolchain (470) ``` `make test` includes the desktop app's own Rust suite, which compiles Tauri, so diff --git a/apps/cli/tests/names.rs b/apps/cli/tests/names.rs index f657316..f12fb9a 100644 --- a/apps/cli/tests/names.rs +++ b/apps/cli/tests/names.rs @@ -265,8 +265,11 @@ fn a_failing_entry_names_itself_in_the_error() { message.contains("x/y.txt"), "the entry that could not be written is named: {message}" ); + // See the twin in apps/core/tests/names.rs: on Windows the message is built + // from a canonicalized root, so compare against what was actually resolved. + let resolved = out.canonicalize().unwrap_or_else(|_| out.clone()); assert!( - message.contains(&out.display().to_string()), + message.contains(&resolved.display().to_string()), "and where it was going: {message}" ); } diff --git a/apps/core/src/compression.rs b/apps/core/src/compression.rs index cdcd153..8d829a1 100644 --- a/apps/core/src/compression.rs +++ b/apps/core/src/compression.rs @@ -37,7 +37,24 @@ pub(crate) fn sanitize_entry_path(name: &str) -> Option { let mut safe = PathBuf::new(); for component in Path::new(name).components() { match component { - Component::Normal(part) => safe.push(part), + Component::Normal(part) => { + // `PathBuf::push` **replaces** what it holds when handed a path + // carrying a prefix, and Windows reads any `x:` at the start of + // a component as a drive. A prefix is only parsed at the head of + // a whole path, so `docs/c:evil.txt` offers no `Prefix` component + // for the arm below to reject, yet pushing its second component + // discards `docs` and leaves `c:evil.txt`: a drive-relative path + // that resolves against the current directory of C:, not the + // output directory. Re-parsing each part and demanding it still + // be exactly one `Normal` component is what closes that, and it + // leaves Unix (where the same string is an ordinary file name) + // untouched. + let mut parts = Path::new(part).components(); + match (parts.next(), parts.next()) { + (Some(Component::Normal(only)), None) if only == part => safe.push(only), + _ => return None, + } + } Component::CurDir => {} Component::ParentDir | Component::RootDir | Component::Prefix(_) => return None, } @@ -92,6 +109,35 @@ pub enum CompressionError { Name(#[from] NameError), } +/// Refuse a destination that resolved to somewhere outside the output directory. +/// +/// A lexical check on an entry name cannot see two things: a component the +/// host's path parser reads differently from the way the name was judged, and a +/// symlink that was already sitting in the output directory before extraction +/// began. Resolving the directory that was just created and requiring it to +/// still be inside covers both, and it is what tar's `unpack_in` has always +/// done through `validate_inside_dst`. zip and 7z had no equivalent at all: they +/// joined a sanitized path and wrote. +/// +/// `output` must already be canonical, and `resolved` must exist, so call this +/// after the parent directory has been created and before anything is written +/// into it. +pub(crate) fn ensure_inside( + output: &Path, + resolved: &Path, + entry: &str, +) -> Result<(), CompressionError> { + let real = resolved + .canonicalize() + .map_err(|e| entry_error(entry, resolved, e))?; + if !real.starts_with(output) { + return Err(CompressionError::Failed(format!( + "Path traversal detected in archive entry: {entry}" + ))); + } + Ok(()) +} + /// Attach the entry and the destination to an IO failure at a write site. pub(crate) fn entry_error( entry: &str, diff --git a/apps/core/src/compression/names.rs b/apps/core/src/compression/names.rs index 8056acf..551e225 100644 --- a/apps/core/src/compression/names.rs +++ b/apps/core/src/compression/names.rs @@ -27,19 +27,25 @@ //! `NUL`), and the superscript digits `¹²³` count as digits in `COM#`/`LPT#`. use std::collections::{BTreeMap, HashMap}; -use std::path::{Component, Path, PathBuf}; +use std::path::{Path, PathBuf}; use serde::Serialize; use thiserror::Error; -/// Characters Win32 refuses in a file name outright, minus the two separators. +/// Characters Win32 refuses in a file name outright, minus the one separator. /// -/// `/` and `\` are on the documented list as well, and are deliberately absent -/// here: a [`NameRules`] judges one *component* of a path, and splitting a name -/// into components is the caller's job (extraction does it while checking for -/// traversal). Reporting a separator as an offending character would ask the -/// user to replace something that is not in any name we ever check. -const WINDOWS_REJECTED: &[char] = &['<', '>', '"', '|', '?', '*']; +/// `/` is on the documented list as well and is deliberately absent here: it is +/// the separator **the archive formats define** (ZIP APPNOTE 4.4.17.1, and tar +/// by convention), so [`entry_components`] has already split on it and no +/// component reaching a [`NameRules`] can contain one. Reporting it would ask +/// the user to replace something that is not in any name we ever check. +/// +/// `\` is a different case and belongs here. It is *not* an archive separator, +/// so it arrives as an ordinary character inside a component, and Windows +/// genuinely cannot hold it: there it is a path separator, which is precisely +/// why the component cannot carry one. Unix can, and does (see [`UNIX_REJECTED`]), +/// which is the whole reason this is a rule and not a constant. +const WINDOWS_REJECTED: &[char] = &['<', '>', '"', '|', '?', '*', '\\']; /// The colon is not refused by Win32, it is *honoured*: `notes.txt:hidden` names /// the `hidden` alternate data stream of `notes.txt`, so the write succeeds, the @@ -182,12 +188,13 @@ impl NameRules { /// [`Self::problems`] over every component of an entry name, deduplicated: /// a `?` in two components is one question, not two. /// - /// Components that are not `Normal` (a root, a drive, `.`, `..`) are - /// skipped. They are containment's business, not this module's, and - /// extraction settles them before it gets here. + /// Split on `/` by [`entry_components`], so the answer does not depend on + /// which machine is asking. Empty components, `.` and `..` are skipped: + /// they are containment's business, not this module's, and extraction + /// settles them before it gets here. pub fn entry_problems(&self, name: &str) -> Vec { let mut problems: Vec = Vec::new(); - for component in normal_components(name) { + for component in entry_components(name) { for problem in self.problems(component) { if !problems.contains(&problem) { problems.push(problem); @@ -249,11 +256,22 @@ impl NameRules { // `..`, which would climb out of the output directory, and an empty // answer can leave nothing at all. A caller cannot reach the write path // without coming through here. + // + // `/` is checked structurally because it is the archive separator and so + // is in no ruleset; a component holding one would silently become two. + // A backslash is deliberately **not** checked here any more. It used to + // be, on the premise that a separator could only appear because a + // replacement put it there, and that premise was wrong twice over: + // `check_replacement` already refuses both separators before either is + // pushed, so the test could not fire for its stated reason, and on Unix + // a backslash is an ordinary, legal character, so the only thing it ever + // caught was a name the host could hold perfectly well. Windows cannot, + // and says so through `can_write` below, because the backslash is in + // WINDOWS_REJECTED where it belongs. let unnameable = written.is_empty() || written == "." || written == ".." || written.contains('/') - || written.contains('\\') || !self.can_write(&written); if unnameable { return Err(NameError::Unnameable { @@ -267,7 +285,7 @@ impl NameRules { /// [`Self::rewrite`] over a whole entry name, rebuilt as a relative path. /// - /// **Not a traversal guard**: components that are not `Normal` are dropped, + /// **Not a traversal guard**: `.`, `..` and empty components are dropped, /// exactly as `unpack_in` drops a root and as `sanitize_entry_path` reduces /// a name to what is left. Callers check containment first; all three /// extractors in this crate do. @@ -277,7 +295,7 @@ impl NameRules { replacements: &Substitutions, ) -> Result { let mut written = PathBuf::new(); - for component in normal_components(name) { + for component in entry_components(name) { written.push( self.rewrite(component, replacements) .map_err(|e| e.in_entry(name))?, @@ -715,14 +733,37 @@ pub(crate) fn plan_names>( /// The relative path an extractor derives from an entry name with no rules /// applied: its `Normal` components and nothing else. fn natural_path(name: &str) -> PathBuf { - normal_components(name).collect() + entry_components(name).collect() } -fn normal_components(name: &str) -> impl Iterator { - Path::new(name).components().filter_map(|c| match c { - // A component of a `&str` path is always valid UTF-8, so `to_str` never - // drops one here. - Component::Normal(part) => part.to_str(), - _ => None, - }) +/// Split an archive entry name into its components, the same way on every host. +/// +/// **The separator is `/`, always.** An archive entry name is not a host path: +/// ZIP mandates the forward slash (APPNOTE 4.4.17.1) and tar has used it since +/// v7, so an entry means the same thing whatever machine reads it, and this +/// module has to agree with that rather than with the local convention. +/// +/// It used to be `Path::new(name).components()`, and that was wrong in both +/// directions at once, because `std::path` is `#[cfg]`-dependent while +/// [`NameRules`] is data: +/// +/// * on Windows, `\` is a separator, so `dir\file.txt` split into two +/// components; on Unix it is an ordinary character, so the same entry was one +/// component that [`NameRules::rewrite`] then refused, and a legal Unix file +/// name became an archive this crate could build but not extract; +/// * on Windows, a leading `a:` parses as a drive prefix, and a prefix is not a +/// `Normal` component, so it was silently *discarded*: `a:b/c.txt` was judged, +/// reported and written as `b/c.txt`. That hole was in the colon handling that +/// issue #63 exists for, on the only platform issue #63 is about. +/// +/// Splitting here instead means `NameRules::windows()` answers the same question +/// on a Mac as on Windows, which is the property the whole design rests on. +/// +/// Empty components, `.` and `..` are skipped, exactly as the `Component` filter +/// skipped everything that was not `Normal`. **This is not a traversal guard**: +/// containment is [`super::sanitize_entry_path`]'s job and it rejects, rather +/// than skips, the same input. +fn entry_components(name: &str) -> impl Iterator { + name.split('/') + .filter(|part| !part.is_empty() && *part != "." && *part != "..") } diff --git a/apps/core/src/compression/sevenz.rs b/apps/core/src/compression/sevenz.rs index 30e0392..662e62d 100644 --- a/apps/core/src/compression/sevenz.rs +++ b/apps/core/src/compression/sevenz.rs @@ -286,27 +286,37 @@ pub(crate) fn extract_7z_planned( "Path traversal detected in archive entry: {name}" )) })?; - // See `extract_zip_planned`: a planned path is built from this - // entry's own `Normal` components, so it stays inside the output. + // See `extract_zip_planned`: the plan renames inside the output, + // and `ensure_inside` below is the backstop. let rel = plan.written_as(&name).map_or(rel, Path::to_path_buf); let dest = canonical_output.join(&rel); - let mut failed = |e: std::io::Error| { - write_failure = Some(super::entry_error(&name, &dest, e)); + // The callback can only fail with sevenz's own error type, so the + // real one is stashed for the caller and a placeholder returned. + let mut stash = |e: CompressionError| { + write_failure = Some(e); sevenz_rust2::Error::other("the entry could not be written") }; if entry.is_directory() { - fs::create_dir_all(&dest).map_err(&mut failed)?; + fs::create_dir_all(&dest) + .map_err(|e| super::entry_error(&name, &dest, e)) + .and_then(|()| super::ensure_inside(&canonical_output, &dest, &name)) + .map_err(&mut stash)?; } else { if let Some(parent) = dest.parent() { - fs::create_dir_all(parent).map_err(&mut failed)?; + fs::create_dir_all(parent) + .map_err(|e| super::entry_error(&name, &dest, e)) + .and_then(|()| super::ensure_inside(&canonical_output, parent, &name)) + .map_err(&mut stash)?; } let mut buf = Vec::new(); reader .read_to_end(&mut buf) .map_err(sevenz_rust2::Error::io)?; - fs::write(&dest, &buf).map_err(&mut failed)?; + fs::write(&dest, &buf) + .map_err(|e| super::entry_error(&name, &dest, e)) + .map_err(&mut stash)?; extracted.push(rel.to_string_lossy().to_string()); } Ok(true) diff --git a/apps/core/src/compression/zip.rs b/apps/core/src/compression/zip.rs index 441ce61..a13283f 100644 --- a/apps/core/src/compression/zip.rs +++ b/apps/core/src/compression/zip.rs @@ -181,17 +181,21 @@ pub(crate) fn extract_zip_planned( let rel = super::sanitize_entry_path(&name).ok_or_else(|| { CompressionError::Failed(format!("Path traversal detected in archive entry: {name}")) })?; - // The plan is built from the same names, and every path in it is made - // of the entry's own `Normal` components, so it can only ever rename - // inside the output directory. + // The plan is built from the same name, one component at a time, so it + // can only rename inside the output directory. `ensure_inside` below is + // the backstop for the cases a lexical rule cannot reach: a caller that + // judged the name under another host's rules, and a symlink already + // sitting in the output. let rel = plan.written_as(&name).map_or(rel, Path::to_path_buf); let dest = canonical_output.join(&rel); if entry.is_dir() { fs::create_dir_all(&dest).map_err(|e| super::entry_error(&name, &dest, e))?; + super::ensure_inside(&canonical_output, &dest, &name)?; } else { if let Some(parent) = dest.parent() { fs::create_dir_all(parent).map_err(|e| super::entry_error(&name, &dest, e))?; + super::ensure_inside(&canonical_output, parent, &name)?; } let mut buf = Vec::new(); entry.read_to_end(&mut buf)?; diff --git a/apps/core/tests/names.rs b/apps/core/tests/names.rs index 148b730..7bfe740 100644 --- a/apps/core/tests/names.rs +++ b/apps/core/tests/names.rs @@ -849,9 +849,127 @@ fn a_failing_entry_names_itself_and_its_destination() { message.contains("a.txt/b.txt") || message.contains(r"a.txt\b.txt"), "{format}: the message must name the entry: {message}" ); + // Windows renders this from a canonicalized root, which carries a `\\?\` + // verbatim prefix and expands any 8.3 short name on the way, so the + // message legitimately does not contain the path this test built. What + // it must contain is where extraction actually resolved to. + let resolved = out.canonicalize().unwrap_or_else(|_| out.clone()); assert!( - message.contains(&out.display().to_string()), + message.contains(&resolved.display().to_string()), "{format}: the message must say where it was going: {message}" ); } } + +// ------------------------------------- the seam: splitting is not the host's -- + +/// An archive entry name is not a host path, and this is the test that says so. +/// +/// It used to be split with `Path::new(name).components()`, and `std::path` is +/// `#[cfg]`-dependent while `NameRules` is data, so the rules were portable and +/// the splitting they ran over was not. Both directions were wrong at once, and +/// neither was visible from a Mac: +/// +/// * Windows parses a leading `a:` as a drive prefix, which is not a `Normal` +/// component, so it was silently dropped and `a:b/c.txt` was judged, reported +/// and written as `b/c.txt`. The colon that issue #63 is entirely about went +/// unasked on the only platform issue #63 concerns. +/// * Windows treats `\` as a separator and Unix does not, so one name split +/// into a different number of components depending on who was reading. +/// +/// ZIP mandates `/` (APPNOTE 4.4.17.1) and tar has used it since v7, so the +/// component count is a property of the archive and must not move. +#[test] +fn an_entry_splits_the_same_way_on_every_host() { + // One component, whatever std would make of it. `\` is not a separator in + // an archive, and `a:` is not a drive. + for name in ["a:b", "C:x", r"dir\file.txt", r"\\server\share"] { + assert_eq!( + NameRules::unix() + .rewrite_entry(name, &Substitutions::new()) + .ok(), + Some(PathBuf::from(name)), + "{name} must stay one component: Unix can hold every character in it" + ); + } + // And the split happens exactly where the archive says it does. + assert_eq!( + NameRules::unix() + .rewrite_entry("a:b/c.txt", &Substitutions::new()) + .unwrap(), + PathBuf::from("a:b").join("c.txt") + ); +} + +/// The Windows half of the same seam: the report must ask about a colon +/// wherever it sits, including the leading component that used to vanish. +/// +/// Note this one passed on Unix before the fix and failed only on Windows, +/// since `a:b` was already a single component here. It is a pin, not a +/// reproduction: what it stops is the answer diverging by host again. +#[test] +fn the_report_sees_a_colon_in_the_first_component_too() { + for name in ["a:b/c.txt", "C:/x/y", "deep/a:b.txt"] { + let problems = NameRules::windows().entry_problems(name); + assert_eq!( + problems, + vec![NameProblem::Character { + character: ':', + fault: CharacterFault::Reinterpreted, + }], + "{name}: the colon must be a question on every host" + ); + } +} + +/// A backslash is an ordinary character in an archive entry, so the two rulesets +/// must disagree about it, and each must be right about its own filesystem. +#[test] +fn only_windows_refuses_a_backslash_inside_a_component() { + assert_eq!( + NameRules::windows().entry_problems(r"dir\file.txt"), + vec![NameProblem::Character { + character: '\\', + fault: CharacterFault::Rejected, + }] + ); + assert!(NameRules::unix().entry_problems(r"dir\file.txt").is_empty()); +} + +/// Issue caught by nothing until Windows CI ran: collapse could build an archive +/// it then refused to extract. +/// +/// A backslash is a legal character in a Unix file name. The old splitter made +/// it one component on Unix, and `rewrite` refused any rewritten component +/// holding a separator, so `extract` failed the **whole** archive and wrote +/// nothing, after `compress_dir` had happily archived it and the verification +/// pass had signed it off. The user could have deleted the originals by then. +#[cfg(unix)] +#[test] +fn a_unix_name_holding_a_backslash_survives_the_round_trip() { + use collapse_core::{compress_dir, Algorithm, Verify}; + + for algorithm in [Algorithm::Zip, Algorithm::Tar, Algorithm::SevenZ] { + let dir = tempfile::TempDir::new().unwrap(); + let tree = dir.path().join("tree"); + fs::create_dir_all(&tree).unwrap(); + fs::write(tree.join(r"a\b.txt"), b"payload").unwrap(); + fs::write(tree.join("ok.txt"), b"fine").unwrap(); + + let archive = dir.path().join(format!("t.{}", algorithm.extension())); + compress_dir(&tree, &archive, algorithm, 3, Verify::Index).unwrap(); + + let out = dir.path().join("back"); + let mut files = extract(&archive, &out).unwrap(); + files.sort(); + assert_eq!( + files, + vec![r"tree/a\b.txt".to_string(), "tree/ok.txt".to_string()], + "{algorithm}: the archive this crate just built must extract" + ); + assert_eq!( + fs::read(out.join("tree").join(r"a\b.txt")).unwrap(), + b"payload" + ); + } +} diff --git a/apps/core/tests/security.rs b/apps/core/tests/security.rs index 5ff614c..63851ff 100644 --- a/apps/core/tests/security.rs +++ b/apps/core/tests/security.rs @@ -698,3 +698,104 @@ fn compress_dir_skips_symlinks_for_every_format() { ); } } + +// -- writing through something already in the output directory -- + +/// A symlink the extractor did not create, sitting in the output directory +/// before extraction begins, is the one traversal a name-only guard cannot see. +/// +/// It was reachable and silent: with `link` a symlink in the output directory, +/// an archive holding `link/evil.txt` wrote straight through it and returned +/// `Ok`. tar was immune, because `unpack_in` resolves the directory it is about +/// to write into; zip and 7z joined a sanitized name and wrote. Extracting into +/// a directory that already holds a symlink is ordinary, and this predates the +/// naming work rather than arriving with it. +#[cfg(unix)] +#[test] +fn no_format_writes_through_a_symlink_already_in_the_output() { + for ext in ["zip", "7z", "tar"] { + let dir = tempfile::TempDir::new().unwrap(); + let outside = dir.path().join("outside"); + std::fs::create_dir_all(&outside).unwrap(); + let out = dir.path().join("out"); + std::fs::create_dir_all(&out).unwrap(); + std::os::unix::fs::symlink(&outside, out.join("link")).unwrap(); + + let archive = dir.path().join(format!("a.{ext}")); + match ext { + "zip" => malicious_zip(&archive, "link/evil.txt"), + "7z" => malicious_7z(&archive, "link/evil.txt"), + _ => malicious_tar(&archive, "link/evil.txt"), + } + + let escaped = outside.join("evil.txt"); + assert_contained(extract(&archive, &out), &escaped); + } +} + +/// The same guard, reached through the naming layer rather than around it: a +/// planned rename must not be able to land outside either. +#[cfg(unix)] +#[test] +fn a_renamed_entry_cannot_be_written_through_such_a_symlink() { + for ext in ["zip", "7z", "tar"] { + let dir = tempfile::TempDir::new().unwrap(); + let outside = dir.path().join("outside"); + std::fs::create_dir_all(&outside).unwrap(); + let out = dir.path().join("out"); + std::fs::create_dir_all(&out).unwrap(); + std::os::unix::fs::symlink(&outside, out.join("link")).unwrap(); + + let archive = dir.path().join(format!("a.{ext}")); + match ext { + "zip" => malicious_zip(&archive, "link/ev?l.txt"), + "7z" => malicious_7z(&archive, "link/ev?l.txt"), + _ => malicious_tar(&archive, "link/ev?l.txt"), + } + + // Windows rules make the `?` a question, so this entry is renamed and + // takes the planned write path rather than the untouched one. + let options = ExtractOptions::new() + .with_rules(NameRules::windows()) + .with_replacements(Substitutions::new().with('?', "i")); + let escaped = outside.join("evil.txt"); + assert_contained(extract_with(&archive, &out, &options), &escaped); + } +} + +/// `PathBuf::push` replaces what it holds when handed a path carrying a prefix, +/// and Windows reads `c:` at the head of a component as a drive. Prefixes are +/// parsed only at the head of a whole path, so a colon in a *later* component +/// gives `sanitize_entry_path` nothing to reject while still clearing the +/// buffer it is building, which would drop `docs` and leave a drive-relative +/// path resolving against the current directory of C:. +/// +/// On Unix a colon is an ordinary character and this is simply a nested file, +/// which is the half this can assert here. Either way the bytes must land under +/// the output directory and nowhere else. +#[test] +fn a_colon_in_a_later_component_cannot_clear_the_path_being_built() { + for ext in ["zip", "7z", "tar"] { + let dir = tempfile::TempDir::new().unwrap(); + let out = dir.path().join("out"); + let archive = dir.path().join(format!("a.{ext}")); + match ext { + "zip" => malicious_zip(&archive, "docs/c:evil.txt"), + "7z" => malicious_7z(&archive, "docs/c:evil.txt"), + _ => malicious_tar(&archive, "docs/c:evil.txt"), + } + + match extract(&archive, &out) { + // Unix: written, and written inside. + Ok(files) => { + assert_eq!(listing(files), vec!["docs/c:evil.txt"], "{ext}"); + assert!(out.join("docs").join("c:evil.txt").exists(), "{ext}"); + } + // Windows: refused, and nothing written anywhere. + Err(_) => assert!( + !out.join("c:evil.txt").exists(), + "{ext}: a drive-relative path was written" + ), + } + } +} diff --git a/apps/desktop/src/App.vue b/apps/desktop/src/App.vue index 2783e1f..13ba606 100644 --- a/apps/desktop/src/App.vue +++ b/apps/desktop/src/App.vue @@ -304,7 +304,19 @@ async function runExtraction(archive, outputDir, replacements) { replacements, }) if (outcome.status === 'nameProblem') { - nameProblem.value = outcome.message + // The sheet is where a naming question belongs, but it is only open when + // the report found something to ask about. A refusal can still arrive + // without one, because the report and the extractor read the archive in two + // separate passes and can disagree: a listing the first pass could not read + // is reported as "nothing to ask", and then extraction refuses a name. With + // `nameProblem` rendered only inside the sheet, that combination made the + // Extract button do nothing at all: no files, no question, no banner. Send + // it to the banner when there is no sheet to hold it. + if (naming.value) { + nameProblem.value = outcome.message + } else { + error.value = outcome.message + } return } closeNaming() diff --git a/apps/desktop/tests/App.test.js b/apps/desktop/tests/App.test.js index 3ac5cac..99b34e8 100644 --- a/apps/desktop/tests/App.test.js +++ b/apps/desktop/tests/App.test.js @@ -441,6 +441,23 @@ describe('App', () => { expect(w.find('.error').exists()).toBe(false) }) + it('shows a naming refusal in the banner when no dialog is open to hold it', async () => { + // The report and the extractor read the archive in two separate passes and + // can disagree: a listing the first pass could not read is reported as + // "nothing to ask", and then extraction refuses a name. `nameProblem` is + // rendered only inside the sheet, so this combination used to make the + // Extract button do nothing at all: no files, no question, no banner. + const message = 'the archive entry "x:y.txt" contains \':\' and no replacement for it was given' + const w = await ask({ + report: NOTHING_TO_ASK, + extraction: { status: 'nameProblem', message }, + }) + + expect(namingSheet(w).exists()).toBe(false) + expect(w.find('.error').text()).toContain(message) + expect(w.text()).not.toContain('Extracted') + }) + it('states the adjustment for a problem with no character to replace', async () => { // A trailing dot and a device name have nothing to substitute: the host // would mangle them whatever anyone typed. So they are explained, not diff --git a/docs/threat_model.md b/docs/threat_model.md index 166d5e2..e4d009d 100644 --- a/docs/threat_model.md +++ b/docs/threat_model.md @@ -68,12 +68,31 @@ the link and then writes *through* it, landing `evil` outside the output dir. - **tar** skips any entry that is not a regular file or a directory (symlinks, hardlinks, and special nodes are dropped). A follow-up file whose parent was the skipped link is created as a real subdirectory *inside* the output dir - instead. `unpack_in` additionally canonicalizes each entry's parent and - blocks writing through any pre-existing symlink. + instead. + +A symlink the archive did not bring is a separate case, and until v0.7.0 only +tar defended against it. Extracting into a directory that **already** holds a +symlink named `link` and an archive holding `link/evil.txt` wrote straight +through it and returned success: tar was immune because `unpack_in` +canonicalizes each entry's parent, while zip and 7z sanitized the entry name, +joined it to the output and wrote, with no check on where the join had landed. +Extracting into a directory that already has a symlink is ordinary, so this was +reachable without a hostile archive doing anything but naming a path. + +Every write site now resolves the directory it is about to write into and +refuses one that came out from under the output directory (`ensure_inside`). +This also covers a second case a name-only rule cannot see: on Windows, +`PathBuf::push` replaces what it holds when handed a component that parses as a +drive, so `docs/c:evil.txt` would resolve against the current directory of C:. +`sanitize_entry_path` additionally re-parses each component and requires it to +still be exactly one `Normal` component, which closes that at the source. **Covered by** `tar_symlink_write_through_does_not_escape`, `zip_symlink_entry_is_not_materialized_as_symlink`, -`tar_symlink_entry_is_not_materialized`. +`tar_symlink_entry_is_not_materialized`, +`no_format_writes_through_a_symlink_already_in_the_output`, +`a_renamed_entry_cannot_be_written_through_such_a_symlink`, +`a_colon_in_a_later_component_cannot_clear_the_path_being_built`. ### 3. Hardlink escape (tar) @@ -96,6 +115,46 @@ materializes a symlink, so nothing is planted. --- +### 4b. Entry names this host cannot write + +**Attack.** An entry name that a filesystem does not reject but *reinterprets*. +The colon is the one that matters: on Windows `notes.txt:hidden` names the +`hidden` alternate data stream of `notes.txt`, so the write succeeds, the bytes +land somewhere no listing shows, and the extractor reports a file that exists +nowhere (issue #63). Reserved device names (`CON`, `NUL`, `COM1`, reserved with +an extension too) and trailing dots or spaces are the quieter members of the +same family: the host silently gives the file a different name than the archive +asked for. + +**Prevention.** Extraction judges every entry name against the host's rules +before anything is written, and refuses rather than guessing. A character the +host cannot hold is a question for the user, who supplies a replacement; the two +adjustments nobody can be asked about (a trailing run, a device name) are +applied and reported. The whole listing is planned before the first byte, so an +unanswerable name or two entries that would collide leave the output directory +as they were found. + +The rules are **data**, not `#[cfg]`, so a Mac can be asked what a Windows host +would refuse, which is what makes them testable at all: nobody working on this +repository runs Windows day to day. + +That property is only as good as what it runs over, and v0.7.0 shipped a hole +worth recording. Entry names were split into components with +`std::path::Path::components`, which *is* `#[cfg]`-dependent. On Windows a +leading `a:` parses as a drive prefix rather than a component, so it was +discarded before any rule saw it: `a:b/c.txt` was reported clean, and written as +`b/c.txt`. The colon defence had a hole on the only platform it exists for. An +archive entry name is not a host path (ZIP mandates `/`, APPNOTE 4.4.17.1, and +tar has used it since v7), so the split is now on `/` on every machine, and a +backslash is judged as what it is: an ordinary character, legal on Unix, refused +by Windows. + +**Covered by** `apps/core/tests/names.rs`, in particular +`an_entry_splits_the_same_way_on_every_host`, +`the_report_sees_a_colon_in_the_first_component_too`, +`only_windows_refuses_a_backslash_inside_a_component`, and +`a_unix_name_holding_a_backslash_survives_the_round_trip`. + ## Compression measures ### 5. Symlinks are never followed out of the tree