Merge PR #1559

# Conflicts:
#	docs/whats-new.md

crates/engine/src/preset_import.rs

@@ -28,28 +28,45 @@ pub const PRESET_EXTS: &[&str] = &["xmp", "lrtemplate", "zip", "dng", "lmp", "mp
 
 // ------------------------------------------------------------------------------------- zip
 
+/// The most a preset zip entry may hold unpacked (a "DNG preset" photo is the largest kind, tens
+/// of MB): its stated size is the file's word, so a small archive could otherwise ask for 8 GB.
+pub const MAX_ZIP_ENTRY: usize = 256 << 20;
+/// The most a whole preset zip may hold unpacked.
+pub const MAX_ZIP_TOTAL: usize = 1 << 30;
+
 /// The files of a zip archive: (path inside the archive, bytes). Stored and deflated entries;
-/// directories, macOS resource forks and hidden files are skipped.
+/// directories, macOS resource forks and hidden files are skipped. An entry that unpacks past
+/// [`MAX_ZIP_ENTRY`], or an archive past [`MAX_ZIP_TOTAL`], is an error (a zip bomb would make
+/// the allocation fail, which ends the app).
 pub fn read_zip(bytes: &[u8]) -> Result<Vec<(String, Vec<u8>)>, String> {
-    let u16_at = |o: usize| bytes.get(o..o + 2).map(|b| u16::from_le_bytes([b[0], b[1]]) as usize);
-    let u32_at = |o: usize| bytes.get(o..o + 4).map(|b| u32::from_le_bytes([b[0], b[1], b[2], b[3]]) as usize);
+    read_zip_limited(bytes, MAX_ZIP_ENTRY, MAX_ZIP_TOTAL)
+}
+
+fn read_zip_limited(bytes: &[u8], max_entry: usize, max_total: usize) -> Result<Vec<(String, Vec<u8>)>, String> {
+    let u16_at = |o: usize| bytes.get(o..o.checked_add(2)?).map(|b| u16::from_le_bytes([b[0], b[1]]) as usize);
+    let u32_at = |o: usize| bytes.get(o..o.checked_add(4)?).map(|b| u32::from_le_bytes([b[0], b[1], b[2], b[3]]) as usize);
+    // offsets from the file, added up without overflowing (a 32-bit build reads them too)
+    let add = |a: usize, b: usize| a.checked_add(b).ok_or("bad zip");
     // end of central directory: the last "PK\x05\x06" within the trailing 64 KiB + 22 bytes
     let tail = bytes.len().saturating_sub(65_557);
     let eocd = (tail..bytes.len().saturating_sub(21)).rev().find(|&i| bytes[i..].starts_with(b"PK\x05\x06")).ok_or("not a zip file")?;
     let count = u16_at(eocd + 10).ok_or("bad zip")?;
     let mut at = u32_at(eocd + 16).ok_or("bad zip")?;
     let mut out = Vec::new();
+    let mut total = 0usize;
     for _ in 0..count {
         if !bytes.get(at..).is_some_and(|b| b.starts_with(b"PK\x01\x02")) {
             return Err("bad zip central directory".into());
         }
-        let method = u16_at(at + 10).ok_or("bad zip")?;
-        let csize = u32_at(at + 20).ok_or("bad zip")?;
-        let usize_ = u32_at(at + 24).ok_or("bad zip")?;
-        let (nlen, xlen, clen) = (u16_at(at + 28).ok_or("bad zip")?, u16_at(at + 30).ok_or("bad zip")?, u16_at(at + 32).ok_or("bad zip")?);
-        let local = u32_at(at + 42).ok_or("bad zip")?;
-        let name = String::from_utf8_lossy(bytes.get(at + 46..at + 46 + nlen).ok_or("bad zip")?).replace('\\', "/");
-        at += 46 + nlen + xlen + clen;
+        let method = u16_at(add(at, 10)?).ok_or("bad zip")?;
+        let csize = u32_at(add(at, 20)?).ok_or("bad zip")?;
+        let usize_ = u32_at(add(at, 24)?).ok_or("bad zip")?;
+        let (nlen, xlen, clen) =
+            (u16_at(add(at, 28)?).ok_or("bad zip")?, u16_at(add(at, 30)?).ok_or("bad zip")?, u16_at(add(at, 32)?).ok_or("bad zip")?);
+        let local = u32_at(add(at, 42)?).ok_or("bad zip")?;
+        let name_at = add(at, 46)?;
+        let name = String::from_utf8_lossy(bytes.get(name_at..add(name_at, nlen)?).ok_or("bad zip")?).replace('\\', "/");
+        at = add(add(add(name_at, nlen)?, xlen)?, clen)?;
         let skip = name.ends_with('/') || name.split('/').any(|p| p.starts_with('.') || p == "__MACOSX");
         if skip {
             continue;
@@ -57,13 +74,20 @@ pub fn read_zip(bytes: &[u8]) -> Result<Vec<(String, Vec<u8>)>, String> {
         if !bytes.get(local..).is_some_and(|b| b.starts_with(b"PK\x03\x04")) {
             return Err(format!("{name}: bad local header"));
         }
-        let data_at = local + 30 + u16_at(local + 26).ok_or("bad zip")? + u16_at(local + 28).ok_or("bad zip")?;
-        let data = bytes.get(data_at..data_at + csize).ok_or_else(|| format!("{name}: truncated"))?;
+        let data_at = add(add(add(local, 30)?, u16_at(add(local, 26)?).ok_or("bad zip")?)?, u16_at(add(local, 28)?).ok_or("bad zip")?)?;
+        let data = bytes.get(data_at..add(data_at, csize)?).ok_or_else(|| format!("{name}: truncated"))?;
+        let left = max_total.saturating_sub(total);
+        let limit = max_entry.min(left);
+        let too_big = || format!("{name}: unpacks to more than {} MB, too much for a preset", limit >> 20);
         let content = match method {
+            0 if data.len() > limit => return Err(too_big()),
             0 => data.to_vec(),
-            8 => miniz_oxide::inflate::decompress_to_vec_with_limit(data, usize_.max(1) * 2 + 1024).map_err(|e| format!("{name}: {e:?}"))?,
+            8 => miniz_oxide::inflate::decompress_to_vec_with_limit(data, usize_.max(1).saturating_mul(2).saturating_add(1024).min(limit)).map_err(
+                |e| if matches!(e.status, miniz_oxide::inflate::TINFLStatus::HasMoreOutput) { too_big() } else { format!("{name}: {e:?}") },
+            )?,
             m => return Err(format!("{name}: unsupported compression method {m}")),
         };
+        total = total.saturating_add(content.len());
         out.push((name, content));
     }
     Ok(out)
@@ -695,6 +719,33 @@ text]], z = ZSTR "loc" }"#,
         assert!(read_presets("plain.jpg", b"\xff\xd8 no xmp", None).is_err());
     }
 
