craft-ora, craft-kra: charge every entry to one document-wide total (#1339)

An archive could hold many individually allowed members whose combined
decompression was unbounded, because both readers passed
max_total_bytes: u64::MAX to craft-zip. Each now passes MAX_TOTAL (4 GiB)
and every entry read is charged to it.

craft-fig's declared-entry cap drops from 1,000,000 to 100,000 to match the
other ZIP readers; its images were already bounded by Limits::max_image_total.

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)]

crates/kra/README.md

@@ -31,6 +31,7 @@ here. There is no writer.
 | `<layer>` 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 |
 

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<Archive<'a>> {
-        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<Archive<'a>> {
+        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"));

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 `<image name>/layers/x`.
+    let (first, second) = ("img/layers/x", "img/layers/y");
+    let files: Vec<(&str, Vec<u8>)> = 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() {

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
 

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<Archive<'a>> {
-        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<Archive<'a>> {
+        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"));

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 = "<image w='1' h='1'><stack/></image>";
+    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() {