security(engine): one unpack budget across a preset zip's packs; linear comment skipping; small zip errors (#3675)

* security(engine): one unpack budget across a preset zip's packs; linear comment skipping; small zip errors

* address review: charge a failed pack's unpacked bytes to the shared zip budget

* address review: charge a corrupt deflate entry's partial output to the shared budget

* address review: an entry past its stated size spends only what it unpacked

crates/engine/src/preset_import.rs

@@ -42,7 +42,13 @@ pub fn read_zip(bytes: &[u8]) -> Result<Vec<(String, Vec<u8>)>, String> {
     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> {
+pub(crate) fn read_zip_limited(bytes: &[u8], max_entry: usize, max_total: usize) -> Result<Vec<(String, Vec<u8>)>, String> {
+    read_zip_spending(bytes, max_entry, max_total, &mut 0)
+}
+
+/// [`read_zip_limited`] that adds to `spent` the bytes of each entry as it is unpacked, so they
+/// are counted when a later entry fails; an entry that fails for size spends its whole limit.
+pub(crate) fn read_zip_spending(bytes: &[u8], max_entry: usize, max_total: usize, spent: &mut 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)
@@ -53,7 +59,7 @@ fn read_zip_limited(bytes: &[u8], max_entry: usize, max_total: usize) -> Result<
     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;
+    let total = spent;
     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());
@@ -76,18 +82,40 @@ fn read_zip_limited(bytes: &[u8], max_entry: usize, max_total: usize) -> Result<
         }
         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 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 if data.len() > limit => {
+                *total = total.saturating_add(limit);
+                return Err(too_big());
+            }
             0 => data.to_vec(),
-            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:?}") },
-            )?,
+            8 => {
+                // the entry's own stated size, with slack, caps the inflate below `limit`
+                let cap = usize_.max(1).saturating_mul(2).saturating_add(1024);
+                match miniz_oxide::inflate::decompress_to_vec_with_limit(data, cap.min(limit)) {
+                    Ok(v) => v,
+                    Err(e) if matches!(e.status, miniz_oxide::inflate::TINFLStatus::HasMoreOutput) && cap < limit => {
+                        // not over the budget: only what was unpacked is spent
+                        *total = total.saturating_add(e.output.len());
+                        return Err(format!("{name}: unpacks to more than its stated size"));
+                    }
+                    Err(e) if matches!(e.status, miniz_oxide::inflate::TINFLStatus::HasMoreOutput) => {
+                        // past the budget (or the entry limit): its whole limit is spent
+                        *total = total.saturating_add(limit);
+                        return Err(too_big());
+                    }
+                    Err(e) => {
+                        // what a corrupt stream unpacked before failing was still unpacked: charge it
+                        *total = total.saturating_add(e.output.len());
+                        return Err(format!("{name}: {:?}", e.status));
+                    }
+                }
+            }
             m => return Err(format!("{name}: unsupported compression method {m}")),
         };
-        total = total.saturating_add(content.len());
+        *total = total.saturating_add(content.len());
         out.push((name, content));
     }
     Ok(out)
