Repository navigation
texttopdf: range-check the column-width computation - #268
Open
ManasBagul23 wants to merge 1 commit into
Open
ManasBagul23 wants to merge 1 commit into
ManasBagul23 wants to merge 1 commit into
Conversation
write_prolog() derives ColumnWidth from the "columns" option
(doc->PageColumns), which only has a lower-bound check (>= 1) -- there
is no upper bound. The computation itself:
doc->ColumnWidth = (doc->SizeColumns - doc->ColumnGutter *
(doc->PageColumns - 1)) / doc->PageColumns;
is done entirely in plain "int" arithmetic. A large PageColumns value
(e.g. columns=429496747) overflows the ColumnGutter * (PageColumns - 1)
product (and/or the final division), wrapping what should be a deeply
negative/rejected result into a small positive ColumnWidth instead.
That bogus ColumnWidth then corrupts the page-column write-site math in
cfFilterTextToPDF(): "i = column + page_column * (ColumnWidth +
ColumnGutter)" is used to index doc->Page[line][i] directly with no
further bound check, and page_column is only bounds-checked against the
(also wrong, since it mirrors the same user input) doc->PageColumns.
With ColumnWidth wrongly small, page_column can climb far past what
doc->SizeColumns cells were actually allocated for, producing a
heap-buffer-overflow write.
Do the gutter product and the resulting column width in "long long"
first, and reject the page (matching write_prolog()'s existing error
convention -- log and return 1, already used a few lines above for a
bad charset file) if the real result doesn't fit between 1 and
doc->SizeColumns, instead of silently using a wrapped value.
Verified by tracing doc->ColumnWidth's only two downstream uses that
matter for allocation safety: the "page_column >= doc->PageColumns"
page-wrap check (which the corrupted PageColumns value itself doesn't
protect) and the direct doc->Page[line][i] write in
cfFilterTextToPDF() circa line 1404, confirming the invariant
page_column * (ColumnWidth + ColumnGutter) + column < SizeColumns only
holds when ColumnWidth is computed without overflow. No C build
toolchain available locally to compile-check.
Fixes OpenPrinting#195
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
write_prolog()derivesColumnWidthfrom the "columns" option(
doc->PageColumns), which only has a lower-bound check (>= 1) — there isno upper bound. The computation itself:
is done entirely in plain
intarithmetic. A largePageColumnsvalue(e.g.
columns=429496747) overflows theColumnGutter * (PageColumns - 1)product (and/or the final division), wrapping what should be a deeply
negative/rejected result into a small positive
ColumnWidthinstead.That bogus
ColumnWidththen corrupts the page-column write-site math incfFilterTextToPDF():i = column + page_column * (ColumnWidth + ColumnGutter)is used to indexdoc->Page[line][i]directly with nofurther bound check, and
page_columnis only bounds-checked against the(also wrong, since it mirrors the same user input)
doc->PageColumns. WithColumnWidthwrongly small,page_columncan climb far past whatdoc->SizeColumnscells were actually allocated for, producing aheap-buffer-overflow write.
Fixes #195
Fix
Do the gutter product and the resulting column width in
long longfirst,and reject the page (matching
write_prolog()'s existing error convention— log and return 1, already used a few lines above for a bad charset file)
if the real result doesn't fit between 1 and
doc->SizeColumns, instead ofsilently using a wrapped value.
Verified
No C build toolchain available locally to compile-check. Verified by
tracing
doc->ColumnWidth's only two downstream uses that matter forallocation safety: the
page_column >= doc->PageColumnspage-wrap check(which the corrupted
PageColumnsvalue itself doesn't protect) and thedirect
doc->Page[line][i]write incfFilterTextToPDF(), confirming theinvariant
page_column * (ColumnWidth + ColumnGutter) + column < SizeColumnsonly holds whenColumnWidthis computed without overflow.