GHSA-5h23-36rv-pm65

Dashboard / Vulnerabilities / GHSA-5h23-36rv-pm65

GHSA-5h23-36rv-pm65

Summary: Excelize: GetStyle panics on a negative fillId, borderId or fontId in styles.xml

Details: ## Summary `File.GetStyle` indexes the fill, border and font tables with values taken straight out of `xl/styles.xml`, and the conditions gating those lookups check only the upper bound. A workbook whose `cellXfs` entry carries `fillId="-1"`, `borderId="-1"` or `fontId="-1"` reaches a negative slice index and panics. excelize has no `recover()`, so the panic leaves `GetStyle` and takes the calling process with it. Same defect class as GHSA-48hm-4h8j-58fg, the negative shared-string index, in a different file. That one was fixed by adding the missing lower bound; these three sites still lack it. ## Where it is `styles.go`, in `GetStyle`, lines 1683, 1686 and 1689: ```go xf := s.CellXfs.Xf[idx] if extractStyleCondFuncs["fill"](xf, s) { f.extractFills(s.Fills.Fill[*xf.FillID], s, style) } if extractStyleCondFuncs["border"](xf, s) { f.extractBorders(s.Borders.Border[*xf.BorderID], s, style) } if extractStyleCondFuncs["font"](xf, s) { style.Font = extractFont(s.Fonts.Font[*xf.FontID]) } ``` The conditions, at lines 1171 to 1185, bound only the top: ```go "fill": func(xf xlsxXf, s *xlsxStyleSheet) bool { return (xf.ApplyFill == nil || (xf.ApplyFill != nil && *xf.ApplyFill)) && xf.FillID != nil && s.Fills != nil && *xf.FillID < len(s.Fills.Fill) }, ``` `*xf.FillID < len(...)` is satisfied by any negative value. `FillID`, `BorderID` and `FontID` are `*int` unmarshalled directly from the `fillId`, `borderId` and `fontId` attributes, so the value is whatever the file says. The border and font conditions have the same shape. The correct pattern is already in the same function, six lines above at 1677: ```go if idx < 0 || s.CellXfs == nil || len(s.CellXfs.Xf) <= idx { return style, newInvalidStyleID(idx) } ``` and again at 1783. So the style index itself is guarded on both sides while the three ids it leads to are not. ## Proof of concept Executed against `master` at `f98df08`, which is the merge of #2366, so this is current rather than historical. The harness builds a normal styled workbook with excelize, rewrites one attribute of the `cellXfs` entry inside the zip to `-1`, reopens it and calls `GetCellStyle` then `GetStyle`: ``` control (all >= 0) ok style=true err=<nil> fillId=-1 PANIC: runtime error: index out of range [-1] borderId=-1 PANIC: runtime error: index out of range [-1] fontId=-1 PANIC: runtime error: index out of range [-1] ``` The control confirms the harness reads a valid file correctly, so the three panics are the negative ids rather than a broken fixture. Worth mentioning because my first attempt at this harness patched only `fillId` and appeared to show the other two were fine; they are not, the replacement had simply missed them. ## Impact Any application that opens an untrusted `.xlsx` and reads cell styling crashes. `GetStyle` is reached through `GetCellStyle` and from the style-copying and rendering paths, so it sits on the ordinary read path. With no `recover()` anywhere in excelize, a CLI or worker exits and a service returns 500 per request unless the caller installed its own recovery middleware. No memory corruption and no information disclosure. Availability only. ## Suggested fix Add the lower bound to each of the three conditions, matching what line 1677 already does for the style index: ```go *xf.FillID >= 0 && *xf.FillID < len(s.Fills.Fill) ``` and the same for `BorderID` and `FontID`. Three one-line changes in `extractStyleCondFuncs`.

References: https://github.com/qax-os/excelize/security/advisories/GHSA-5h23-36rv-pm65, https://github.com/qax-os/excelize/pull/2367, https://github.com/qax-os/excelize/commit/ae2113b410e51f6a141c396a59eda8c42b91bc22, https://github.com/qax-os/excelize