diff --git a/Cargo.lock b/Cargo.lock index 66accc510..2c97dfa99 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1059,6 +1059,7 @@ name = "craft-fig" version = "0.1.0" dependencies = [ "craft-kiwi", + "craft-zip", "flate2", "ruzstd", "serde_json", @@ -1257,7 +1258,7 @@ name = "craft-kra" version = "0.1.0" dependencies = [ "craft-xml", - "flate2", + "craft-zip", "thiserror 2.0.21", "zip", ] @@ -1356,6 +1357,7 @@ dependencies = [ "aes", "base64 0.23.1", "craft-xml", + "craft-zip", "flate2", "getrandom 0.3.4", "hmac", @@ -1387,7 +1389,7 @@ name = "craft-opc" version = "0.1.0" dependencies = [ "craft-xml", - "flate2", + "craft-zip", "log", "thiserror 2.0.21", "zip", @@ -1398,7 +1400,7 @@ name = "craft-ora" version = "0.1.0" dependencies = [ "craft-xml", - "flate2", + "craft-zip", "thiserror 2.0.21", "zip", ] @@ -1447,7 +1449,7 @@ dependencies = [ name = "craft-penpot" version = "0.1.0" dependencies = [ - "flate2", + "craft-zip", "serde_json", "thiserror 2.0.21", "zip", @@ -1630,7 +1632,7 @@ dependencies = [ name = "craft-sketch" version = "0.1.0" dependencies = [ - "flate2", + "craft-zip", "serde_json", "thiserror 2.0.21", "zip", diff --git a/crates/fig/Cargo.toml b/crates/fig/Cargo.toml index a2430ede0..19c25c069 100644 --- a/crates/fig/Cargo.toml +++ b/crates/fig/Cargo.toml @@ -14,9 +14,12 @@ workspace = true craft-kiwi = { workspace = true } serde_json = { 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). A shared bounded ZIP reader is -# planned (#126); until it lands this crate reads through `zip` directly. -zip = { version = "9", default-features = false, features = ["deflate-flate2"] } +craft-zip = { workspace = true } +# The canvas chunks are deflate or zlib streams. flate2 = { version = "1", default-features = false, features = ["rust_backend"] } ruzstd = { version = "0.9", default-features = false, features = ["std"] } + +[dev-dependencies] +# The tests build archives, hostile ones included, by hand. +zip = { version = "9", default-features = false, features = ["deflate-flate2"] } +craft-zip = { workspace = true, features = ["test-support"] } diff --git a/crates/fig/src/archive.rs b/crates/fig/src/archive.rs index a62ee6d30..e408ddbe7 100644 --- a/crates/fig/src/archive.rs +++ b/crates/fig/src/archive.rs @@ -1,9 +1,8 @@ //! The zip around `canvas.fig`. use std::cell::Cell; -use std::io::{Cursor, Read, Seek, Write}; -use zip::write::SimpleFileOptions; +use craft_zip::Compression; use crate::canvas::MAX_CANVAS; use crate::images::MAX_IMAGE_HASH; @@ -13,6 +12,8 @@ use crate::{Error, corrupt}; const MAX_META: u64 = 1 << 20; /// Largest file [`write_archive`] writes (the canvas and the images together). const MAX_FILE: u64 = 2 * 1024 * 1024 * 1024; +/// Most entries an archive may declare; checked before the archive is indexed. +pub const MAX_ENTRIES: u64 = 1_000_000; /// What one open may read from `images/`. #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -37,8 +38,7 @@ impl Default for Limits { /// Whether `bytes` look like a `.fig` file: a zip holding `canvas.fig`, or a bare canvas. pub fn is_fig(bytes: &[u8]) -> bool { - bytes.starts_with(b"fig-") - || (bytes.starts_with(b"PK") && zip::ZipArchive::new(Cursor::new(bytes)).is_ok_and(|z| z.index_for_name("canvas.fig").is_some())) + bytes.starts_with(b"fig-") || (bytes.starts_with(b"PK") && open_zip(bytes).is_ok_and(|z| z.contains("canvas.fig"))) } /// The parts of a `.fig` archive (or of a bare canvas, which has no images). @@ -50,7 +50,7 @@ pub struct Archive<'b> { /// `meta.json`'s `file_name`. pub file_name: Option, /// `images/` members (or nothing for a bare canvas). - zip: Option>>, + zip: Option>, max_image: u64, /// Bytes left for image reads. A reader that caches what it resolved, failures included, /// reads or counts each referenced member only once. @@ -69,7 +69,11 @@ impl<'b> Archive<'b> { if bytes.starts_with(b"fig-") { return Ok(Archive { canvas: bytes.to_vec(), file_name: None, zip: None, max_image, remaining, skipped }); } - let mut z = zip::ZipArchive::new(Cursor::new(bytes)).map_err(|e| corrupt(e.to_string()))?; + let mut z = open_zip(bytes).map_err(|e| corrupt(e.to_string()))?; + // `zip` keeps one entry of a repeated name; which one the writer meant is not known. + if z.has_duplicate_names() { + return Err(corrupt("two entries of the archive share a name")); + } let canvas = read_member(&mut z, "canvas.fig", MAX_CANVAS)?; let file_name = read_member(&mut z, "meta.json", MAX_META) .ok() @@ -85,7 +89,7 @@ impl<'b> Archive<'b> { /// Whether `images/` has a member for image `hash`. pub fn has_image(&self, hash: &str) -> bool { - self.zip.as_ref().is_some_and(|z| z.index_for_name(&format!("images/{hash}")).is_some()) + self.zip.as_ref().is_some_and(|z| z.contains(&format!("images/{hash}"))) } /// What is left of the total image budget ([`Limits::max_image_total`]). @@ -107,8 +111,8 @@ impl<'b> Archive<'b> { } // A clone shares the parsed directory and the bytes; the resolver only has shared access. let mut z = self.zip.clone()?; - let f = z.by_name(&format!("images/{hash}")).ok()?; - let size = f.size(); + let member = format!("images/{hash}"); + let size = z.declared_size(&member)?; if size > self.max_image { return None; } @@ -117,46 +121,32 @@ impl<'b> Archive<'b> { self.skipped.set(self.skipped.get() + 1); return None; } - let checksum = f.crc32(); - let mut data = Vec::new(); - // Probe past the per-image cap only when the total budget has room. Even a dishonest - // size cannot make the read exceed the remaining total; failed reads keep their charge. - let limit = remaining.min(self.max_image.saturating_add(1)); - let read = f.take(limit).read_to_end(&mut data); - let used = data.len() as u64; - self.remaining.set(remaining - used); - if used > size && used == remaining { - self.skipped.set(self.skipped.get() + 1); - } - if read.is_err() || used != size { - return None; - } - // `take` can stop exactly at the budget without asking ZIP for EOF (where it checks CRC). - // Check it here too, without spending an extra byte on an overflow probe. - if used == limit { - let mut crc = flate2::Crc::new(); - crc.update(&data); - if crc.sum() != checksum { - return None; - } - } - Some(data) + // The read stops one byte past the declared size, so a failed read (a bad checksum, a + // stream that says it is shorter than it is) cost at most that: it keeps the charge. + let read = z.read(&member, size); + let used = match &read { + Ok(Some(data)) => data.len() as u64, + _ => size, + }; + self.remaining.set(remaining.saturating_sub(used)); + read.ok().flatten() } } +/// The archive, its declared entry count checked before it is indexed. The image budget is this +/// crate's own ([`Limits`]), so the archive's total is not used. +fn open_zip(bytes: &[u8]) -> Result, craft_zip::Error> { + craft_zip::Archive::open(bytes, craft_zip::Limits { max_entries: MAX_ENTRIES, max_total_bytes: u64::MAX }) +} + /// Member `name` of `z`, refused when it is (or unpacks to) more than `limit` bytes. -fn read_member(z: &mut zip::ZipArchive, name: &str, limit: u64) -> Result, Error> { - let f = z.by_name(name).map_err(|e| corrupt(format!("{name}: {e}")))?; - let too_large = || corrupt(format!("{name} is too large")); - if f.size() > limit { - return Err(too_large()); +fn read_member(z: &mut craft_zip::Archive<'_>, name: &str, limit: u64) -> Result, Error> { + match z.read(name, limit) { + Ok(Some(data)) => Ok(data), + Ok(None) => Err(corrupt(format!("{name}: not in the archive"))), + Err(craft_zip::Error::EntryTooLarge { .. }) => Err(corrupt(format!("{name} is too large"))), + Err(e) => Err(corrupt(e.to_string())), } - let mut data = Vec::new(); - f.take(limit.saturating_add(1)).read_to_end(&mut data).map_err(|e| corrupt(format!("{name}: {e}")))?; - if data.len() as u64 > limit { - return Err(too_large()); - } - Ok(data) } /// What [`write_archive`] puts in a `.fig` file. @@ -178,15 +168,11 @@ pub struct Package<'a> { /// (stored, as they are compressed already; `meta.json` deflated). pub fn write_archive(package: &Package) -> Result, Error> { let io = |e: &dyn std::fmt::Display| Error::Io(e.to_string()); - let mut buf = Cursor::new(Vec::new()); + let mut z = craft_zip::Writer::new(); + let (stored, deflated) = (Compression::Stored, Compression::Level(3)); { - let mut z = zip::ZipWriter::new(&mut buf); - let stored = SimpleFileOptions::default().compression_method(zip::CompressionMethod::Stored); - let deflated = SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated).compression_level(Some(3)); - let mut put = |name: &str, options: SimpleFileOptions, bytes: &[u8]| -> Result<(), Error> { - z.start_file(name, options).map_err(|e| io(&e))?; - z.write_all(bytes).map_err(|e| io(&e)) - }; + let mut put = + |name: &str, compression: Compression, bytes: &[u8]| -> Result<(), Error> { z.add(name, bytes, compression).map_err(|e| io(&e)) }; put("canvas.fig", stored, package.canvas)?; if let Some(t) = package.thumbnail { put("thumbnail.png", stored, t)?; @@ -213,7 +199,6 @@ pub fn write_archive(package: &Package) -> Result, Error> { } put(&format!("images/{name}"), stored, bytes)?; } - z.finish().map_err(|e| io(&e))?; } - Ok(buf.into_inner()) + z.finish().map_err(|e| io(&e)) } diff --git a/crates/fig/src/lib.rs b/crates/fig/src/lib.rs index b35ea327a..b8ea98615 100644 --- a/crates/fig/src/lib.rs +++ b/crates/fig/src/lib.rs @@ -30,7 +30,7 @@ mod tests; pub use craft_kiwi as kiwi; -pub use archive::{Archive, Limits, Package, is_fig, write_archive}; +pub use archive::{Archive, Limits, MAX_ENTRIES, Package, is_fig, write_archive}; pub use canvas::{ DESIGN_HEADER, JAM_HEADER, MAX_CANVAS, MAX_CHUNK, MAX_CHUNKS, MAX_SCHEMA, VERSION, chunks, decode_canvas, encode_canvas, inflate, value_budget, }; diff --git a/crates/fig/src/tests.rs b/crates/fig/src/tests.rs index ae31ac6ab..63b23d0b3 100644 --- a/crates/fig/src/tests.rs +++ b/crates/fig/src/tests.rs @@ -523,3 +523,29 @@ fn a_geometry_blob_round_trips_and_its_limits_hold() { assert_eq!(decode(&vec![0; path::MAX_COMMANDS]).map(|c| c.len()), Some(path::MAX_COMMANDS)); assert_eq!(decode(&vec![0; path::MAX_COMMANDS + 1]), None); } + +/// A ZIP64 end record can declare millions of entries in a small file: refused on the raw bytes +/// before the archive is indexed, and such a file is not taken for a `.fig`. +#[test] +fn an_archive_declaring_too_many_entries_is_refused_before_it_is_indexed() { + let bomb = craft_zip::zip64_entry_count_sample(MAX_ENTRIES + 1); + let e = Archive::open(&bomb).err().map(|e| e.to_string()).unwrap_or_default(); + assert!(e.contains(&format!("declares {} entries", MAX_ENTRIES + 1)), "{e}"); + assert!(!is_fig(&bomb)); +} + +/// Two entries of one name: `zip` keeps one, and which one the writer meant is not known. +#[test] +fn two_entries_of_one_name_are_refused() { + let zip = file(&[(1, image(1)), (2, image(2))]); + assert!(Archive::open(&zip).is_ok()); + let (one, two) = (name(1), name(2)); + let mut renamed = zip.clone(); + let mut i = 0; + while let Some(at) = renamed[i..].windows(two.len()).position(|w| w == two.as_bytes()) { + renamed[i + at..i + at + two.len()].copy_from_slice(one.as_bytes()); + i += at + two.len(); + } + let e = Archive::open(&renamed).err().map(|e| e.to_string()).unwrap_or_default(); + assert!(e.contains("two entries of the archive share a name"), "{e}"); +} diff --git a/crates/kra/Cargo.toml b/crates/kra/Cargo.toml index 5ea8219d4..aca592c28 100644 --- a/crates/kra/Cargo.toml +++ b/crates/kra/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 archives, hostile ones included, 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/kra/src/lib.rs b/crates/kra/src/lib.rs index 72557ba53..7485b9dd5 100644 --- a/crates/kra/src/lib.rs +++ b/crates/kra/src/lib.rs @@ -20,8 +20,6 @@ mod tests; pub mod tiles; -use std::io::{Cursor, Read}; - use craft_xml::El; use tiles::Space; @@ -56,6 +54,8 @@ pub const MAX_NODES: usize = 8192; pub const MAX_MAINDOC: usize = 32 << 20; /// Largest pixel file, inflated. pub const MAX_LAYER_FILE: usize = 1 << 30; +/// Most entries the archive may declare; checked before the archive is indexed. +pub const MAX_ENTRIES: u64 = 100_000; /// Largest ICC profile. pub const MAX_ICC: usize = 16 << 20; /// Deepest element nesting of `maindoc.xml`. @@ -65,19 +65,7 @@ pub const MAX_XML_DEPTH: usize = 512; /// bytes, which is how Krita (and OpenRaster, OpenDocument, EPUB) mark their archives so the /// type is readable without unzipping. pub fn zip_mimetype(bytes: &[u8]) -> Option<&[u8]> { - let u16_at = |o: usize| bytes.get(o..o + 2).and_then(|s| <[u8; 2]>::try_from(s).ok()).map(u16::from_le_bytes); - let u32_at = |o: usize| bytes.get(o..o + 4).and_then(|s| <[u8; 4]>::try_from(s).ok()).map(u32::from_le_bytes); - if u32_at(0)? != 0x0403_4b50 || u16_at(8)? != 0 { - return None; - } - let size = usize::try_from(u32_at(18)?).ok()?; - let name_len = usize::from(u16_at(26)?); - let extra_len = usize::from(u16_at(28)?); - if bytes.get(30..30 + name_len)? != b"mimetype" || size > 256 { - return None; - } - let start = 30 + name_len + extra_len; - bytes.get(start..start.checked_add(size)?) + craft_zip::stored_mimetype(bytes) } /// `true` for a Krita archive (by its `mimetype` entry). @@ -262,14 +250,19 @@ impl Image { /// An open Krita archive: the parsed `maindoc.xml` and access to the archive's other entries. pub struct Archive<'a> { - zip: zip::ZipArchive>, + zip: craft_zip::Archive<'a>, image: Image, } impl<'a> Archive<'a> { /// Opens the archive, checks its `mimetype` and reads `maindoc.xml`. pub fn open(bytes: &'a [u8]) -> Result> { - let mut zip = zip::ZipArchive::new(Cursor::new(bytes)).map_err(|e| invalid(e.to_string()))?; + let mut zip = + craft_zip::Archive::open(bytes, craft_zip::Limits { max_entries: MAX_ENTRIES, max_total_bytes: u64::MAX }).map_err(zip_error)?; + // `zip` keeps one entry of a repeated name; which one the writer meant is not known. + if zip.has_duplicate_names() { + return Err(invalid("two entries of the archive share a name")); + } let mime = read(&mut zip, "mimetype", 256)?.ok_or_else(|| invalid("the archive has no mimetype"))?; if mime.trim_ascii() != MIMETYPE { return Err(invalid("the archive's mimetype is not application/x-krita")); @@ -320,18 +313,14 @@ impl<'a> Archive<'a> { } } -fn read(zip: &mut zip::ZipArchive>, name: &str, max: usize) -> Result>> { - let Some(index) = zip.index_for_name(name) else { return Ok(None) }; - let file = zip.by_index(index).map_err(|e| invalid(format!("{name}: {e}")))?; - let too_large = || invalid(format!("{name}: larger than {max} bytes")); - if file.size() > max as u64 { - return Err(too_large()); +fn read(zip: &mut craft_zip::Archive<'_>, name: &str, max: usize) -> Result>> { + zip.read(name, max as u64).map_err(zip_error) +} + +fn zip_error(e: craft_zip::Error) -> Error { + match e { + craft_zip::Error::NotZip(m) => invalid(m), + craft_zip::Error::EntryTooLarge { name, limit } => invalid(format!("{name}: larger than {limit} bytes")), + e => invalid(e.to_string()), } - // The declared size is the archive's claim; the read is what is bounded. - let mut out = Vec::new(); - file.take(max as u64 + 1).read_to_end(&mut out).map_err(|e| invalid(format!("{name}: {e}")))?; - if out.len() > max { - return Err(too_large()); - } - Ok(Some(out)) } diff --git a/crates/kra/src/tests.rs b/crates/kra/src/tests.rs index b2bae41d4..31656e4e1 100644 --- a/crates/kra/src/tests.rs +++ b/crates/kra/src/tests.rs @@ -1,4 +1,4 @@ -use std::io::Write; +use std::io::{Cursor, Write}; use super::tiles::{Space, TileFile, lzf_decompress}; use super::*; @@ -374,3 +374,28 @@ fn the_mimetype_is_read_from_a_leading_stored_entry_only() { assert_eq!(Space::from_name("RGBA"), Some(Space::Rgba8)); assert_eq!(Space::from_name("RGBAF32"), None); } + +/// A ZIP64 end record can declare millions of entries in a small file: 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(MAX_ENTRIES + 1); + assert_eq!( + Archive::open(&bomb).err(), + Some(Error::Invalid(format!("the archive declares {} entries, more than {MAX_ENTRIES}", MAX_ENTRIES + 1))) + ); +} + +/// Two entries of one name: `zip` keeps one, and which one the writer meant is not known. +#[test] +fn two_entries_of_one_name_are_refused() { + let bytes = archive(&maindoc("RGBA", ""), &[("dupA", vec![1]), ("dupB", vec![2])]); + assert!(Archive::open(&bytes).is_ok()); + let mut renamed = bytes.clone(); + let mut i = 0; + while let Some(at) = renamed[i..].windows(4).position(|w| w == b"dupB") { + renamed[i + at + 3] = b'A'; + i += at + 4; + } + assert_eq!(Archive::open(&renamed).err(), Some(Error::Invalid("two entries of the archive share a name".into()))); +} diff --git a/crates/odf/Cargo.toml b/crates/odf/Cargo.toml index 33bfe5c2c..d6fb7bff2 100644 --- a/crates/odf/Cargo.toml +++ b/crates/odf/Cargo.toml @@ -13,9 +13,8 @@ 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). -zip = { version = "9", default-features = false, features = ["deflate-flate2"] } +# `test-support`: `zip_guard::zip64_entry_count_sample` is public here. +craft-zip = { workspace = true, features = ["test-support"] } flate2 = { version = "1", default-features = false, features = ["rust_backend"] } base64 = "0.23" # Password-protected packages (ODF 1.3 Part 2 section 3.4): AES-256-CBC, SHA-1/SHA-256, PBKDF2 @@ -27,6 +26,10 @@ hmac = "0.13" getrandom = "0.3" zeroize = "1" +[dev-dependencies] +# The tests build archives, hostile ones included, by hand. +zip = { version = "9", default-features = false, features = ["deflate-flate2"] } + [target.'cfg(all(target_arch = "wasm32", target_os = "unknown"))'.dependencies] # The browser's `crypto.getRandomValues` for the salts and vectors of encrypted saves. getrandom = { version = "0.3", features = ["wasm_js"] } diff --git a/crates/odf/src/crypt.rs b/crates/odf/src/crypt.rs index ff6eb7742..f90eb9ab9 100644 --- a/crates/odf/src/crypt.rs +++ b/crates/odf/src/crypt.rs @@ -10,7 +10,7 @@ //! [`decrypt`] gives a plain package, which a reader opens as any other; [`encrypt`] takes a //! plain package. Neither looks at what the files hold. -use std::io::{Cursor, Read, Write}; +use std::io::{Read, Write}; use aes::cipher::{BlockCipherDecrypt, BlockCipherEncrypt, KeyInit}; use base64::Engine as _; @@ -80,24 +80,29 @@ fn read_manifest(xml: &[u8]) -> Vec<(String, Params, Option)> { .collect() } -/// `name` read from `z`, at most [`MAX_ENTRY`] bytes of it. -fn read_entry(z: &mut zip::ZipArchive>, name: &str) -> Option> { - let f = z.by_name(name).ok()?; - if f.size() > MAX_ENTRY { - return None; - } - let mut out = Vec::new(); - f.take(MAX_ENTRY).read_to_end(&mut out).ok()?; - Some(out) +/// The package, its declared entry count checked before it is indexed (with +/// [`zip_guard`]'s wording). +fn open_zip(bytes: &[u8]) -> Result, CryptError> { + zip_guard::check(bytes, MAX_ZIP_ENTRIES).map_err(CryptError::Unsupported)?; + craft_zip::Archive::open(bytes, craft_zip::Limits { max_entries: MAX_ZIP_ENTRIES, max_total_bytes: u64::MAX }) + .map_err(|e| CryptError::Unsupported(format!("not a valid package ({e})"))) +} + +/// `name` read from `z`, at most [`MAX_ENTRY`] bytes of it. `None` when it is missing, damaged +/// or larger. +fn read_entry(z: &mut craft_zip::Archive<'_>, name: &str) -> Option> { + z.read(name, MAX_ENTRY).ok().flatten() +} + +/// The files of a package in its order, folders left out, each at most [`MAX_ENTRY`] bytes. +fn files_of(z: &craft_zip::Archive<'_>) -> Vec { + z.names().filter(|n| !n.ends_with('/')).map(|n| n.into_owned()).collect() } /// Does this package hold encrypted files? pub fn is_encrypted(bytes: &[u8]) -> bool { // A package with too many entries isn't parsed here: opening it refuses it (`zip_guard`). - if zip_guard::check(bytes, MAX_ZIP_ENTRIES).is_err() { - return false; - } - let Ok(mut z) = zip::ZipArchive::new(Cursor::new(bytes)) else { return false }; + let Ok(mut z) = open_zip(bytes) else { return false }; read_entry(&mut z, MANIFEST_PATH).is_some_and(|m| !read_manifest(&m).is_empty()) } @@ -235,25 +240,16 @@ fn plain_manifest(xml: &str) -> String { /// A password-protected package decrypted into a plain one. pub fn decrypt(bytes: &[u8], password: &str) -> Result, CryptError> { - let broken = |e: zip::result::ZipError| CryptError::Unsupported(format!("not a valid package ({e})")); - zip_guard::check(bytes, MAX_ZIP_ENTRIES).map_err(CryptError::Unsupported)?; - let mut z = zip::ZipArchive::new(Cursor::new(bytes)).map_err(broken)?; + let broken = |e: craft_zip::Error| CryptError::Unsupported(format!("not a valid package ({e})")); + let mut z = open_zip(bytes)?; let manifest = read_entry(&mut z, MANIFEST_PATH).unwrap_or_default(); let encrypted = read_manifest(&manifest); - let mut w = zip::ZipWriter::new(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 io = |e: std::io::Error| CryptError::Unsupported(e.to_string()); + let mut w = craft_zip::Writer::new(); let mut checked = false; let mut work = 0u64; - for i in 0..z.len().min(MAX_FILES) { - let Ok(mut f) = z.by_index(i) else { continue }; - let Ok(name) = f.name().map(|n| n.into_owned()) else { continue }; - if f.is_dir() { - continue; - } - let mut data = Vec::new(); - f.by_ref().take(MAX_ENTRY).read_to_end(&mut data).map_err(io)?; + for name in files_of(&z).into_iter().take(MAX_FILES) { + // A file past the limit is refused, not cut short. + let Some(data) = z.read(&name, MAX_ENTRY).map_err(broken)? else { continue }; let data = match encrypted.iter().find(|(p, ..)| *p == name) { Some((_, p, size)) => { charge(&mut work, p)?; @@ -264,13 +260,13 @@ pub fn decrypt(bytes: &[u8], password: &str) -> Result, CryptError> { None if name == MANIFEST_PATH => plain_manifest(&String::from_utf8_lossy(&data)).into_bytes(), None => data, }; - w.start_file(name.as_str(), if name == "mimetype" { stored } else { deflated }).map_err(broken)?; - w.write_all(&data).map_err(io)?; + let compression = if name == "mimetype" { craft_zip::Compression::Stored } else { craft_zip::Compression::Deflated }; + w.add(&name, &data, compression).map_err(broken)?; } if !checked { return Err(CryptError::Unsupported("no encrypted file could be read".into())); } - Ok(w.finish().map_err(broken)?.into_inner()) + w.finish().map_err(broken) } /// An attribute value's text (the manifest's own entity escapes). @@ -287,23 +283,17 @@ fn random(n: usize) -> Result, CryptError> { /// A package encrypted with `password`: every file but the `mimetype` and the manifest, the /// manifest saying how. pub fn encrypt(bytes: &[u8], password: &str) -> Result, CryptError> { - let err = |e: zip::result::ZipError| CryptError::Unsupported(e.to_string()); + let err = |e: craft_zip::Error| CryptError::Unsupported(e.to_string()); let io = |e: std::io::Error| CryptError::Unsupported(e.to_string()); - zip_guard::check(bytes, MAX_ZIP_ENTRIES).map_err(CryptError::Unsupported)?; - let mut z = zip::ZipArchive::new(Cursor::new(bytes)).map_err(err)?; + let mut z = open_zip(bytes)?; let start = start_key(SHA256, Some(32), password)?; let mut files: Vec<(String, Vec, bool)> = Vec::new(); let mut manifest = String::new(); // name → its encryption data and size, for the manifest. let mut data_of: Vec<(String, String)> = Vec::new(); - for i in 0..z.len() { - let mut f = z.by_index(i).map_err(err)?; - if f.is_dir() { - continue; - } - let Ok(name) = f.name().map(|n| n.into_owned()) else { continue }; - let mut data = Vec::new(); - f.read_to_end(&mut data).map_err(io)?; + for name in files_of(&z) { + // At most `MAX_ENTRY` a file: it was read without a limit. + let Some(data) = z.read(&name, MAX_ENTRY).map_err(err)? else { continue }; if name == MANIFEST_PATH { manifest = String::from_utf8_lossy(&data).into_owned(); continue; @@ -370,21 +360,17 @@ pub fn encrypt(bytes: &[u8], password: &str) -> Result, CryptError> { } None => out.push_str(rest), } - let mut w = zip::ZipWriter::new(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); + use craft_zip::Compression::{Deflated, Stored}; + let mut w = craft_zip::Writer::new(); for (name, data, _) in files.iter().filter(|(n, _, _)| n == "mimetype") { - w.start_file(name.as_str(), stored).map_err(err)?; - w.write_all(data).map_err(io)?; + w.add(name, data, Stored).map_err(err)?; } - w.start_file(MANIFEST_PATH, deflated).map_err(err)?; - w.write_all(out.as_bytes()).map_err(io)?; + w.add(MANIFEST_PATH, out.as_bytes(), Deflated).map_err(err)?; // Encrypted files are stored: they're deflated already (§3.4.1). for (name, data, _) in files.iter().filter(|(n, _, _)| n != "mimetype") { - w.start_file(name.as_str(), stored).map_err(err)?; - w.write_all(data).map_err(io)?; + w.add(name, data, Stored).map_err(err)?; } - Ok(w.finish().map_err(err)?.into_inner()) + w.finish().map_err(err) } #[cfg(test)] diff --git a/crates/odf/src/package.rs b/crates/odf/src/package.rs index 48272e85a..4249e7e15 100644 --- a/crates/odf/src/package.rs +++ b/crates/odf/src/package.rs @@ -1,8 +1,6 @@ //! The ZIP container (ODF 1.3 Part 2 section 3): a `mimetype` file first and stored, a //! manifest, and the document's files. -use std::io::{Cursor, Read, Write}; - use crate::manifest::{FileEntry, MANIFEST_PATH, Manifest}; use crate::{Error, zip_guard}; @@ -31,17 +29,16 @@ impl Default for Limits { /// The longest `mimetype` file read: media types are short, and a longer one is no media type. const MAX_MIMETYPE: u64 = 255; -pub(crate) fn zip_err(e: zip::result::ZipError) -> Error { - Error::Zip(e.to_string()) -} - -pub(crate) fn io_err(e: std::io::Error) -> Error { - Error::Zip(e.to_string()) +pub(crate) fn zip_err(e: craft_zip::Error) -> Error { + match e { + craft_zip::Error::NotZip(m) | craft_zip::Error::Write(m) => Error::Zip(m), + e => Error::Zip(e.to_string()), + } } /// An OpenDocument package open for reading. pub struct Package<'a> { - zip: zip::ZipArchive>, + zip: craft_zip::Archive<'a>, /// The files' names (folders and names that are not text left out), in the ZIP's order. names: Vec, limits: Limits, @@ -54,11 +51,22 @@ impl<'a> Package<'a> { /// [`Limits::max_entries`] before anything is parsed. No file is inflated yet. pub fn open(bytes: &'a [u8], limits: &Limits) -> Result, Error> { zip_guard::check(bytes, limits.max_entries).map_err(Error::TooManyEntries)?; - let zip = zip::ZipArchive::new(Cursor::new(bytes)).map_err(zip_err)?; - if zip.len() as u64 > limits.max_entries { - return Err(Error::TooManyEntries(format!("the package has {} entries, more than {}", zip.len(), limits.max_entries))); + // This reader keeps its own count (a damaged file is charged what it cost), so the + // archive's total is not used. + let zip = + craft_zip::Archive::open(bytes, craft_zip::Limits { max_entries: limits.max_entries, max_total_bytes: u64::MAX }).map_err( + |e| match e { + craft_zip::Error::TooManyEntries { declared, limit } => { + Error::TooManyEntries(format!("the package has {declared} entries, more than {limit}")) + } + e => zip_err(e), + }, + )?; + // `zip` keeps one file of a repeated name; which one the writer meant is not known. + if zip.has_duplicate_names() { + return Err(Error::Zip("two files of the package share a name".into())); } - let names = zip.file_names().filter_map(|n| n.ok()).filter(|n| !n.ends_with('/')).map(|n| n.into_owned()).collect(); + let names = zip.names().filter(|n| !n.ends_with('/')).map(|n| n.into_owned()).collect(); Ok(Package { zip, names, limits: *limits, spent: 0 }) } @@ -69,13 +77,13 @@ impl<'a> Package<'a> { /// Is there a file `name`? pub fn contains(&self, name: &str) -> bool { - self.zip.index_for_name(name).is_some() + self.zip.contains(name) } /// The size the ZIP declares for `name` (what it claims to inflate to; [`Package::read`] /// does not trust it). pub fn declared_size(&mut self, name: &str) -> Option { - self.zip.by_name(name).ok().map(|f| f.size()) + self.zip.declared_size(name) } /// File `name` inflated. `Ok(None)` when the package has no such file, or it cannot be @@ -84,29 +92,26 @@ impl<'a> Package<'a> { /// held to the same limits. pub fn read(&mut self, name: &str) -> Result>, Error> { let (max_entry, room) = (self.limits.max_entry, self.limits.max_total.saturating_sub(self.spent)); - let Ok(f) = self.zip.by_name(name) else { return Ok(None) }; - if f.size() > max_entry { + let Some(size) = self.zip.declared_size(name) else { return Ok(None) }; + if size > max_entry { return Err(Error::EntryTooLarge(name.to_string(), max_entry)); } - if f.size() > room { + if size > room { return Err(Error::TotalTooLarge(self.limits.max_total)); } - let cap = max_entry.min(room); - let mut out = Vec::new(); - // One byte past the cap tells a file at the cap from one beyond it. - if f.take(cap.saturating_add(1)).read_to_end(&mut out).is_err() { - self.spent = self.spent.saturating_add(out.len() as u64); - return Ok(None); + match self.zip.read(name, max_entry.min(room)) { + Ok(Some(out)) => { + self.spent = self.spent.saturating_add(out.len() as u64); + Ok(Some(out)) + } + Ok(None) => Ok(None), + // Damaged, or inflating past what it declares (the read stops one byte past that): + // unreadable, and charged its declared size. + Err(_) => { + self.spent = self.spent.saturating_add(size); + Ok(None) + } } - self.spent = self.spent.saturating_add(out.len() as u64); - if out.len() as u64 > cap { - return Err(if cap == max_entry { - Error::EntryTooLarge(name.to_string(), max_entry) - } else { - Error::TotalTooLarge(self.limits.max_total) - }); - } - Ok(Some(out)) } /// Bytes inflated from this package so far. @@ -117,15 +122,7 @@ impl<'a> Package<'a> { /// The package's media type: what its `mimetype` file holds, surrounding whitespace /// dropped. `None` when it has none, or one longer than a media type can be. pub fn mimetype(&mut self) -> Option { - let f = self.zip.by_name("mimetype").ok()?; - if f.size() > MAX_MIMETYPE { - return None; - } - let mut out = Vec::new(); - f.take(MAX_MIMETYPE + 1).read_to_end(&mut out).ok()?; - if out.len() as u64 > MAX_MIMETYPE { - return None; - } + let out = self.zip.read("mimetype", MAX_MIMETYPE).ok()??; self.spent = self.spent.saturating_add(out.len() as u64); Some(String::from_utf8_lossy(&out).trim().to_string()) } @@ -220,18 +217,14 @@ impl PackageWriter { /// The package's bytes. pub fn finish(self) -> Result, Error> { - let stored = zip::write::SimpleFileOptions::default().compression_method(zip::CompressionMethod::Stored); - let deflated = zip::write::SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated); - let mut z = zip::ZipWriter::new(Cursor::new(Vec::new())); - z.start_file("mimetype", stored).map_err(zip_err)?; - z.write_all(self.mime.as_bytes()).map_err(io_err)?; + use craft_zip::Compression::{Deflated, Stored}; + let mut z = craft_zip::Writer::new(); + z.add("mimetype", self.mime.as_bytes(), Stored).map_err(zip_err)?; let manifest = self.manifest_xml.unwrap_or_else(|| self.manifest.to_xml()); - z.start_file(MANIFEST_PATH, deflated).map_err(zip_err)?; - z.write_all(manifest.as_bytes()).map_err(io_err)?; + z.add(MANIFEST_PATH, manifest.as_bytes(), Deflated).map_err(zip_err)?; for e in &self.entries { - z.start_file(e.path.as_str(), if e.stored { stored } else { deflated }).map_err(zip_err)?; - z.write_all(&e.data).map_err(io_err)?; + z.add(&e.path, &e.data, if e.stored { Stored } else { Deflated }).map_err(zip_err)?; } - Ok(z.finish().map_err(zip_err)?.into_inner()) + z.finish().map_err(zip_err) } } diff --git a/crates/odf/src/tests.rs b/crates/odf/src/tests.rs index 7a7d2eaca..dd144ea61 100644 --- a/crates/odf/src/tests.rs +++ b/crates/odf/src/tests.rs @@ -368,3 +368,29 @@ fn hostile_flat_documents_do_not_panic() { // A package with no media type has no flat form. assert_eq!(flat::from_package(&zip_of(&[("content.xml", b"")]), &Limits::DEFAULT), Err(Error::Flat("the package has no media type"))); } + +/// Two files of one name: `zip` keeps one, and which one the writer meant is not known. +#[test] +fn two_files_of_one_name_are_refused() { + let bytes = zip_of(&[("mimetype", mime::SPREADSHEET.as_bytes()), ("dupA.xml", b""), ("dupB.xml", b"")]); + assert!(Package::open(&bytes, &Limits::DEFAULT).is_ok()); + let mut renamed = bytes.clone(); + let mut i = 0; + while let Some(at) = renamed[i..].windows(4).position(|w| w == b"dupB") { + renamed[i + at + 3] = b'A'; + i += at + 4; + } + assert_eq!(Package::open(&renamed, &Limits::DEFAULT).err(), Some(Error::Zip("two files of the package share a name".into()))); +} + +/// Encrypting and decrypting read each file within the per-file limit: a file declaring more is +/// refused before it is inflated (encrypting read every file whole; decrypting cut it short). +#[test] +fn encrypting_and_decrypting_refuse_a_file_past_the_limit() { + let over = u32::try_from(Limits::DEFAULT.max_entry + 1).unwrap(); + let bytes = lying_zip(b"", over); + assert!(crate::crypt::encrypt(&bytes, "pw").is_err()); + assert!(crate::crypt::decrypt(&bytes, "pw").is_err()); + // The same file declaring its real size encrypts. + assert!(crate::crypt::encrypt(&lying_zip(b"", 4), "pw").is_ok()); +} diff --git a/crates/odf/src/zip_guard.rs b/crates/odf/src/zip_guard.rs index fe31708d8..baa277465 100644 --- a/crates/odf/src/zip_guard.rs +++ b/crates/odf/src/zip_guard.rs @@ -1,188 +1,17 @@ -//! A bytes-only check of a ZIP package's declared entry count before the `zip` crate opens it. -//! [`crate::Package::open`] calls [`check`] first; a reader of any other ZIP package may too. -//! -//! `zip::ZipArchive::new` reserves room for, and parses, as many central directory headers as the -//! end record declares before its caller can look at `len()`. A ZIP64 end record can declare -//! millions of entries for a file of a few hundred megabytes (only 46 bytes each have to fit), and -//! each parsed entry costs several times that in memory. Here the end records are found the way -//! `zip` finds them (APPNOTE.TXT §4.3.14 to §4.3.16), and a package declaring more than the -//! reader's limit is refused before `zip` parses anything. Anything else that is wrong is left for -//! `zip` to report. +//! A bytes-only check of a ZIP package's declared entry count before the archive is indexed +//! ([`craft_zip::check_entry_count`]). [`crate::Package::open`] calls it first; a reader of any +//! other ZIP package may too. -const EOCD: &[u8; 4] = b"PK\x05\x06"; -const EOCD64_LOCATOR: &[u8; 4] = b"PK\x06\x07"; -const EOCD64: &[u8; 4] = b"PK\x06\x06"; -const CENTRAL_HEADER: &[u8; 4] = b"PK\x01\x02"; - -/// End records looked at, at most. A package has one; a few more can turn up inside stored data. -const MAX_END_RECORDS: usize = 16; - -/// Refuse `bytes` when the end record `zip` would use declares more than `max_entries` entries. -/// -/// `zip` searches the file backwards for an end of central directory record. A ZIP64 one points -/// through its locator at the ZIP64 end record, which `zip` looks for from the locator's offset -/// on: every such record there is checked, and as `zip` may fall back to an earlier end record -/// when one doesn't parse, the search goes on. A plain one is used when it declares no entries, or -/// a central directory header lies between its directory offset and itself: it is the last one -/// looked at. +/// Refuse `bytes` when the end record the `zip` crate would use declares more than `max_entries` +/// entries. pub fn check(bytes: &[u8], max_entries: u64) -> Result<(), String> { - let u16_at = |at: usize| bytes.get(at..at.checked_add(2)?).and_then(|b| <[u8; 2]>::try_from(b).ok()).map(u16::from_le_bytes); - let u32_at = |at: usize| bytes.get(at..at.checked_add(4)?).and_then(|b| <[u8; 4]>::try_from(b).ok()).map(u32::from_le_bytes); - let u64_at = |at: usize| bytes.get(at..at.checked_add(8)?).and_then(|b| <[u8; 8]>::try_from(b).ok()).map(u64::from_le_bytes); - let refuse = |n: u64| -> Result<(), String> { - if n > max_entries { Err(format!("the package declares {n} entries, more than {max_entries}")) } else { Ok(()) } - }; - let mut end = bytes.len(); - let mut seen = 0; - while let Some(at) = bytes.get(..end).and_then(|b| b.windows(4).rposition(|w| w == EOCD)) { - end = at; - seen += 1; - if seen > MAX_END_RECORDS { - return Err(format!("more than {MAX_END_RECORDS} end of central directory records")); - } - // `zip` skips a record whose comment runs past the end of the file. - let Some(comment) = u16_at(at + 20) else { continue }; - if at + 22 + usize::from(comment) > bytes.len() { - continue; - } - let (Some(on_disk), Some(total), Some(cd_offset)) = (u16_at(at + 8), u16_at(at + 10), u32_at(at + 16)) else { continue }; - let zip64 = total == u16::MAX || cd_offset == u32::MAX; - if zip64 - && let Some(locator) = at.checked_sub(20) - && bytes.get(locator..locator + 4) == Some(EOCD64_LOCATOR.as_slice()) - { - let (Some(eocd64), Some(disks)) = (u64_at(locator + 8), u32_at(locator + 16)) else { continue }; - let Some(from) = usize::try_from(eocd64).ok().filter(|&f| f < locator) else { continue }; - if disks > 1 { - continue; - } - let records = bytes.get(from..locator).unwrap_or_default().windows(4).enumerate().filter(|(_, w)| *w == EOCD64); - for (i, _) in records { - let p = from + i; - refuse(u64_at(p + 24).unwrap_or(0).max(u64_at(p + 32).unwrap_or(0)))?; - } - continue; - } - if total == 0 { - return Ok(()); - } - let cd = cd_offset as usize; - if cd < at && bytes.get(cd..at).is_some_and(|b| b.windows(4).any(|w| w == CENTRAL_HEADER)) { - return refuse(u64::from(on_disk.max(total))); - } - } - Ok(()) + craft_zip::check_entry_count(bytes, max_entries).map_err(|e| match e { + craft_zip::Error::TooManyEntries { declared, limit } => format!("the package declares {declared} entries, more than {limit}"), + e => e.to_string(), + }) } /// A package of one empty stored file, `a`, whose central directory lists it `entries` times (at -/// most as many as fit in 64 MiB) under a ZIP64 end record that declares them all: `zip` parses and -/// keeps every one of them before its caller can count them. Regression input for readers' tests. -pub fn zip64_entry_count_sample(entries: u64) -> Vec { - // Local file header (APPNOTE.TXT §4.3.7): version 10, stored, no data, name "a". - let mut f = b"PK\x03\x04".to_vec(); - f.extend_from_slice(&[10, 0, 0, 0, 0, 0, 0, 0, 0x21, 0]); // version, flags, method, time, date - f.extend_from_slice(&[0; 12]); // crc, sizes - f.extend_from_slice(&[1, 0, 0, 0]); // name and extra lengths - f.push(b'a'); - let cd_at = f.len() as u64; - // Its central header (§4.3.12), 47 bytes. - let mut header = CENTRAL_HEADER.to_vec(); - header.extend_from_slice(&[10, 0, 10, 0, 0, 0, 0, 0, 0, 0, 0x21, 0]); // versions, flags, method, time, date - header.extend_from_slice(&[0; 12]); // crc, sizes - header.extend_from_slice(&[1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]); // name, extra, comment, disk, attributes - header.extend_from_slice(&[0; 4]); // local header offset - header.push(b'a'); - let count = entries.min((64 << 20) / header.len() as u64); - for _ in 0..count { - f.extend_from_slice(&header); - } - let cd_len = f.len() as u64 - cd_at; - let eocd64_at = f.len() as u64; - // ZIP64 end of central directory record (§4.3.14), 56 bytes. - f.extend_from_slice(EOCD64); - f.extend_from_slice(&44u64.to_le_bytes()); // size of the rest of the record - f.extend_from_slice(&[45, 0, 45, 0]); // versions - f.extend_from_slice(&[0; 8]); // this disk, disk with the central directory - f.extend_from_slice(&entries.to_le_bytes()); // entries on this disk - f.extend_from_slice(&entries.to_le_bytes()); // entries - f.extend_from_slice(&cd_len.to_le_bytes()); - f.extend_from_slice(&cd_at.to_le_bytes()); - // Its locator (§4.3.15), 20 bytes. - f.extend_from_slice(EOCD64_LOCATOR); - f.extend_from_slice(&0u32.to_le_bytes()); - f.extend_from_slice(&eocd64_at.to_le_bytes()); - f.extend_from_slice(&1u32.to_le_bytes()); - // End of central directory record (§4.3.16), 22 bytes, each count and offset "see ZIP64". - f.extend_from_slice(EOCD); - f.extend_from_slice(&[0; 4]); - f.extend_from_slice(&[0xFF; 12]); - f.extend_from_slice(&[0; 2]); - f -} - -#[cfg(test)] -mod tests { - use super::{CENTRAL_HEADER, EOCD, check, zip64_entry_count_sample}; - - /// A plain package's tail: one central header at 0, then an end record declaring `entries`. - fn plain(entries: u16, comment: &[u8]) -> Vec { - let mut f = CENTRAL_HEADER.to_vec(); - f.resize(46, 0); - f.extend_from_slice(EOCD); - f.extend_from_slice(&[0, 0, 0, 0]); - f.extend_from_slice(&entries.to_le_bytes()); - f.extend_from_slice(&entries.to_le_bytes()); - f.extend_from_slice(&46u32.to_le_bytes()); - f.extend_from_slice(&0u32.to_le_bytes()); - f.extend_from_slice(&(comment.len() as u16).to_le_bytes()); - f.extend_from_slice(comment); - f - } - - #[test] - fn a_zip64_end_record_declaring_too_many_entries_is_refused() { - // `zip` would reserve and parse them all before the reader's cap. - let sample = zip64_entry_count_sample(20_001); - assert!(check(&sample, 20_000).is_err_and(|e| e.contains("20001 entries"))); - assert!(check(&zip64_entry_count_sample(5), 20_000).is_ok()); - // Junk before the package (a self-extracting stub) doesn't hide it. - let mut prefixed = vec![0x55u8; 4096]; - prefixed.extend_from_slice(&sample); - assert!(check(&prefixed, 20_000).is_err()); - } - - #[test] - fn a_plain_end_record_is_checked_as_zip_reads_it() { - assert!(check(&plain(3, b""), 20_000).is_ok()); - assert!(check(&plain(30_000, b"a comment"), 20_000).is_err_and(|e| e.contains("30000 entries"))); - // A record whose comment runs past the end is not the one `zip` uses. - let mut cut = plain(30_000, b""); - cut.truncate(cut.len() - 2); - cut.extend_from_slice(&500u16.to_le_bytes()); - assert!(check(&cut, 20_000).is_ok()); - assert!(check(b"not a package", 20_000).is_ok()); - assert!(check(b"", 20_000).is_ok()); - } - - #[test] - fn a_fake_end_record_after_the_real_one_does_not_hide_it() { - // `zip` skips a last record with no central header before it and uses the one before. - let mut f = zip64_entry_count_sample(1_000_000); - f.extend_from_slice(EOCD); - f.extend_from_slice(&[0, 0, 0, 0]); - f.extend_from_slice(&7u16.to_le_bytes()); - f.extend_from_slice(&7u16.to_le_bytes()); - f.extend_from_slice(&0u32.to_le_bytes()); - f.extend_from_slice(&((f.len() - 12) as u32).to_le_bytes()); - f.extend_from_slice(&0u16.to_le_bytes()); - assert!(check(&f, 20_000).is_err()); - } - - #[test] - fn many_end_records_are_refused() { - // Each declares one entry and has no central header before it, so `zip` would try them all. - let record = [0, 0, 0, 0, 1, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]; - let f: Vec = (0..20).flat_map(|_| EOCD.iter().copied().chain(record)).collect(); - assert!(check(&f, 20_000).is_err_and(|e| e.contains("end of central directory records"))); - } -} +/// most as many as fit in 64 MiB) under a ZIP64 end record that declares them all. Regression +/// input for readers' tests. +pub use craft_zip::zip64_entry_count_sample; diff --git a/crates/opc/Cargo.toml b/crates/opc/Cargo.toml index ee1cac1fd..0beb72c07 100644 --- a/crates/opc/Cargo.toml +++ b/crates/opc/Cargo.toml @@ -12,16 +12,15 @@ workspace = true [features] # `zip64_entry_count_sample`, the regression input for a reader's own entry-count test. -test-support = [] +test-support = ["craft-zip/test-support"] [dependencies] craft-xml = { workspace = true } log = "0.4" thiserror = { workspace = true } -# `deflate-flate2` with flate2's miniz_oxide backend (chosen below), as in craft-update: zip's own -# `deflate` feature would switch a whole app's flate2 to zlib-rs. -zip = { version = "9", default-features = false, features = ["deflate-flate2"] } -flate2 = { version = "1", default-features = false, features = ["rust_backend"] } +craft-zip = { workspace = true } [dev-dependencies] +craft-zip = { workspace = true, features = ["test-support"] } +# The tests build hostile archives by hand. zip = { version = "9", default-features = false, features = ["deflate-flate2"] } diff --git a/crates/opc/src/guard.rs b/crates/opc/src/guard.rs deleted file mode 100644 index 23e9bb8d6..000000000 --- a/crates/opc/src/guard.rs +++ /dev/null @@ -1,192 +0,0 @@ -//! A bytes-only check of a ZIP package's declared entry count before the `zip` crate opens it. -//! -//! `zip::ZipArchive::new` reserves room for, and parses, as many central directory headers as the -//! end record declares before its caller can look at `len()`. A ZIP64 end record can declare -//! millions of entries for a file of a few hundred megabytes (only 46 bytes each have to fit), and -//! each parsed entry costs several times that in memory. Here the end records are found the way -//! `zip` finds them (APPNOTE.TXT §4.3.14 to §4.3.16), and a package declaring more than the -//! reader's limit is refused before `zip` parses anything. Anything else that is wrong is left for -//! `zip` to report. - -use crate::Error; - -const EOCD: &[u8; 4] = b"PK\x05\x06"; -const EOCD64_LOCATOR: &[u8; 4] = b"PK\x06\x07"; -const EOCD64: &[u8; 4] = b"PK\x06\x06"; -const CENTRAL_HEADER: &[u8; 4] = b"PK\x01\x02"; - -/// End records looked at, at most. A package has one; a few more can turn up inside stored data. -const MAX_END_RECORDS: usize = 16; - -/// Refuse `bytes` when the end record `zip` would use declares more than `max_entries` entries. -/// -/// `zip` searches the file backwards for an end of central directory record. A ZIP64 one points -/// through its locator at the ZIP64 end record, which `zip` looks for from the locator's offset -/// on: every such record there is checked, and as `zip` may fall back to an earlier end record -/// when one doesn't parse, the search goes on. A plain one is used when it declares no entries, or -/// a central directory header lies between its directory offset and itself: it is the last one -/// looked at. -pub fn check_entry_count(bytes: &[u8], max_entries: u64) -> Result<(), Error> { - let u16_at = |at: usize| bytes.get(at..at.checked_add(2)?).map(|b| u16::from_le_bytes([b[0], b[1]])); - let u32_at = |at: usize| bytes.get(at..at.checked_add(4)?).map(|b| u32::from_le_bytes([b[0], b[1], b[2], b[3]])); - let u64_at = |at: usize| bytes.get(at..at.checked_add(8)?).and_then(|b| <[u8; 8]>::try_from(b).ok()).map(u64::from_le_bytes); - let refuse = - |n: u64| -> Result<(), Error> { if n > max_entries { Err(Error::TooManyEntries { declared: n, limit: max_entries }) } else { Ok(()) } }; - let mut end = bytes.len(); - let mut seen = 0; - while let Some(at) = bytes.get(..end).and_then(|b| b.windows(4).rposition(|w| w == EOCD)) { - end = at; - seen += 1; - if seen > MAX_END_RECORDS { - return Err(Error::TooManyEndRecords(MAX_END_RECORDS)); - } - // `zip` skips a record whose comment runs past the end of the file. - let Some(comment) = u16_at(at + 20) else { continue }; - if at + 22 + usize::from(comment) > bytes.len() { - continue; - } - let (Some(on_disk), Some(total), Some(cd_offset)) = (u16_at(at + 8), u16_at(at + 10), u32_at(at + 16)) else { continue }; - let zip64 = total == u16::MAX || cd_offset == u32::MAX; - if zip64 - && let Some(locator) = at.checked_sub(20) - && bytes.get(locator..locator + 4) == Some(EOCD64_LOCATOR.as_slice()) - { - let (Some(eocd64), Some(disks)) = (u64_at(locator + 8), u32_at(locator + 16)) else { continue }; - let Some(from) = usize::try_from(eocd64).ok().filter(|&f| f < locator) else { continue }; - if disks > 1 { - continue; - } - let records = bytes.get(from..locator).unwrap_or_default().windows(4).enumerate().filter(|(_, w)| *w == EOCD64); - for (i, _) in records { - let p = from + i; - refuse(u64_at(p + 24).unwrap_or(0).max(u64_at(p + 32).unwrap_or(0)))?; - } - continue; - } - if total == 0 { - return Ok(()); - } - let cd = cd_offset as usize; - if cd < at && bytes.get(cd..at).is_some_and(|b| b.windows(4).any(|w| w == CENTRAL_HEADER)) { - return refuse(u64::from(on_disk.max(total))); - } - } - Ok(()) -} - -/// A package of one empty stored file, `a`, whose central directory lists it `entries` times (at -/// most as many as fit in 64 MiB) under a ZIP64 end record that declares them all: `zip` -/// parses and keeps every one of them before its caller can count them. Regression input for the -/// readers' tests: built only for tests, this crate's and (through the `test-support` feature, -/// which they turn on as a dev-dependency) the readers', never into an app. -#[cfg(any(test, feature = "test-support"))] -pub fn zip64_entry_count_sample(entries: u64) -> Vec { - // Local file header (APPNOTE.TXT §4.3.7): version 10, stored, no data, name "a". - let mut f = b"PK\x03\x04".to_vec(); - f.extend_from_slice(&[10, 0, 0, 0, 0, 0, 0, 0, 0x21, 0]); // version, flags, method, time, date - f.extend_from_slice(&[0; 12]); // crc, sizes - f.extend_from_slice(&[1, 0, 0, 0]); // name and extra lengths - f.push(b'a'); - let cd_at = f.len() as u64; - // Its central header (§4.3.12), 47 bytes. - let mut header = CENTRAL_HEADER.to_vec(); - header.extend_from_slice(&[10, 0, 10, 0, 0, 0, 0, 0, 0, 0, 0x21, 0]); // versions, flags, method, time, date - header.extend_from_slice(&[0; 12]); // crc, sizes - header.extend_from_slice(&[1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]); // name, extra, comment, disk, attributes - header.extend_from_slice(&[0; 4]); // local header offset - header.push(b'a'); - let count = entries.min((64 << 20) / header.len() as u64); - for _ in 0..count { - f.extend_from_slice(&header); - } - let cd_len = f.len() as u64 - cd_at; - let eocd64_at = f.len() as u64; - // ZIP64 end of central directory record (§4.3.14), 56 bytes. - f.extend_from_slice(EOCD64); - f.extend_from_slice(&44u64.to_le_bytes()); // size of the rest of the record - f.extend_from_slice(&[45, 0, 45, 0]); // versions - f.extend_from_slice(&[0; 8]); // this disk, disk with the central directory - f.extend_from_slice(&entries.to_le_bytes()); // entries on this disk - f.extend_from_slice(&entries.to_le_bytes()); // entries - f.extend_from_slice(&cd_len.to_le_bytes()); - f.extend_from_slice(&cd_at.to_le_bytes()); - // Its locator (§4.3.15), 20 bytes. - f.extend_from_slice(EOCD64_LOCATOR); - f.extend_from_slice(&0u32.to_le_bytes()); - f.extend_from_slice(&eocd64_at.to_le_bytes()); - f.extend_from_slice(&1u32.to_le_bytes()); - // End of central directory record (§4.3.16), 22 bytes, each count and offset "see ZIP64". - f.extend_from_slice(EOCD); - f.extend_from_slice(&[0; 4]); - f.extend_from_slice(&[0xFF; 12]); - f.extend_from_slice(&[0; 2]); - f -} - -#[cfg(test)] -mod tests { - use super::{CENTRAL_HEADER, EOCD, check_entry_count as check, zip64_entry_count_sample}; - use crate::Error; - - /// A plain package's tail: one central header at 0, then an end record declaring `entries`. - fn plain(entries: u16, comment: &[u8]) -> Vec { - let mut f = CENTRAL_HEADER.to_vec(); - f.resize(46, 0); - f.extend_from_slice(EOCD); - f.extend_from_slice(&[0, 0, 0, 0]); - f.extend_from_slice(&entries.to_le_bytes()); - f.extend_from_slice(&entries.to_le_bytes()); - f.extend_from_slice(&46u32.to_le_bytes()); - f.extend_from_slice(&0u32.to_le_bytes()); - f.extend_from_slice(&(comment.len() as u16).to_le_bytes()); - f.extend_from_slice(comment); - f - } - - #[test] - fn a_zip64_end_record_declaring_too_many_entries_is_refused() { - // `zip` would reserve and parse them all before the reader's cap. - let sample = zip64_entry_count_sample(20_001); - assert_eq!(check(&sample, 20_000), Err(Error::TooManyEntries { declared: 20_001, limit: 20_000 })); - assert!(check(&zip64_entry_count_sample(5), 20_000).is_ok()); - // Junk before the package (a self-extracting stub) doesn't hide it. - let mut prefixed = vec![0x55u8; 4096]; - prefixed.extend_from_slice(&sample); - assert!(check(&prefixed, 20_000).is_err()); - } - - #[test] - fn a_plain_end_record_is_checked_as_zip_reads_it() { - assert!(check(&plain(3, b""), 20_000).is_ok()); - assert_eq!(check(&plain(30_000, b"a comment"), 20_000), Err(Error::TooManyEntries { declared: 30_000, limit: 20_000 })); - // A record whose comment runs past the end is not the one `zip` uses. - let mut cut = plain(30_000, b""); - cut.truncate(cut.len() - 2); - cut.extend_from_slice(&500u16.to_le_bytes()); - assert!(check(&cut, 20_000).is_ok()); - assert!(check(b"not a package", 20_000).is_ok()); - assert!(check(b"", 20_000).is_ok()); - } - - #[test] - fn a_fake_end_record_after_the_real_one_does_not_hide_it() { - // `zip` skips a last record with no central header before it and uses the one before. - let mut f = zip64_entry_count_sample(1_000_000); - f.extend_from_slice(EOCD); - f.extend_from_slice(&[0, 0, 0, 0]); - f.extend_from_slice(&7u16.to_le_bytes()); - f.extend_from_slice(&7u16.to_le_bytes()); - f.extend_from_slice(&0u32.to_le_bytes()); - f.extend_from_slice(&((f.len() - 12) as u32).to_le_bytes()); - f.extend_from_slice(&0u16.to_le_bytes()); - assert!(check(&f, 20_000).is_err()); - } - - #[test] - fn many_end_records_are_refused() { - // Each declares one entry and has no central header before it, so `zip` would try them all. - let record = [0, 0, 0, 0, 1, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]; - let f: Vec = (0..20).flat_map(|_| EOCD.iter().copied().chain(record)).collect(); - assert_eq!(check(&f, 20_000), Err(Error::TooManyEndRecords(16))); - } -} diff --git a/crates/opc/src/lib.rs b/crates/opc/src/lib.rs index a6b3c11f7..dec293322 100644 --- a/crates/opc/src/lib.rs +++ b/crates/opc/src/lib.rs @@ -33,15 +33,29 @@ #[doc = include_str!("../README.md")] struct ReadmeExamples; -mod guard; mod name; mod read; mod rels; mod write; -pub use guard::check_entry_count; #[cfg(any(test, feature = "test-support"))] -pub use guard::zip64_entry_count_sample; +pub use craft_zip::zip64_entry_count_sample; + +/// Refuse `bytes` when the end record the `zip` crate would use declares more than +/// `max_entries` entries, before anything is indexed ([`craft_zip::check_entry_count`]). +pub fn check_entry_count(bytes: &[u8], max_entries: u64) -> Result<(), Error> { + craft_zip::check_entry_count(bytes, max_entries).map_err(zip_error) +} + +fn zip_error(e: craft_zip::Error) -> Error { + match e { + craft_zip::Error::TooManyEntries { declared, limit } => Error::TooManyEntries { declared, limit }, + craft_zip::Error::TooManyEndRecords(n) => Error::TooManyEndRecords(n), + craft_zip::Error::Write(m) => Error::Write(m), + craft_zip::Error::NotZip(m) => Error::Zip(m), + e => Error::Zip(e.to_string()), + } +} pub use name::{dir_of, file_of, normalize, percent_decode, relative, rels_path, resolve, split_dir}; pub use read::{LazyPackage, Package, declared_oversized}; pub use rels::{ContentTypes, Rel, Rels, type_is}; diff --git a/crates/opc/src/read.rs b/crates/opc/src/read.rs index 0bb93f2e3..c080327ff 100644 --- a/crates/opc/src/read.rs +++ b/crates/opc/src/read.rs @@ -2,27 +2,25 @@ use std::collections::hash_map::Entry; use std::collections::{BTreeMap, HashMap, HashSet}; -use std::io::{Cursor, Read}; use std::sync::Arc; use craft_xml::{El, Options}; -use crate::guard::check_entry_count; use crate::name::{normalize, rels_path}; use crate::rels::{ContentTypes, Rels, targets_of}; use crate::{Error, Limits, Oversized}; -/// Most bytes reserved for a part before any of it has been read, whatever size it declares. -const MAX_PREALLOC: u64 = 16 * 1024 * 1024; +type Zip<'a> = craft_zip::Archive<'a>; -type Zip<'a> = zip::ZipArchive>; - -fn open_zip<'a>(bytes: &'a [u8], limits: &Limits) -> Result, Error> { - // Before `zip` parses every entry the end record declares. - check_entry_count(bytes, limits.max_entries as u64)?; - let zip = zip::ZipArchive::new(Cursor::new(bytes)).map_err(|e| Error::Zip(e.to_string()))?; - if zip.len() > limits.max_entries { - return Err(Error::TooManyEntries { declared: zip.len() as u64, limit: limits.max_entries as u64 }); +/// The archive, its declared entry count checked before it is indexed. `max_total` is the +/// archive's own total; a reader that keeps its own counts passes `u64::MAX`. +fn open_zip<'a>(bytes: &'a [u8], limits: &Limits, max_total: u64) -> Result, Error> { + let zip = craft_zip::Archive::open(bytes, craft_zip::Limits { max_entries: limits.max_entries as u64, max_total_bytes: max_total }) + .map_err(crate::zip_error)?; + // `zip` keeps one entry of a repeated name. Part names are compared by spelling rules this + // crate applies itself (see `Package::new`); an exact repeat is noted, as other spellings are. + if zip.has_duplicate_names() { + log::warn!("opc: the archive repeats an entry name; one of the entries is read"); } Ok(zip) } @@ -52,62 +50,55 @@ impl Package { /// inflate without limit. A part over a limit refuses the package or is left out /// ([`Limits::oversized`]). pub fn open(bytes: &[u8], limits: &Limits) -> Result { - let mut zip = open_zip(bytes, limits)?; + // This reader keeps its own two counts (bytes kept, bytes inflated), so the archive's is + // not used. + let mut zip = open_zip(bytes, limits, u64::MAX)?; let skip = limits.oversized == Oversized::Skip; let mut files = BTreeMap::new(); let (mut total, mut work) = (0u64, 0u64); - for i in 0..zip.len() { + let entries: Vec = zip.names().map(|n| n.into_owned()).collect(); + for (i, entry) in entries.iter().enumerate() { if skip && work >= limits.max_work { - log::warn!("opc: decompression budget spent; skipping the remaining {} entries", zip.len().saturating_sub(i)); + log::warn!("opc: decompression budget spent; skipping the remaining {} entries", entries.len().saturating_sub(i)); break; } - let mut f = match zip.by_index(i) { - Ok(f) => f, - Err(e) => { - log::warn!("opc: skipping unreadable zip entry {i}: {e}"); - continue; - } - }; - if f.is_dir() { + if entry.ends_with('/') { continue; } - let name = match f.name() { - Ok(name) => stored_name(&name), - Err(e) => { - log::warn!("opc: skipping zip entry {i} with an unreadable name: {e}"); - continue; - } - }; - if f.size() > limits.max_part { + let name = stored_name(entry); + let size = zip.declared_size(entry).unwrap_or(0); + if size > limits.max_part { if skip { - log::warn!("opc: skipping oversized part {name} ({} bytes)", f.size()); + log::warn!("opc: skipping oversized part {name} ({size} bytes)"); continue; } return Err(Error::PartTooLarge { part: name, limit: limits.max_part }); } let left = limits.max_total.saturating_sub(total).min(limits.max_work.saturating_sub(work)); let room = limits.max_part.min(left); - let mut buf = Vec::with_capacity(f.size().min(MAX_PREALLOC) as usize); - let read = (&mut f).take(room.saturating_add(1)).read_to_end(&mut buf); - // Charged whether or not the part is kept. - work = work.saturating_add(buf.len() as u64); - if buf.len() as u64 > room { + if size > room { + // It fits the part limit but not what is left: refused before inflating anything. if skip { log::warn!("opc: part {name} exceeds size limits; skipped"); continue; } - return Err(if left < limits.max_part { - Error::PackageTooLarge { limit: limits.max_total.min(limits.max_work) } - } else { - Error::PartTooLarge { part: name, limit: limits.max_part } - }); + return Err(Error::PackageTooLarge { limit: limits.max_total.min(limits.max_work) }); } - if let Err(e) = read { - log::warn!("opc: zip entry {name} unreadable: {e}"); - continue; + match zip.read(entry, room) { + Ok(Some(buf)) => { + work = work.saturating_add(buf.len() as u64); + total = total.saturating_add(buf.len() as u64); + files.insert(name, Arc::new(buf)); + } + Ok(None) => {} + // A damaged entry, or one that inflates past the size it declares: left out and + // charged what it may have inflated, its declared size (the read stops one byte + // past it). + Err(e) => { + work = work.saturating_add(size); + log::warn!("opc: zip entry {name} unreadable: {e}"); + } } - total = total.saturating_add(buf.len() as u64); - files.insert(name, Arc::new(buf)); } Ok(Package::new(files)) } @@ -207,8 +198,8 @@ impl Package { /// reader that needs a few parts of a large file, or one part at a time. pub struct LazyPackage<'a> { zip: Zip<'a>, - /// Normalized part name → zip index (the first entry of a name). - names: HashMap, + /// Normalized part name → entry name (the first entry of a name). + names: HashMap, total: u64, limits: Limits, } @@ -217,12 +208,11 @@ impl<'a> LazyPackage<'a> { /// Indexes the archive. Nothing is inflated; [`Limits::max_work`] and /// [`Limits::oversized`] are not used. pub fn open(bytes: &'a [u8], limits: &Limits) -> Result, Error> { - let zip = open_zip(bytes, limits)?; + // This reader keeps its own count, which a failed read spends (see `read_limited`). + let zip = open_zip(bytes, limits, u64::MAX)?; let mut names = HashMap::new(); - for i in 0..zip.len() { - if let Some(Ok(n)) = zip.name_for_index(i) { - names.entry(normalize(&n)).or_insert(i); - } + for n in zip.names() { + names.entry(normalize(&n)).or_insert_with(|| n.into_owned()); } Ok(LazyPackage { zip, names, total: 0, limits: *limits }) } @@ -270,31 +260,30 @@ impl<'a> LazyPackage<'a> { /// pay it again and again. Every later read is then a cheap error. pub fn read_limited(&mut self, part: &str, maximum: u64) -> Result>, Error> { let maximum = maximum.min(self.limits.max_part); - let Some(&idx) = self.names.get(&normalize(part)) else { + let Some(entry) = self.names.get(&normalize(part)) else { return Ok(None); }; - let mut f = self.zip.by_index(idx).map_err(|e| Error::Zip(format!("{part}: {e}")))?; - if f.size() > maximum { + let size = self.zip.declared_size(entry).unwrap_or(0); + if size > maximum { return Err(Error::PartTooLarge { part: part.to_string(), limit: maximum }); } let remaining = self.limits.max_total.saturating_sub(self.total); - let limit = maximum.min(remaining); - let mut out = Vec::with_capacity(f.size().min(MAX_PREALLOC) as usize); - let read = (&mut f).take(limit.saturating_add(1)).read_to_end(&mut out); - if out.len() as u64 > limit { + if size > remaining { self.total = self.limits.max_total; - return Err(if remaining < maximum { - Error::PackageTooLarge { limit: self.limits.max_total } - } else { - Error::PartTooLarge { part: part.to_string(), limit: maximum } - }); + return Err(Error::PackageTooLarge { limit: self.limits.max_total }); } - if let Err(e) = read { - self.total = self.limits.max_total; - return Err(Error::Zip(format!("{part}: {e}"))); + match self.zip.read(entry, maximum.min(remaining)) { + Ok(Some(out)) => { + self.total = self.total.saturating_add(out.len() as u64); + Ok(Some(out)) + } + Ok(None) => Ok(None), + // Damaged, or inflating past the size it declares. + Err(e) => { + self.total = self.limits.max_total; + Err(Error::Zip(format!("{part}: {e}"))) + } } - self.total = self.total.saturating_add(out.len() as u64); - Ok(Some(out)) } /// Part `name` as XML, read with `options`. `Ok(None)` when there is no such part. @@ -316,11 +305,9 @@ impl<'a> LazyPackage<'a> { /// The first part whose declared size is over [`Limits::max_part`], with that size: for telling /// the user which part made a package too large. pub fn declared_oversized(bytes: &[u8], limits: &Limits) -> Option<(String, u64)> { - let mut zip = open_zip(bytes, limits).ok()?; - (0..zip.len()).find_map(|i| { - let f = zip.by_index_raw(i).ok()?; - (f.size() > limits.max_part).then(|| (f.name().map(|n| n.into_owned()).unwrap_or_default(), f.size())) - }) + let mut zip = open_zip(bytes, limits, u64::MAX).ok()?; + let names: Vec = zip.names().map(|n| n.into_owned()).collect(); + names.into_iter().find_map(|n| zip.declared_size(&n).filter(|size| *size > limits.max_part).map(|size| (n, size))) } #[cfg(test)] diff --git a/crates/opc/src/write.rs b/crates/opc/src/write.rs index aba1977f8..9c55270c5 100644 --- a/crates/opc/src/write.rs +++ b/crates/opc/src/write.rs @@ -1,7 +1,6 @@ //! Writing a package. use std::collections::{HashMap, HashSet}; -use std::io::{Cursor, Write}; use craft_xml::Writer; @@ -27,14 +26,9 @@ pub fn compression_for(name: &str) -> Compression { /// A ZIP archive being written. The output is deterministic: deflate level 6 and the same /// (1980) timestamp on every entry. +#[derive(Default)] pub struct ZipOut { - zip: zip::ZipWriter>>, -} - -impl Default for ZipOut { - fn default() -> ZipOut { - ZipOut { zip: zip::ZipWriter::new(Cursor::new(Vec::new())) } - } + zip: craft_zip::Writer, } impl ZipOut { @@ -44,22 +38,15 @@ impl ZipOut { /// Adds an entry. The caller keeps names unique. pub fn add(&mut self, name: &str, data: &[u8], compression: Compression) -> Result<(), Error> { - let method = match compression { - Compression::Deflate => zip::CompressionMethod::Deflated, - Compression::Store => zip::CompressionMethod::Stored, + let compression = match compression { + Compression::Deflate => craft_zip::Compression::Deflated, + Compression::Store => craft_zip::Compression::Stored, }; - let opts = zip::write::SimpleFileOptions::default() - .compression_method(method) - .compression_level((compression == Compression::Deflate).then_some(6)) - .last_modified_time(zip::DateTime::default()) - // Deflate can grow incompressible data a little, so well before 4 GiB. - .large_file(data.len() as u64 >= u64::from(u32::MAX) / 2); - self.zip.start_file(name, opts).map_err(|e| Error::Write(e.to_string()))?; - self.zip.write_all(data).map_err(|e| Error::Write(e.to_string())) + self.zip.add(name, data, compression).map_err(crate::zip_error) } pub fn finish(self) -> Result, Error> { - self.zip.finish().map(Cursor::into_inner).map_err(|e| Error::Write(e.to_string())) + self.zip.finish().map_err(crate::zip_error) } } diff --git a/crates/opc/tests/package.rs b/crates/opc/tests/package.rs index 67e8ac924..48aefaf1f 100644 --- a/crates/opc/tests/package.rs +++ b/crates/opc/tests/package.rs @@ -156,9 +156,8 @@ fn parts_are_shared_not_copied() { #[test] fn skipped_parts_count_against_the_inflate_budget() { - // Kept bytes cap at 100: b, c and d are inflated past what's left and left out. That work - // is charged, so after d the budget (130) is spent and e is not read, although it would - // have fit. + // Kept bytes cap at 100: b, c and d declare more than is left and are left out before + // anything is inflated, so they cost no work and e, which fits, is still read. let mut zw = zip::ZipWriter::new(Cursor::new(Vec::new())); let o = zip::write::SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated); for (name, len) in [("a", 80), ("b", 80), ("c", 80), ("d", 80), ("e", 5), ("big", 2000)] { @@ -170,7 +169,8 @@ fn skipped_parts_count_against_the_inflate_budget() { let p = Package::open(&z, &skip(130)).unwrap(); assert!(p.get("a").is_some()); assert!(p.get("b").is_none() && p.get("d").is_none()); - assert!(p.get("e").is_none(), "the inflate budget stops reading"); + assert!(p.get("e").is_some(), "parts refused by their declared size cost nothing"); + assert_eq!(p.size(), 85); // With room to spare the same small part is kept, and a part declaring more than the part // limit is left out without being inflated. let p = Package::open(&z, &skip(10_000)).unwrap(); @@ -348,3 +348,17 @@ fn packed_media_are_stored() { assert_eq!(p.size(), 100_000); } } + +/// Two entries of exactly one name: `zip` keeps one. This crate is lenient about part names +/// (spellings that differ only in case are one part), so the package still opens. +#[test] +fn an_entry_name_repeated_exactly_still_opens() { + let mut z = zip_of(&[("word/a.xml", b"".to_vec()), ("word/b.xml", b"".to_vec())]); + let mut i = 0; + while let Some(at) = z[i..].windows(10).position(|w| w == b"word/b.xml") { + z[i + at + 5] = b'a'; + i += at + 10; + } + assert_eq!(Package::open(&z, &LIMITS).unwrap().names().collect::>(), ["word/a.xml"]); + assert!(LazyPackage::open(&z, &LIMITS).unwrap().read("word/a.xml").unwrap().is_some()); +} diff --git a/crates/ora/Cargo.toml b/crates/ora/Cargo.toml index 67c941f6f..446046fa4 100644 --- a/crates/ora/Cargo.toml +++ b/crates/ora/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 archives, hostile ones included, 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/ora/src/lib.rs b/crates/ora/src/lib.rs index cd7258a3a..b2a91cc40 100644 --- a/crates/ora/src/lib.rs +++ b/crates/ora/src/lib.rs @@ -16,8 +16,6 @@ #[cfg(test)] mod tests; -use std::io::{Cursor, Read, Write}; - use craft_xml::El; /// Why an OpenRaster document was not read or written. @@ -51,6 +49,8 @@ pub const MAX_NODES: usize = 8192; pub const MAX_STACK_XML: usize = 16 << 20; /// Largest layer image file a reader should ask [`Archive::entry`] for. pub const MAX_ENTRY: usize = 512 << 20; +/// Most entries the archive may declare; checked before the archive is indexed. +pub const MAX_ENTRIES: u64 = 100_000; /// Deepest element nesting of `stack.xml`. pub const MAX_XML_DEPTH: usize = 512; @@ -78,19 +78,7 @@ pub const SVG_COMPOSITE_OPS: [&str; 15] = [ /// bytes, which is how OpenRaster (and Krita, OpenDocument, EPUB) mark their archives so the /// type is readable without unzipping. pub fn zip_mimetype(bytes: &[u8]) -> Option<&[u8]> { - let u16_at = |o: usize| bytes.get(o..o + 2).and_then(|s| <[u8; 2]>::try_from(s).ok()).map(u16::from_le_bytes); - let u32_at = |o: usize| bytes.get(o..o + 4).and_then(|s| <[u8; 4]>::try_from(s).ok()).map(u32::from_le_bytes); - if u32_at(0)? != 0x0403_4b50 || u16_at(8)? != 0 { - return None; - } - let size = usize::try_from(u32_at(18)?).ok()?; - let name_len = usize::from(u16_at(26)?); - let extra_len = usize::from(u16_at(28)?); - if bytes.get(30..30 + name_len)? != b"mimetype" || size > 256 { - return None; - } - let start = 30 + name_len + extra_len; - bytes.get(start..start.checked_add(size)?) + craft_zip::stored_mimetype(bytes) } /// `true` for an OpenRaster archive (by its `mimetype` entry). @@ -333,14 +321,19 @@ fn push_props(xml: &mut String, p: &Props) { /// An open OpenRaster archive: the parsed `stack.xml` and access to the archive's entries. pub struct Archive<'a> { - zip: zip::ZipArchive>, + zip: craft_zip::Archive<'a>, image: Image, } impl<'a> Archive<'a> { /// Opens the archive, checks its `mimetype` and reads `stack.xml`. pub fn open(bytes: &'a [u8]) -> Result> { - let mut zip = zip::ZipArchive::new(Cursor::new(bytes)).map_err(|e| invalid(e.to_string()))?; + let mut zip = + craft_zip::Archive::open(bytes, craft_zip::Limits { max_entries: MAX_ENTRIES, max_total_bytes: u64::MAX }).map_err(zip_error)?; + // `zip` keeps one entry of a repeated name; which one the writer meant is not known. + if zip.has_duplicate_names() { + return Err(invalid("two entries of the archive share a name")); + } let mime = read(&mut zip, "mimetype", 256)?.ok_or_else(|| invalid("the archive has no mimetype"))?; if mime.trim_ascii() != MIMETYPE { return Err(invalid("the archive's mimetype is not image/openraster")); @@ -362,26 +355,29 @@ impl<'a> Archive<'a> { } } -fn read(zip: &mut zip::ZipArchive>, name: &str, max: usize) -> Result>> { - let Some(index) = zip.index_for_name(name) else { return Ok(None) }; - let file = zip.by_index(index).map_err(|e| invalid(format!("{name}: {e}")))?; - let too_large = || invalid(format!("{name}: larger than {max} bytes")); - if file.size() > max as u64 { - return Err(too_large()); +fn read(zip: &mut craft_zip::Archive<'_>, name: &str, max: usize) -> Result>> { + zip.read(name, max as u64).map_err(zip_error) +} + +fn write_error(e: craft_zip::Error) -> Error { + match e { + craft_zip::Error::Write(m) => Error::Write(m), + e => Error::Write(e.to_string()), } - // The declared size is the archive's claim; the read is what is bounded. - let mut out = Vec::new(); - file.take(max as u64 + 1).read_to_end(&mut out).map_err(|e| invalid(format!("{name}: {e}")))?; - if out.len() > max { - return Err(too_large()); +} + +fn zip_error(e: craft_zip::Error) -> Error { + match e { + craft_zip::Error::NotZip(m) => invalid(m), + craft_zip::Error::EntryTooLarge { name, limit } => invalid(format!("{name}: larger than {limit} bytes")), + e => invalid(e.to_string()), } - Ok(Some(out)) } /// Writes an OpenRaster archive. Entries are stored (PNG is already compressed), in the order /// they are added, after the `mimetype`. pub struct Writer { - zip: zip::ZipWriter>>, + zip: craft_zip::Writer, failed: Option, } @@ -395,7 +391,7 @@ impl Writer { /// An archive holding the `mimetype`: it must be the first entry, stored, so the type is /// readable at a fixed offset. pub fn new() -> Writer { - let mut w = Writer { zip: zip::ZipWriter::new(Cursor::new(Vec::new())), failed: None }; + let mut w = Writer { zip: craft_zip::Writer::new(), failed: None }; w.failed = w.add("mimetype", MIMETYPE).err(); w } @@ -404,10 +400,7 @@ impl Writer { /// (the whole image, which the specification requires) or `Thumbnails/thumbnail.png` (at /// most 256 pixels a side, also required). pub fn add(&mut self, name: &str, data: &[u8]) -> Result<()> { - let stored = - zip::write::SimpleFileOptions::default().compression_method(zip::CompressionMethod::Stored).large_file(data.len() > u32::MAX as usize); - self.zip.start_file(name, stored).map_err(|e| Error::Write(format!("{name}: {e}")))?; - self.zip.write_all(data).map_err(|e| Error::Write(format!("{name}: {e}"))) + self.zip.add(name, data, craft_zip::Compression::Stored).map_err(write_error) } /// Adds `stack.xml` for `image`. @@ -420,6 +413,6 @@ impl Writer { if let Some(e) = self.failed { return Err(e); } - self.zip.finish().map(Cursor::into_inner).map_err(|e| Error::Write(e.to_string())) + self.zip.finish().map_err(write_error) } } diff --git a/crates/ora/src/tests.rs b/crates/ora/src/tests.rs index 490084441..cfaac7c41 100644 --- a/crates/ora/src/tests.rs +++ b/crates/ora/src/tests.rs @@ -1,3 +1,5 @@ +use std::io::{Cursor, Write}; + use super::*; fn zip_of(files: &[(&str, &[u8])], deflate: bool) -> Vec { @@ -301,3 +303,28 @@ fn zip_mimetype_reads_only_a_leading_stored_entry() { assert!(!is_ora(&zip_of(&[("mimetype", b"application/x-krita")], false))); assert_eq!(zip_mimetype(&zip_of(&[("mimetype", &[b'a'; 257])], false)), None); } + +/// A ZIP64 end record can declare millions of entries in a small file: 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(MAX_ENTRIES + 1); + assert_eq!( + Archive::open(&bomb).err(), + Some(Error::Invalid(format!("the archive declares {} entries, more than {MAX_ENTRIES}", MAX_ENTRIES + 1))) + ); +} + +/// Two entries of one name: `zip` keeps one, and which one the writer meant is not known. +#[test] +fn two_entries_of_one_name_are_refused() { + let bytes = archive("", &[("dupA", vec![1]), ("dupB", vec![2])]); + assert!(Archive::open(&bytes).is_ok()); + let mut renamed = bytes.clone(); + let mut i = 0; + while let Some(at) = renamed[i..].windows(4).position(|w| w == b"dupB") { + renamed[i + at + 3] = b'A'; + i += at + 4; + } + assert_eq!(Archive::open(&renamed).err(), Some(Error::Invalid("two entries of the archive share a name".into()))); +} diff --git a/crates/penpot/Cargo.toml b/crates/penpot/Cargo.toml index 800377f3f..d693f771b 100644 --- a/crates/penpot/Cargo.toml +++ b/crates/penpot/Cargo.toml @@ -13,8 +13,9 @@ workspace = true [dependencies] serde_json = { 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). A shared bounded ZIP reader is -# planned (#126); until it lands this crate reads through `zip` directly. +craft-zip = { workspace = true } + +[dev-dependencies] +# The tests build archives, hostile ones included, by hand. zip = { version = "9", default-features = false, features = ["deflate-flate2"] } -flate2 = { version = "1", default-features = false, features = ["rust_backend"] } +craft-zip = { workspace = true, features = ["test-support"] } diff --git a/crates/penpot/src/lib.rs b/crates/penpot/src/lib.rs index fcea41da6..ac6b045f3 100644 --- a/crates/penpot/src/lib.rs +++ b/crates/penpot/src/lib.rs @@ -28,10 +28,8 @@ mod tests; use std::collections::{BTreeMap, BTreeSet}; -use std::io::{self, Cursor, Read, Write}; use serde_json::Value; -use zip::ZipArchive; /// Largest JSON member read. pub const MAX_JSON: u64 = 16 * 1024 * 1024; @@ -86,23 +84,9 @@ fn safe_name(name: &str) -> bool { !(name.starts_with('/') || name.contains('\\') || name.split('/').any(|part| part == "." || part == "..") || name.chars().any(char::is_control)) } -/// The number of members the archive's end-of-central-directory record declares (the ZIP64 record -/// when the classic one is saturated). `None` when no record is found. -/// shortcut: reads the two records by hand; goes away with the shared bounded ZIP reader (#126). -fn declared_entries(bytes: &[u8]) -> Option { - let eocd = bytes.windows(4).rposition(|w| w == b"PK\x05\x06")?; - let count = u16::from_le_bytes(bytes.get(eocd + 10..eocd + 12)?.try_into().ok()?); - if count != u16::MAX { - return Some(u64::from(count)); - } - let zip64 = bytes.get(..eocd)?.windows(4).rposition(|w| w == b"PK\x06\x06")?; - Some(u64::from_le_bytes(bytes.get(zip64 + 32..zip64 + 40)?.try_into().ok()?)) -} - -/// Counts actual ZIP output and JSON structure throughout one archive, independently of member -/// header sizes and the serialized size of dense arrays and objects. +/// Whether one archive's byte budget (counted by the archive, of bytes actually read) has run out, +/// and its JSON structure budget, independent of the serialized size of dense arrays and objects. struct Budget { - remaining: u64, exceeded: bool, json_values: usize, json_exceeded: bool, @@ -150,35 +134,6 @@ impl Budget { } } -/// Charges every byte read to the archive's [`Budget`]. -struct BudgetReader<'a, R> { - inner: R, - budget: &'a mut Budget, -} - -impl Read for BudgetReader<'_, R> { - fn read(&mut self, out: &mut [u8]) -> io::Result { - if out.is_empty() { - return Ok(0); - } - if self.budget.remaining == 0 { - // Distinguish exact-budget EOF from data past the limit. - let mut probe = [0_u8; 1]; - return match self.inner.read(&mut probe)? { - 0 => Ok(0), - _ => { - self.budget.exceeded = true; - Err(io::Error::other("Penpot decompression budget exhausted")) - } - }; - } - let limit = out.len().min(usize::try_from(self.budget.remaining).unwrap_or(usize::MAX)); - let read = self.inner.read(out.get_mut(..limit).unwrap_or_default())?; - self.budget.remaining -= read as u64; - Ok(read) - } -} - /// One entry of the manifest's `files`. #[derive(Clone, Debug, PartialEq)] pub struct FileEntry { @@ -189,7 +144,8 @@ pub struct FileEntry { /// An opened Penpot export. pub struct Archive<'b> { - zip: ZipArchive>, + /// Counts the bytes read against [`MAX_DECOMPRESSED`]. + zip: craft_zip::Archive<'b>, /// Member names in archive order. names: Vec, budget: Budget, @@ -206,37 +162,30 @@ impl<'b> Archive<'b> { if bytes.len() > MAX_ARCHIVE { return Err(corrupt("archive exceeds 512 MiB")); } - let mut zip = ZipArchive::new(Cursor::new(bytes)).map_err(|e| corrupt(format!("invalid ZIP: {e}")))?; - if zip.len() > MAX_ENTRIES { - return Err(corrupt("too many ZIP members")); - } - // The ZIP reader keeps one member per name, so two members of one name never reach the - // loop below: the directory's own count tells them. - if declared_entries(bytes) != Some(zip.len() as u64) { + let mut zip = craft_zip::Archive::open(bytes, craft_zip::Limits { max_entries: MAX_ENTRIES as u64, max_total_bytes: MAX_DECOMPRESSED }) + .map_err(|e| match e { + craft_zip::Error::TooManyEntries { .. } | craft_zip::Error::TooManyEndRecords(_) => corrupt("too many ZIP members"), + e => corrupt(format!("invalid ZIP: {e}")), + })?; + // The ZIP reader keeps one member per name: reject duplicates instead of guessing which + // version is real. + if zip.has_duplicate_names() { return Err(corrupt("duplicate ZIP member names")); } // Reject oversized logical archives before decoding any JSON. This guards against ZIP bombs // whose compressed size is innocuous but whose advertised uncompressed size is enormous. + let names: Vec = zip.names().map(|n| n.into_owned()).collect(); let mut expanded = 0_u64; - let mut seen = BTreeSet::new(); - let mut names = Vec::with_capacity(zip.len()); - for index in 0..zip.len() { - let member = zip.by_index(index).map_err(|e| corrupt(e.to_string()))?; - let name = member.name().map_err(|e| corrupt(e.to_string()))?.into_owned(); - // Reject duplicates and traversal instead of guessing which version is real. - if !seen.insert(name.clone()) { - return Err(corrupt(format!("duplicate ZIP member: {}", name.chars().take(200).collect::()))); - } - if !safe_name(&name) { + for name in &names { + if !safe_name(name) { return Err(corrupt("unsafe ZIP member path")); } - expanded = expanded.saturating_add(member.size()); + expanded = expanded.saturating_add(zip.declared_size(name).unwrap_or(0)); if expanded > MAX_DECOMPRESSED { return Err(corrupt("ZIP contents exceed 1 GiB")); } - names.push(name); } - let budget = Budget { remaining: MAX_DECOMPRESSED, exceeded: false, json_values: MAX_JSON_VALUES, json_exceeded: false }; + let budget = Budget { exceeded: false, json_values: MAX_JSON_VALUES, json_exceeded: false }; let mut archive = Archive { zip, names, budget, manifest: Value::Null, files: Vec::new() }; let manifest = archive.json("manifest.json")?; if manifest.get("type").and_then(Value::as_str) != Some(MANIFEST_TYPE) { @@ -268,12 +217,12 @@ impl<'b> Archive<'b> { /// Bytes this archive may still unpack. pub fn bytes_left(&self) -> u64 { - self.budget.remaining + self.zip.budget().remaining() } /// Whether the archive has member `path`. pub fn has(&self, path: &str) -> bool { - self.zip.index_for_name(path).is_some() + self.zip.contains(path) } /// Every member name, in archive order. @@ -307,41 +256,46 @@ impl<'b> Archive<'b> { /// Member `path` read whole when it is at most `declared_max` bytes by its header and at most /// `max` bytes read. fn read(&mut self, path: &str, declared_max: u64, max: u64, too_large: impl Fn() -> String) -> Result, Error> { - let entry = self.zip.by_name(path).map_err(|e| corrupt(format!("{path}: {e}")))?; - if entry.size() > declared_max { + self.budget.check()?; + let declared = self.zip.declared_size(path).ok_or_else(|| corrupt(format!("{path}: not in the archive")))?; + if declared > declared_max { return Err(corrupt(too_large())); } - let mut data = Vec::new(); - BudgetReader { inner: entry, budget: &mut self.budget } - .take(max.saturating_add(1)) - .read_to_end(&mut data) - .map_err(|e| corrupt(e.to_string()))?; - if data.len() as u64 > max { - return Err(corrupt(too_large())); + self.read_charged(path, declared, max, || corrupt(too_large())) + } + + /// Member `path` (declaring `declared` bytes) up to `max` bytes. Running out of the archive's + /// byte budget fails every later [`Archive::check`]; a read that fails otherwise is charged + /// its declared size (the read stops one byte past it), so it is never free to repeat. + fn read_charged(&mut self, path: &str, declared: u64, max: u64, too_large: impl Fn() -> Error) -> Result, Error> { + match self.zip.read(path, max) { + Ok(Some(data)) => Ok(data), + Ok(None) => Err(corrupt(format!("{path}: not in the archive"))), + Err(craft_zip::Error::EntryTooLarge { .. }) => Err(too_large()), + Err(craft_zip::Error::BudgetSpent { .. }) => { + self.budget.exceeded = true; + Err(corrupt("Penpot decompression budget exhausted")) + } + Err(e) => { + let charge = declared.min(self.zip.budget().remaining()); + let _ = self.zip.budget_mut().charge(charge); + Err(corrupt(e.to_string())) + } } - Ok(data) } /// Shape JSON member `path`, within [`MAX_JSON`] and what is left of `page_budget` (bytes of /// shape JSON for its page, counted down). pub fn shape_json(&mut self, path: &str, page_budget: &mut u64) -> Result { - let declared = self.zip.by_name(path).map_err(|e| corrupt(format!("{path}: {e}")))?.size(); + self.budget.check()?; + let declared = self.zip.declared_size(path).ok_or_else(|| corrupt(format!("{path}: not in the archive")))?; if declared > MAX_JSON { return Err(corrupt(format!("JSON member too large: {path}"))); } if declared > *page_budget { return Err(corrupt("page shapes exceed byte budget")); } - let entry = self.zip.by_name(path).map_err(|e| corrupt(format!("{path}: {e}")))?; - let mut data = Vec::new(); - let max_read = (MAX_JSON + 1).min(page_budget.saturating_add(1)); - BudgetReader { inner: entry, budget: &mut self.budget }.take(max_read).read_to_end(&mut data).map_err(|e| corrupt(e.to_string()))?; - if data.len() as u64 > MAX_JSON { - return Err(corrupt(format!("JSON member too large: {path}"))); - } - if data.len() as u64 > *page_budget { - return Err(corrupt("page shapes exceed byte budget")); - } + let data = self.read_charged(path, declared, MAX_JSON.min(*page_budget), || corrupt(format!("JSON member too large: {path}")))?; *page_budget -= data.len() as u64; self.budget.parse_json(&data, path) } @@ -402,12 +356,10 @@ impl<'b> Archive<'b> { self.names.iter().filter(|n| n.starts_with("objects/") || n.starts_with("fonts/") || n.starts_with("assets/fonts/")).cloned().collect(); let mut count = 0; for name in names { - let Ok(member) = self.zip.by_name(&name) else { continue }; - let mut header = [0; 4]; - if (BudgetReader { inner: member, budget: &mut self.budget }).read_exact(&mut header).is_ok() - && (matches!(&header, b"OTTO" | b"true" | b"ttcf" | b"wOFF" | b"wOF2") || header == [0, 1, 0, 0]) - { - count += 1; + match self.zip.read_head(&name, 4) { + Ok(Some(header)) if matches!(header.as_slice(), b"OTTO" | b"true" | b"ttcf" | b"wOFF" | b"wOF2" | [0, 1, 0, 0]) => count += 1, + Err(craft_zip::Error::BudgetSpent { .. }) => self.budget.exceeded = true, + _ => {} } } count @@ -423,20 +375,13 @@ pub fn manifest(generated_by: &str, files: &[(&str, &str, Value)]) -> Value { /// A Penpot archive of `members` (path, bytes), deflated, in the order given. Paths must be safe /// and distinct, as [`Archive::open`] requires of what it reads. pub fn write_archive<'a>(members: impl IntoIterator) -> Result, Error> { - let io = |e: &dyn std::fmt::Display| Error::Io(e.to_string()); - let options = zip::write::SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated).compression_level(Some(3)); - let mut buf = Cursor::new(Vec::new()); - { - let mut z = zip::ZipWriter::new(&mut buf); - let mut seen = BTreeSet::new(); - for (path, bytes) in members { - if path.is_empty() || !safe_name(path) || !seen.insert(path) { - return Err(Error::Io(format!("unsafe or repeated member path: {}", path.chars().take(200).collect::()))); - } - z.start_file(path, options).map_err(|e| io(&e))?; - z.write_all(bytes).map_err(|e| io(&e))?; + let mut z = craft_zip::Writer::new(); + let mut seen = BTreeSet::new(); + for (path, bytes) in members { + if path.is_empty() || !safe_name(path) || !seen.insert(path) { + return Err(Error::Io(format!("unsafe or repeated member path: {}", path.chars().take(200).collect::()))); } - z.finish().map_err(|e| io(&e))?; + z.add(path, bytes, craft_zip::Compression::Level(3)).map_err(|e| Error::Io(e.to_string()))?; } - Ok(buf.into_inner()) + z.finish().map_err(|e| Error::Io(e.to_string())) } diff --git a/crates/penpot/src/tests.rs b/crates/penpot/src/tests.rs index c949818ce..c5f14ef72 100644 --- a/crates/penpot/src/tests.rs +++ b/crates/penpot/src/tests.rs @@ -2,6 +2,9 @@ #![allow(clippy::unwrap_used, clippy::panic)] use serde_json::json; +use std::io::{Cursor, Write}; + +use zip::ZipArchive; use zip::write::SimpleFileOptions; use super::*; @@ -183,36 +186,38 @@ fn an_archive_with_a_zip64_directory_opens() { z.start_file(format!("{i}"), stored).unwrap(); } let bytes = z.finish().unwrap().into_inner(); - assert_eq!(declared_entries(&bytes), Some(70_001)); assert_eq!(Archive::open(&bytes).unwrap().names().len(), 70_001); } #[test] fn the_byte_budget_counts_bytes_read_even_when_headers_claim_less() { - // Two independent readers share the same actual output limit. - let mut budget = Budget { remaining: 5, exceeded: false, json_values: MAX_JSON_VALUES, json_exceeded: false }; - let mut first = Vec::new(); - BudgetReader { inner: Cursor::new(b"abcd"), budget: &mut budget }.read_to_end(&mut first).unwrap(); - assert_eq!(first, b"abcd"); - assert_eq!(budget.remaining, 1); - - let mut second = Vec::new(); - let result = BudgetReader { inner: Cursor::new(b"xyz"), budget: &mut budget }.read_to_end(&mut second); - assert!(result.is_err(), "bytes beyond the combined budget must be rejected"); - assert_eq!(budget.remaining, 0); - assert!(budget.check().is_err()); - // Ending exactly at the budget is not an overrun. - let mut budget = Budget { remaining: 3, exceeded: false, json_values: MAX_JSON_VALUES, json_exceeded: false }; - let mut exact = Vec::new(); - BudgetReader { inner: Cursor::new(b"xyz"), budget: &mut budget }.read_to_end(&mut exact).unwrap(); - assert!(budget.check().is_ok()); + // b.json's headers say one byte; it holds five. The read stops past the claim and is charged it. + let mut bytes = zip_of(&[good_manifest(), ("a.json".into(), b"[0,1]".to_vec()), ("b.json".into(), b"[2,3]".to_vec())]); + let start = { + let mut z = ZipArchive::new(Cursor::new(&bytes[..])).unwrap(); + z.by_name("b.json").unwrap().central_header_start() as usize + }; + bytes[start + 24..start + 28].copy_from_slice(&1u32.to_le_bytes()); + let mut a = Archive::open(&bytes).unwrap(); + let before = a.bytes_left(); + assert!(a.json("b.json").is_err()); + assert_eq!(before - a.bytes_left(), 1); + assert!(a.check().is_ok(), "a lying member is an error of its own, not the budget's"); + // Two members share one limit; ending exactly at it is not an overrun. + let left = a.bytes_left(); + a.zip.budget_mut().charge(left - 5).unwrap(); + assert_eq!(a.json("a.json").unwrap(), json!([0, 1])); + assert!(a.check().is_ok()); + assert!(a.json("a.json").is_err(), "bytes beyond the combined budget must be rejected"); + assert!(a.check().is_err()); } #[test] fn an_exhausted_byte_budget_fails_the_archive_even_for_optional_members() { let bytes = zip_of(&[good_manifest(), ("a.json".into(), b"[0,0]".to_vec())]); let mut a = Archive::open(&bytes).unwrap(); - a.budget.remaining = 3; + let left = a.bytes_left(); + a.zip.budget_mut().charge(left - 3).unwrap(); assert!(a.json("a.json").is_err()); assert_eq!(a.check().err(), Some(Error::Corrupt("actual decompressed ZIP contents exceed 1 GiB".into()))); } @@ -232,7 +237,7 @@ fn the_json_value_budget_is_shared_by_shape_and_other_members() { } fn budget_of(json_values: usize) -> Budget { - Budget { remaining: MAX_DECOMPRESSED, exceeded: false, json_values, json_exceeded: false } + Budget { exceeded: false, json_values, json_exceeded: false } } #[test] @@ -425,3 +430,11 @@ fn mangled_archives_never_panic() { read(&b); } } + +/// A ZIP64 end record can declare millions of members in a small file: refused on the raw bytes +/// before the archive is indexed. +#[test] +fn an_archive_declaring_too_many_members_is_refused_before_it_is_indexed() { + let bomb = craft_zip::zip64_entry_count_sample(MAX_ENTRIES as u64 + 1); + assert_eq!(Archive::open(&bomb).err(), Some(Error::Corrupt("too many ZIP members".into()))); +} diff --git a/crates/sketch/Cargo.toml b/crates/sketch/Cargo.toml index 38b1696cc..d4a7cf6ad 100644 --- a/crates/sketch/Cargo.toml +++ b/crates/sketch/Cargo.toml @@ -13,8 +13,9 @@ workspace = true [dependencies] serde_json = { 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). A shared bounded ZIP reader is -# planned (#126); until it lands this crate reads through `zip` directly. +craft-zip = { workspace = true } + +[dev-dependencies] +# The tests build archives, hostile ones included, by hand. zip = { version = "9", default-features = false, features = ["deflate-flate2"] } -flate2 = { version = "1", default-features = false, features = ["rust_backend"] } +craft-zip = { workspace = true, features = ["test-support"] } diff --git a/crates/sketch/src/lib.rs b/crates/sketch/src/lib.rs index 2ef5a9a76..3d9e00b9d 100644 --- a/crates/sketch/src/lib.rs +++ b/crates/sketch/src/lib.rs @@ -22,7 +22,6 @@ mod tests; use std::cell::Cell; use std::collections::{BTreeSet, HashSet}; -use std::io::{Cursor, Read}; use serde_json::{Value, json}; @@ -47,6 +46,8 @@ pub const MAX_JSON_TOTAL: u64 = 2 * 1024 * 1024 * 1024; pub const MAX_JSON_TOTAL: u64 = 512 * 1024 * 1024; /// Most pages read from one file. pub const MAX_PAGES: usize = 1000; +/// Most entries the archive may declare; checked before the archive is indexed. +pub const MAX_ENTRIES: u64 = 100_000; /// The SQLite header: what Sketch wrote before version 43. const SQLITE: &[u8] = b"SQLite format 3\0"; @@ -91,10 +92,7 @@ impl Default for Limits { /// Whether `bytes` look like a `.sketch` file: a zip holding `document.json` and `meta.json`, or /// the SQLite database Sketch wrote before version 43 (so that it gets the "too old" message). pub fn is_sketch(bytes: &[u8]) -> bool { - bytes.starts_with(SQLITE) - || (bytes.starts_with(b"PK") - && zip::ZipArchive::new(Cursor::new(bytes)) - .is_ok_and(|z| z.index_for_name("document.json").is_some() && z.index_for_name("meta.json").is_some())) + bytes.starts_with(SQLITE) || (bytes.starts_with(b"PK") && open_zip(bytes).is_ok_and(|z| z.contains("document.json") && z.contains("meta.json"))) } /// The parts of a `.sketch` archive: the document, the pages in document order and the images. @@ -110,7 +108,7 @@ pub struct Archive<'b> { pub pages: Vec<(String, Value)>, /// Problems met while reading (pages skipped, a newer format than tested). pub warnings: Vec, - zip: zip::ZipArchive>, + zip: craft_zip::Archive<'b>, limits: Limits, /// Image bytes read so far. image_total: Cell, @@ -126,7 +124,11 @@ impl<'b> Archive<'b> { if bytes.starts_with(SQLITE) { return Err(Error::TooOld); } - let mut z = zip::ZipArchive::new(Cursor::new(bytes)).map_err(|e| corrupt(e.to_string()))?; + let mut z = open_zip(bytes).map_err(|e| corrupt(e.to_string()))?; + // `zip` keeps one entry of a repeated name; which one the writer meant is not known. + if z.has_duplicate_names() { + return Err(corrupt("two entries of the archive share a name")); + } let mut total = 0u64; let meta = json_member(&mut z, "meta.json", MAX_JSON_MEMBER, &mut total)?; if !meta.is_object() { @@ -147,9 +149,8 @@ impl<'b> Archive<'b> { let why = other.err().map_or_else(|| "not an object".to_string(), |e| e.to_string()); warnings.push(format!("the document's shared styles and library symbols were skipped: {why}")); let pages: Vec = z - .file_names() + .names() .filter_map(|n| { - let n = n.ok()?; let id = n.strip_prefix("pages/")?.strip_suffix(".json")?; Some(json!({"_ref": format!("pages/{id}")})) }) @@ -217,28 +218,34 @@ pub fn embedded_font_families(document: &Value) -> Vec { /// Member `name`, refused before unpacking when it would be larger than `limit`, than /// [`MAX_RATIO`] times what it takes in the archive (past a floor), or than what is left of /// `max_total` (`total`: read so far, counted up). -fn capped_member(z: &mut zip::ZipArchive>, name: &str, limit: u64, total: &mut u64, max_total: u64) -> Result, Error> { - let f = z.by_name(name).map_err(|e| corrupt(format!("{name}: {e}")))?; - let (size, packed) = (f.size(), f.compressed_size()); +fn capped_member(z: &mut craft_zip::Archive<'_>, name: &str, limit: u64, total: &mut u64, max_total: u64) -> Result, Error> { + let not_found = || corrupt(format!("{name}: not in the archive")); + let (size, packed) = (z.declared_size(name).ok_or_else(not_found)?, z.compressed_size(name).ok_or_else(not_found)?); let allowed = limit.min(packed.saturating_mul(MAX_RATIO).max(MIN_ALLOWANCE)).min(max_total.saturating_sub(*total)); if size > allowed { return Err(corrupt(format!("{name} is too large"))); } - let mut data = Vec::new(); - let read = f.take(allowed.saturating_add(1)).read_to_end(&mut data); // A failed read (a bad checksum at the end, say) unpacked all the same: charged in full, so - // a page list naming the member again can't repeat it within the budget. - let result = match read { - Err(e) => Err(corrupt(format!("{name}: {e}"))), - Ok(_) if data.len() as u64 > allowed => Err(corrupt(format!("{name} is too large"))), - Ok(_) => Ok(data), + // a page list naming the member again can't repeat it within the budget. The read stops one + // byte past the declared size, so the declared size is what it cost. + let result = match z.read(name, allowed) { + Ok(Some(data)) => Ok(data), + Ok(None) => Err(not_found()), + Err(craft_zip::Error::EntryTooLarge { .. }) => Err(corrupt(format!("{name} is too large"))), + Err(e) => Err(corrupt(e.to_string())), }; *total = total.saturating_add(result.as_ref().map_or(size, |d| d.len() as u64)); result } +/// The archive, its declared entry count checked before it is indexed. Every budget is this +/// crate's own ([`Limits`], [`MAX_JSON_TOTAL`]), so the archive's total is not used. +fn open_zip(bytes: &[u8]) -> Result, craft_zip::Error> { + craft_zip::Archive::open(bytes, craft_zip::Limits { max_entries: MAX_ENTRIES, max_total_bytes: u64::MAX }) +} + /// JSON member `name`, read by [`capped_member`] within the file's [`MAX_JSON_TOTAL`]. -fn json_member(z: &mut zip::ZipArchive>, name: &str, limit: u64, total: &mut u64) -> Result { +fn json_member(z: &mut craft_zip::Archive<'_>, name: &str, limit: u64, total: &mut u64) -> Result { let data = capped_member(z, name, limit, total, MAX_JSON_TOTAL)?; serde_json::from_slice(&data).map_err(|e| corrupt(format!("{name}: {e}"))) } diff --git a/crates/sketch/src/tests.rs b/crates/sketch/src/tests.rs index 88a46076a..b4a299b43 100644 --- a/crates/sketch/src/tests.rs +++ b/crates/sketch/src/tests.rs @@ -284,3 +284,28 @@ fn embedded_fonts_are_listed_by_family() { let many: Vec = (0..50).map(|i| json!({"fontFamilyName": format!("F{i:02}"), "fontData": {}})).collect(); assert_eq!(embedded_font_families(&json!({"fontReferences": many})).len(), 20); } + +/// A ZIP64 end record can declare millions of entries in a small file: refused on the raw bytes +/// before the archive is indexed, and such a file is not taken for a `.sketch`. +#[test] +fn an_archive_declaring_too_many_entries_is_refused_before_it_is_indexed() { + let bomb = craft_zip::zip64_entry_count_sample(MAX_ENTRIES + 1); + let e = Archive::open(&bomb).err().map(|e| e.to_string()).unwrap_or_default(); + assert!(e.contains(&format!("declares {} entries", MAX_ENTRIES + 1)), "{e}"); + assert!(!is_sketch(&bomb)); +} + +/// Two entries of one name: `zip` keeps one, and which one the writer meant is not known. +#[test] +fn two_entries_of_one_name_are_refused() { + let z = sketch_zip("50.2", 103, document(&["P1"]), &[page("P1", "Page 1", vec![])], &[("images/dupA.png", b"a"), ("images/dupB.png", b"b")]); + assert!(Archive::open(&z).is_ok()); + let mut renamed = z.clone(); + let mut i = 0; + while let Some(at) = renamed[i..].windows(4).position(|w| w == b"dupB") { + renamed[i + at + 3] = b'A'; + i += at + 4; + } + let e = Archive::open(&renamed).err().map(|e| e.to_string()).unwrap_or_default(); + assert!(e.contains("two entries of the archive share a name"), "{e}"); +} diff --git a/crates/zip/README.md b/crates/zip/README.md index 23bd398c0..079267b9f 100644 --- a/crates/zip/README.md +++ b/crates/zip/README.md @@ -38,6 +38,10 @@ assert_eq!(zip.budget().used(), 4); charged. - `has_duplicate_names`: `zip` keeps one entry per name, so a central directory with more headers than `zip` has entries repeats a name. +- `read_head(name, n)` reads the first `n` bytes only (a signature), charged to the total. +- `compressed_size` with `declared_size` gives the compression ratio an entry claims, for a + format that limits it. +- A clone shares the directory and the bytes and keeps its own count, for reading behind `&self`. - `budget_mut` is for a caller with its own charging rule (charging a failed read, say). - `Error::is_limit` tells a limit from a broken archive. diff --git a/crates/zip/src/archive.rs b/crates/zip/src/archive.rs index e23364010..bdcf64cd7 100644 --- a/crates/zip/src/archive.rs +++ b/crates/zip/src/archive.rs @@ -1,7 +1,7 @@ //! An archive open for reading: entry-count guard, duplicate names, budgeted entry reads. use std::borrow::Cow; -use std::io::Cursor; +use std::io::{Cursor, Read}; use crate::preflight::{ReadError, read_entry}; use crate::{Budget, Error, check_entry_count}; @@ -16,6 +16,10 @@ pub struct Limits { } /// A ZIP archive in memory. Nothing is decompressed until an entry is read. +/// +/// A clone shares the parsed directory and the bytes, and carries a copy of the budget: a +/// caller that reads through a clone keeps its own count. +#[derive(Clone)] pub struct Archive<'a> { zip: zip::ZipArchive>, budget: Budget, @@ -86,6 +90,33 @@ impl<'a> Archive<'a> { self.zip.by_index_raw(index).ok().map(|f| f.size()) } + /// The size entry `name` takes in the archive, compressed. With [`Archive::declared_size`] it + /// gives the compression ratio the entry claims. + pub fn compressed_size(&mut self, name: &str) -> Option { + let index = self.zip.index_for_name(name)?; + self.zip.by_index_raw(index).ok().map(|f| f.compressed_size()) + } + + /// The first `n` bytes of entry `name` (all of it when it is shorter), or `None` when there + /// is no such entry: for telling a file by its signature without inflating it. The bytes are + /// charged to the total; a total with less left than the bytes wanted refuses before reading. + /// + /// The entry's checksum is not checked: it covers the whole entry. + pub fn read_head(&mut self, name: &str, n: usize) -> Result>, Error> { + let Some(index) = self.zip.index_for_name(name) else { return Ok(None) }; + let entry = |msg: String| Error::Entry { name: name.to_string(), msg }; + let f = self.zip.by_index(index).map_err(|e| entry(e.to_string()))?; + // Never more than the entry declares: a shorter entry costs only what it has. + let want = (n as u64).min(f.size()); + if self.budget.remaining() < want { + return Err(Error::BudgetSpent { name: name.to_string(), limit: self.budget.limit() }); + } + let mut out = Vec::with_capacity(n.min(64 * 1024)); + f.take(want).read_to_end(&mut out).map_err(|e| entry(e.to_string()))?; + self.budget.charge(out.len() as u64).map_err(|e| Error::BudgetSpent { name: name.to_string(), limit: e.limit })?; + Ok(Some(out)) + } + /// The bytes of entry `name`, or `None` when there is no such entry. /// /// The entry may declare, and expand to, at most `max` bytes, and at most what is left of diff --git a/crates/zip/src/lib.rs b/crates/zip/src/lib.rs index cd138df3f..4e802d795 100644 --- a/crates/zip/src/lib.rs +++ b/crates/zip/src/lib.rs @@ -32,6 +32,25 @@ pub use guard::check_entry_count; pub use guard::zip64_entry_count_sample; pub use write::{Compression, Writer}; +/// The payload of a ZIP's first entry when it is a stored file named `mimetype` of at most 256 +/// bytes: how OpenDocument, EPUB, IDML, OpenRaster and Krita mark their archives so the type is +/// readable at a fixed offset without unzipping. +pub fn stored_mimetype(bytes: &[u8]) -> Option<&[u8]> { + let u16_at = |o: usize| bytes.get(o..o + 2).and_then(|s| <[u8; 2]>::try_from(s).ok()).map(u16::from_le_bytes); + let u32_at = |o: usize| bytes.get(o..o + 4).and_then(|s| <[u8; 4]>::try_from(s).ok()).map(u32::from_le_bytes); + if u32_at(0)? != 0x0403_4b50 || u16_at(8)? != 0 { + return None; + } + let size = usize::try_from(u32_at(18)?).ok()?; + let name_len = usize::from(u16_at(26)?); + let extra_len = usize::from(u16_at(28)?); + if bytes.get(30..30 + name_len)? != b"mimetype" || size > 256 { + return None; + } + let start = 30 + name_len + extra_len; + bytes.get(start..start.checked_add(size)?) +} + /// Why an archive was not read or not written. #[derive(Clone, Debug, PartialEq, Eq, thiserror::Error)] pub enum Error { diff --git a/crates/zip/tests/archive.rs b/crates/zip/tests/archive.rs index 632c98a53..2c4b84113 100644 --- a/crates/zip/tests/archive.rs +++ b/crates/zip/tests/archive.rs @@ -146,3 +146,39 @@ fn written_archives_pass_the_strict_preflight() { let directory = preflight(&bytes, Limits { max_entries: 10, max_central_directory_bytes: 1 << 16, max_name_bytes: 256 }).unwrap(); assert_eq!(directory.entries, 2); } + +#[test] +fn a_stored_mimetype_is_read_in_place() { + let bytes = archive(&[("mimetype", b"image/openraster", Compression::Stored), ("a", b"x", Compression::Deflated)]); + assert_eq!(craft_zip::stored_mimetype(&bytes), Some(&b"image/openraster"[..])); + assert_eq!(craft_zip::stored_mimetype(&archive(&[("mimetype", b"x", Compression::Deflated)])), None, "compressed"); + assert_eq!(craft_zip::stored_mimetype(&archive(&[("a", b"x", Compression::Stored), ("mimetype", b"x", Compression::Stored)])), None, "not first"); + assert_eq!(craft_zip::stored_mimetype(&archive(&[("mimetype", &[b'a'; 257], Compression::Stored)])), None, "too long"); + assert_eq!(craft_zip::stored_mimetype(b"PK\x03\x04"), None); + assert_eq!(craft_zip::stored_mimetype(&[]), None); +} + +#[test] +fn a_head_is_read_without_inflating_the_rest_and_charged() { + let bytes = archive(&[("font", &[b"wOF2".as_slice(), &[0u8; 100_000]].concat(), Compression::Deflated), ("short", b"wO", Compression::Stored)]); + let mut z = Archive::open(&bytes, Limits { max_total_bytes: 7, ..ROOMY }).unwrap(); + assert_eq!(z.compressed_size("short"), Some(2)); + assert!(z.compressed_size("font").unwrap() < 1000); + assert_eq!(z.compressed_size("nope"), None); + assert_eq!(z.read_head("font", 4).unwrap().as_deref(), Some(&b"wOF2"[..])); + assert_eq!(z.read_head("short", 4).unwrap().as_deref(), Some(&b"wO"[..]), "shorter than asked"); + assert_eq!(z.read_head("nope", 4).unwrap(), None); + assert_eq!(z.budget().used(), 6); + assert_eq!(z.read_head("font", 4), Err(Error::BudgetSpent { name: "font".into(), limit: 7 }), "one byte left"); + assert_eq!(z.budget().used(), 6); +} + +#[test] +fn a_clone_keeps_its_own_count() { + let bytes = archive(&[("a", b"abc", Compression::Deflated)]); + let mut z = Archive::open(&bytes, ROOMY).unwrap(); + let mut c = z.clone(); + assert_eq!(c.read("a", 10).unwrap().as_deref(), Some(&b"abc"[..])); + assert_eq!((c.budget().used(), z.budget().used()), (3, 0)); + assert!(z.read("a", 10).unwrap().is_some()); +}