@@ -139,7 +167,11 @@ impl LuaParser<'_> {
             if self.s[self.i..].starts_with(b"--") {
                 self.i += 2;
                 if let Some(level) = self.long_bracket_level() {
-                    let _ = self.long_string(level);
+                    // an unterminated long comment runs to the end (as in Lua): going on past its
+                    // opening would search the rest of the file again for every such opening
+                    if self.long_string(level).is_err() {
+                        self.i = self.s.len();
+                    }
                 } else {
                     while self.i < self.s.len() && self.s[self.i] != b'\n' {
                         self.i += 1;
@@ -546,14 +578,23 @@ pub fn read_presets(name: &str, bytes: &[u8], group: Option<String>) -> Result<V
         "zip" => {
             let mut out = Vec::new();
             let mut errors = Vec::new();
-            for (inner, data) in read_zip(bytes)? {
+            let entries = read_zip(bytes)?;
+            // what the archive's entries left of the unpack budget: the Luminar packs inside it
+            // (archives themselves) unpack within it, not each with a budget of its own
+            let mut budget = MAX_ZIP_TOTAL.saturating_sub(entries.iter().map(|(_, d)| d.len()).sum());
+            for (inner, data) in entries {
                 let iext = Path::new(&inner).extension().map(|e| e.to_string_lossy().to_ascii_lowercase()).unwrap_or_default();
                 if !(PRESET_EXTS.contains(&iext.as_str()) || iext == crate::presets::LCPRESET_EXT) || iext == "zip" {
                     continue;
                 }
                 let dir_group =
                     inner.rsplit_once('/').and_then(|(d, _)| group_from_dir(d)).or_else(|| group_from_dir(&stem)).or_else(|| group.clone());
-                match read_presets(&inner, &data, dir_group) {
+                let read = if iext == "mplumpack" {
+                    crate::preset_luminar::read_mplumpack_within(&inner, &data, &mut budget)
+                } else {
+                    read_presets(&inner, &data, dir_group)
+                };
+                match read {
                     Ok(v) => out.extend(v),
                     Err(e) => errors.push(format!("{inner}: {e}")),
                 }
@@ -771,6 +812,68 @@ text]], z = ZSTR "loc" }"#,
         assert!(photo(r#"crs:WhiteBalance="As Shot""#).unmapped.is_empty());
     }
 
+    #[test]
+    fn packs_inside_a_zip_share_its_unpack_budget() {
+        // two packs unpacking to 2 MB each on a 3 MB budget: the first spends it, the second is refused
+        let pad = vec![b' '; 2 << 20];
+        let pack = zip(&[("a.lmp", b"<plist/>"), ("pad.txt", &pad)], true);
+        let mut budget = 3 << 20;
+        let first = crate::preset_luminar::read_mplumpack_within("one.mplumpack", &pack, &mut budget).unwrap_err();
+        assert!(!first.contains("too much"), "{first}");
+        assert_eq!(budget, (1 << 20) - 8);
+        let second = crate::preset_luminar::read_mplumpack_within("two.mplumpack", &pack, &mut budget).unwrap_err();
+        assert!(second.contains("too much"), "{second}");
+    }
+
+    #[test]
+    fn a_pack_that_fails_part_way_still_spends_what_it_unpacked() {
+        // 2 MB unpacks, then an entry that does not fit the 3 MB budget: the next pack gets nothing
+        let pad = vec![b' '; 2 << 20];
+        let pack = zip(&[("a.txt", &pad), ("b.txt", &pad)], true);
+        let mut budget = 3 << 20;
+        let first = crate::preset_luminar::read_mplumpack_within("one.mplumpack", &pack, &mut budget).unwrap_err();
+        assert!(first.contains("too much"), "{first}");
+        assert_eq!(budget, 0);
+    }
+
+    #[test]
+    fn a_corrupt_deflate_entry_spends_what_it_unpacked() {
+        // 2 MB declared, the stream cut in half: it unpacks a part, then fails; the budget keeps the part
+        let pad = vec![b' '; 2 << 20];
+        let deflated = miniz_oxide::deflate::compress_to_vec(&pad, 6);
+        let mut pack = zip(&[("a.txt", &deflated[..deflated.len() / 2])], false);
+        let central = 30 + "a.txt".len() + deflated.len() / 2;
+        pack[8] = 8; // local header: method deflate
+        pack[central + 10] = 8; // central header: method deflate
+        pack[central + 24..central + 28].copy_from_slice(&(pad.len() as u32).to_le_bytes());
+        let mut budget = 3 << 20;
+        let err = crate::preset_luminar::read_mplumpack_within("one.mplumpack", &pack, &mut budget).unwrap_err();
+        assert!(!err.contains("too much"), "{err}");
+        assert!(budget < 3 << 20, "{budget}");
+    }
+
+    #[test]
+    fn an_unterminated_long_comment_ends_the_template() {
+        // 20 000 openings of a long comment that never closes: each used to search the rest of
+        // the file for its close (seconds to minutes); now the first ends the file
+        let text = format!("s = {{ a = 1 }} {}", "--[==[ ".repeat(20_000));
+        let t = std::time::Instant::now();
+        let _ = parse_lua(&text);
+        assert!(t.elapsed().as_secs() < 5, "took {:?}", t.elapsed());
+    }
+
+    #[test]
+    fn an_entry_past_its_stated_size_spends_only_what_it_unpacked() {
+        // the stated size (1 byte) caps the inflate at 1026 bytes, far below the 1 MB limit
+        let mut lying = zip(&[("a.xmp", &vec![b' '; 10_000])], true);
+        let cd = lying.windows(4).position(|w| w == b"PK\x01\x02").unwrap();
+        lying[cd + 24..cd + 28].copy_from_slice(&1u32.to_le_bytes());
+        let mut spent = 0;
+        let e = read_zip_spending(&lying, 1 << 20, 1 << 20, &mut spent).unwrap_err();
+        assert!(e.contains("stated size"), "{e}");
+        assert!(spent > 0 && spent <= 2048, "{spent}");
+    }
+
     #[test]
     fn zip_bombs_are_refused() {
         // an entry unpacking past the limit (3 MB of zeros pack into a few kB): refused, named

crates/engine/src/preset_luminar.rs

@@ -19,7 +19,7 @@ use std::collections::BTreeMap;
 use std::path::Path;
 
 use crate::crs::Props;
-use crate::preset_import::{Imported, build, group_from_dir, read_zip};
+use crate::preset_import::{Imported, MAX_ZIP_ENTRY, MAX_ZIP_TOTAL, build, group_from_dir, read_zip_spending};
 
 // ----------------------------------------------------------------------------- property lists
 
@@ -455,7 +455,18 @@ pub fn read_lmp(path: &str, bytes: &[u8], group: Option<String>) -> Result<Impor
 /// Read a Luminar looks collection (`.mplumpack`, a zip): every look, grouped by the pack's
 /// `GroupName` (else the pack's file name).
 pub fn read_mplumpack(path: &str, bytes: &[u8]) -> Result<Vec<Imported>, String> {
-    let entries = read_zip(bytes)?;
+    let mut budget = MAX_ZIP_TOTAL;
+    read_mplumpack_within(path, bytes, &mut budget)
+}
+
+/// [`read_mplumpack`] unpacking at most `budget` bytes, which it spends: packs inside one zip
+/// share the zip's budget.
+pub(crate) fn read_mplumpack_within(path: &str, bytes: &[u8], budget: &mut usize) -> Result<Vec<Imported>, String> {
+    // spent as each entry unpacks: a pack that fails part way still used its budget
+    let mut spent = 0;
+    let entries = read_zip_spending(bytes, MAX_ZIP_ENTRY, *budget, &mut spent);
+    *budget = budget.saturating_sub(spent);
+    let entries = entries?;
     let info = entries
         .iter()
         .find(|(n, _)| n.rsplit('/').next().is_some_and(|b| b.eq_ignore_ascii_case("PresetsInfo.plist")))