Skip to content

This closes #GHSA-5h23-36rv-pm65, fix panic on negative style index - #2367

Merged
xuri merged 2 commits into
qax-os:masterfrom
arpitjain099:fix/getstyle-negative-style-index
Jul 31, 2026
Merged

This closes #GHSA-5h23-36rv-pm65, fix panic on negative style index#2367
xuri merged 2 commits into
qax-os:masterfrom
arpitjain099:fix/getstyle-negative-style-index

Conversation

@arpitjain099

Copy link
Copy Markdown
Contributor

The fill, border and font conditions in extractStyleCondFuncs bounded only the top of the range:

*xf.FillID < len(s.Fills.Fill)

Any negative value satisfies that, so a cellXfs entry carrying fillId="-1", borderId="-1" or fontId="-1" passed the guard and GetStyle then indexed the fill, border or font table with -1. Reading cell styling out of an untrusted workbook panicked with index out of range [-1].

GetStyle already checks the low end for the style index itself a few lines above, so this just brings the three table lookups in line with it:

if idx < 0 || s.CellXfs == nil || len(s.CellXfs.Xf) <= idx {

Behaviour for valid workbooks is unchanged. A negative id now makes the condition false and the style keeps its default, which is what already happened for an out-of-range positive id.

The regression test writes the styles part directly so the negative attribute goes through the real XML unmarshal rather than being set on the struct, and covers all three ids. Reverting the styles.go change makes it panic at styles.go:1683, and the full suite passes with the change in.

Same shape as the shared string index fix in #2366, in a different file.

The fill, border and font conditions in extractStyleCondFuncs bounded
only the top of the range, so a cellXfs entry carrying fillId="-1",
borderId="-1" or fontId="-1" satisfied the guard and GetStyle then
indexed the fill, border or font table with a negative value. Reading
cell styling from an untrusted workbook panicked with index out of
range [-1].

Add the lower bound to all three, matching the check GetStyle already
applies to the style index itself a few lines above.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.62%. Comparing base (0f7445b) to head (6e6f5f3).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2367   +/-   ##
=======================================
  Coverage   99.62%   99.62%           
=======================================
  Files          32       32           
  Lines       26979    26979           
=======================================
  Hits        26877    26877           
  Misses         53       53           
  Partials       49       49           
Flag Coverage Δ
unittests 99.62% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@xuri xuri added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 30, 2026
@xuri xuri added this to v2.11.1 Jul 30, 2026
@xuri xuri moved this to Security in v2.11.1 Jul 30, 2026

@xuri xuri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution.

@xuri
xuri merged commit ae2113b into qax-os:master Jul 31, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

Status: Security

Development

Successfully merging this pull request may close these issues.

2 participants