Invalid values in GPOS table leading to 'division by zero' [@hb_sanitize_context_t::check_array]

RESOLVED FIXED

Status

()

--
critical
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: posidron, Assigned: jfkthame)

Tracking

(Blocks: 1 bug)

Trunk
x86_64
Mac OS X
Points:
---

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(4 attachments)

(Reporter)

Description

8 years ago
Created attachment 458648 [details]
testcase

Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; en-US; rv:2.0b2pre) Gecko/20100718 Minefield/4.0b2pre

Table: GPOS
Table Offset: 0x11c
Value Offset: +0x5d
Values: ff c4 40 0f

Load the provided html file
(Reporter)

Comment 1

8 years ago
Created attachment 458649 [details]
callstack
(Assignee)

Comment 2

8 years ago
Created attachment 458660 [details] [diff] [review]
patch, v1 - avoid potential divide-by-zero in hb sanitizer
Assignee: nobody → jfkthame
Attachment #458660 - Flags: review?(jdaggett)
blocking2.0: --- → ?
blocking2.0: ? → final+

Comment 3

8 years ago
Hmmm, isn't record_size = 0 and len > 0 an overflow condition?  Not sure if that's possible to get but it seems like you might allow access to the first element of an array in that case, which would be out-of-bounds.
(Assignee)

Comment 4

8 years ago
(In reply to comment #3)
> Hmmm, isn't record_size = 0 and len > 0 an overflow condition? 

Not in the sense of this check; the "overflows" flag here relates to the computation of (record_size * len) that's going to be done when calling check_range(), which could otherwise suffer integer overflow if the values were wild.

> Not sure if
> that's possible to get but it seems like you might allow access to the first
> element of an array in that case, which would be out-of-bounds.

If record_size = 0, the array elements are nonexistent, in which case any "access" to them is an error elsewhere. More specifically, the case of record_size = 0 arises in GPOS ValueRecords in the (degenerate) case where NONE of the bits in ValueFormat was set, indicating that none of the optional fields is in fact present. So in that case the GPOS code should not be attempting to look at any of the optional fields.
(Assignee)

Comment 5

8 years ago
Created attachment 458955 [details] [diff] [review]
add the testcase as a crashtest
Attachment #458955 - Flags: review?(jdaggett)

Updated

8 years ago
Attachment #458660 - Flags: review?(jdaggett) → review+

Updated

8 years ago
Attachment #458955 - Flags: review?(jdaggett) → review+
(Assignee)

Comment 6

8 years ago
Pushed patch + crashtest:
http://hg.mozilla.org/mozilla-central/rev/0f5fc40c6a0f

(This patch has also landed in upstream harfbuzz-ng.)
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Reporter)

Updated

7 years ago
Blocks: 750695
You need to log in before you can comment on or make changes to this bug.