+    #[test]
+    fn zip_bombs_are_refused() {
+        // an entry unpacking past the limit (3 MB of zeros pack into a few kB): refused, named
+        let zeros = vec![0u8; 3 << 20];
+        let z = zip(&[("Pack/huge.xmp", &zeros)], true);
+        assert!(z.len() < 64 << 10, "a small archive: {} bytes", z.len());
+        let e = read_zip_limited(&z, 1 << 20, 1 << 30).unwrap_err();
+        assert!(e.contains("Pack/huge.xmp") && e.contains("more than 1 MB"), "{e}");
+        // the whole archive counts too
+        let two = zip(&[("a.xmp", &zeros[..1 << 20]), ("b.xmp", &zeros[..1 << 20])], true);
+        assert!(read_zip_limited(&two, 2 << 20, (3 << 20) / 2).is_err());
+        assert_eq!(read_zip_limited(&two, 2 << 20, 3 << 20).unwrap().len(), 2);
+        // and stored entries
+        let stored = zip(&[("s.xmp", &zeros[..2 << 20])], false);
+        assert!(read_zip_limited(&stored, 1 << 20, 1 << 30).is_err());
+        // the size an entry states (here 4 GB) no longer sets how much may be unpacked
+        let mut lying = zip(&[("a.xmp", b"<x:xmpmeta/>")], true);
+        let cd = lying.windows(4).position(|w| w == b"PK\x01\x02").unwrap();
+        lying[cd + 24..cd + 28].copy_from_slice(&u32::MAX.to_le_bytes());
+        assert_eq!(read_zip(&lying).unwrap()[0].1, b"<x:xmpmeta/>");
+        // offsets near the top of a u32 are an error, not an overflow
+        let mut far = zip(&[("a.xmp", b"x")], false);
+        let cd = far.windows(4).position(|w| w == b"PK\x01\x02").unwrap();
+        far[cd + 42..cd + 46].copy_from_slice(&u32::MAX.to_le_bytes());
+        assert!(read_zip(&far).is_err());
+    }
+
     #[test]
     fn zip_bundles_with_folders_as_groups() {
         for deflate in [false, true] {

docs/whats-new.md

@@ -16,6 +16,10 @@
 - A photo whose Defringe amount was stored far past the sliders' 20 (in a damaged catalog or sidecar) no longer
   crashes LightCraft each time it renders: the amount acts as 20.
 
+### Reliability
+- Importing a preset zip that unpacks to far more than any preset (a "zip bomb") is refused with a message naming
+  the file, instead of asking for gigabytes of memory, which could end LightCraft.
+
 ### Slider drags on large raws
 - Dragging a slider no longer re-renders the edited photo's filmstrip thumbnail on every frame; it follows once you
   let go. On a 64 MP raw the first of those renders decoded the whole file again, which held up the loupe's