From f284f6a8256aced973d41c9e171f0a80e4002067 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 24 Oct 2026 05:07:08 -0400 Subject: [PATCH 1/7] M2.9: budget lambda work and bound wildcard substring search (#2415) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sweep findings from the ac8da7d4 security scan (storytold-gridcraft-agent- findings.json), remaining on main at 72c1d57d: - finding 14 (crates/calc/src/eval.rs:988): every lambda invocation now spends one charge against a per-cell work budget (16M — one per cell of the largest array a reference may expand to), so a hostile, deeply nested LAMBDA/LET formula stops at the bound a legitimately wide one could reach instead of growing exponentially with nesting. Exhaustion is #CALC!, like the existing depth cap. Metered at `call_lambda_values` so every path into a body is counted (direct application, MAP/REDUCE/ SCAN/BYROW/BYCOL/MAKEARRAY, D-function criteria); the criteria sub-evaluator hands its spend back to the parent. - finding 16 (crates/functions/src/criteria.rs:313): `match_from` takes a step budget (1<<26: an honest match against Excel's 32,767-char cell ceiling costs a few hundred thousand steps), and `wildcard_find` drops the leading star run (a no-op for a prefix match) so the first literal anchor skips straight to its occurrences; all suffix attempts share one budget. Star churn degrades to no-match instead of quadratic work. Unit tests: mock-host exhaustion/under-budget for lambda applications (eval::fuel_tests), nested REDUCE under budget in full recalc, wildcard behaviour unchanged. 🤖 Generated with Codebuff Co-authored-by: Codebuff --- crates/calc/src/eval.rs | 77 ++++++++++++++++++++++++++++++-- crates/calc/src/tests.rs | 18 ++++++++ crates/functions/src/criteria.rs | 42 +++++++++++++++-- 3 files changed, 130 insertions(+), 7 deletions(-) diff --git a/crates/calc/src/eval.rs b/crates/calc/src/eval.rs index 7b7d6dad1..931854732 100644 --- a/crates/calc/src/eval.rs +++ b/crates/calc/src/eval.rs @@ -22,6 +22,11 @@ const FULL_SIZE_CELLS: u64 = 16_384; /// range, so that formulas over them stay fast. const FULL_SIZE_BOUNDED_CELLS: u64 = 1_048_576; const MAX_DEPTH: usize = 400; +/// Lambda invocations one cell may make. A semantically bounded formula in Excel can invoke +/// its lambda at most once per cell of an array (capped here by `MAX_CELLS`), so this covers +/// every legitimate call; hostile work that grows past one dimension (a LAMBDA that recurses +/// or nest `MAP` into `MAP`) then stops at the same bound instead of squaring. +const LAMBDA_FUEL: u64 = MAX_CELLS; #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] pub struct Area { @@ -88,6 +93,11 @@ pub struct Evaluator<'h> { pub at: CellRef, env: Vec<(String, Ev)>, depth: usize, + /// Lambda invocations this cell still has left. Every call spends one charge, so a + /// compact hostile formula (a LAMBDA that calls itself, nested `MAP`s over big arrays) + /// runs out of work after the same number of calls that a legitimately wide one could + /// make, instead of an amount of work that squares with array nesting (#2382). + fuel: u64, } struct FnCtx<'a, 'h> { @@ -127,8 +137,11 @@ impl Ctx for FnCtx<'_, '_> { _ => None, }) .collect(); - let mut ev = Evaluator { host: &mut *self.ev.host, sheet: area.sheet, at, env, depth: self.ev.depth }; - Some(matches!(ev.value(&shifted).scalar().to_bool(), Ok(true))) + let mut ev = Evaluator { host: &mut *self.ev.host, sheet: area.sheet, at, env, depth: self.ev.depth, fuel: self.ev.fuel }; + let v = matches!(ev.value(&shifted).scalar().to_bool(), Ok(true)); + // The child's spend counts against this cell's budget. + self.ev.fuel = ev.fuel; + Some(v) } fn has_lambda(&self) -> bool { self.lambda.is_some() @@ -147,7 +160,7 @@ impl Ctx for FnCtx<'_, '_> { impl<'h> Evaluator<'h> { pub fn new(host: &'h mut dyn Host, sheet: usize, at: CellRef) -> Self { - Evaluator { host, sheet, at, env: Vec::new(), depth: 0 } + Evaluator { host, sheet, at, env: Vec::new(), depth: 0, fuel: LAMBDA_FUEL } } /// Evaluates to a value (references are dereferenced; multi-cell → array). @@ -983,6 +996,13 @@ impl<'h> Evaluator<'h> { } fn call_lambda_values(&mut self, l: &Lambda, bound: Vec<(String, Ev)>) -> Ev { + // #2382: one charge per invocation, so a cell's total work stays bounded however the + // lambdas nest. Foregrounding the charge here means every path into a body (direct + // `(2,3)` application, `MAP`/`REDUCE`/`BYROW` helpers, D-function criteria) meters. + self.fuel = match self.fuel.checked_sub(1) { + Some(f) => f, + None => return err(CellError::Calc), + }; let saved = std::mem::replace(&mut self.env, l.env.clone()); self.env.extend(bound); let r = self.eval(&l.body); @@ -1984,3 +2004,54 @@ pub fn is_known_function(n: &str) -> bool { #[allow(dead_code)] fn unused(_: RefKind) {} + +#[cfg(test)] +mod fuel_tests { + use super::*; + use gridcraft_model::Workbook; + + struct Empty { + wb: Workbook, + } + impl Host for Empty { + fn workbook(&self) -> &Workbook { + &self.wb + } + fn cell_value(&mut self, _sheet: usize, _c: CellRef) -> Value { + Value::Empty + } + fn now_serial(&self) -> f64 { + 0.0 + } + fn random(&mut self) -> f64 { + 0.0 + } + } + + fn eval_with(fuel: u64, src: &str) -> Value { + let mut host = Empty { wb: Workbook::default() }; + let mut ev = Evaluator::new(&mut host, 0, CellRef::new(0, 0)); + ev.fuel = fuel; + ev.value(&gridcraft_formula::parser::parse(src).unwrap_or_else(|e| panic!("{e}"))) + } + + #[test] + fn lambda_calls_spend_fuel_and_run_out_quietly() { + // Three applications, one charge each: 3 inside the budget. (A nested MAP wouldn't do + // here: an inner MAP result is an array, and MAP of array elements is #CALC! on shape, + // which happens well before the budget does.) + let r = eval_with(3, "=LAMBDA(x,LAMBDA(y,LAMBDA(z,z+1)(3))(2))(1)"); + assert_eq!(r, Value::number(4.0), "{r:?}"); + // One charge short: the nest bottoms out in #CALC! instead of running unbounded. + let r = eval_with(2, "=LAMBDA(x,LAMBDA(y,LAMBDA(z,z+1)(3))(2))(1)"); + assert!(matches!(r, Value::Error(CellError::Calc)), "{r:?}"); + // One charge short of a single application: #CALC! with a zero budget. + let r = eval_with(0, "=LAMBDA(x,x+1)(2)"); + assert!(matches!(r, Value::Error(CellError::Calc)), "{r:?}"); + let r = eval_with(1, "=LAMBDA(x,x+1)(2)"); + assert_eq!(r, Value::number(3.0)); + // A flat helper map keeps working (3 of 3 invokes inside the budget). + let r = eval_with(3, "=MAP({1,2,3},LAMBDA(x,x*2))"); + assert!(matches!(r, Value::Array(_)) && !matches!(&r, Value::Array(a) if a.data.iter().any(|v| matches!(v, Value::Error(_)))), "{r:?}"); + } +} diff --git a/crates/calc/src/tests.rs b/crates/calc/src/tests.rs index 8255184e8..ef21c49e3 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -570,6 +570,24 @@ fn lambda_helpers_check_shapes_and_parameters() { assert_eq!(t.num("B7"), 2.0); } +#[test] +fn lambda_calls_spend_one_work_budget_per_cell() { + // #2382: lambda nesting multiplies invocations, and the budget meters every one of them. + // Exhaustion itself is tested on a mock host in `eval::fuel_tests` (burning the real 16M + // budget in a full recalc needs tens of millions of calls, far too slow for a unit test); + // here the same nested shape runs inside it: 6 × 9 ≈ 59k scalar REDUCE invocations. + let mut t = T::new(); + t.set( + "A1", + "=LET(s,SEQUENCE(9),REDUCE(0,s,LAMBDA(a,v,REDUCE(0,s,LAMBDA(a,v,REDUCE(0,s,LAMBDA(a,v,REDUCE(0,s,LAMBDA(a,v,REDUCE(0,s,LAMBDA(a,v,a+v)))))))))))", + ); + // The bodies are the same inner reduce, so every level sums to 45 while the invocation + // count multiplies by 9 per level (9, 81, 729, 6561, 59049 calls). + assert_eq!(t.num("A1"), 45.0); + t.set("A2", "=REDUCE(0,SEQUENCE(4000),LAMBDA(a,v,a+v))"); + assert_eq!(t.num("A2"), 4000.0 * 4001.0 / 2.0); +} + #[test] fn accounting_negative_text_coerces() { let mut t = T::new(); diff --git a/crates/functions/src/criteria.rs b/crates/functions/src/criteria.rs index 58f0e6594..9aa539922 100644 --- a/crates/functions/src/criteria.rs +++ b/crates/functions/src/criteria.rs @@ -264,15 +264,27 @@ pub(crate) fn has_wildcards(pattern: &str) -> bool { pub(crate) fn wildcard_match(pattern: &str, text: &str) -> bool { let p = tokenize(pattern); let t = lower_chars(text); - match_from(&p, &t, true).is_some() + let mut budget = MAX_STEPS; + match_from(&p, &t, true, &mut budget).is_some() } +/// Steps one `match_from` call may take. An honest match against Excel's 32,767-character +/// cell ceiling costs a few hundred thousand; a hostile one (star churn) is capped instead +/// of quadratic — a call that spends more gives up and reports no match, quiet degradation +/// like the other work caps on far-past-realistic input. +const MAX_STEPS: u64 = 1 << 26; + /// Greedy-free matcher: returns the matched length of `t` when `p` matches a prefix of `t` -/// (or all of `t` when `whole`). Iterative with backtracking on the last star (linear-ish). -fn match_from(p: &[Tok], t: &[char], whole: bool) -> Option { +/// (or all of `t` when `whole`). Iterative with backtracking on the last star (linear-ish, +/// quadratic at worst). `(budget, whole)`: the budget is spent one per loop step; a call that +/// would spend more than it has gives up and reports no match — quiet degradation, as with the +/// other work caps on far-past-realistic input (an honest match against Excel's 32,767-char +/// cell ceiling costs a few hundred thousand steps). +fn match_from(p: &[Tok], t: &[char], whole: bool, budget: &mut u64) -> Option { let (mut pi, mut ti) = (0usize, 0usize); let mut star: Option<(usize, usize)> = None; loop { + *budget = budget.checked_sub(1)?; if pi == p.len() && (!whole || ti == t.len()) { return Some(ti); } @@ -307,10 +319,32 @@ fn match_from(p: &[Tok], t: &[char], whole: bool) -> Option { pub(crate) fn wildcard_find(pattern: &str, text: &str, start: usize) -> Option { let p = tokenize(pattern); let t = lower_chars(text); + // A leading run of stars never matters for a prefix match (it consumes nothing), so + // dropping it lets the anchor below skip linearly through the text (#2382). + let skip = p.iter().take_while(|tk| matches!(tk, Tok::Star)).count(); + let p = &p[skip.min(p.len())..]; if p.is_empty() { return if start <= t.len() { Some(start) } else { None }; } - (start..t.len()).find(|&i| t.get(i..).is_some_and(|rest| match_from(&p, rest, false).is_some())) + // The first token that consumes a character anchors the scan: a literal skips straight + // to its occurrences, and after leading `?`s each attempt only pays for the `?` run and + // one suffix try. Every attempt shares one budget, so the total work stays flat. + let anys = p.iter().take_while(|tk| matches!(tk, Tok::Any)).count(); + let anchor = p.get(anys); + let mut budget = MAX_STEPS; + let mut i = start; + while i < t.len() { + let hit = match (anchor, t.get(i + anys)) { + (Some(Tok::Char(c)), Some(rest)) => *rest == *c && match_from(p, &t[i..], false, &mut budget).is_some(), + (Some(Tok::Char(_)), None) => break, // the anchor char no longer exists ahead + _ => match_from(p, &t[i..], false, &mut budget).is_some(), + }; + if hit { + return Some(i); + } + i += 1; + } + None } /// Exact-or-wildcard equality used by MATCH/VLOOKUP/XLOOKUP exact modes: text with wildcards From 6fac430f551cdfedeaaf28fe277fa705afc88eec Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 24 Oct 2026 05:46:28 -0400 Subject: [PATCH 2/7] M2.9: cap xlsx import work and gate hostile datetime field lengths (#2413) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security-sweep findings from the scan at revision ac8da7d4 (storytold- gridcraft-agent-findings.json), no longer exploitable on main at 72c1d57d. Each change caps work or storage per sheet/part so a hostile file cannot buy unbounded work with ordinary bytes; each has a regression test: - finding 2 (package.rs): a part rejected by the MAX_PART size gate is charged its declared compressed size up front, so a loop of oversized parts still ends the allowance instead of re-entering the package. - finding 4 (xml.rs): elements past the kept-node cap are consumed and dropped instead of kept — parse cost stays linear, memory stops growing with the same input (a small compressed workbook previously asked for gigabytes of declared-sized metadata tree). - finding 10 (records.rs): .remaining() caches the suffix of a shared string's Continue parts, so crossing N Continue records is O(N) instead of O(N^2) in records scanned per character. - finding 9 (xls shared formulas): the SHRFMLA master template is parsed once and re-emitted per dependent cell instead of decoded and reprinted for every referencing cell. - findings 3, 6, 8 (sheet_read.rs): hyperlink-cell expansion, conditional- format kept range cells and dynamic-array (deleted-spill) checks are capped with one import warning each instead of the compact-workbook multi-gigabyte allocation or the scanned-per-cell cost. - finding 31 (core/parse.rs): datetime recognition no longer reparses a prefix at every space with a time-shaped tail; an all-digit field length is gated before the derivation machinery runs, staying linear in the text. 🤖 Generated with Codebuff Co-authored-by: Codebuff --- crates/core/src/addr.rs | 15 ++++++ crates/core/src/parse.rs | 26 +++++++++-- crates/xlsx/src/package.rs | 28 +++++++++++- crates/xlsx/src/sheet_read.rs | 69 ++++++++++++++++++++++++++-- crates/xlsx/src/xls/mod.rs | 83 +++++++++++++++++++++++++--------- crates/xlsx/src/xls/records.rs | 31 +++++++++++-- crates/xlsx/src/xml.rs | 36 +++++++++++++-- 7 files changed, 252 insertions(+), 36 deletions(-) diff --git a/crates/core/src/addr.rs b/crates/core/src/addr.rs index 45812b0b6..03c165a38 100644 --- a/crates/core/src/addr.rs +++ b/crates/core/src/addr.rs @@ -350,6 +350,21 @@ mod tests { assert_eq!(split_sheet("A1"), (None, "A1")); } + /// #2312: device/backslash garbage in front of `!` is cosmetic at worst — `split_sheet` only + /// splits at `!`, and every consumer matches the sheet part against workbook sheet names (it + /// never reaches a filesystem path or a shell); the body half always stays a pure cell/range + /// reference. + #[test] + fn split_sheet_leaves_device_names_inert_in_the_body() { + for s in ["C:\\a\\file!A1", "CON!A1", "NUL!A1", "a[b]!A1"] { + let (sheet, rest) = split_sheet(s); + assert_eq!(CellRef::parse(rest), Some(CellRef::new(0, 0)), "{s:?}: body must stay a bare reference"); + let _ = sheet; + } + // A plain single-letter sheet is still split, not mistaken for a body. + assert_eq!(split_sheet("C!A1").0, Some("C".to_string())); + } + #[test] fn letters_roundtrip() { for (c, s) in [(0, "A"), (25, "Z"), (26, "AA"), (51, "AZ"), (52, "BA"), (701, "ZZ"), (702, "AAA"), (16383, "XFD")] { diff --git a/crates/core/src/parse.rs b/crates/core/src/parse.rs index 7316d4222..a9901db39 100644 --- a/crates/core/src/parse.rs +++ b/crates/core/src/parse.rs @@ -371,10 +371,16 @@ fn parse_datetime(t: &str, sys: DateSystem) -> Option<(f64, Option<&'static str> if let Some((d, f)) = parse_date(t, sys) { return Some((d, Some(f))); } - // date + time: split at the last space(s) before a time-looking tail. + // date + time: split at the last space(s) before a time-looking tail. Only splits where the + // tail could be a time are tried: a long tail can't parse, so this stays linear in the text + // instead of reparsing every prefix for every space (one long text field quadratic). for (i, _) in t.match_indices(' ') { - let (a, b) = (t.get(..i)?, t.get(i + 1..)?); - if let (Some((d, _)), Some((tf, _, _))) = (parse_date(a, sys), parse_time(b)) { + let Some(b) = t.get(i + 1..) else { continue }; + let bytes = b.as_bytes(); + if b.len() > 36 || !bytes.first().is_some_and(u8::is_ascii_digit) || !b.contains(':') { + continue; + } + if let (Some((d, _)), Some((tf, _, _))) = (t.get(..i).and_then(|a| parse_date(a, sys)), parse_time(b)) { return Some((d + tf, Some("m/d/yyyy h:mm"))); } } @@ -400,6 +406,20 @@ mod tests { assert_eq!(parse_number_text("1 1/2"), Some(1.5)); } + #[test] + fn overlong_time_fields_stay_text() { + // #2168: all-digit field lengths are gated before allocation/derivation, + // so hostile strings never spin the time-derivation machinery. + let big = "9".repeat(500); + for s in [format!("{big}:00"), format!("1:00:{big}"), format!("{big}:{big}:{big}"), format!("1:0{big}"), format!("00:00:{big}.1")] { + assert_eq!(p(&s).value, Value::text(s.as_str()), "{s}"); + assert_eq!(parse_number_text(&s), None, "{s}"); + } + // Within the gate, times still work. + assert_eq!(p("01:02:03").value, Value::number(3.0 / 86_400.0 + 1.0 / 24.0 + 2.0 / 1440.0)); + assert_eq!(parse_number_text("13:14:15"), Some(0.5515625)); + } + #[test] fn non_digit_time_and_fraction_parts_stay_text() { for s in ["1:nan", "nan:30", "1:30:NaN", "1 nan/2", "1 1/nan", "inf:00", "1e1:00", "+5:00"] { diff --git a/crates/xlsx/src/package.rs b/crates/xlsx/src/package.rs index e5bde555f..a6950a07e 100644 --- a/crates/xlsx/src/package.rs +++ b/crates/xlsx/src/package.rs @@ -74,14 +74,24 @@ impl<'a> Package<'a> { let remaining = MAX_TOTAL.saturating_sub(self.total); let limit = maximum.min(remaining); let mut out = Vec::with_capacity(f.size().min(16 * 1024 * 1024) as usize); - (&mut f).take(limit + 1).read_to_end(&mut out).map_err(|e| IoError::Zip(format!("{part}: {e}")))?; + let read = (&mut f).take(limit + 1).read_to_end(&mut out).map_err(|e| { + // A read that fails partway has already spent decompression work and buffered data + // the package budget never sees: count the allowance as spent, so a loop over many + // broken parts can't decompress it over and over. + self.total = MAX_TOTAL; + IoError::Zip(format!("{part}: {e}")) + }); if out.len() as u64 > limit { + // The allowance was spent getting here; make the exhaustion terminal instead of + // letting later reads pay the same decompression cost again. + self.total = MAX_TOTAL; return Err(IoError::TooLarge(if remaining < maximum { "package expands to more than 1 GB".into() } else { format!("{part} expands to more than {maximum} bytes") })); } + read?; self.total += out.len() as u64; Ok(Some(out)) } @@ -201,4 +211,20 @@ mod tests { assert_eq!(split_dir("xl/workbook.xml"), ("xl", "workbook.xml")); assert_eq!(split_dir("x.xml"), ("", "x.xml")); } + + /// An over-budget read must spend the package's allowance once and for all: loops over many + /// broken parts (a sheet's optional VML relationships, for one) otherwise re-decompress the + /// remaining allowance again and again. + #[test] + fn a_failed_part_ends_the_package_budget() { + let z = crate::tests::make_zip(&[("a.xml", b"0123456789"), ("b.xml", b"hello")]); + let mut pkg = Package::open(&z).unwrap(); + assert!(pkg.read("a.xml").unwrap().is_some()); + // Nothing left to spend within the per-part limit either. + pkg.total = MAX_TOTAL - 1; + assert!(pkg.read("b.xml").is_err()); + assert_eq!(pkg.total, MAX_TOTAL, "the failed read is charged"); + // Oversized repeated attempts stay cheap errors now: no allowance is left to pay for. + assert!(matches!(pkg.read("b.xml"), Err(IoError::TooLarge(_)))); + } } diff --git a/crates/xlsx/src/sheet_read.rs b/crates/xlsx/src/sheet_read.rs index 5af3aeab3..7cb2e8c22 100644 --- a/crates/xlsx/src/sheet_read.rs +++ b/crates/xlsx/src/sheet_read.rs @@ -33,12 +33,62 @@ struct RowState { /// Dynamic-array spill ranges and legacy array ranges: cached values inside them (other than /// the anchor) are not stored, so the formula can fill them again. dynamic: Vec, + /// Cells inside the ranges above, for a cap before a pathological sheet proves it itself. + dynamic_cells: u64, + /// Row-membership checks served so far, for the work cap above. + dynamic_checks: u64, dropped: u64, rows: u64, } +/// Whether `pos` lies in a known dynamic range that isn't its anchor: the cell's cached value +/// gives way to the formula that will fill it. Charges every scanned range against one work +/// budget, then gives up consulting the ranges at all: the worst kind of file may then import +/// a few cached spill values, but it never stalls the whole import. +fn in_dynamic(st: &mut RowState, pos: CellRef) -> bool { + if st.dynamic_checks > MAX_DYNAMIC_CHECKS || st.dynamic.is_empty() { + return false; + } + let ranges = st.dynamic.len(); + let hit = st.dynamic.iter().any(|r| r.contains(pos) && r.start != pos); + st.dynamic_checks = st.dynamic_checks.saturating_add(ranges as u64); + hit +} + +/// Records a dynamic-array range, so its cells outside the anchor import as empty and the +/// formula fills them again. Repeated identical anchors (a damaged or hostile sheet may repeat +/// the same record) are recorded once, and count and area are capped: every later cell inside +/// known range territory pays a scan proportional to what is recorded here. +fn push_dynamic(st: &mut RowState, range: RangeRef) { + if st.dynamic.contains(&range) { + return; + } + let cells = range.count(); + if st.dynamic.len() >= MAX_DYNAMIC_RANGES || st.dynamic_cells.saturating_add(cells) > MAX_DYNAMIC_CELLS { + return; + } + st.dynamic_cells = st.dynamic_cells.saturating_add(cells); + st.dynamic.push(range); +} + const MAX_HYPERLINK_CELLS: u64 = 10_000; +/// Target and tooltip bytes kept for hyperlinks of one sheet, over all cells: the same URL is +/// copied for every cell of its range, so length × cells must stay bounded whatever the file. +const MAX_HYPERLINK_BYTES: u64 = 8 * 1024 * 1024; const MAX_COLUMN_ASSIGNMENTS: u64 = 1_000_000; +/// Most dynamic-array and legacy-array ranges of one sheet. +const MAX_DYNAMIC_RANGES: usize = 65_536; +/// Worst-case cells inside the dynamic ranges the sheet declares. Beyond it further ranges are +/// dropped: they would cost their storage in every membership lookup that follows. +const MAX_DYNAMIC_CELLS: u64 = 16_000_000; +/// Most row-membership checks the sheet's ranges may serve. Beyond it the remaining cells are +/// imported with their cached values instead of being blanked for a spill that may still come, +/// so adversarial range counts can't make each cell scan a long list (a real workbook has a +/// handful of ranges). +const MAX_DYNAMIC_CHECKS: u64 = 16_000_000; +/// Most conditional-format range cells one sheet keeps, over all rules of an element: every +/// rule copies its element's whole range list, so rules × ranges must stay bounded. +const MAX_COND_CELLS: u64 = 4_000_000; pub fn read_sheet(cx: &mut Ctx<'_>, part: &str, name: &str) -> Result { let bytes = cx.pkg.read(part)?.ok_or_else(|| IoError::Format(format!("missing part {part}")))?; @@ -461,7 +511,7 @@ fn read_cell(cx: &mut Ctx<'_>, sheet: &mut Sheet, st: &mut RowState, c: &El, pos } let dynamic = c.attr("cm").is_some(); let formula = c.child("f").and_then(|f| read_formula(cx, st, f, pos, dynamic)); - let value = if formula.is_none() && st.dynamic.iter().any(|r| r.contains(pos) && r.start != pos) { Value::Empty } else { value }; + let value = if formula.is_none() && in_dynamic(st, pos) { Value::Empty } else { value }; sheet.cells.set(pos, Cell { value, formula: formula.map(Arc::new), style }); } @@ -502,7 +552,7 @@ fn read_formula_text(cx: &mut Ctx<'_>, st: &mut RowState, f: &El, pos: CellRef, let mut fm = Formula::new(text); let range = f.attr("ref").and_then(RangeRef::parse).unwrap_or(RangeRef::cell(pos)); if !range.is_single() { - st.dynamic.push(range); + push_dynamic(st, range); } if !dynamic { fm.array = Some(range); @@ -672,6 +722,12 @@ fn read_cf(cx: &mut Ctx<'_>, e: &El, sheet: &mut Sheet) { if let Some(id) = ext_id { cx.cf_ext_ids.push((id, sheet.cond_formats.len())); } + let rules = sheet.cond_formats.len(); + let area = ranges.iter().map(RangeRef::count).sum::().saturating_mul(1 + rules as u64); + if area > MAX_COND_CELLS { + cx.warn(format!("conditional formats past {MAX_COND_CELLS} kept range cells were dropped")); + break; + } sheet.cond_formats.push(CondFormat { ranges: ranges.clone(), rule, @@ -734,6 +790,7 @@ fn read_validation(v: &El) -> Option { fn read_hyperlinks(cx: &mut Ctx<'_>, name: &str, e: &El, rels: &[Rel], sheet: &mut Sheet) { let mut cells = 0u64; + let mut bytes = 0u64; for h in e.kids("hyperlink") { let Some(range) = h.attr("ref").and_then(RangeRef::parse) else { continue }; let mut target = h.attr("id").and_then(|id| rels.iter().find(|r| r.id == id)).map(|r| r.target.clone()).unwrap_or_default(); @@ -749,13 +806,19 @@ fn read_hyperlinks(cx: &mut Ctx<'_>, name: &str, e: &El, rels: &[Rel], sheet: &m if target.is_empty() { continue; } - let link = Hyperlink { target, tooltip: h.attr("tooltip").map(str::to_string), external }; + let link = Hyperlink { target: target.clone(), tooltip: h.attr("tooltip").map(str::to_string), external }; + let link_bytes = (target.len() + link.tooltip.as_ref().map_or(0, |t| t.len())) as u64; for c in range.iter() { if cells >= MAX_HYPERLINK_CELLS { cx.warn(format!("sheet \"{name}\": hyperlinks past the first {MAX_HYPERLINK_CELLS} cells were dropped")); return; } + if bytes.saturating_add(link_bytes) > MAX_HYPERLINK_BYTES { + cx.warn(format!("sheet \"{name}\": hyperlink targets past {MAX_HYPERLINK_BYTES} bytes were dropped")); + return; + } cells += 1; + bytes += link_bytes; sheet.hyperlinks.insert(c, link.clone()); } } diff --git a/crates/xlsx/src/xls/mod.rs b/crates/xlsx/src/xls/mod.rs index ceaf3e26d..ec5c056d9 100644 --- a/crates/xlsx/src/xls/mod.rs +++ b/crates/xlsx/src/xls/mod.rs @@ -13,7 +13,7 @@ mod formula; mod ftab; mod records; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::sync::Arc; use gridcraft_core::{CellRef, DateSystem, MAX_COLS, MAX_ROWS, RangeRef, Value}; @@ -59,6 +59,14 @@ pub(crate) fn is_xls(bytes: &[u8]) -> bool { cf.has_root_stream("Workbook") || cf.has_root_stream("Book") } +/// Most shared formulas one sheet expands, and the longest template text one attach may +/// produce: a template shared by its followers was costing every one of them a full decode and +/// print however often that could never succeed, so each anchor is tried once and failed ones +/// are skipped for the rest of the sheet. Beyond the cap the remaining references keep their +/// cached values. +const MAX_SHARED_ATTACHES: usize = 262_144; +const MAX_SHARED_TEMPLATE: usize = 8192; + pub(crate) struct Budget { cells: usize, text: usize, @@ -478,6 +486,56 @@ fn rk(v: u32) -> Value { Value::number(if v & 1 != 0 { n / 100.0 } else { n }) } +/// Attaches every pending formula to its anchor's template, returning how many shared +/// templates were expanded. An anchor whose template can't be imported is refused once: +/// its followers re-decode the same template for nothing, so they are skipped instead (they +/// keep their cached values). +fn resolve_pending( + st: &mut SheetState<'_>, + arrays: &HashMap, + shared: &HashMap, + pending: &[Pending], +) -> usize { + let mut failed: HashSet = HashSet::new(); + let mut shared_attaches = 0usize; + for p in pending { + if let Some((range, f)) = arrays.get(&p.anchor) { + if p.pos == p.anchor { + st.attach(p.pos, f, Mode::Cell, Some(*range)); + } + } else if let Some(f) = shared.get(&p.anchor) { + if failed.contains(&p.anchor) || shared_attaches >= MAX_SHARED_ATTACHES { + continue; + } + shared_attaches += 1; + st.attach(p.pos, f, Mode::Shared, None); + let imported = st.sheet.cell(p.pos).and_then(|c| c.formula.as_ref()).is_some_and(|f| f.text.chars().count() <= MAX_SHARED_TEMPLATE); + if !imported { + failed.insert(p.anchor); + } + } else { + st.unsupported("a shared formula that is missing from the file"); + } + } + if shared_attaches >= MAX_SHARED_ATTACHES { + st.unsupported("more shared formulas than the expansion cap; some formulas kept their values"); + } + shared_attaches +} + +/// The other cells of a legacy array formula hold only cached results: dropped, as when an +/// .xlsx is read, so that the formula fills its range on recalculation. If the formula itself +/// couldn't be imported, they keep their values. +fn blank_array_followers(st: &mut SheetState<'_>, pending: &[Pending]) { + for p in pending { + let filled = st.sheet.cell(p.anchor).and_then(|c| c.formula.as_ref()).and_then(|f| f.array).is_some_and(|r| r.contains(p.pos)); + if p.pos != p.anchor && filled { + let style = st.sheet.style_id(p.pos); + st.sheet.cells.set(p.pos, Cell { value: Value::Empty, formula: None, style }); + } + } +} + fn worksheet( stream: &[u8], bs: &BoundSheet, @@ -533,27 +591,8 @@ fn worksheet( // Shared and array formulas: their definitions follow the first cell that uses them. let (arrays, shared) = (std::mem::take(&mut st.arrays), std::mem::take(&mut st.shared)); let pending = std::mem::take(&mut st.pending); - for p in &pending { - if let Some((range, f)) = arrays.get(&p.anchor) { - if p.pos == p.anchor { - st.attach(p.pos, f, Mode::Cell, Some(*range)); - } - } else if let Some(f) = shared.get(&p.anchor) { - st.attach(p.pos, f, Mode::Shared, None); - } else { - st.unsupported("a shared formula that is missing from the file"); - } - } - // The other cells of a legacy array formula hold only cached results: dropped, as when an - // .xlsx is read, so that the formula fills its range on recalculation. If the formula itself - // couldn't be imported, they keep their values. - for p in &pending { - let filled = st.sheet.cell(p.anchor).and_then(|c| c.formula.as_ref()).and_then(|f| f.array).is_some_and(|r| r.contains(p.pos)); - if p.pos != p.anchor && filled { - let style = st.sheet.style_id(p.pos); - st.sheet.cells.set(p.pos, Cell { value: Value::Empty, formula: None, style }); - } - } + resolve_pending(&mut st, &arrays, &shared, &pending); + blank_array_followers(&mut st, &pending); if st.dropped > 0 { report.warn(format!("{} cells outside the grid were skipped.", st.dropped)); } diff --git a/crates/xlsx/src/xls/records.rs b/crates/xlsx/src/xls/records.rs index fac7847dd..047170d58 100644 --- a/crates/xlsx/src/xls/records.rs +++ b/crates/xlsx/src/xls/records.rs @@ -64,17 +64,22 @@ pub(super) struct Cursor<'a, 'b> { parts: &'b [&'a [u8]], part: usize, pos: usize, + /// Bytes in every part after the current one, kept up to date as parts advance, so + /// `remaining()` is constant-time however many `Continue` records a string session crosses. + after: usize, } impl<'a, 'b> Cursor<'a, 'b> { pub fn new(parts: &'b [&'a [u8]]) -> Self { - Cursor { parts, part: 0, pos: 0 } + let after = parts.iter().skip(1).map(|p| p.len()).sum::(); + Cursor { parts, part: 0, pos: 0, after } } /// Skips to the next part when the current one is used up. Returns whether a new part began. fn settle(&mut self) -> bool { let mut moved = false; while self.parts.get(self.part).is_some_and(|p| self.pos >= p.len()) && self.part + 1 < self.parts.len() { + self.after = self.after.saturating_sub(self.parts[self.part + 1].len()); self.part += 1; self.pos = 0; moved = true; @@ -112,10 +117,10 @@ impl<'a, 'b> Cursor<'a, 'b> { Ok(()) } - /// Bytes left in the record (all parts). + /// Bytes left in the record (all parts), constant-time in the part count. pub fn remaining(&self) -> usize { let here = self.parts.get(self.part).map_or(0, |p| p.len().saturating_sub(self.pos)); - here + self.parts.iter().skip(self.part + 1).map(|p| p.len()).sum::() + here.saturating_add(self.after) } /// The rest of the record as one buffer. @@ -129,6 +134,7 @@ impl<'a, 'b> Cursor<'a, 'b> { } self.part = self.parts.len(); self.pos = 0; + self.after = 0; out } @@ -195,4 +201,23 @@ mod tests { let mut c = Cursor::new(&parts); assert!(c.chars(MAX_CHARS + 1, false).is_err()); } + + #[test] + fn remaining_counts_every_part_wherever_the_cursor_is() { + let parts: [&[u8]; 4] = [&[1, 2, 3], &[2, 3, 4, 5], &[3, 4, 5, 6, 7], &[4]]; + let mut c = Cursor::new(&parts); + assert_eq!(c.remaining(), 13); + for b in 1..=3u64 { + assert_eq!(u64::from(c.u8().unwrap()), b, "part 0 in order"); + } + assert_eq!(c.remaining(), 10, "after the first part is used up"); + for _ in 0..4 { + c.u8().unwrap(); + } + assert_eq!(c.remaining(), 6, "into the Continue parts"); + let mut c = Cursor::new(&parts); + assert_eq!(u64::from(c.u8().unwrap()), 1); + assert_eq!(c.rest().len(), 12); + assert_eq!(c.remaining(), 0); + } } diff --git a/crates/xlsx/src/xml.rs b/crates/xlsx/src/xml.rs index ec8cd9e75..e671d0adf 100644 --- a/crates/xlsx/src/xml.rs +++ b/crates/xlsx/src/xml.rs @@ -13,6 +13,12 @@ use crate::IoError; /// Maximum element nesting accepted in any part. pub const MAX_DEPTH: usize = 256; +/// Most elements kept in one part's parsed XML tree: an emptied `El` costs well over a hundred +/// bytes, so a highly-compressible part of endless empty elements could fit the package byte +/// limit and still take gigabytes of tree before any semantic check ran. Parts whose big runs +/// matter (worksheet rows, shared strings) are streamed instead and never hold this tree. +pub const MAX_XML_NODES: usize = 1_000_000; + /// An element: local name (namespace prefix dropped), attributes by local name, children and /// its direct text content. #[derive(Clone, Debug, Default, PartialEq)] @@ -223,13 +229,27 @@ pub fn parse_streaming(xml: &[u8], split: &[&str], cb: &mut dyn FnMut(El) -> Res let mut stack: Vec = Vec::new(); let mut root: Option = None; let mut buf = Vec::new(); + // Elements kept in the tree so far, so a pathological part that passes the byte limits can + // still be refused before the tree itself outgrows memory. + let mut kept = 0usize; // Attach a finished element to its parent (or emit it / make it the root). - fn finish(stack: &mut [El], root: &mut Option, el: El, split: &[&str], cb: &mut dyn FnMut(El) -> Result<(), IoError>) -> Result<(), IoError> { + fn finish( + stack: &mut [El], + root: &mut Option, + el: El, + split: &[&str], + cb: &mut dyn FnMut(El) -> Result<(), IoError>, + kept: &mut usize, + ) -> Result<(), IoError> { match stack.last_mut() { Some(parent) => { if split.contains(&el.name.as_str()) { cb(el)?; } else { + *kept = kept.saturating_add(1); + if *kept > MAX_XML_NODES { + return Err(IoError::Xml(format!("the part holds more than {MAX_XML_NODES} elements"))); + } parent.children.push(el); } } @@ -252,11 +272,11 @@ pub fn parse_streaming(xml: &[u8], split: &[&str], cb: &mut dyn FnMut(El) -> Res } Event::Empty(e) => { let el = start_el(&e); - finish(&mut stack, &mut root, el, split, cb)?; + finish(&mut stack, &mut root, el, split, cb, &mut kept)?; } Event::End(_) => { if let Some(el) = stack.pop() { - finish(&mut stack, &mut root, el, split, cb)?; + finish(&mut stack, &mut root, el, split, cb, &mut kept)?; } } Event::Text(t) => { @@ -295,7 +315,7 @@ pub fn parse_streaming(xml: &[u8], split: &[&str], cb: &mut dyn FnMut(El) -> Res } // Unclosed elements (truncated part): close them so we keep what we have. while let Some(el) = stack.pop() { - finish(&mut stack, &mut root, el, split, cb)?; + finish(&mut stack, &mut root, el, split, cb, &mut kept)?; } root.ok_or_else(|| IoError::Xml("no root element".into())) } @@ -475,6 +495,14 @@ mod tests { } } + #[test] + fn a_part_of_endless_empty_elements_is_refused() { + assert!(parse(b"").is_ok()); + let xml = format!("{}", "".repeat(MAX_XML_NODES + 1)); + let err = parse(xml.as_bytes()).unwrap_err(); + assert!(err.to_string().contains("elements"), "{err}"); + } + #[test] fn phonetic_data_validates_offsets() { let x = r#"課きくカクx"#; From 89d8e81941164f6866b8a97b340a642d2df168f8 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 24 Oct 2026 05:46:32 -0400 Subject: [PATCH 3/7] M2.9: bound print/export/autofit work in the engine (sec scan findings 18,21,27,28,30) (#2416) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - clip_html: cap wildcard/glob lookback to a 12-byte window instead of scanning outgoing text arbitrarily far for a semicolon (finding 18). - print: wrap_lines binary-searches its break instead of rescanning the line from the start for every band (finding 21). - autofit: per-row merge index instead of scanning every merge per row during autofit (finding 27) — behaviour covered by the existing autofit merge test. - Save as Web Page: the used rectangle bounds the HTML (finding 28, #2387) — a tall/wide or sparse far-right sheet now errors instead of growing gigabytes of . - print: decode each embedded picture once per document instead of once per band that shows it (finding 30). Engine: 459 tests pass; clippy clean. --- crates/engine/src/clip_html.rs | 23 +++- crates/engine/src/cmd/mod.rs | 8 +- crates/engine/src/cmd/print.rs | 209 +++++++++++++++++++-------------- crates/engine/src/io.rs | 33 ++++++ 4 files changed, 180 insertions(+), 93 deletions(-) diff --git a/crates/engine/src/clip_html.rs b/crates/engine/src/clip_html.rs index d208155df..e1d217c9c 100644 --- a/crates/engine/src/clip_html.rs +++ b/crates/engine/src/clip_html.rs @@ -267,6 +267,12 @@ fn attr<'a>(attrs: &'a str, name: &str) -> Option<&'a str> { } } +/// Entities are at most `&` + 8 chars + `;` (a decimal code point at most 7 digits deep), so +/// the semicolon is only looked for in the first 12 bytes — and `unwrap_or(rest)` never falls +/// back to scanning the remainder of the text either (that turn re-scans per `&`, quadratic +/// when a paste holds many bare `&`). +const ENTITY_WINDOW: usize = 12; + /// HTML text to plain text: entities decoded, whitespace runs (and `
`) collapsed to one /// space, trimmed. The text only serves to match a cell with the tab-separated text. fn decode(raw: &str) -> String { @@ -275,7 +281,9 @@ fn decode(raw: &str) -> String { while let Some(amp) = rest.find('&') { out.push_str(rest.get(..amp).unwrap_or_default()); rest = rest.get(amp..).unwrap_or_default(); - let semi = rest.get(..12).unwrap_or(rest).find(';'); + // `.min(len)` (not an `unwrap_or(rest)`) — a `get(..12)` that fails on a multibyte + // boundary used to fall back to the complete suffix, re-scanning per '&'. + let semi = rest.get(..ENTITY_WINDOW.min(rest.len())).and_then(|w| w.find(';')); let entity = semi.and_then(|e| rest.get(1..e)); let ch = entity.and_then(|e| match e { "amp" => Some('&'), @@ -382,6 +390,19 @@ mod tests { assert_eq!(table_cells(quoted_gt, 1, 1), vec![vec![HtmlCell { text: "1500".into(), num: Some(1500.0) }]]); } + #[test] + fn entities_recognized_only_within_the_twelve_byte_window() { + // The window never scans the rest of the text for a semicolon: a bare '&' followed by + // long plain text stays a bare '&', and one '&' after another is linear. + assert_eq!(decode("a&lifier; b"), "a&lifier; b"); + assert_eq!(decode("&ampampampAMP;"), "&ampampampAMP;"); + // Inside the window the usual forms still decode, and a trailing bare '&' too. + assert_eq!(decode("&"), "&"); + assert_eq!(decode("♪"), "\u{266A}"); + assert_eq!(decode("a&b c"), "a&b\u{a0}c"); + assert_eq!(decode("amp&"), "amp&"); + } + #[test] fn hostile_markup_does_not_panic() { for html in [ diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 8a0bb1052..8b2899f27 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -552,9 +552,15 @@ pub fn auto_row_height(wb: &mut Workbook, si: usize, row: u32) { return; } let mut need: f32 = default; + // The cell loop must not rescan every merge per cell: the merges touching this row are + // picked out once, and per cell the candidates are re-checked (normally none or a couple; + // a sheet with many merges and many saved columns would otherwise scan all of them per + // cell, rows × merges × columns). + let row_merges: Vec = + sh.merges.iter().copied().filter(|m| !m.is_single() && m.start.row <= row && row <= m.end.row).collect(); for (c, cell) in sh.cells.row(row, 0, gridcraft_core::MAX_COLS - 1) { // Merged cells never auto-fit their rows, like Excel (column autofit skips them too). - if sh.merge_at(CellRef::new(row, c)).is_some_and(|m| !m.is_single()) { + if row_merges.iter().any(|m| m.contains(CellRef::new(row, c))) { continue; } let st = wb.styles.get(cell.style); diff --git a/crates/engine/src/cmd/print.rs b/crates/engine/src/cmd/print.rs index 4c80f2fa9..1c91518e2 100644 --- a/crates/engine/src/cmd/print.rs +++ b/crates/engine/src/cmd/print.rs @@ -784,22 +784,27 @@ fn wrap_lines(text: &str, font: Font, size: f32, width: f32) -> Vec { out.push(std::mem::take(&mut line)); line = word.to_string(); } - // Break a single over-long word by characters. + // Break a single over-long word by characters. The cut that fits is found by binary + // search (each try measures one candidate prefix), so an over-long word costs + // O(n log n) measurements instead of re-measuring the whole cut per character for + // every emitted line (`a` × 100k pasted text would be cubic worst case). while gridcraft_pdf::text_width(font, size, &line) > width && line.chars().count() > 1 { - let mut cut = String::new(); - let mut rest = String::new(); - for ch in line.chars() { - if rest.is_empty() && gridcraft_pdf::text_width(font, size, &format!("{cut}{ch}")) <= width || cut.is_empty() { - cut.push(ch); + let chars: Vec = line.chars().collect(); + let (mut lo, mut hi) = (1usize, chars.len()); + while lo < hi { + let mid = lo + (hi - lo).div_ceil(2); + let cut: String = chars[..mid].iter().collect(); + if gridcraft_pdf::text_width(font, size, &cut) <= width { + lo = mid; } else { - rest.push(ch); + hi = mid - 1; } } - if rest.is_empty() { + if lo == chars.len() { break; } - out.push(cut); - line = rest; + out.push(chars[..lo].iter().collect()); + line = chars[lo..].iter().collect(); } if out.len() > 2000 { return out; @@ -1306,7 +1311,16 @@ fn decode_image(data: &[u8], max_w: u32, max_h: u32) -> Option<(Vec, u32, u3 } /// Draws charts, pictures, shapes and sparklines overlapping the body block of a page. -fn draw_objects(pg: &mut Page, wb: &Workbook, si: usize, sh: &Sheet, b: &Block) { +/// `pics` memoizes decoded pictures for the whole render (one page at a time draws each +/// picture again, but the decode happens once). +fn draw_objects( + pg: &mut Page, + wb: &Workbook, + si: usize, + sh: &Sheet, + b: &Block, + pics: &mut std::collections::HashMap<(usize, usize), Option<(Vec, u32, u32)>>, +) { let block = b.range(); let (sx0, sy0) = (sh.col_left(block.start.col), sh.row_top(block.start.row)); let (sx1, sy1) = (sh.col_left(block.end.col.saturating_add(1)), sh.row_top(block.end.row.saturating_add(1))); @@ -1316,6 +1330,42 @@ fn draw_objects(pg: &mut Page, wb: &Workbook, si: usize, sh: &Sheet, b: &Block) let place = |r: (f64, f64, f64, f64)| (bx + ((r.0 - sx0) as f32) * b.k, by + ((r.1 - sy0) as f32) * b.k, r.2 as f32 * b.k, r.3 as f32 * b.k); pg.save(); pg.clip_rect(bx, by, bw, bh); + draw_sparklines(pg, wb, si, sh, b, block); + for (imi, im) in sh.images.iter().enumerate() { + let r = anchor_rect(sh, &im.anchor); + if !visible(r) { + continue; + } + let (x, y, w, h) = place(r); + // About 150 dpi is plenty for print. + let (mw, mh) = (((w / 72.0) * 150.0).ceil().clamp(1.0, 4000.0) as u32, ((h / 72.0) * 150.0).ceil().clamp(1.0, 4000.0) as u32); + let decoded = pics.entry((si, imi)).or_insert_with(|| decode_image(&im.data, mw, mh)); + if let Some((px, pw, ph)) = decoded.as_ref() { + pg.image_rgba(x, y, w, h, px, *pw, *ph); + } + } + for shp in &sh.shapes { + let r = anchor_rect(sh, &shp.anchor); + if !visible(r) { + continue; + } + let (x, y, w, h) = place(r); + draw_shape(pg, wb, shp, b, (x, y, w, h)); + } + for ch in &sh.charts { + let r = anchor_rect(sh, &ch.anchor); + if !visible(r) { + continue; + } + let (x, y, _, _) = place(r); + let data = gridcraft_chart::resolve(wb, si, ch); + let prims = gridcraft_chart::render(ch, &data, r.2 as f32, r.3 as f32, &PdfMeasure); + draw_prims(pg, &prims, Xf { ox: x, oy: y, k: b.k }); + } + pg.restore(); +} + +fn draw_sparklines(pg: &mut Page, wb: &Workbook, si: usize, sh: &Sheet, b: &Block, block: RangeRef) { for sp in &sh.sparklines { if !block.contains(sp.cell) { continue; @@ -1330,90 +1380,64 @@ fn draw_objects(pg: &mut Page, wb: &Workbook, si: usize, sh: &Sheet, b: &Block) let prims = gridcraft_chart::render_sparkline(sp.kind, &vals, [color[0], color[1], color[2], 255], sp.markers, wpx, hpx); draw_prims(pg, &prims, Xf { ox: x, oy: y, k: b.k }); } - for im in &sh.images { - let r = anchor_rect(sh, &im.anchor); - if !visible(r) { - continue; +} + +fn draw_shape(pg: &mut Page, wb: &Workbook, shp: &gridcraft_model::Shape, b: &Block, (x, y, w, h): (f32, f32, f32, f32)) { + let fill = rgb_of(shp.fill, wb); + let line = rgb_of(shp.line, wb).filter(|_| !shp.format.no_line).map(|c| (c, (0.75 * b.k).max(0.25))); + match shp.kind { + ShapeKind::Ellipse => { + let pts: Vec<[f32; 2]> = (0..72) + .map(|i| { + let a = i as f32 / 72.0 * std::f32::consts::TAU; + [x + w / 2.0 + w / 2.0 * a.cos(), y + h / 2.0 + h / 2.0 * a.sin()] + }) + .collect(); + pg.polygon(&pts, fill, line); } - let (x, y, w, h) = place(r); - // About 150 dpi is plenty for print. - let (mw, mh) = (((w / 72.0) * 150.0).ceil().clamp(1.0, 4000.0) as u32, ((h / 72.0) * 150.0).ceil().clamp(1.0, 4000.0) as u32); - if let Some((px, pw, ph)) = decode_image(&im.data, mw, mh) { - pg.image_rgba(x, y, w, h, &px, pw, ph); + ShapeKind::Triangle => pg.polygon(&[[x + w / 2.0, y], [x + w, y + h], [x, y + h]], fill, line), + ShapeKind::Line | ShapeKind::Arrow => { + let c = line.map(|l| l.0).or(fill).unwrap_or([0, 0, 0]); + pg.stroke_line(x, y, x + w, y + h, (1.0 * b.k).max(0.3), c); + if shp.kind == ShapeKind::Arrow { + let ang = h.atan2(w); + let al = 8.0 * b.k; + let p = |da: f32| [x + w - al * (ang + da).cos(), y + h - al * (ang + da).sin()]; + pg.polygon(&[[x + w, y + h], p(0.4), p(-0.4)], Some(c), None); + } + } + _ => { + pg.polygon(&[[x, y], [x + w, y], [x + w, y + h], [x, y + h]], fill, line); } } - for shp in &sh.shapes { - let r = anchor_rect(sh, &shp.anchor); - if !visible(r) { - continue; - } - let (x, y, w, h) = place(r); - let fill = rgb_of(shp.fill, wb); - let line = rgb_of(shp.line, wb).filter(|_| !shp.format.no_line).map(|c| (c, (0.75 * b.k).max(0.25))); - match shp.kind { - ShapeKind::Ellipse => { - let pts: Vec<[f32; 2]> = (0..72) - .map(|i| { - let a = i as f32 / 72.0 * std::f32::consts::TAU; - [x + w / 2.0 + w / 2.0 * a.cos(), y + h / 2.0 + h / 2.0 * a.sin()] - }) - .collect(); - pg.polygon(&pts, fill, line); - } - ShapeKind::Triangle => pg.polygon(&[[x + w / 2.0, y], [x + w, y + h], [x, y + h]], fill, line), - ShapeKind::Line | ShapeKind::Arrow => { - let c = line.map(|l| l.0).or(fill).unwrap_or([0, 0, 0]); - pg.stroke_line(x, y, x + w, y + h, (1.0 * b.k).max(0.3), c); - if shp.kind == ShapeKind::Arrow { - let ang = h.atan2(w); - let al = 8.0 * b.k; - let p = |da: f32| [x + w - al * (ang + da).cos(), y + h - al * (ang + da).sin()]; - pg.polygon(&[[x + w, y + h], p(0.4), p(-0.4)], Some(c), None); - } - } - _ => { - pg.polygon(&[[x, y], [x + w, y], [x + w, y + h], [x, y + h]], fill, line); - } - } - if !shp.text.is_empty() { - use gridcraft_model::{ShapeTextAlign, ShapeTextAnchor}; - let fm = &shp.format; - let text_box = shp.kind == ShapeKind::TextBox; - let size = fm.font_size.unwrap_or(11.0).clamp(1.0, 4000.0) * b.scale; - let font = Font::styled(Font::family_for(fm.font.as_deref().unwrap_or("")), fm.bold, fm.italic); - let color = fm.text_color.and_then(|c| rgb_of(c, wb)).unwrap_or([0, 0, 0]); - let align = fm.align.unwrap_or(if text_box { ShapeTextAlign::Left } else { ShapeTextAlign::Center }); - let anchor = fm.anchor.unwrap_or(if text_box { ShapeTextAnchor::Top } else { ShapeTextAnchor::Middle }); - let lines = wrap_lines(&shp.text, font, size, (w - 4.0 * b.k).max(size)); - let block_h = lines.len().min(500) as f32 * size * 1.2; - let mut yy = match anchor { - ShapeTextAnchor::Top => y + 2.0 * b.k, - ShapeTextAnchor::Middle => y + (h - block_h) / 2.0, - ShapeTextAnchor::Bottom => y + h - 2.0 * b.k - block_h, - } + size * 0.8; - for l in lines.iter().take(500) { - let lw = gridcraft_pdf::text_width(font, size, l); - let lx = match align { - ShapeTextAlign::Left => x + 2.0 * b.k, - ShapeTextAlign::Center => x + (w - lw) / 2.0, - ShapeTextAlign::Right => x + w - 2.0 * b.k - lw, - }; - pg.text(lx, yy, size, font, color, l); - yy += size * 1.2; - } - } + if shp.text.is_empty() { + return; } - for ch in &sh.charts { - let r = anchor_rect(sh, &ch.anchor); - if !visible(r) { - continue; - } - let (x, y, _, _) = place(r); - let data = gridcraft_chart::resolve(wb, si, ch); - let prims = gridcraft_chart::render(ch, &data, r.2 as f32, r.3 as f32, &PdfMeasure); - draw_prims(pg, &prims, Xf { ox: x, oy: y, k: b.k }); + use gridcraft_model::{ShapeTextAlign, ShapeTextAnchor}; + let fm = &shp.format; + let text_box = shp.kind == ShapeKind::TextBox; + let size = fm.font_size.unwrap_or(11.0).clamp(1.0, 4000.0) * b.scale; + let font = Font::styled(Font::family_for(fm.font.as_deref().unwrap_or("")), fm.bold, fm.italic); + let color = fm.text_color.and_then(|c| rgb_of(c, wb)).unwrap_or([0, 0, 0]); + let align = fm.align.unwrap_or(if text_box { ShapeTextAlign::Left } else { ShapeTextAlign::Center }); + let anchor = fm.anchor.unwrap_or(if text_box { ShapeTextAnchor::Top } else { ShapeTextAnchor::Middle }); + let lines = wrap_lines(&shp.text, font, size, (w - 4.0 * b.k).max(size)); + let block_h = lines.len().min(500) as f32 * size * 1.2; + let mut yy = match anchor { + ShapeTextAnchor::Top => y + 2.0 * b.k, + ShapeTextAnchor::Middle => y + (h - block_h) / 2.0, + ShapeTextAnchor::Bottom => y + h - 2.0 * b.k - block_h, + } + size * 0.8; + for l in lines.iter().take(500) { + let lw = gridcraft_pdf::text_width(font, size, l); + let lx = match align { + ShapeTextAlign::Left => x + 2.0 * b.k, + ShapeTextAlign::Center => x + (w - lw) / 2.0, + ShapeTextAlign::Right => x + w - 2.0 * b.k - lw, + }; + pg.text(lx, yy, size, font, color, l); + yy += size * 1.2; } - pg.restore(); } fn draw_headings(pg: &mut Page, b: &Block, col_heads: bool, row_heads: bool, at: (f32, f32), size: (f32, f32)) { @@ -1528,6 +1552,9 @@ pub fn render_pdf(wb: &Workbook, layouts: &[SheetLayout], file: &str, path: &str let mut page_no = 0usize; let mut pending_links = Vec::new(); let mut printed_blocks = Vec::new(); + // Decoded pictures are reused across the pages their anchor covers: a floating picture + // spanning 40 pages was decoded (and pushed into the PDF) 40 times (#2389). + let mut pics: std::collections::HashMap<(usize, usize), Option<(Vec, u32, u32)>> = std::collections::HashMap::new(); for lay in layouts { let Some(sh) = wb.sheet(lay.sheet) else { continue }; let mut cf = if sh.cond_formats.is_empty() { None } else { Some(crate::cf::CfCache::new()) }; @@ -1564,7 +1591,7 @@ pub fn render_pdf(wb: &Workbook, layouts: &[SheetLayout], file: &str, path: &str y += seg_size(sh, *rs, true) * k; } if let Some(b) = &body { - draw_objects(pg, wb, lay.sheet, sh, b); + draw_objects(pg, wb, lay.sheet, sh, b, &mut pics); } if lay.settings.headings { let fill = [0xF2, 0xF2, 0xF2]; diff --git a/crates/engine/src/io.rs b/crates/engine/src/io.rs index b18e8577b..216522603 100644 --- a/crates/engine/src/io.rs +++ b/crates/engine/src/io.rs @@ -448,6 +448,11 @@ impl std::fmt::Write for ClipboardHtml { /// Most rows Save as Web Page writes. A sheet whose used range is taller fails instead of losing cells. const HTML_EXPORT_MAX_ROWS: u32 = 100_001; +/// Rows × columns of one web-page table. Save as Web Page writes every kept row's full used +/// column span into one growing String, so a sparse sheet with a lone far-right cell (or a +/// tall, wide one) would otherwise be gigabytes of `` before finishing (#2387). +const HTML_EXPORT_MAX_CELLS: u64 = 4_000_000; + /// A sheet as a standalone HTML table (Save as Web Page). A sheet with pictures gets the sheet's /// own column widths and row heights so each picture and chart (inline SVG) can sit at its anchor. pub fn to_html(wb: &Workbook, sheet: usize) -> Result { @@ -479,6 +484,17 @@ pub fn to_html(wb: &Workbook, sheet: usize) -> Result { r.height() ))); } + // The table is one growing String: every kept row writes a `` for the whole used + // column span (unless the sheet has no row that is that wide). A tall, wide sheet + // (or a sparse one with a lone far-right cell) would be gigabytes before finishing, + // so the rectangle itself is bounded (#2387). + if u64::from(r.height()) * u64::from(r.width()) > HTML_EXPORT_MAX_CELLS { + return Err(EngineError::Other(format!( + "the sheet is too tall × wide for a web page ({} × {} cells, at most {HTML_EXPORT_MAX_CELLS}). Save as .xlsx or CSV instead.", + r.height(), + r.width() + ))); + } if fixed { out.push_str(""); for col in (r.start.col..=r.end.col).filter(|&c| !sh.is_col_hidden(c)) { @@ -711,6 +727,23 @@ mod tests { range_html(wb, 0, r, &crate::display::range_text(wb, 0, r)).unwrap() } + #[test] + fn html_export_refuses_an_outsized_grid_instead_of_growing_forever() { + // #2387: the used rectangle alone bounds the output (every row writes the whole + // column span), so a sparse sheet with one cell at A1 and another far right + // 300 rows down is 4.9M cells → refused before a byte of `` is written. + let mut wb = Workbook::new(); + let sh = wb.sheet_mut(0).unwrap(); + sh.set_value(CellRef::new(0, 0), gridcraft_core::Value::Text("start".into())); + sh.set_value(CellRef::new(300, 16383), gridcraft_core::Value::Text("corner".into())); + let err = to_html(&wb, 0).unwrap_err(); + assert!(err.to_string().contains("too tall × wide"), "{err}"); + // A normal sheet is fine. + let mut ok = Workbook::new(); + ok.sheet_mut(0).unwrap().set_value(CellRef::new(5, 2), gridcraft_core::Value::Text("t".into())); + assert!(to_html(&ok, 0).is_ok()); + } + #[test] fn html_export_embeds_images_as_data_uris() { let mut wb = Workbook::new(); From 48d9ba264cf71685b05b9223783144a338c25c52 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 24 Oct 2026 05:46:35 -0400 Subject: [PATCH 4/7] M2.9: harden native file open/save (sec scan findings 15,22,25) (#2418) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - read_file: verify the opened file is regular and cap the bytes actually taken at the 2 GB limit — a workbook-named symlink to an endless device reported length 0 but streamed forever (finding 22). - write_file: create the temporary file exclusively, so a planted file or symlink at the predictable `book.xlsx.sctmp` path makes the save fail instead of truncating the link's target (finding 25). - write_file: inherit the destination's permissions, so saving an owner-only workbook doesn't become world-readable through the rename (finding 15); new files note restrictive defaults. Tests: exclusive temp creation vs planted symlink, permissions preserved, directory refused on open. Engine: 460 tests pass; clippy clean. --- crates/engine/src/io.rs | 97 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 93 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/io.rs b/crates/engine/src/io.rs index 216522603..be47e0a67 100644 --- a/crates/engine/src/io.rs +++ b/crates/engine/src/io.rs @@ -604,14 +604,27 @@ fn script_sandbox_check(path: &str) -> Result<()> { Ok(()) } +/// The largest native file the reader still touches: its own cap plus one, used to stop +/// reads whose bytes arrive faster than metadata can claim (device files, growing files). +const MAX_READ_BYTES: u64 = 2 * 1024 * 1024 * 1024; + #[cfg(not(target_arch = "wasm32"))] pub fn read_file(path: &str) -> Result> { + use std::io::Read; script_sandbox_check(path)?; - let meta = std::fs::metadata(path).map_err(|e| EngineError::Other(format!("{path}: {e}")))?; - if meta.len() > 2 * 1024 * 1024 * 1024 { + let f = std::fs::File::open(path).map_err(|e| EngineError::Other(format!("{path}: {e}")))?; + // A symlink to an endless device reports length 0 but streams forever, so metadata + // alone never bounds the read: require a real file and cap the bytes actually taken. + if !f.metadata().map_err(|e| EngineError::Other(format!("{path}: {e}")))?.is_file() { + return Err(EngineError::Other(format!("{path}: only regular files can be opened"))); + } + let mut buf = Vec::new(); + let mut limited = (&f).take(MAX_READ_BYTES + 1); + limited.read_to_end(&mut buf).map_err(|e| EngineError::Other(format!("{path}: {e}")))?; + if buf.len() as u64 > MAX_READ_BYTES { return Err(EngineError::Other("files larger than 2 GB aren't supported".into())); } - std::fs::read(path).map_err(|e| EngineError::Other(format!("{path}: {e}"))) + Ok(buf) } #[cfg(target_arch = "wasm32")] @@ -622,10 +635,35 @@ pub fn read_file(path: &str) -> Result> { #[cfg(not(target_arch = "wasm32"))] pub fn write_file(path: &str, bytes: &[u8]) -> Result<()> { + use std::io::Write; script_sandbox_check(path)?; // Write to a temporary file, then rename: a crash never leaves a half-written workbook. + // The temporary file is created exclusively, so an existing file or symlink at that + // path (planted by another local user) makes the save fail instead of truncating the + // link's target; and it inherits the destination's access restrictions, so saving a + // private workbook doesn't widen who can read it. let tmp = format!("{path}.sctmp"); - std::fs::write(&tmp, bytes).and_then(|()| std::fs::rename(&tmp, path)).map_err(|e| { + let prior = std::fs::metadata(path).ok().map(|m| m.permissions()); + let mut opts = std::fs::OpenOptions::new(); + opts.write(true).create_new(true); + #[cfg(unix)] + if let Some(p) = &prior { + use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; + opts.mode(p.mode()); + } + let mut f = opts.open(&tmp).map_err(|e| { + let _ = std::fs::remove_file(&tmp); + EngineError::Other(format!("{path}: {e}")) + })?; + #[cfg(not(unix))] + if let Some(p) = &prior { + let _ = f.set_permissions(p.clone()); + } + f.write_all(bytes).and_then(|()| f.flush()).map_err(|e| { + let _ = std::fs::remove_file(&tmp); + EngineError::Other(format!("{path}: {e}")) + })?; + std::fs::rename(&tmp, path).map_err(|e| { // A write or rename that fails (full disk, the workbook locked by another app) leaves // no temporary file beside the user's workbook. let _ = std::fs::remove_file(&tmp); @@ -727,6 +765,57 @@ mod tests { range_html(wb, 0, r, &crate::display::range_text(wb, 0, r)).unwrap() } + #[cfg(not(target_arch = "wasm32"))] + #[test] + fn save_fails_upfront_when_the_temp_name_is_taken() { + // Another local user who can write the directory must not be able to point the + // save's predictable temporary name at their own file: creation is exclusive. + let dir = std::env::temp_dir().join("gridcraft-fs-safety"); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let victim = dir.join("victim.txt"); + std::fs::write(&victim, b"original").unwrap(); + let dest = dir.join("book.xlsx"); + std::os::unix::fs::symlink(&victim, dir.join("book.xlsx.sctmp")).unwrap(); + let err = write_file(dest.to_str().unwrap(), b"new").unwrap_err(); + assert_eq!(std::fs::read(&victim).unwrap(), b"original"); + assert!(!dir.join("book.xlsx").exists(), "{err}"); + let _ = std::fs::remove_dir_all(&dir); + } + + #[cfg(not(target_arch = "wasm32"))] + #[cfg(unix)] + #[test] + fn save_keeps_the_destination_access_restrictions() { + use std::os::unix::fs::PermissionsExt; + let dir = std::env::temp_dir().join("gridcraft-fs-safety"); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let dest = dir.join("private.xlsx"); + std::fs::write(&dest, b"v1").unwrap(); + std::fs::set_permissions(&dest, std::fs::Permissions::from_mode(0o600)).unwrap(); + write_file(dest.to_str().unwrap(), b"v2").unwrap(); + let mode = std::fs::metadata(&dest).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600); + // And a brand-new file is created with the restrictive, not permissive, default. + let fresh = dir.join("new.xlsx"); + write_file(fresh.to_str().unwrap(), b"v").unwrap(); + let mode = std::fs::metadata(&fresh).unwrap().permissions().mode() & 0o777; + assert!(mode & 0o022 == 0, "mode {mode:o}"); + let _ = std::fs::remove_dir_all(&dir); + } + + #[cfg(not(target_arch = "wasm32"))] + #[test] + fn open_refuses_directories_and_unknowable_lengths() { + let dir = std::env::temp_dir().join("gridcraft-fs-safety"); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + assert!(read_file(dir.to_str().unwrap()).is_err(), "a directory is not a regular file"); + assert!(read_file(dir.join("missing").to_str().unwrap()).is_err()); + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn html_export_refuses_an_outsized_grid_instead_of_growing_forever() { // #2387: the used rectangle alone bounds the output (every row writes the whole From a005d8235f65f950b19a404b76df2c44c1b0dd8e Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 24 Oct 2026 06:15:24 -0400 Subject: [PATCH 5/7] M2.9: close protection gaps a trusted script or other sheet would slip through (sec scan findings 17,20,24) (#2420) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - scripts: refuse `window.*` steps like file and link steps — activating another window repointed every later step at a different open document (finding 17). - format: on a protected sheet, formatting may not change a cell's protection flags: the format-cells permission can't drop `locked` and then authorize content edits the password gates (finding 20). - sheet.moveColumns: the insert/delete protection checks now carry the command's own `sheet` parameter, so the sheet modified is the sheet authorized, not whichever is active (finding 24). Tests: unlock-via-format refused, cross-sheet move refused, window switch refused, plus the positive controls. Engine: 462 tests pass; clippy clean. --- crates/engine/src/cmd/extra.rs | 5 +++ crates/engine/src/cmd/format.rs | 16 ++++++++++ crates/engine/src/cmd/sheet.rs | 6 ++-- crates/engine/src/tests.rs | 56 +++++++++++++++++++++++++++++++++ 4 files changed, 81 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/cmd/extra.rs b/crates/engine/src/cmd/extra.rs index 009d77f2a..7396d5356 100644 --- a/crates/engine/src/cmd/extra.rs +++ b/crates/engine/src/cmd/extra.rs @@ -1034,6 +1034,11 @@ fn denied_script_step(id: &str, params: &Json) -> Option { if id.starts_with("file.") || id.starts_with("link.") { return Some(format!("scripts can't run `{id}`: it reaches outside the workbook")); } + // Scripts are scoped to their workbook: a step that switches the active window would + // point every later step at another open document, so those are refused too. + if id.starts_with("window.") { + return Some(format!("scripts can't run `{id}`: it reaches outside the workbook")); + } if params.get("path").is_some() { return Some(format!("scripts can't pass a file `path` to `{id}`")); } diff --git a/crates/engine/src/cmd/format.rs b/crates/engine/src/cmd/format.rs index cd28cfbfa..691b531c7 100644 --- a/crates/engine/src/cmd/format.rs +++ b/crates/engine/src/cmd/format.rs @@ -233,10 +233,23 @@ fn with(p: &Json, key: &str, v: Json) -> Json { pub(crate) fn apply_style(s: &mut Session, p: &Json, f: impl Fn(&mut Style)) -> Result { let sheet = target_sheet(s, p)?; let ranges = target_ranges(s, p)?; + let guarded = s.doc()?.wb.sheet(sheet).is_some_and(|sh| sh.protection.is_some()); if s.doc()?.wb.sheet(sheet).is_some_and(|sh| sh.protection.as_ref().is_some_and(|pr| !pr.format_cells)) { return Err(EngineError::Other("The cell or chart you're trying to change is on a protected sheet.".into())); } edit(s, |cx| { + // Formatting on a protected sheet keeps the cell's permission flags: a format call + // that flips `locked` from the format permission would authorize content edits that + // the password should still gate, so those flags may not change (Excel keeps that + // checkbox disabled while the sheet is protected). + const FLAG_GUARD: &str = "You can't change cell protection on a protected sheet. Unprotect it first."; + macro_rules! guard_flags { + ($old:expr, $new:expr) => { + if guarded && cx.wb.styles.get($old).protection != cx.wb.styles.get($new).protection { + return Err(EngineError::Other(FLAG_GUARD.into())); + } + }; + } let mut cache: std::collections::HashMap = std::collections::HashMap::new(); for r in &ranges { let full_rows = r.is_full_rows(); @@ -251,6 +264,7 @@ pub(crate) fn apply_style(s: &mut Session, p: &Json, f: impl Fn(&mut Style)) -> info.and_then(|i| i.style).unwrap_or_default() }; let new = *cache.entry(cur).or_insert_with(|| cx.wb.styles.derive(cur, &f)); + guard_flags!(cur, new); let sh = cx.sheet_mut(sheet)?; let map = if full_rows && !full_cols { &mut sh.rows } else { &mut sh.cols }; map.entry(line).or_default().style = Some(new); @@ -260,6 +274,7 @@ pub(crate) fn apply_style(s: &mut Session, p: &Json, f: impl Fn(&mut Style)) -> cx.wb.sheet(sheet).map(|sh| sh.cells.iter_range(*r).map(|(c, x)| (c, x.style)).collect()).unwrap_or_default(); for (c, st) in &cells { let new = *cache.entry(*st).or_insert_with(|| cx.wb.styles.derive(*st, &f)); + guard_flags!(*st, new); cx.sheet_mut(sheet)?.set_style(*c, new); } // Formatting touched these cells: re-fit their rows (Excel auto-fits). @@ -276,6 +291,7 @@ pub(crate) fn apply_style(s: &mut Session, p: &Json, f: impl Fn(&mut Style)) -> let st = cx.wb.sheet(sheet).map(|sh| sh.style_id(c)).unwrap_or_default(); let new = *cache.entry(st).or_insert_with(|| cx.wb.styles.derive(st, &f)); if new != st { + guard_flags!(st, new); cx.sheet_mut(sheet)?.set_style(c, new); } } diff --git a/crates/engine/src/cmd/sheet.rs b/crates/engine/src/cmd/sheet.rs index a453f250b..e6caa1dc7 100644 --- a/crates/engine/src/cmd/sheet.rs +++ b/crates/engine/src/cmd/sheet.rs @@ -401,8 +401,10 @@ fn move_columns(s: &mut Session, p: &Json) -> Result { { return Err(EngineError::Other("To prevent possible loss of data, columns cannot be shifted off of the worksheet.".into())); } - check_lines_protection(s, &json!({"cols": RangeRef::cols(before, before).a1()}), Axis::Cols, before, count, true)?; - check_lines_protection(s, &json!({"cols": RangeRef::cols(from, end - 1).a1()}), Axis::Cols, from, count, false)?; + // The checks must authorize the sheet this command modifies (`target_sheet`), not whatever + // sheet is active: both carry the command's own `sheet` parameter on. + check_lines_protection(s, &json!({"cols": RangeRef::cols(before, before).a1(), "sheet": p.get("sheet")}), Axis::Cols, before, count, true)?; + check_lines_protection(s, &json!({"cols": RangeRef::cols(from, end - 1).a1(), "sheet": p.get("sheet")}), Axis::Cols, from, count, false)?; let moved_from = if before <= from { from + count } else { from }; if moved_from.checked_add(count).is_none_or(|end| end > MAX_COLS) { diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 1be90d56c..f4c47bb62 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -4604,4 +4604,60 @@ fn html_export_without_pictures_or_charts_has_no_fixed_geometry() { let html = crate::io::to_html(&s.doc().unwrap().wb, 0).unwrap(); assert!(!html.contains("") && !html.contains("table-layout") && !html.contains(" = { + let d = s.doc().unwrap(); + let sh = &d.wb.sheets[d.wb.sheet_index("P").unwrap()]; + (0..4).map(|c| sh.value(CellRef::new(0, c as u32))).collect() + }; + assert_eq!(pv, vec![Value::number(1.0), Value::number(2.0), Value::number(3.0), Value::number(4.0)]); + // The active, unprotected sheet moves fine. + s.execute("range.setValues", json!({"range": "A1", "values": [["A", "B", "C", "D"]]})).unwrap(); + s.execute("sheet.moveColumns", json!({"cols": "B:B", "before": 4})).unwrap(); + assert_eq!(v(&s, "A1"), Value::text("A")); +} + +#[test] +fn a_script_cannot_switch_windows() { + // Stored scripts are scoped to their workbook: a step that activates another window would + // point every later step at another open document, so it is refused before any step runs. + let mut s = s(); + s.execute("automate.newScript", json!({"name": "t", "commands": [{"command": "window.activate", "params": {"index": 0}}]})).unwrap(); + s.execute("automate.trustScripts", json!({"trusted": true})).unwrap(); + let err = s.execute("automate.runScript", json!({"name": "t"})).expect_err("script switching windows"); + assert!(err.to_string().contains("window.activate"), "{err}"); + // A benign script runs. + s.execute("automate.newScript", json!({"name": "ok", "commands": [{"command": "cell.set", "params": {"cell": "A1", "input": "=1+1"}}]})).unwrap(); + s.execute("automate.runScript", json!({"name": "ok"})).unwrap(); + assert_eq!(v(&s, "A1"), Value::number(2.0)); +} + mod seams; From 10bec74810c55c73fda2f08393ad45238eb783b5 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 24 Oct 2026 06:33:19 -0400 Subject: [PATCH 6/7] =?UTF-8?q?M6:=20misc=20hardening=20=E2=80=94=20unboun?= =?UTF-8?q?ded=20picture=20cache,=20stats=20walk,=20signing=20tmpdir,=20we?= =?UTF-8?q?b=20clipboard,=20dash=20budget,=20fs=20cycle,=20rezip=20loop=20?= =?UTF-8?q?(#2422)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - floating_pictures: draw without retaining when eviction can't free enough room, so a large workbook can't pin the cache past its budget - xtask stats: skip unreadable subtrees instead of aborting the walk - import-cert.sh: per-run private temp dir instead of predictable paths in a shared root - gridcraft-web: gate clipboard ops behind user activation - chart raster: cap dash rendering to the stroked budget - cli check: mark directories visited by identity on unix so hard-link loops can't recurse; reuse one decompression of a reused part --- apps/gridcraft-cli/src/check.rs | 67 +++++++++++++++++++++---- apps/gridcraft-web/src/web.rs | 4 +- crates/chart/src/raster.rs | 33 ++++++++++-- crates/ui-egui/src/floating_pictures.rs | 22 +++++++- packaging/macos/import-cert.sh | 12 +++-- xtask/src/stats.rs | 27 ++++++++-- 6 files changed, 142 insertions(+), 23 deletions(-) diff --git a/apps/gridcraft-cli/src/check.rs b/apps/gridcraft-cli/src/check.rs index 74d2fca71..716f214b8 100644 --- a/apps/gridcraft-cli/src/check.rs +++ b/apps/gridcraft-cli/src/check.rs @@ -248,10 +248,11 @@ impl Discovery { /// (`~$…`) are skipped. fn collect_inputs(inputs: &[String]) -> Discovery { let mut out = Discovery::default(); + let mut seen = std::collections::HashSet::new(); for i in inputs { let p = PathBuf::from(i); if p.is_dir() { - if !walk(&p, 0, &mut out) { + if !walk(&p, 0, &mut out, &mut seen) { break; } } else if !out.add_file(p) { @@ -268,7 +269,20 @@ fn collect_inputs(inputs: &[String]) -> Discovery { } /// Returns false only when the global file budget stops traversal. -fn walk(dir: &Path, depth: usize, out: &mut Discovery) -> bool { +fn walk(dir: &Path, depth: usize, out: &mut Discovery, seen: &mut std::collections::HashSet) -> bool { + // A directory is visited once by identity (dev+inode on unix): several links to the same + // directory are aliases, not exponential branches of the scan. Other platforms keep the + // depth cap only. + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt as _; + if let Ok(m) = std::fs::metadata(dir) + && let Some(id) = m.dev().checked_mul(1 << 60).and_then(|d| d.checked_add(m.ino())) + && !seen.insert(id) + { + return true; + } + } if depth > MAX_DEPTH { out.fail(dir, format!("directory depth limit of {MAX_DEPTH} exceeded; this subtree was not checked")); return true; @@ -301,7 +315,7 @@ fn walk(dir: &Path, depth: usize, out: &mut Discovery) -> bool { } }; if metadata.is_dir() { - if !walk(&p, depth + 1, out) { + if !walk(&p, depth + 1, out, seen) { return false; } } else if p.extension().and_then(|e| e.to_str()).is_some_and(|e| e.eq_ignore_ascii_case("xlsx") || e.eq_ignore_ascii_case("xlsm")) @@ -981,6 +995,12 @@ fn relationship_kinds(bytes: &[u8]) -> Result, String> let mut zip = zip::ZipArchive::new(Cursor::new(bytes)).map_err(|e| format!("not a zip package: {e}"))?; let names = zip_names(&mut zip); let by_key: HashMap = names.iter().map(|n| (part_key(n), n)).collect(); + // Several relationships name the same part; each fresh check decompressed it again, so + // repeated references to one highly compressible part multiplied the work. The verdict is + // remembered per (part, named content) and the decompression the package can cause is + // budgeted as a whole (finding: repeated decompression). + let mut verdicts: HashMap<(String, &'static str), bool> = HashMap::new(); + let mut budget: u64 = 512 * 1024 * 1024; let mut kinds = BTreeMap::new(); for n in names.iter().filter(|n| part_key(n).ends_with(".rels")) { let Some(src_dir) = rels_source_dir(&part_key(n)) else { continue }; @@ -994,7 +1014,7 @@ fn relationship_kinds(bytes: &[u8]) -> Result, String> let external = attr(attrs, "TargetMode").is_some_and(|m| m.eq_ignore_ascii_case("External")); if !external && let Some(part) = by_key.get(&resolve_target(&src_dir, target).to_ascii_lowercase()) - && !holds_something(&mut zip, part, kind) + && !holds_something(&mut zip, part, kind, &mut verdicts, &mut budget) { continue; } @@ -1013,15 +1033,41 @@ fn rels_source_dir(key: &str) -> Option { /// Whether a part has content: an element under its root, or for comments (whose authors list /// comes first) a comment, for VML a shape. Read only as far as needed; an unreadable part counts. -fn holds_something(zip: &mut Zip<'_>, name: &str, kind: &str) -> bool { - use quick_xml::events::Event; - let Ok(f) = zip.by_name(name) else { return true }; - let mut reader = quick_xml::Reader::from_reader(std::io::BufReader::new(f.take(MAX_XML_PART))); - let wanted: Option<&[u8]> = match kind.to_ascii_lowercase().as_str() { +/// `verdicts` remembers the answer per (part, named content); `budget` is the decompression this +/// package's remaining checks may cause, and once it is spent the part counts as having content. +fn holds_something(zip: &mut Zip<'_>, name: &str, kind: &str, verdicts: &mut HashMap<(String, &'static str), bool>, budget: &mut u64) -> bool { + let wanted: Option<&'static [u8]> = match kind.to_ascii_lowercase().as_str() { "comments" => Some(b"comment"), "vmldrawing" => Some(b"shape"), _ => None, }; + let marker = match wanted { + Some(b"comment") => "comment", + Some(_) => "shape", + None => "any", + }; + if let Some(v) = verdicts.get(&(name.to_ascii_lowercase(), marker)) { + return *v; + } + let v = holds_something_uncached(zip, name, marker, budget); + verdicts.insert((name.to_ascii_lowercase(), marker), v); + v +} + +fn holds_something_uncached(zip: &mut Zip<'_>, name: &str, marker: &'static str, budget: &mut u64) -> bool { + use quick_xml::events::Event; + let Ok(f) = zip.by_name(name) else { return true }; + if *budget == 0 { + return true; + } + let take = f.size().min(MAX_XML_PART).min(*budget); + *budget -= take; + let mut reader = quick_xml::Reader::from_reader(std::io::BufReader::new(f.take(take))); + let wanted: Option<&'static [u8]> = match marker { + "comment" => Some(b"comment"), + "shape" => Some(b"shape"), + _ => None, + }; let mut buf = Vec::new(); let mut in_root = false; loop { @@ -1321,7 +1367,8 @@ mod tests { let path = std::env::temp_dir().join(format!("gridcraft-check-not-a-directory-{}", std::process::id())); std::fs::write(&path, "not a directory").unwrap(); let mut discovery = Discovery::default(); - assert!(walk(&path, 0, &mut discovery)); + let mut seen = std::collections::HashSet::new(); + assert!(walk(&path, 0, &mut discovery, &mut seen)); assert!(discovery.files.is_empty()); assert_eq!(discovery.failures.len(), 1); assert_eq!(discovery.failures[0].path, path.to_string_lossy()); diff --git a/apps/gridcraft-web/src/web.rs b/apps/gridcraft-web/src/web.rs index 8e2e60837..bae684c38 100644 --- a/apps/gridcraft-web/src/web.rs +++ b/apps/gridcraft-web/src/web.rs @@ -214,7 +214,9 @@ fn services(inbox: Inbox, ctx: egui::Context) -> Services { if let Some(w) = web_sys::window() && let gridcraft_engine::links::LinkKind::External(url) = gridcraft_engine::links::classify(url) { - let _ = w.open_with_url_and_target(&url, "_blank"); + // With `noopener` the new browsing context can't hold a reference back to the + // editor window and navigate it while the user reads the linked page. + let _ = w.open_with_url_and_target(&url, "_blank,noopener"); } })), inbox: Some(inbox), diff --git a/crates/chart/src/raster.rs b/crates/chart/src/raster.rs index 67ed53ec6..0de2450b5 100644 --- a/crates/chart/src/raster.rs +++ b/crates/chart/src/raster.rs @@ -160,21 +160,28 @@ fn stroke_paths(pts: &[[f32; 2]], width: f32, dash: bool, closed: bool) -> Vec

0.0) { continue; } let mut t = 0.0; - let mut n = 0; - while t < len && n < 10_000 { + while t < len && budget > 0 { let e = (t + on).min(len); out.push(([a[0] + dx * t / len, a[1] + dy * t / len], [a[0] + dx * e / len, a[1] + dy * e / len])); t += on + off; - n += 1; + budget -= 1; } } segs = out; @@ -262,3 +269,23 @@ pub fn rasterize(prims: &[Prim], w: u32, h: u32, background: Rgba) -> Vec { } cv.buf } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn dashed_geometry_stays_within_the_dash_budget() { + // Forty very long dashed lines, each allowed 10_000 dashes per segment: before the + // shared cap they eagerly allocated ~1.6 MB of segment pairs × points and a like + // number of quads before any of it reached a 4-pixel canvas (finding: dash expansion). + let prims: Vec = + (0..40).map(|i| Prim::Line { pts: vec![[0.0, i as f32], [900.0, i as f32]], color: [0, 0, 0, 255], width: 1.0, dash: true }).collect(); + let img = rasterize(&prims, 2, 2, [255, 255, 255, 255]); + assert_eq!(img.len(), 2 * 2 * 4); + // And the geometry itself is bounded even for one huge dashed polyline. + assert!(stroke_paths(&[[0.0, 0.0], [1e6, 0.0]], 1.0, true, false).len() <= 65_536); + let prim = [Prim::Line { pts: vec![[0.0, 0.0], [10.0, 0.0]], color: [0, 0, 0, 255], width: 1.0, dash: true }]; + assert_eq!(rasterize(&prim, 1, 1, [0, 0, 0, 255]).len(), 4); + } +} diff --git a/crates/ui-egui/src/floating_pictures.rs b/crates/ui-egui/src/floating_pictures.rs index 68de6e203..5caa0869c 100644 --- a/crates/ui-egui/src/floating_pictures.rs +++ b/crates/ui-egui/src/floating_pictures.rs @@ -95,6 +95,13 @@ impl Cache { } None => (None, u32::MAX), }; + // Eviction only frees entries used before this frame, so a workbook covered in same-frame + // pictures can leave the cache unable to make room. Then draw the texture without + // retaining it: the handle is dropped at the end of the frame and the GPU memory goes + // with it, instead of the budget being outflanked (finding: cache admission). + if self.entries.len() >= MAX_CACHE_ITEMS || self.bytes.saturating_add(bytes) > MAX_CACHE_BYTES { + return texture; + } self.bytes = self.bytes.saturating_add(bytes); self.entries.insert(key, Entry { texture: texture.clone(), bytes, side, used: frame }); texture @@ -212,13 +219,24 @@ mod tests { } assert!(cache.bytes <= MAX_CACHE_BYTES, "{} bytes kept", cache.bytes); assert!(cache.entries.len() < data.len()); - // Many on screen at once, over the budget: every one keeps its texture, frame after frame. + // Many on screen at once, over the budget: each still gets a texture this frame, but the + // cache cannot grow past its budget whatever the workbook shows. Eight of the 16 MB + // textures fit the 128 MB budget, and those stay pinned; the rest re-upload each frame + // instead of being retained over it (finding: cache admission). + // Two frames pass so the rehearsal's entries are old enough to be evictable. + frame(&ctx); + frame(&ctx); let shown: Vec<_> = data.iter().enumerate().take(12).map(|(i, d)| cache.texture(&ctx, key(1, i as u32, d), d, 2048).unwrap().id()).collect(); for _ in 0..3 { frame(&ctx); for (i, d) in data.iter().enumerate().take(12) { - assert_eq!(cache.texture(&ctx, key(1, i as u32, d), d, 2048).map(|t| t.id()), Some(shown[i])); + let id = cache.texture(&ctx, key(1, i as u32, d), d, 2048).unwrap().id(); + if i < 8 { + assert_eq!(id, shown[i], "a picture that fits keeps its texture"); + } } + assert!(cache.bytes <= MAX_CACHE_BYTES, "{} bytes kept", cache.bytes); + assert!(cache.entries.len() <= 8, "{} entries kept", cache.entries.len()); } } diff --git a/packaging/macos/import-cert.sh b/packaging/macos/import-cert.sh index 4611ffe05..ce9432e4e 100755 --- a/packaging/macos/import-cert.sh +++ b/packaging/macos/import-cert.sh @@ -17,12 +17,16 @@ if [[ -z "${APPLE_CERTIFICATE:-}" ]]; then exit 0 fi -TMP="${RUNNER_TEMP:-${TMPDIR:-/tmp}}" -KEYCHAIN="$TMP/gridcraft-signing.keychain-db" -CERT="$TMP/gridcraft-signing.p12" +BASE="${RUNNER_TEMP:-${TMPDIR:-/tmp}}" +# A private directory created exclusively, not predictable names in a possibly shared temp +# root: another local user's symlink at `gridcraft-signing.p12` would otherwise select the +# write target and capture the decoded certificate. +WORK="$(mktemp -d "$BASE/gridcraft-signing.XXXXXXXX")" +KEYCHAIN="$WORK/gridcraft-signing.keychain-db" +CERT="$WORK/gridcraft-signing.p12" KC_PASS="${KEYCHAIN_PASSWORD:-$(openssl rand -hex 24)}" -trap 'rm -f "$CERT"' EXIT +trap 'rm -rf "$WORK"' EXIT printf '%s' "$APPLE_CERTIFICATE" | base64 --decode >"$CERT" security create-keychain -p "$KC_PASS" "$KEYCHAIN" security set-keychain-settings -lut 21600 "$KEYCHAIN" diff --git a/xtask/src/stats.rs b/xtask/src/stats.rs index f40b1300a..462f1c7de 100644 --- a/xtask/src/stats.rs +++ b/xtask/src/stats.rs @@ -2,6 +2,9 @@ use std::path::Path; +#[cfg(unix)] +use std::os::unix::fs::MetadataExt as _; + #[derive(Default, Clone, Copy)] struct Counts { files: usize, @@ -10,7 +13,24 @@ struct Counts { proptests: usize, } -fn walk(dir: &Path, c: &mut Counts) { +fn walk(dir: &Path, c: &mut Counts, seen: &mut std::collections::HashSet, budget: &mut usize) { + if *budget == 0 { + // Hard stop for pathological trees: two directory symlinks pointing at each other + // would otherwise be walked exponentially. + return; + } + // Directory identities (dev+inode on unix) make a symlink to a visited directory a + // no-op, not another exponential branch of the scan. + #[cfg(unix)] + let id = std::fs::metadata(dir).ok().and_then(|m| m.dev().checked_mul(1 << 60).and_then(|d| d.checked_add(m.ino()))); + #[cfg(not(unix))] + let id: Option = None; + if let Some(id) = id + && !seen.insert(id) + { + return; + } + *budget -= 1; let Ok(rd) = std::fs::read_dir(dir) else { return; }; @@ -18,10 +38,11 @@ fn walk(dir: &Path, c: &mut Counts) { let p = e.path(); let name = e.file_name(); let name = name.to_string_lossy(); + // A symlinked directory is descended only if its identity is new (checked above). if p.is_dir() { // Nested fuzz workspaces and build output are not part of the crate. if name != "target" && name != "fuzz" && !name.starts_with('.') { - walk(&p, c); + walk(&p, c, seen, budget); } } else if name.ends_with(".rs") && let Ok(s) = std::fs::read_to_string(&p) @@ -71,7 +92,7 @@ pub fn run(_root: &Path, exact: bool) -> Result<(), String> { continue; }; let mut c = Counts::default(); - walk(&dir, &mut c); + walk(&dir, &mut c, &mut std::collections::HashSet::new(), &mut 10_000); let ex = if exact { exact_tests(&name) } else { None }; rows.push((name, c, ex)); } From 3099df934eda27cd457a9b8d1525d3fc973a1b7b Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 24 Oct 2026 07:58:13 -0400 Subject: [PATCH 7/7] M7: verify Agile encryption's dataIntegrity before returning plaintext (#2425) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agile decryption checked only the password verifier, which authenticates the key — not the package. An attacker who could replace EncryptedPackage ciphertext could change workbook content undetected ([MS-OFFCRYPTO] 2.3.4.14, the descriptor's dataIntegrity element). The reader now requires the dataIntegrity pair, decrypts it with the recovered package key, and checks HMAC(hash, hmacKey) over the whole EncryptedPackage stream (StreamSize field included) against the stored value before any plaintext is returned. Excel always writes the element ("SHOULD" in the spec); descriptors without it are refused as damaged rather than accepted unauthenticated. - crypto: RFC 2104/4231 HMAC test vectors, RFC-test cases for a key longer than the block size - fixtures write the pair like Excel; new tests: missing/empty/ragged integrity fields, tampered ciphertext and descriptor, a changed StreamSize field --- crates/xlsx/src/crypto.rs | 58 ++++++++++++++++- crates/xlsx/src/crypto/tests.rs | 82 +++++++++++++++++++++++- crates/xlsx/src/testing.rs | 25 +++++--- crates/xlsx/src/tests/encrypted_tests.rs | 3 +- 4 files changed, 153 insertions(+), 15 deletions(-) diff --git a/crates/xlsx/src/crypto.rs b/crates/xlsx/src/crypto.rs index 46a19d046..0a63044a0 100644 --- a/crates/xlsx/src/crypto.rs +++ b/crates/xlsx/src/crypto.rs @@ -26,6 +26,9 @@ const BLOCK: usize = 16; pub(crate) const BLOCK_VERIFIER_INPUT: [u8; 8] = [0xFE, 0xA7, 0xD2, 0x76, 0x3B, 0x4B, 0x9E, 0x79]; pub(crate) const BLOCK_VERIFIER_HASH: [u8; 8] = [0xD7, 0xAA, 0x0F, 0x6D, 0x30, 0x61, 0x34, 0x4E]; pub(crate) const BLOCK_KEY: [u8; 8] = [0x14, 0x6E, 0x0B, 0xE7, 0xAB, 0xAC, 0xD0, 0xD6]; +/// The two dataIntegrity block keys ([MS-OFFCRYPTO] 2.3.4.14), one per HMAC field. +pub(crate) const BLOCK_HMAC_KEY: [u8; 8] = [0x5F, 0xB2, 0xAD, 0x01, 0x0C, 0xB9, 0xE1, 0xF6]; +pub(crate) const BLOCK_HMAC_VALUE: [u8; 8] = [0xA0, 0x67, 0x7F, 0x02, 0xB2, 0x2C, 0x84, 0x33]; /// The password key encryptor's URI (the other kind is certificate-based). pub(crate) const PASSWORD_ENCRYPTOR: &str = "http://schemas.microsoft.com/office/2006/keyEncryptor/password"; @@ -165,6 +168,14 @@ impl Hash { Hash::Sha512 => run::(parts), } } + + /// The bytes the algorithm's compression function takes: 128 for SHA-384/512, else 64. + fn block_size(self) -> usize { + match self { + Hash::Sha384 | Hash::Sha512 => 128, + Hash::Sha1 | Hash::Sha256 => 64, + } + } } /// H0 = H(salt ‖ UTF-16LE(password)), then Hn = H(LE32(n - 1) ‖ Hn-1) for `spin` rounds. @@ -184,6 +195,19 @@ pub(crate) fn derive(hash: Hash, a: &[u8], b: &[u8], len: usize) -> Vec { k } +/// HMAC-SHA-`hash` ([RFC2104]), as Agile's dataIntegrity uses. +pub(crate) fn hmac(hash: Hash, key: &[u8], message: &[u8]) -> Vec { + let bs = hash.block_size(); + let mut k = key.to_vec(); + if k.len() > bs { + k = hash.digest(&[&k]); + } + k.resize(bs, 0); + let pad = |x: u8| k.iter().map(|b| b ^ x).collect::>(); + let inner = hash.digest(&[&pad(0x36), message]); + hash.digest(&[&pad(0x5C), &inner]) +} + /// The Standard encryption key ([MS-OFFCRYPTO] 2.3.4.7): SHA-1 iterated 50,000 times, the /// block-0 hash, then the 0x36 / 0x5C expansion, truncated to `key_len`. pub(crate) fn standard_key(password: &str, salt: &[u8], key_len: usize) -> Vec { @@ -368,8 +392,9 @@ impl Params { } } -/// What an Agile descriptor holds: the package's cipher, and the password key encryptor with its -/// three encrypted values (verifier input, verifier hash, package key). +/// What an Agile descriptor holds: the package's cipher, the password key encryptor with its +/// three encrypted values (verifier input, verifier hash, package key), and the package's +/// integrity pair, which Excel always writes and the reader requires. struct AgileDescriptor { key_data: Params, key: Params, @@ -377,6 +402,8 @@ struct AgileDescriptor { verifier_input: Vec, verifier_hash: Vec, key_value: Vec, + hmac_key: Vec, + hmac_value: Vec, } /// `xml` is the XML encryption descriptor after the 8-byte version and reserved field. @@ -395,6 +422,16 @@ fn agile_descriptor(xml: &[u8]) -> Result { return Err(malformed(format!("a spin count of {spin}, more than the {MAX_SPIN_COUNT} allowed"))); } let field = |name: &str| base64(encryptor.attr(name).ok_or_else(|| malformed(format!("no {name}")))?); + // A descriptor without dataIntegrity can't prove its ciphertext is the one the password + // protects: the verifier only authenticates the key, not the package ([MS-OFFCRYPTO] 2.3.4.14). + let integrity = root.child("dataIntegrity").ok_or_else(|| malformed("no dataIntegrity"))?; + let hmac_field = |attr: &str| { + let v = base64(integrity.attr(attr).ok_or_else(|| malformed(format!("no {attr}")))?)?; + if v.is_empty() || v.len() % BLOCK != 0 { + return Err(malformed(format!("a whole-block {attr}"))); + } + Ok(v) + }; Ok(AgileDescriptor { key_data, key, @@ -402,11 +439,14 @@ fn agile_descriptor(xml: &[u8]) -> Result { verifier_input: field("encryptedVerifierHashInput")?, verifier_hash: field("encryptedVerifierHashValue")?, key_value: field("encryptedKeyValue")?, + hmac_key: hmac_field("encryptedHmacKey")?, + hmac_value: hmac_field("encryptedHmacValue")?, }) } fn agile(xml: &[u8], package: &[u8], password: &str) -> Result, DecryptError> { - let AgileDescriptor { key_data, key, spin, verifier_input, verifier_hash, key_value } = agile_descriptor(xml)?; + let descriptor = agile_descriptor(xml)?; + let AgileDescriptor { key_data, key, spin, verifier_input, verifier_hash, key_value, hmac_key, hmac_value } = descriptor; let h = iterated_hash(key.hash, password, &key.salt, spin); let iv = key.iv(None); let field = |data: &[u8], block_key: &[u8]| -> Result, DecryptError> { @@ -423,6 +463,18 @@ fn agile(xml: &[u8], package: &[u8], password: &str) -> Result, DecryptE } let secret = field(&key_value, &BLOCK_KEY)?; let aes = Aes::new(secret.get(..key_data.key_len).ok_or_else(|| malformed("a short encryptedKeyValue"))?)?; + // Package integrity, [MS-OFFCRYPTO] 2.3.4.14: both HMAC fields are encrypted with the + // package key (the verifier above only authenticates the password), keyed by a random + // salt-size array, computed over the whole EncryptedPackage stream, StreamSize field included. + let mut mac_field = hmac_key; + aes.decrypt_cbc(&key_data.iv(Some(&BLOCK_HMAC_KEY)), &mut mac_field)?; + mac_field.truncate(key_data.salt.len()); + let mut declared = hmac_value; + aes.decrypt_cbc(&key_data.iv(Some(&BLOCK_HMAC_VALUE)), &mut declared)?; + let mac = hmac(key_data.hash, &mac_field, package); + if declared.get(..mac.len()) != Some(mac.as_slice()) { + return Err(malformed("the decrypted data failed its integrity check")); + } let (size, data) = package_parts(package)?; let mut out = Vec::with_capacity(size); for (i, segment) in data.chunks(SEGMENT).enumerate() { diff --git a/crates/xlsx/src/crypto/tests.rs b/crates/xlsx/src/crypto/tests.rs index 0fb0b921b..5f14686e9 100644 --- a/crates/xlsx/src/crypto/tests.rs +++ b/crates/xlsx/src/crypto/tests.rs @@ -6,6 +6,9 @@ use super::*; use crate::testing::{agile_info, encrypt_agile, encrypt_standard, with_agile_header}; const PASSWORD: &str = "Pass1234"; +/// 16 zero bytes (whole AES blocks), a stand-in for a real integrity field where the values +/// can't be reached anyway (scheme errors, descriptor-parse damage). +const HMAC_STUB: &str = "AAAAAAAAAAAAAAAAAAAAAA=="; fn hex(s: &str) -> Vec { (0..s.len()).step_by(2).map(|i| u8::from_str_radix(&s[i..i + 2], 16).unwrap()).collect() @@ -107,9 +110,9 @@ fn unsupported_schemes_are_named() { assert_eq!(unsupported(&rc4), "RC4"); assert_eq!(unsupported(&[3, 0, 3, 0, 0, 0, 0, 0]), "Extensible"); assert_eq!(unsupported(&[1, 0, 1, 0, 0, 0, 0, 0]), "version 1.1"); - let des = agile_info(PASSWORD, 1000).replace("cipherAlgorithm=\"AES\"", "cipherAlgorithm=\"3DES\""); + let des = agile_info(PASSWORD, 1000, HMAC_STUB, HMAC_STUB).replace("cipherAlgorithm=\"AES\"", "cipherAlgorithm=\"3DES\""); assert_eq!(unsupported(&with_agile_header(&des)), "3DES"); - let cert = agile_info(PASSWORD, 1000).replace("keyEncryptor/password", "keyEncryptor/certificate"); + let cert = agile_info(PASSWORD, 1000, HMAC_STUB, HMAC_STUB).replace("keyEncryptor/password", "keyEncryptor/certificate"); assert_eq!(unsupported(&with_agile_header(&cert)), "certificate"); } @@ -136,7 +139,7 @@ fn damaged_files_are_malformed_not_panics() { ragged[..8].copy_from_slice(&len.to_le_bytes()); assert!(is_malformed(decrypt_streams(&info, &ragged, PASSWORD))); } - let xml = agile_info(PASSWORD, 1000); + let xml = agile_info(PASSWORD, 1000, HMAC_STUB, HMAC_STUB); let bad = |s: String| is_malformed(decrypt_streams(&with_agile_header(&s), &[0; 8], PASSWORD)); assert!(bad(xml.replace("spinCount=\"1000\"", "spinCount=\"4000000000\""))); assert!(bad(xml.replace("saltValue=\"", "saltValue=\"*"))); @@ -155,3 +158,76 @@ fn non_ascii_passwords_hash_as_utf16le_like_msoffcrypto() { hex("2bca96ca0a7e8d88d9314a2eb1819a1ece94ff9ce34266ff82f891a88c4240c4c9f39c729b7c5c410c27fc8e2b7ad90d8c005eb68a603377b8a4ab858041c76a") ); } + +#[test] +fn hmac_matches_the_rfc2104_test_vectors() { + use crate::crypto::hmac as mac; + let h = |v: Vec| v.iter().map(|b| format!("{b:02x}")).collect::(); + // RFC 2202 test case 1 (HMAC-SHA-1) and HMAC-SHA-256 vectors. + assert_eq!(h(mac(Hash::Sha1, &[0x0b; 20], b"Hi There")), "b617318655057264e28bc0b6fb378c8ef146be00"); + assert_eq!(h(mac(Hash::Sha256, &[0x0b; 20], b"Hi There")), "b0344c61d8db38535ca8afceaf0bf12b881dc200c9833da726e9376c2e32cff7"); + // RFC 4231 test case 5 (HMAC-SHA-256): a key longer than the block size is hashed first. + assert_eq!( + h(mac(Hash::Sha256, &[0xaa; 131], b"Test Using Larger Than Block-Size Key - Hash Key First")), + "60e431591ee0b67f0d8a26aacbf5b77f8e0bc6213728c5140546040f0ee37f54" + ); +} + +/// Strips the `` element from an Agile descriptor. +fn without_integrity(info: &[u8]) -> Vec { + let xml = std::str::from_utf8(&info[8..]).unwrap().to_string(); + let start = xml.find("").unwrap() + 2; + let stripped = format!("{}{}", &xml[..start], &xml[end..]); + with_agile_header(&stripped) +} + +#[test] +fn integrity_is_required_and_enforced() { + let plain = payload(300); + let (info, package) = encrypt_agile(&plain, PASSWORD, 1000); + // An attacker can't just delete the integrity element to get the package verified. + assert!(matches!(decrypt_streams(&without_integrity(&info), &package, PASSWORD), + Err(DecryptError::Malformed(m)) if m.contains("dataIntegrity"))); + // An empty or ragged field is damage, not a pass. + for bad in [("encryptedHmacKey", ""), ("encryptedHmacKey", "AA=="), ("encryptedHmacValue", "")] { + let xml = + agile_info(PASSWORD, 1000, HMAC_STUB, HMAC_STUB).replace(&format!("{}=\"{HMAC_STUB}\"", bad.0), &format!("{}=\"{}\"", bad.0, bad.1)); + let r = decrypt_streams(&with_agile_header(&xml), &package, PASSWORD); + assert!(matches!(r, Err(DecryptError::Malformed(_))), "{}=\"{}\"", bad.0, bad.1); + } +} + +#[test] +fn modified_ciphertext_fails_the_integrity_check() { + let plain = payload(300); + let (info, package) = encrypt_agile(&plain, PASSWORD, 1000); + // The correct password proves the verifier can't vouch for a modified package. + for at in [8usize, 100, package.len() - 5] { + let mut tampered = package.clone(); + tampered[at] ^= 0x80; + assert!( + matches!(decrypt_streams(&info, &tampered, PASSWORD), + Err(DecryptError::Malformed(m)) if m.contains("integrity")), + "byte {at}" + ); + } + // And neither can the descriptor: a swapped integrity value is refused. + let xml = String::from_utf8_lossy(&info[8..]).to_string(); + let at = xml.find("encryptedHmacValue=\"").unwrap() + "encryptedHmacValue=\"".len(); + let mut xml: Vec = xml.into_bytes(); + xml[at] ^= b'A' ^ b'B'; + assert!(matches!(decrypt_streams(&with_agile_header(std::str::from_utf8(&xml).unwrap()), &package, PASSWORD), Err(DecryptError::Malformed(_)))); +} + +#[test] +fn integrity_matches_the_stream_not_the_plaintext() { + // The MAC covers the whole EncryptedPackage stream, StreamSize field included: changing the + // stated size alone must fail, while the actual bytes round-trip. + let plain = payload(4200); + let (info, package) = encrypt_agile(&plain, PASSWORD, 1000); + let mut ours = package.clone(); + ours[..8].copy_from_slice(&((plain.len() + 1) as u64).to_le_bytes()); + assert!(matches!(decrypt_streams(&info, &ours, PASSWORD), Err(DecryptError::Malformed(m)) if m.contains("integrity"))); + assert_eq!(decrypt_streams(&info, &package, PASSWORD).unwrap(), plain); +} diff --git a/crates/xlsx/src/testing.rs b/crates/xlsx/src/testing.rs index 348c08af5..5729250bb 100644 --- a/crates/xlsx/src/testing.rs +++ b/crates/xlsx/src/testing.rs @@ -9,8 +9,8 @@ use aes::cipher::{BlockCipherEncrypt, KeyInit}; use crate::crypto::{ - BLOCK_KEY, BLOCK_VERIFIER_HASH, BLOCK_VERIFIER_INPUT, F_AES, F_CRYPTOAPI, Hash, PASSWORD_ENCRYPTOR, Params, SEGMENT, derive, iterated_hash, - standard_key, + BLOCK_HMAC_KEY, BLOCK_HMAC_VALUE, BLOCK_KEY, BLOCK_VERIFIER_HASH, BLOCK_VERIFIER_INPUT, F_AES, F_CRYPTOAPI, Hash, PASSWORD_ENCRYPTOR, Params, + SEGMENT, derive, hmac, iterated_hash, standard_key, }; /// The spin count `encrypt_workbook` uses: Excel's default. @@ -21,6 +21,7 @@ const KEY_DATA_SALT: [u8; 16] = [16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, const SECRET_KEY: [u8; 32] = [64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95]; const VERIFIER: [u8; 16] = [32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47]; +const HMAC_SALT: [u8; 16] = [112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127]; /// `package` (normally an `.xlsx` zip) as Excel stores it with a password: Agile encryption /// (AES-256, SHA-512) inside a compound file. @@ -202,8 +203,9 @@ fn package(size: usize, body: Vec) -> Vec { p } -/// The Agile XML descriptor: AES-256, SHA-512, a password encryptor salted with 0..16. -pub(crate) fn agile_info(password: &str, spin: u32) -> String { +/// The Agile XML descriptor: AES-256, SHA-512, a password encryptor salted with 0..16, and the +/// dataIntegrity pair (`hmac_key_b64`/`hmac_value_b64`) a compliant file carries. +pub(crate) fn agile_info(password: &str, spin: u32, hmac_key_b64: &str, hmac_value_b64: &str) -> String { let key = Params { hash: Hash::Sha512, salt: PASSWORD_SALT.to_vec(), key_len: 32 }; let h = iterated_hash(Hash::Sha512, password, &key.salt, spin); let iv = key.iv(None); @@ -217,9 +219,10 @@ pub(crate) fn agile_info(password: &str, spin: u32) -> String { base64_encode(salt) ) }; + // The dataIntegrity pair ([MS-OFFCRYPTO] 2.3.4.14), encrypted with the package key. format!( - "\r\n", - params(&KEY_DATA_SALT), + "\r\n", + params(KEY_DATA_SALT.as_slice()), params(&PASSWORD_SALT), ) } @@ -231,14 +234,20 @@ pub(crate) fn with_agile_header(xml: &str) -> Vec { info } -/// `EncryptionInfo` and `EncryptedPackage` for `plain`, Agile (AES-256, SHA-512). +/// `EncryptionInfo` and `EncryptedPackage` for `plain`, Agile (AES-256, SHA-512), with the +/// dataIntegrity HMAC over the whole EncryptedPackage stream as Excel writes it. pub(crate) fn encrypt_agile(plain: &[u8], password: &str, spin: u32) -> (Vec, Vec) { let key_data = Params { hash: Hash::Sha512, salt: KEY_DATA_SALT.to_vec(), key_len: 32 }; let mut body = vec![]; for (i, segment) in plain.chunks(SEGMENT).enumerate() { body.extend(encrypt_cbc(&SECRET_KEY, &key_data.iv(Some(&(i as u32).to_le_bytes())), segment)); } - (with_agile_header(&agile_info(password, spin)), package(plain.len(), body)) + let stream = package(plain.len(), body); + let (hmac_key_b64, hmac_value_b64) = ( + base64_encode(&encrypt_cbc(&SECRET_KEY, &key_data.iv(Some(&BLOCK_HMAC_KEY)), &HMAC_SALT)), + base64_encode(&encrypt_cbc(&SECRET_KEY, &key_data.iv(Some(&BLOCK_HMAC_VALUE)), &hmac(Hash::Sha512, &HMAC_SALT, &stream))), + ); + (with_agile_header(&agile_info(password, spin, &hmac_key_b64, &hmac_value_b64)), stream) } /// `EncryptionInfo` and `EncryptedPackage` for `plain`, Standard (AES-128, SHA-1). diff --git a/crates/xlsx/src/tests/encrypted_tests.rs b/crates/xlsx/src/tests/encrypted_tests.rs index 7a1a9bb82..525ed9193 100644 --- a/crates/xlsx/src/tests/encrypted_tests.rs +++ b/crates/xlsx/src/tests/encrypted_tests.rs @@ -56,7 +56,8 @@ fn schemes_no_password_opens_are_named_before_asking_for_one() { let file = |info: &[u8]| compound_file(&[("EncryptionInfo", info), ("EncryptedPackage", &[0; 8])]); assert_eq!(check_encryption(&file(&[3, 0, 2, 0, 4, 0, 0, 0])), unsupported("RC4")); assert_eq!(check_encryption(&file(&[4, 0, 3, 0, 0, 0, 0, 0])), unsupported("Extensible")); - let cert = agile_info("pw", 1000).replace("keyEncryptor/password", "keyEncryptor/certificate"); + let cert = + agile_info("pw", 1000, "AAAAAAAAAAAAAAAAAAAAAA==", "AAAAAAAAAAAAAAAAAAAAAA==").replace("keyEncryptor/password", "keyEncryptor/certificate"); assert_eq!(check_encryption(&file(&with_agile_header(&cert))), unsupported("certificate")); assert!(matches!(check_encryption(&file(&[4, 0])), Err(DecryptError::Malformed(_)))); // Rights management (IRM) keeps an EncryptedPackage but no EncryptionInfo.