Skip to content

Reversed <dimension ref> underflows unchecked u32 subtraction in get_dimension and Dimensions::len #692

Description

@krickert

A <dimension> whose end cell precedes its start underflows two unchecked u32 subtractions. ECMA-376 does not require ref to be ordered, so this is reachable from a file a writer can legitimately emit, and from any untrusted input.

Two sites, same root, both still present on master:

  • src/xlsx/mod.rs:2789-2790let rows = parts[1].0 - parts[0].0; and the column line under it
  • src/lib.rs:181-183Dimensions::len()(self.end.0 - self.start.0 + 1) as u64 * ...

Repro

A minimal OPC package (one sheet, no shared strings, no styles) whose xl/worksheets/sheet1.xml is:

<worksheet xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main">
<dimension ref="C5:A1"/>
<sheetData>
<row r="1"><c r="A1"><v>1</v></c></row>
<row r="5"><c r="C5"><v>9</v></c></row>
</sheetData>
</worksheet>
let mut wb: Xlsx<_> = open_workbook(&path)?;
let name = wb.sheet_names()[0].clone();
let dims = wb.worksheet_cells_reader(&name)?.dimensions();
println!("declared start={:?} end={:?}", dims.start, dims.end);
println!("Dimensions::len() = {}", dims.len());

Debug build (calamine 0.36.0):

thread 'main' panicked at src/xlsx/mod.rs:2789:24:
attempt to subtract with overflow

Release build:

declared start=(4, 2) end=(0, 0)
Dimensions::len() = 18446744056529682435

Why it is worth fixing rather than documenting

worksheet_range masks this in release, because from_sparse rebuilds the extent from the cells it actually saw. The damage surfaces through dimensions(), which is the natural pre-allocation hint for the incremental readers, and through Dimensions::len(), which is public API with no documented precondition. Reading the declared extent and calling len() on it is the obvious thing to do, and it silently yields 1.8e19.

This is the same shape as #150 and #174, which were fixed in the xls path. The xlsx path and the public Dimensions type still have it.

Suggested fix

Normalise the corners in get_dimension, or reject the ref outright, or make the public computation total:

pub fn len(&self) -> u64 {
    if self.end.0 < self.start.0 || self.end.1 < self.start.1 {
        return 0;
    }
    u64::from(self.end.0 - self.start.0 + 1) * u64::from(self.end.1 - self.start.1 + 1)
}

Found while building the gRPC service in #688, which streams cells through worksheet_cells_reader and reports dimensions() to clients. Glad to send a PR if you have a preference for which of the three directions you want.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions