diff --git a/Cargo.lock b/Cargo.lock index b531fce60..c7fe9c680 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1025,7 +1025,7 @@ name = "craft-epub" version = "0.1.0" dependencies = [ "craft-xml", - "flate2", + "craft-zip", "thiserror 2.0.21", "zip", ] @@ -1185,7 +1185,7 @@ name = "craft-idml" version = "0.1.0" dependencies = [ "craft-xml", - "flate2", + "craft-zip", "thiserror 2.0.21", "zip", ] diff --git a/crates/epub/Cargo.toml b/crates/epub/Cargo.toml index 690a89778..67875fe89 100644 --- a/crates/epub/Cargo.toml +++ b/crates/epub/Cargo.toml @@ -13,8 +13,9 @@ workspace = true [dependencies] craft-xml = { workspace = true } thiserror = { workspace = true } -# `deflate-flate2` with flate2's default miniz_oxide backend, as craft-update pins it (zip's own -# `deflate` feature would switch a whole app's flate2 to zlib-rs). +craft-zip = { workspace = true } + +[dev-dependencies] +# The tests build hostile archives by hand. zip = { version = "9", default-features = false, features = ["deflate-flate2"] } -# The backend of zip's deflate: `deflate-flate2` leaves the choice to the build. -flate2 = { version = "1", default-features = false, features = ["rust_backend"] } +craft-zip = { workspace = true, features = ["test-support"] } diff --git a/crates/epub/README.md b/crates/epub/README.md index c15c13876..c9ba7d134 100644 --- a/crates/epub/README.md +++ b/crates/epub/README.md @@ -94,7 +94,8 @@ for path in &spine { | `max_total_bytes` (all entries read from one package) | 256 MiB | `Error::TooLarge` | | `max_manifest`, `max_spine` | 10,000 each | further items are ignored | -Sizes are checked against what an entry really expands to, not what the archive declares. The two +The ZIP layer is craft-zip: the declared entry count is checked before the archive is indexed, and +sizes are checked against what an entry really expands to, not what the archive declares. The two XML documents are read with craft-xml's limits (depth 256, 2,000,000 nodes) and `max_entry_bytes`. ## Where it came from diff --git a/crates/epub/src/read.rs b/crates/epub/src/read.rs index ac93c5b0e..0540e70f8 100644 --- a/crates/epub/src/read.rs +++ b/crates/epub/src/read.rs @@ -1,7 +1,6 @@ //! An EPUB 2 or 3 package, read: container, package document, and bounded entry reads. use std::collections::HashMap; -use std::io::{Cursor, Read}; use craft_xml::{El, Local, Options}; @@ -224,10 +223,8 @@ pub fn parse_package(xml: &[u8], opf_dir: &str, limits: &Limits) -> Result { - zip: zip::ZipArchive>, + zip: craft_zip::Archive<'a>, limits: Limits, - /// Bytes the entries still to be read may expand to. - left: u64, /// The archive path of the package document. pub opf_path: String, pub package: Package, @@ -242,11 +239,16 @@ impl<'a> Epub<'a> { /// Reads the container and the package document. The `mimetype` entry is not required: /// packages without one, or with it compressed, are common. pub fn open_with(bytes: &'a [u8], limits: Limits) -> Result { - let zip = zip::ZipArchive::new(Cursor::new(bytes)).map_err(|e| Error::NotEpub(e.to_string()))?; - if zip.len() > limits.max_entries { - return Err(Error::TooManyEntries { found: zip.len(), limit: limits.max_entries }); - } - let mut epub = Epub { zip, limits, left: limits.max_total_bytes, opf_path: String::new(), package: Package::default() }; + let zip = + craft_zip::Archive::open(bytes, craft_zip::Limits { max_entries: limits.max_entries as u64, max_total_bytes: limits.max_total_bytes }) + .map_err(|e| match e { + craft_zip::Error::TooManyEntries { declared, limit } => Error::TooManyEntries { + found: usize::try_from(declared).unwrap_or(usize::MAX), + limit: usize::try_from(limit).unwrap_or(usize::MAX), + }, + e => Error::NotEpub(e.to_string()), + })?; + let mut epub = Epub { zip, limits, opf_path: String::new(), package: Package::default() }; let container = epub.read("META-INF/container.xml")?.ok_or_else(|| Error::NotEpub("META-INF/container.xml is missing".into()))?; let opf_path = parse_container(&container)?.ok_or_else(|| Error::NotEpub("container.xml names no package document".into()))?; // The container gives a path from the archive's root; resolving it also drops a leading `/`. @@ -262,28 +264,20 @@ impl<'a> Epub<'a> { /// Does the archive have an entry `path`? pub fn contains(&self, path: &str) -> bool { - self.zip.index_for_name(path).is_some() + self.zip.contains(path) } /// The bytes of archive entry `path`, or `None` when the archive has no such entry. An entry /// past the limits is an error, and so is one that expands to more than it declared. pub fn read(&mut self, path: &str) -> Result>, Error> { - let Some(index) = self.zip.index_for_name(path).filter(|_| !path.ends_with('/')) else { return Ok(None) }; - let part = |msg: String| Error::Part { part: path.to_string(), msg }; - let f = self.zip.by_index(index).map_err(|e| part(e.to_string()))?; - let limit = self.limits.max_entry_bytes.min(self.left); - let too_large = || Error::TooLarge { part: path.to_string(), limit }; - if f.size() > limit { - return Err(too_large()); + if path.ends_with('/') { + return Ok(None); } - let mut buf = Vec::new(); - f.take(limit.saturating_add(1)).read_to_end(&mut buf).map_err(|e| part(e.to_string()))?; - let n = buf.len() as u64; - if n > limit { - return Err(too_large()); - } - self.left = self.left.saturating_sub(n); - Ok(Some(buf)) + let limit = self.limits.max_entry_bytes.min(self.zip.budget().remaining()); + self.zip.read(path, self.limits.max_entry_bytes).map_err(|e| match e { + craft_zip::Error::EntryTooLarge { .. } | craft_zip::Error::BudgetSpent { .. } => Error::TooLarge { part: path.to_string(), limit }, + e => Error::Part { part: path.to_string(), msg: e.to_string() }, + }) } /// [`Epub::read`] for what a file of the package refers to: `href` as written in the file at @@ -295,6 +289,6 @@ impl<'a> Epub<'a> { /// Bytes the entries still to be read may expand to. pub fn budget_left(&self) -> u64 { - self.left + self.zip.budget().remaining() } } diff --git a/crates/epub/src/write.rs b/crates/epub/src/write.rs index 0c77951a8..fbc741b55 100644 --- a/crates/epub/src/write.rs +++ b/crates/epub/src/write.rs @@ -3,7 +3,6 @@ use std::borrow::Cow; use std::collections::HashSet; use std::fmt::Write as _; -use std::io::Write as _; use craft_xml::{esc, esc_attr, is_name}; @@ -316,10 +315,8 @@ pub fn write(book: &Book) -> Result, Error> { w3cdtf(&book.metadata.modified).ok_or_else(|| Error::Invalid(format!("`{}` is not a modification date", book.metadata.modified)))?; let opf = package_document(book, &modified); let nav = nav_document(book); - let fail = |e: &dyn std::fmt::Display| Error::Invalid(format!("the archive could not be written: {e}")); - let mut z = zip::ZipWriter::new(std::io::Cursor::new(Vec::new())); - let stored = zip::write::SimpleFileOptions::default().compression_method(zip::CompressionMethod::Stored); - let deflated = zip::write::SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated); + let fail = |e: craft_zip::Error| Error::Invalid(e.to_string()); + let mut z = craft_zip::Writer::new(); let own = [ ("mimetype".to_string(), MIMETYPE.as_bytes(), true), ("META-INF/container.xml".to_string(), CONTAINER.as_bytes(), false), @@ -328,10 +325,7 @@ pub fn write(book: &Book) -> Result, Error> { ]; let resources = book.resources.iter().map(|r| (format!("{ROOT}/{}", r.path), r.data.as_ref(), r.stored)); for (name, data, store) in own.into_iter().chain(resources) { - // More than 4 GiB needs the ZIP64 form. - let options = if store { stored } else { deflated }.large_file(data.len() >= u32::MAX as usize); - z.start_file(name, options).map_err(|e| fail(&e))?; - z.write_all(data).map_err(|e| fail(&e))?; + z.add(&name, data, if store { craft_zip::Compression::Stored } else { craft_zip::Compression::Deflated }).map_err(fail)?; } - z.finish().map(std::io::Cursor::into_inner).map_err(|e| fail(&e)) + z.finish().map_err(fail) } diff --git a/crates/epub/tests/read.rs b/crates/epub/tests/read.rs index 88fb27df6..dd1999ce4 100644 --- a/crates/epub/tests/read.rs +++ b/crates/epub/tests/read.rs @@ -252,3 +252,11 @@ fn a_package_document_past_the_entry_limit_is_refused() { let limits = Limits { max_entry_bytes: (xml.len() - 1) as u64, ..Limits::DEFAULT }; assert!(matches!(parse_package(&xml, "", &limits), Err(Error::Part { .. }))); } + +/// A ZIP64 end record can declare millions of entries in a small file; the count is refused on +/// the raw bytes, before the archive is indexed. +#[test] +fn an_archive_declaring_too_many_entries_is_refused_before_it_is_indexed() { + let bomb = craft_zip::zip64_entry_count_sample(100_001); + assert_eq!(Epub::open(&bomb).err(), Some(Error::TooManyEntries { found: 100_001, limit: 100_000 })); +} diff --git a/crates/idml/Cargo.toml b/crates/idml/Cargo.toml index 660456229..41df76d7e 100644 --- a/crates/idml/Cargo.toml +++ b/crates/idml/Cargo.toml @@ -13,8 +13,9 @@ workspace = true [dependencies] craft-xml = { workspace = true } thiserror = { workspace = true } -# `deflate-flate2` with flate2's default miniz_oxide backend, as craft-update pins it (zip's own -# `deflate` feature would switch a whole app's flate2 to zlib-rs). +craft-zip = { workspace = true } + +[dev-dependencies] +# The tests build hostile archives by hand. zip = { version = "9", default-features = false, features = ["deflate-flate2"] } -# The backend of zip's deflate: `deflate-flate2` leaves the choice to the build. -flate2 = { version = "1", default-features = false, features = ["rust_backend"] } +craft-zip = { workspace = true, features = ["test-support"] } diff --git a/crates/idml/README.md b/crates/idml/README.md index 0ef741bf2..16435a361 100644 --- a/crates/idml/README.md +++ b/crates/idml/README.md @@ -53,7 +53,8 @@ for line in &doc.warnings { /* a part designmap.xml lists and the package lacks | `xml.max_depth` | 256 | `Error::Part` | | `xml.max_nodes`, `xml.max_attrs_total` (one part) | 8,000,000 and 32,000,000 | `Error::Part` | -Sizes are checked against what a part really expands to, not what the archive declares. +The ZIP layer is craft-zip: the declared entry count is checked before the archive is indexed, and +sizes are checked against what a part really expands to, not what the archive declares. ## Writing diff --git a/crates/idml/src/package.rs b/crates/idml/src/package.rs index c8b6ebec2..dab9d3626 100644 --- a/crates/idml/src/package.rs +++ b/crates/idml/src/package.rs @@ -1,7 +1,6 @@ //! The ZIP container: bounded part reads, lenient part names, designmap includes, and the writer. use std::collections::HashMap; -use std::io::{Cursor, Read, Write}; use craft_xml::El; @@ -34,12 +33,10 @@ impl Default for Limits { /// An IDML package whose parts are decompressed only when asked for, within byte budgets that /// don't trust the sizes the archive claims. pub struct Package<'a> { - zip: zip::ZipArchive>, + zip: craft_zip::Archive<'a>, /// Normalised part name → entry name, first entry wins (zip order, so the choice is stable). lenient: HashMap, limits: Limits, - /// Bytes the parts still to be read may expand to. - left: u64, } /// `designmap.xml` with the parts it includes. @@ -63,46 +60,40 @@ impl<'a> Package<'a> { Self::open_with(bytes, Limits::DEFAULT) } + /// The declared entry count is checked against [`Limits::max_entries`] before the archive + /// is indexed. pub fn open_with(bytes: &'a [u8], limits: Limits) -> Result { - let zip = zip::ZipArchive::new(Cursor::new(bytes)).map_err(|e| Error::NotIdml(e.to_string()))?; - if zip.len() > limits.max_entries { - return Err(Error::TooManyEntries { found: zip.len(), limit: limits.max_entries }); - } + let zip = + craft_zip::Archive::open(bytes, craft_zip::Limits { max_entries: limits.max_entries as u64, max_total_bytes: limits.max_total_bytes }) + .map_err(|e| match e { + craft_zip::Error::TooManyEntries { declared, limit } => Error::TooManyEntries { + found: usize::try_from(declared).unwrap_or(usize::MAX), + limit: usize::try_from(limit).unwrap_or(usize::MAX), + }, + e => Error::NotIdml(e.to_string()), + })?; let mut lenient: HashMap = HashMap::new(); - for i in 0..zip.len() { - let Some(Ok(name)) = zip.name_for_index(i) else { continue }; - if name.ends_with('/') { - continue; - } + for name in zip.names().filter(|n| !n.ends_with('/')) { lenient.entry(normalize_part_name(&name)).or_insert_with(|| name.into_owned()); } - Ok(Package { zip, lenient, limits, left: limits.max_total_bytes }) + Ok(Package { zip, lenient, limits }) } /// The bytes of part `name` (matched exactly, then leniently: writers differ in /// percent-encoding, case, path separators and a leading `./` or `/`), or `None` if the /// package has no such part. pub fn part(&mut self, name: &str) -> Result>> { - let exact = self.zip.index_for_name(name).filter(|_| !name.ends_with('/')); - let Some(i) = exact.or_else(|| self.lenient.get(&normalize_part_name(name)).and_then(|n| self.zip.index_for_name(n))) else { + let exact = (self.zip.contains(name) && !name.ends_with('/')).then_some(name); + let Some(stored) = exact.or_else(|| self.lenient.get(&normalize_part_name(name)).map(String::as_str)) else { return Ok(None); }; - let part = self.zip.name_for_index(i).and_then(|n| n.ok()).map_or_else(|| name.to_string(), |n| n.into_owned()); - let f = self.zip.by_index(i).map_err(|e| Error::NotIdml(e.to_string()))?; - let limit = self.limits.max_part_bytes.min(self.left); - if f.size() > limit { - return Err(Error::TooLarge { part, limit }); - } - let mut buf = Vec::new(); - if let Err(e) = f.take(limit.saturating_add(1)).read_to_end(&mut buf) { - return Err(Error::Part { part, msg: e.to_string() }); - } - let n = buf.len() as u64; - if n > limit { - return Err(Error::TooLarge { part, limit }); - } - self.left = self.left.saturating_sub(n); - Ok(Some(buf)) + let limit = self.limits.max_part_bytes.min(self.zip.budget().remaining()); + let stored = stored.to_string(); + self.zip.read(&stored, self.limits.max_part_bytes).map_err(|e| match e { + craft_zip::Error::EntryTooLarge { .. } | craft_zip::Error::BudgetSpent { .. } => Error::TooLarge { part: stored.clone(), limit }, + craft_zip::Error::Entry { msg, .. } => Error::Part { part: stored.clone(), msg }, + e => Error::NotIdml(e.to_string()), + }) } /// Part `name` as an element tree ([`crate::parse_part`]). @@ -114,7 +105,7 @@ impl<'a> Package<'a> { /// Bytes the parts still to be read may expand to. pub fn budget_left(&self) -> u64 { - self.left + self.zip.budget().remaining() } /// Reads `designmap.xml` and every part it includes. @@ -173,19 +164,14 @@ fn normalize_part_name(name: &str) -> String { } /// Zips an IDML package: the stored `mimetype` first, then `parts` in order as (name, bytes). +/// Equal parts give equal bytes. pub fn write_package, D: AsRef<[u8]>>(parts: &[(N, D)]) -> Result> { - use zip::write::SimpleFileOptions; - let fail = |e: &dyn std::fmt::Display| Error::Write(e.to_string()); - let mut w = zip::ZipWriter::new(Cursor::new(Vec::new())); - let stored = SimpleFileOptions::default().compression_method(zip::CompressionMethod::Stored); - let deflated = SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated); - w.start_file("mimetype", stored).map_err(|e| fail(&e))?; - w.write_all(MIMETYPE.as_bytes()).map_err(|e| fail(&e))?; + use craft_zip::Compression; + let fail = |e: craft_zip::Error| Error::Write(e.to_string()); + let mut w = craft_zip::Writer::new(); + w.add("mimetype", MIMETYPE.as_bytes(), Compression::Stored).map_err(fail)?; for (name, data) in parts { - let data = data.as_ref(); - // More than 4 GiB needs the ZIP64 form. - w.start_file(name.as_ref(), deflated.large_file(data.len() >= u32::MAX as usize)).map_err(|e| fail(&e))?; - w.write_all(data).map_err(|e| fail(&e))?; + w.add(name.as_ref(), data.as_ref(), Compression::Deflated).map_err(fail)?; } - w.finish().map(Cursor::into_inner).map_err(|e| fail(&e)) + w.finish().map_err(fail) } diff --git a/crates/idml/tests/package.rs b/crates/idml/tests/package.rs index 1c7ffd353..03a42d088 100644 --- a/crates/idml/tests/package.rs +++ b/crates/idml/tests/package.rs @@ -244,3 +244,16 @@ fn parts_are_written_one_element_per_line_without_touching_text() { ); assert!(with_props(El::new("A"), Vec::new()).children.is_empty()); } + +/// A ZIP64 end record can declare millions of entries in a small file; the count is refused on +/// the raw bytes, before the archive is indexed. +#[test] +fn a_package_declaring_too_many_entries_is_refused_before_it_is_indexed() { + let bomb = craft_zip::zip64_entry_count_sample(100_001); + assert_eq!(Package::open(&bomb).err(), Some(Error::TooManyEntries { found: 100_001, limit: 100_000 })); +} + +#[test] +fn equal_parts_give_equal_bytes() { + assert_eq!(sample(), sample()); +}