From e78cc6edc91d35f4ca6af6fd8b7bdcce1bd3198f Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Sun, 11 Oct 2026 12:55:17 -0400 Subject: [PATCH] Fix-up #332: bound .xls formula nesting and share one record budget across sheets Chained PtgParen/PtgUminus (or operator) tokens built a ~16K-deep Expr whose printing or drop overflowed the stack; nesting past the formula parser's MAX_DEPTH (256) is now reported as unsupported. BoundSheet records pointing into one huge substream each got their own 40M-record allowance; the allowance is now shared by the whole Workbook stream, and two sheets at the same offset are refused. Co-Authored-By: Claude Opus 5.5 --- crates/xlsx/src/tests/xls_tests.rs | 39 ++++++++ crates/xlsx/src/xls/formula.rs | 138 ++++++++++++++++++++--------- crates/xlsx/src/xls/mod.rs | 49 +++++++--- 3 files changed, 169 insertions(+), 57 deletions(-) diff --git a/crates/xlsx/src/tests/xls_tests.rs b/crates/xlsx/src/tests/xls_tests.rs index a150dc0b3..fd37795ec 100644 --- a/crates/xlsx/src/tests/xls_tests.rs +++ b/crates/xlsx/src/tests/xls_tests.rs @@ -437,3 +437,42 @@ fn hostile_xls_input_never_panics_or_hangs() { let _ = read_xls(&b); } } + +#[test] +fn sheets_share_one_record_budget() { + // Many BoundSheet records point into one long substream (at successive nested BOFs), so each + // sheet would read the whole tail. The record allowance is shared, not per sheet. + let n = 300u32; + let tail: Vec = (0..2000u16).flat_map(|r| rec(0x0203, &cat(&[&cell(r, 0, 0), &1.0f64.to_le_bytes()]))).collect(); + let boundsheet = |pos: u32, name: &str| rec(0x0085, &cat(&[&pos.to_le_bytes(), &[0, 0, name.len() as u8, 0], name.as_bytes()])); + let globals = |base: u32| { + let mut g = bof(0x0005); + for i in 0..n { + g.extend(boundsheet(base + i * bof(0x0010).len() as u32, &format!("S{i}"))); + } + g.extend(eof()); + g + }; + let base = globals(0).len() as u32; + let mut stream = globals(base); + for _ in 0..n { + stream.extend(bof(0x0010)); + } + stream.extend(tail); + stream.extend(eof()); + let file = compound("Workbook", &stream); + // About n × 2,000 records in all: over a 100,000 shared allowance, though each sheet alone fits. + assert!(matches!(crate::xls::read_xls_with(&file, crate::xls::Budget::with_max_records(100_000)), Err(IoError::TooLarge(_)))); + assert!(crate::xls::read_xls_with(&file, crate::xls::Budget::with_max_records(10_000_000)).is_ok()); + + // Two sheets at the same offset are refused outright. + let mut dup = bof(0x0005); + let at = (dup.len() + 2 * boundsheet(0, "A").len() + eof().len()) as u32; + dup.extend(boundsheet(at, "A")); + dup.extend(boundsheet(at, "B")); + dup.extend(eof()); + dup.extend(bof(0x0010)); + dup.extend(eof()); + let err = read_xls(&compound("Workbook", &dup)).unwrap_err().to_string(); + assert!(err.contains("same substream"), "{err}"); +} diff --git a/crates/xlsx/src/xls/formula.rs b/crates/xlsx/src/xls/formula.rs index 3fc3fc40c..87a512c59 100644 --- a/crates/xlsx/src/xls/formula.rs +++ b/crates/xlsx/src/xls/formula.rs @@ -180,13 +180,35 @@ fn function_name(name: &str) -> String { n.to_ascii_uppercase() } -fn pop(stack: &mut Vec) -> Result { - stack.pop().ok_or(Unsupported("malformed formula (operand missing)")) +/// Expression nesting at most, as in the formula parser: deeper trees would overflow the stack +/// when printed, evaluated or dropped (a hostile file can chain thousands of PtgParen tokens). +const MAX_DEPTH: usize = 256; + +/// Operand stack entries: an expression and its nesting depth. +type Stack = Vec<(Expr, usize)>; + +/// Pops an operand, recording its depth in `kids` (the deepest operand this token consumed). +fn pop(stack: &mut Stack, kids: &mut usize) -> Result { + let (e, d) = stack.pop().ok_or(Unsupported("malformed formula (operand missing)"))?; + *kids = (*kids).max(d); + Ok(e) } -fn pop_n(stack: &mut Vec, n: usize) -> Result, Unsupported> { +fn pop_n(stack: &mut Stack, n: usize, kids: &mut usize) -> Result, Unsupported> { let at = stack.len().checked_sub(n).ok_or(Unsupported("malformed formula (argument missing)"))?; - Ok(stack.split_off(at)) + let popped = stack.split_off(at); + *kids = popped.iter().map(|(_, d)| *d).fold(*kids, usize::max); + Ok(popped.into_iter().map(|(e, _)| e).collect()) +} + +/// Pushes the result of a token one level above the operands it consumed. +fn push(stack: &mut Stack, kids: usize, e: Expr) -> Result<(), Unsupported> { + let depth = kids.saturating_add(1); + if depth > MAX_DEPTH { + return Err(Unsupported("formula nested too deeply")); + } + stack.push((e, depth)); + Ok(()) } /// Decodes `rgce` (tokens) with `rgcb` (the extra data for array constants) evaluated at @@ -197,9 +219,10 @@ pub(super) fn decode(rgce: &[u8], rgcb: &[u8], base: CellRef, mode: Mode, links: } let mut t = Bytes { b: rgce, pos: 0 }; let mut extra = Bytes { b: rgcb, pos: 0 }; - let mut stack: Vec = Vec::new(); + let mut stack: Stack = Vec::new(); while t.pos < rgce.len() { let ptg = t.u8()?; + let mut kids = 0usize; // Operand classes (reference / value / array) share one token number. let id = if ptg & 0x60 != 0 { 0x20 | (ptg & 0x1F) } else { ptg }; match id { @@ -221,9 +244,9 @@ pub(super) fn decode(rgce: &[u8], rgcb: &[u8], base: CellRef, mode: Mode, links: 0x10 => BinOp::Union, _ => BinOp::Range, }; - let b = pop(&mut stack)?; - let a = pop(&mut stack)?; - stack.push(Expr::Binary(op, Box::new(a), Box::new(b))); + let b = pop(&mut stack, &mut kids)?; + let a = pop(&mut stack, &mut kids)?; + push(&mut stack, kids, Expr::Binary(op, Box::new(a), Box::new(b)))?; } 0x12..=0x14 => { let op = match id { @@ -231,18 +254,18 @@ pub(super) fn decode(rgce: &[u8], rgcb: &[u8], base: CellRef, mode: Mode, links: 0x13 => UnOp::Neg, _ => UnOp::Percent, }; - let x = pop(&mut stack)?; - stack.push(Expr::Unary(op, Box::new(x))); + let x = pop(&mut stack, &mut kids)?; + push(&mut stack, kids, Expr::Unary(op, Box::new(x)))?; } 0x15 => { - let x = pop(&mut stack)?; - stack.push(Expr::Paren(Box::new(x))); + let x = pop(&mut stack, &mut kids)?; + push(&mut stack, kids, Expr::Paren(Box::new(x)))?; } - 0x16 => stack.push(Expr::Missing), + 0x16 => push(&mut stack, kids, Expr::Missing)?, 0x17 => { let cch = usize::from(t.u8()?); let high = t.u8()? & 1 != 0; - stack.push(Expr::Text(Arc::from(t.chars(cch, high)?))); + push(&mut stack, kids, Expr::Text(Arc::from(t.chars(cch, high)?)))?; } 0x19 => { let kind = t.u8()?; @@ -255,34 +278,34 @@ pub(super) fn decode(rgce: &[u8], rgcb: &[u8], base: CellRef, mode: Mode, links: } if kind & 0x10 != 0 { // PtgAttrSum: SUM of the single operand. - let x = pop(&mut stack)?; - stack.push(Expr::Call("SUM".into(), vec![x])); + let x = pop(&mut stack, &mut kids)?; + push(&mut stack, kids, Expr::Call("SUM".into(), vec![x]))?; } // Semi (volatile), If, Goto, Space, Baxcel: no effect on the expression. } 0x1C => { let e = t.u8()?; - stack.push(Expr::Error(error_code(e).ok_or(Unsupported("unknown error constant"))?)); + push(&mut stack, kids, Expr::Error(error_code(e).ok_or(Unsupported("unknown error constant"))?))?; } - 0x1D => stack.push(Expr::Bool(t.u8()? != 0)), - 0x1E => stack.push(Expr::Number(f64::from(t.u16()?))), + 0x1D => push(&mut stack, kids, Expr::Bool(t.u8()? != 0))?, + 0x1E => push(&mut stack, kids, Expr::Number(f64::from(t.u16()?)))?, 0x1F => { let n = t.f64()?; if !n.is_finite() { return Err(Unsupported("invalid number constant")); } - stack.push(Expr::Number(n)); + push(&mut stack, kids, Expr::Number(n))?; } 0x20 => { t.take(7)?; - stack.push(array_constant(&mut extra)?); + push(&mut stack, kids, array_constant(&mut extra)?)?; } 0x21 => { let iftab = t.u16()?; let (name, args) = function(iftab).ok_or(Unsupported("unknown function"))?; let n = usize::try_from(args).map_err(|_| Unsupported("unknown function argument count"))?; - let args = pop_n(&mut stack, n)?; - stack.push(Expr::Call(name.to_string(), args)); + let args = pop_n(&mut stack, n, &mut kids)?; + push(&mut stack, kids, Expr::Call(name.to_string(), args))?; } 0x22 => { let n = usize::from(t.u8()?); @@ -290,7 +313,7 @@ pub(super) fn decode(rgce: &[u8], rgcb: &[u8], base: CellRef, mode: Mode, links: if tab & 0x8000 != 0 { return Err(Unsupported("macro sheet commands")); } - let mut args = pop_n(&mut stack, n)?; + let mut args = pop_n(&mut stack, n, &mut kids)?; if tab & 0x7FFF == 0x00FF { // A function called by name (add-in or newer function): the name comes first. if args.is_empty() { @@ -300,10 +323,10 @@ pub(super) fn decode(rgce: &[u8], rgcb: &[u8], base: CellRef, mode: Mode, links: Expr::Name(n) => function_name(&n), _ => return Err(Unsupported("function call through an expression")), }; - stack.push(Expr::Call(name, args)); + push(&mut stack, kids, Expr::Call(name, args))?; } else { let (name, _) = function(tab & 0x7FFF).ok_or(Unsupported("unknown function"))?; - stack.push(Expr::Call(name.to_string(), args)); + push(&mut stack, kids, Expr::Call(name.to_string(), args))?; } } 0x23 => { @@ -314,17 +337,17 @@ pub(super) fn decode(rgce: &[u8], rgcb: &[u8], base: CellRef, mode: Mode, links: .and_then(|i| links.names.get(i)) .filter(|n| !n.is_empty()) .ok_or(Unsupported("reference to a missing defined name"))?; - stack.push(Expr::Name(name.clone())); + push(&mut stack, kids, Expr::Name(name.clone()))?; } 0x24 | 0x2C => { let (r, c) = (t.u16()?, t.u16()?); let a = anchor(r, c, base, id == 0x2C); - stack.push(Expr::Ref(Reference { sheet: SheetSel::Current, kind: RefKind::Cell(a) })); + push(&mut stack, kids, Expr::Ref(Reference { sheet: SheetSel::Current, kind: RefKind::Cell(a) }))?; } 0x25 | 0x2D => { let (r0, r1, c0, c1) = (t.u16()?, t.u16()?, t.u16()?, t.u16()?); let kind = area(r0, r1, c0, c1, base, id == 0x2D); - stack.push(Expr::Ref(Reference { sheet: SheetSel::Current, kind })); + push(&mut stack, kids, Expr::Ref(Reference { sheet: SheetSel::Current, kind }))?; } 0x26 => { // PtgMemArea: a cache of the sub-expression that follows; skip its extra data. @@ -340,42 +363,50 @@ pub(super) fn decode(rgce: &[u8], rgcb: &[u8], base: CellRef, mode: Mode, links: } 0x2A => { t.take(4)?; - stack.push(Expr::Error(CellError::Ref)); + push(&mut stack, kids, Expr::Error(CellError::Ref))?; } 0x2B => { t.take(8)?; - stack.push(Expr::Error(CellError::Ref)); + push(&mut stack, kids, Expr::Error(CellError::Ref))?; } 0x39 => { let ixti = t.u16()?; let i = t.u32()?; - stack.push(Expr::Name(links.extern_name(ixti, i)?)); + push(&mut stack, kids, Expr::Name(links.extern_name(ixti, i)?))?; } 0x3A => { let ixti = t.u16()?; let (r, c) = (t.u16()?, t.u16()?); let a = anchor(r, c, base, mode == Mode::Name); - stack.push(match links.sheet_sel(ixti)? { - Some(sheet) => Expr::Ref(Reference { sheet, kind: RefKind::Cell(a) }), - None => Expr::Error(CellError::Ref), - }); + push( + &mut stack, + kids, + match links.sheet_sel(ixti)? { + Some(sheet) => Expr::Ref(Reference { sheet, kind: RefKind::Cell(a) }), + None => Expr::Error(CellError::Ref), + }, + )?; } 0x3B => { let ixti = t.u16()?; let (r0, r1, c0, c1) = (t.u16()?, t.u16()?, t.u16()?, t.u16()?); let kind = area(r0, r1, c0, c1, base, mode == Mode::Name); - stack.push(match links.sheet_sel(ixti)? { - Some(sheet) => Expr::Ref(Reference { sheet, kind }), - None => Expr::Error(CellError::Ref), - }); + push( + &mut stack, + kids, + match links.sheet_sel(ixti)? { + Some(sheet) => Expr::Ref(Reference { sheet, kind }), + None => Expr::Error(CellError::Ref), + }, + )?; } 0x3C => { t.take(6)?; - stack.push(Expr::Error(CellError::Ref)); + push(&mut stack, kids, Expr::Error(CellError::Ref))?; } 0x3D => { t.take(10)?; - stack.push(Expr::Error(CellError::Ref)); + push(&mut stack, kids, Expr::Error(CellError::Ref))?; } 0x01 => return Err(Unsupported("nested shared or array formula reference")), 0x02 => return Err(Unsupported("what-if data tables")), @@ -386,7 +417,7 @@ pub(super) fn decode(rgce: &[u8], rgcb: &[u8], base: CellRef, mode: Mode, links: return Err(Unsupported("formula too complex")); } } - let e = pop(&mut stack)?; + let (e, _) = stack.pop().ok_or(Unsupported("malformed formula (operand missing)"))?; if !stack.is_empty() { return Err(Unsupported("malformed formula (unused operands)")); } @@ -493,4 +524,25 @@ mod tests { assert!(text(&[0x21, 0xFF, 0x7F], &[]).is_err()); assert!(text(&[0x60, 0, 0, 0, 0, 0, 0, 0], &[0xFF, 0xFF, 0xFF]).is_err()); } + + #[test] + fn deep_nesting_is_refused_not_a_stack_overflow() { + // 1 followed by thousands of PtgParen / PtgUminus tokens: a hostile file used to build a + // 16K-deep tree that overflowed the stack when printed or dropped. + for op in [0x15u8, 0x13] { + let mut rgce = vec![0x1E, 1, 0]; + rgce.resize(MAX_RGCE, op); + assert_eq!(text(&rgce, &[]), Err(Unsupported("formula nested too deeply"))); + } + // Binary chains nest too: 1+1+1+... left-associated. + let mut chain = vec![0x1E, 1, 0]; + while chain.len() + 4 <= MAX_RGCE { + chain.extend([0x1E, 1, 0, 0x03]); + } + assert!(text(&chain, &[]).is_err()); + // Nesting up to the limit still decodes. + let mut ok = vec![0x1E, 1, 0]; + ok.resize(3 + MAX_DEPTH - 1, 0x15); + assert!(text(&ok, &[]).is_ok()); + } } diff --git a/crates/xlsx/src/xls/mod.rs b/crates/xlsx/src/xls/mod.rs index b070478d5..c9aee66a3 100644 --- a/crates/xlsx/src/xls/mod.rs +++ b/crates/xlsx/src/xls/mod.rs @@ -31,7 +31,9 @@ const MAX_SHEETS: usize = 4096; const MAX_NAMES: usize = 65_536; const MAX_XFS: usize = 65_536; const MAX_MERGES: usize = 100_000; -/// Records read from one substream at most (a 65,536 × 256 sheet has fewer). +/// Records read from the whole Workbook stream at most (a 65,536 × 256 sheet has fewer). One +/// allowance shared by every substream: many BoundSheet records pointing into one huge +/// substream must not each get their own. const MAX_RECORDS: usize = 40_000_000; const BOF: u16 = 0x0809; @@ -53,13 +55,32 @@ pub(crate) fn is_xls(bytes: &[u8]) -> bool { cf.has_root_stream("Workbook") || cf.has_root_stream("Book") } -#[derive(Default)] -struct Budget { +pub(crate) struct Budget { cells: usize, text: usize, + records: usize, + max_records: usize, +} + +impl Default for Budget { + fn default() -> Self { + Budget { cells: 0, text: 0, records: 0, max_records: MAX_RECORDS } + } } impl Budget { + /// A budget with a smaller record allowance (tests). + #[cfg(test)] + pub(crate) fn with_max_records(max_records: usize) -> Self { + Budget { max_records, ..Budget::default() } + } + fn record(&mut self) -> Result<(), IoError> { + self.records = self.records.saturating_add(1); + if self.records > self.max_records { + return Err(IoError::TooLarge("XLS: too many records".into())); + } + Ok(()) + } fn cell(&mut self) -> Result<(), IoError> { self.cells += 1; if self.cells > MAX_CELLS { @@ -106,6 +127,11 @@ struct Globals { /// Reads an Excel 97-2003 workbook. Inspect the report's warnings for what was skipped. pub fn read_xls(bytes: &[u8]) -> Result<(Workbook, ReadReport), IoError> { + read_xls_with(bytes, Budget::default()) +} + +/// [`read_xls`] with an explicit resource budget. +pub(crate) fn read_xls_with(bytes: &[u8], mut budget: Budget) -> Result<(Workbook, ReadReport), IoError> { let cf = Compound::open(bytes).map_err(invalid)?; let stream = match cf.root_stream("Workbook").map_err(invalid)? { Some(s) => s, @@ -117,7 +143,6 @@ pub fn read_xls(bytes: &[u8]) -> Result<(Workbook, ReadReport), IoError> { } }; let mut report = ReadReport::default(); - let mut budget = Budget::default(); let g = globals(&stream, &mut budget)?; let mut wb = Workbook::new(); @@ -195,12 +220,9 @@ fn globals(stream: &[u8], budget: &mut Budget) -> Result { return Err(invalid("the Workbook stream does not start with the workbook globals")); } let mut g = Globals::default(); - let mut count = 0usize; + let mut offsets = std::collections::HashSet::new(); while let Some(rec) = recs.next_record()? { - count += 1; - if count > MAX_RECORDS { - return Err(IoError::TooLarge("XLS: too many records".into())); - } + budget.record()?; let mut c = Cursor::new(&rec.parts); match rec.id { EOF => break, @@ -210,6 +232,9 @@ fn globals(stream: &[u8], budget: &mut Budget) -> Result { return Err(IoError::TooLarge("XLS: too many sheets".into())); } let pos = c.u32()? as usize; + if !offsets.insert(pos) { + return Err(invalid("two sheets point at the same substream")); + } let state = c.u8()?; let kind = c.u8()?; let name = c.short_string()?; @@ -460,12 +485,8 @@ fn worksheet( unsupported: vec![], }; let mut depth = 1usize; - let mut count = 0usize; while let Some(rec) = recs.next_record()? { - count += 1; - if count > MAX_RECORDS { - return Err(IoError::TooLarge("XLS: too many records in a sheet".into())); - } + budget.record()?; match rec.id { BOF => { // An embedded chart's substream.