diff --git a/crates/fig/src/archive.rs b/crates/fig/src/archive.rs index e408ddbe7..e083aa1dc 100644 --- a/crates/fig/src/archive.rs +++ b/crates/fig/src/archive.rs @@ -12,8 +12,10 @@ 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; +/// Most entries an archive may declare; checked before the archive is indexed. The archive holds one +/// entry per image, so 100 000 leaves room for far more than a `.fig` file carries, and a header +/// count can no longer make the index alone cost a hundred megabytes. +pub const MAX_ENTRIES: u64 = 100_000; /// What one open may read from `images/`. #[derive(Clone, Copy, Debug, PartialEq, Eq)] diff --git a/crates/kra/README.md b/crates/kra/README.md index 363455bdf..582a0117d 100644 --- a/crates/kra/README.md +++ b/crates/kra/README.md @@ -31,6 +31,7 @@ here. There is no writer. | `` nodes | 8192 (`MAX_NODES`) | | `maindoc.xml` | 32 MiB, 512 levels of nesting, no internal DTD subset | | A pixel file, inflated | 1 GiB (`MAX_LAYER_FILE`) | +| All entries read from one archive | 4 GiB (`MAX_TOTAL`) | | ICC profile | 16 MiB | | Tile | 1024 × 1024 px, 64 bytes per pixel, header lines of 256 bytes | diff --git a/crates/kra/src/lib.rs b/crates/kra/src/lib.rs index f59bd95cd..e2dfb1f64 100644 --- a/crates/kra/src/lib.rs +++ b/crates/kra/src/lib.rs @@ -56,6 +56,10 @@ pub const MAX_MAINDOC: usize = 32 << 20; 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; +/// Most bytes all the entries read from one archive may expand to together. Each pixel file may be +/// [`MAX_LAYER_FILE`] on its own, so without a total a document of many layers could spend far more +/// than one open should. +pub const MAX_TOTAL: u64 = 4 << 30; /// Largest ICC profile. pub const MAX_ICC: usize = 16 << 20; /// Deepest element nesting of `maindoc.xml`. @@ -287,8 +291,13 @@ pub struct Archive<'a> { impl<'a> Archive<'a> { /// Opens the archive, checks its `mimetype` and reads `maindoc.xml`. pub fn open(bytes: &'a [u8]) -> Result> { - let mut zip = - craft_zip::Archive::open(bytes, craft_zip::Limits { max_entries: MAX_ENTRIES, max_total_bytes: u64::MAX }).map_err(zip_error)?; + Self::open_with(bytes, craft_zip::Limits { max_entries: MAX_ENTRIES, max_total_bytes: MAX_TOTAL }) + } + + /// [`Archive::open`] with explicit zip limits, so a test can set a small document-wide total + /// ([`MAX_TOTAL`] is what [`Archive::open`] passes). + pub(crate) fn open_with(bytes: &'a [u8], limits: craft_zip::Limits) -> Result> { + let mut zip = craft_zip::Archive::open(bytes, limits).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")); diff --git a/crates/kra/src/tests.rs b/crates/kra/src/tests.rs index 31656e4e1..b6dfd4b72 100644 --- a/crates/kra/src/tests.rs +++ b/crates/kra/src/tests.rs @@ -386,6 +386,23 @@ fn an_archive_declaring_too_many_entries_is_refused_before_it_is_indexed() { ); } +/// Every entry read is charged to one document-wide total: each pixel file may be +/// [`MAX_LAYER_FILE`] on its own, but a document with many of them still has one budget. +#[test] +fn pixel_files_share_one_document_wide_budget() { + let doc = maindoc("RGBA", ""); + // `layer_file("x")` reads `/layers/x`. + let (first, second) = ("img/layers/x", "img/layers/y"); + let files: Vec<(&str, Vec)> = vec![(first, vec![3; 64]), (second, vec![4; 64])]; + let bytes = archive(&doc, &files); + // The mimetype and maindoc reads are charged too: room for one 64-byte pixel file and 16 bytes. + let total = doc.len() as u64 + MIMETYPE.len() as u64 + 64 + 16; + let limits = craft_zip::Limits { max_entries: MAX_ENTRIES, max_total_bytes: total }; + let mut archive = Archive::open_with(&bytes, limits).unwrap(); + assert_eq!(archive.layer_file("x").unwrap().unwrap(), vec![3; 64]); + assert!(matches!(archive.layer_file("y"), Err(Error::Invalid(_))), "the second pixel file is past the total"); +} + /// 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() { diff --git a/crates/ora/README.md b/crates/ora/README.md index 70cdbd932..488268b3c 100644 --- a/crates/ora/README.md +++ b/crates/ora/README.md @@ -29,6 +29,7 @@ means. `SVG_COMPOSITE_OPS` lists the names the specification defines. | Elements under the root stack | 8192 (`MAX_NODES`) | | `stack.xml` | 16 MiB, 512 levels of nesting, no document type declaration | | An entry | the caller's limit; `MAX_ENTRY` (512 MiB) is the suggested one for a layer image | +| All entries read from one archive | 4 GiB (`MAX_TOTAL`) | ## ZIP diff --git a/crates/ora/src/lib.rs b/crates/ora/src/lib.rs index 69d494d0d..f39d13f06 100644 --- a/crates/ora/src/lib.rs +++ b/crates/ora/src/lib.rs @@ -51,6 +51,10 @@ pub const MAX_STACK_XML: usize = 16 << 20; 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; +/// Most bytes all the entries read from one archive may expand to together. Each layer image may +/// be [`MAX_ENTRY`] on its own, so without a total a document of many layers could spend far more +/// than one open should. +pub const MAX_TOTAL: u64 = 4 << 30; /// Deepest element nesting of `stack.xml`. pub const MAX_XML_DEPTH: usize = 512; @@ -363,8 +367,13 @@ pub struct Archive<'a> { impl<'a> Archive<'a> { /// Opens the archive, checks its `mimetype` and reads `stack.xml`. pub fn open(bytes: &'a [u8]) -> Result> { - let mut zip = - craft_zip::Archive::open(bytes, craft_zip::Limits { max_entries: MAX_ENTRIES, max_total_bytes: u64::MAX }).map_err(zip_error)?; + Self::open_with_total(bytes, MAX_TOTAL) + } + + /// [`Archive::open`], allowing the entries read from the archive to expand to + /// `max_total_bytes` together ([`MAX_TOTAL`] is the default). + pub(crate) fn open_with_total(bytes: &'a [u8], max_total_bytes: u64) -> Result> { + let mut zip = craft_zip::Archive::open(bytes, craft_zip::Limits { max_entries: MAX_ENTRIES, max_total_bytes }).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")); diff --git a/crates/ora/src/tests.rs b/crates/ora/src/tests.rs index cfaac7c41..f8773f79c 100644 --- a/crates/ora/src/tests.rs +++ b/crates/ora/src/tests.rs @@ -315,6 +315,19 @@ fn an_archive_declaring_too_many_entries_is_refused_before_it_is_indexed() { ); } +/// Every entry read is charged to one document-wide total, so an archive of many individually +/// allowed entries cannot spend an unbounded amount of memory in one open. +#[test] +fn entries_are_charged_to_one_document_wide_total() { + let stack = ""; + let bytes = archive(stack, &[("a.png", vec![1; 100]), ("b.png", vec![2; 100])]); + // Room for `stack.xml` and one image, not two. + let mut a = Archive::open_with_total(&bytes, stack.len() as u64 + 120).unwrap(); + assert_eq!(a.entry("a.png", MAX_ENTRY).unwrap().unwrap(), vec![1; 100]); + let err = a.entry("b.png", MAX_ENTRY).unwrap_err(); + assert!(matches!(&err, Error::Invalid(m) if m.contains("b.png") && m.contains("may expand to")), "{err:?}"); +} + /// 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() {