Skip to content

fix(xlsx): checked arithmetic in get_row_and_optional_column - #705

Closed
Svector-anu wants to merge 1 commit into
tafia:masterfrom
Svector-anu:fix/xlsx-range-parse-overflow-panic
Closed

fix(xlsx): checked arithmetic in get_row_and_optional_column#705
Svector-anu wants to merge 1 commit into
tafia:masterfrom
Svector-anu:fix/xlsx-range-parse-overflow-panic

Conversation

@Svector-anu

@Svector-anu Svector-anu commented Aug 8, 2026

Copy link
Copy Markdown

summary

get_row_and_optional_column backs get_dimension/get_row_column/get_row - the worksheet dimension and merged region parsing paths. it accumulates the base-26 column value and base-10 row value with raw */+. a range string with a long enough run of letters or digits overflows u32 during accumulation and panics (attempt to multiply with overflow) instead of returning an error.

Reference::parse already guards the same input class via wrapping_mul/wrapping_add plus a post-hoc validate() bounds check (see test_parse_reference_overflow). get_row_and_optional_column's callers don't do a follow-up bounds check, so checked arithmetic that fails right at the overflow site is the more self-contained fix here - reusing the ColumnNumberOverflow/RowNumberOverflow errors that already exist for the same condition elsewhere in the file.

how this was found

fuzzing a downstream consumer's xlsx target (seeded from its own test fixtures) crashed within a couple minutes:

thread '<unnamed>' panicked at calamine-0.36.1/src/xlsx/mod.rs:2837:38:
attempt to multiply with overflow

call stack: get_dimension -> get_row_and_optional_column, reached while parsing a crafted xlsx's xl/worksheets/sheet1.xml.

no memory-safety impact - rust panics safely - but any service converting untrusted spreadsheets through calamine can be crashed by a small file, since a release build without overflow checks would instead silently wrap into a bogus column/row value with no validation downstream of this function.

what's included

  • checked arithmetic in both accumulation loops, reusing the existing ColumnNumberOverflow/RowNumberOverflow variants
  • a regression test (test_get_row_and_optional_column_overflow) reproducing the panic pre-fix, plus a get_dimension case through the actual crash path
  • cargo test --lib (43 passed) and cargo clippy --lib -- -D warnings both clean

open to matching Reference::parse's wrapping+validate pattern instead if you'd rather - went with checked arithmetic mainly because this function's callers don't validate the result afterward.

get_row_and_optional_column backs get_dimension/get_row_column/get_row -
the worksheet dimension and merged region parsing paths. it accumulates
the column (base-26) and row (base-10) values with raw */+. a range
string with a long enough run of letters or digits overflows u32 during
accumulation and panics ("attempt to multiply with overflow") instead of
returning an error.

Reference::parse already guards the same input class, via wrapping
arithmetic plus a post-hoc validate() bounds check. this function's
callers don't do a follow-up bounds check, so checked arithmetic that
fails right at the overflow site is the more self-contained fix here -
reusing the existing ColumnNumberOverflow/RowNumberOverflow errors
already defined for the same condition elsewhere in the file.

found by fuzzing a downstream consumer's xlsx target (seeded from its
own test fixtures) with cargo-fuzz: a crafted xlsx whose <dimension> or
a merged-cell reference contains a ~20-character run of letters aborts
the whole parse. no memory-safety impact, but any service converting
untrusted spreadsheets through calamine can be crashed by a small file.
reproducer and regression test included.
@Svector-anu
Svector-anu force-pushed the fix/xlsx-range-parse-overflow-panic branch from dc88a76 to 69a27bd Compare August 8, 2026 11:57
@Svector-anu Svector-anu changed the title fix(xlsx): checked arithmetic in get_row_and_optional_column to prevent overflow panic fix(xlsx): checked arithmetic in get_row_and_optional_column Aug 8, 2026
@jmcnamara

Copy link
Copy Markdown
Collaborator

Is this similar to #696?

@Svector-anu

Copy link
Copy Markdown
Author

yeah, same root cause - unchecked u32 accumulation in get_row_and_optional_column. #696 predates this by a couple weeks and is further along (already reworked per your feedback to validate against the actual row/column limits instead of just catching the overflow). closing this one in favor of that.

@Svector-anu Svector-anu closed this Aug 8, 2026
Svector-anu added a commit to Svector-anu/svectors-lab that referenced this pull request Aug 10, 2026
Run the target's own cargo-fuzz harness (when it ships one) as part of
arm A: seed the corpus from tests/fixtures where they exist, run each
target for ~90s (capped at 8 targets), and route a crash through the
same triage as any scanner hit before it counts as a finding.

Crash routing depends on where it lands: target's own code -> PVR,
a dependency -> public PR against that dependency, harness lying ->
drop. Verified end to end against firecrawl/anydoc: its xlsx fuzz
target crashed on an integer-overflow panic inside calamine (a
dependency), root-caused, fixed, and filed upstream (tafia/calamine#705).

Secrets are scrubbed from the fuzz env (env -u GH_TOKEN -u GH_GLOBAL
-u RESEND_*) immediately before compiling/running the target's own
code, since a malicious target's build.rs could otherwise exfiltrate
them.

Split out of aeonfun#863 per triage: this PR is skill-scope only
(skills/vuln-scanner/SKILL.md + eyebrowlock.json). The paired
runtime-infrastructure changes (.github/workflows/aeon.yml,
scripts/stage-vuln-scanner.sh, scripts/skill_mode.sh) are in a
separate follow-up PR for a maintainer to review, since they touch
the fork's trust boundary (Bash(cargo:*) grant, toolchain staging).
aaronjmars pushed a commit to aeonfun/aeon that referenced this pull request Aug 10, 2026
)

Run the target's own cargo-fuzz harness (when it ships one) as part of
arm A: seed the corpus from tests/fixtures where they exist, run each
target for ~90s (capped at 8 targets), and route a crash through the
same triage as any scanner hit before it counts as a finding.

Crash routing depends on where it lands: target's own code -> PVR,
a dependency -> public PR against that dependency, harness lying ->
drop. Verified end to end against firecrawl/anydoc: its xlsx fuzz
target crashed on an integer-overflow panic inside calamine (a
dependency), root-caused, fixed, and filed upstream (tafia/calamine#705).

Secrets are scrubbed from the fuzz env (env -u GH_TOKEN -u GH_GLOBAL
-u RESEND_*) immediately before compiling/running the target's own
code, since a malicious target's build.rs could otherwise exfiltrate
them.

Split out of #863 per triage: this PR is skill-scope only
(skills/vuln-scanner/SKILL.md + eyebrowlock.json). The paired
runtime-infrastructure changes (.github/workflows/aeon.yml,
scripts/stage-vuln-scanner.sh, scripts/skill_mode.sh) are in a
separate follow-up PR for a maintainer to review, since they touch
the fork's trust boundary (Bash(cargo:*) grant, toolchain staging).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants