If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Invalid offsets value in TTs loca table leads to crash [@TTrueTypeFontHandler::ParseOutline]

RESOLVED FIXED

Status

()

Core
Graphics
--
critical
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: posidron, Assigned: jfkthame)

Tracking

(Blocks: 1 bug, {crash})

Trunk
x86_64
Mac OS X
crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: rdar://8339272, crash signature)

Attachments

(6 attachments)

(Reporter)

Description

7 years ago
Created attachment 458620 [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: loca
Table offset: 0x10c
Value offset: +0xe
Value: 40 00

Firefox release build is not affected.
Safari and FontBook are not affected.

Load the provided html file.
(Reporter)

Comment 1

7 years ago
Created attachment 458621 [details]
callstack
(Reporter)

Updated

7 years ago
Summary: Invalid offset in TTs loca table leads to crash [@TTrueTypeFontHandler::ParseOutline] → Invalid offsets value in TTs loca table leads to crash [@TTrueTypeFontHandler::ParseOutline]
(Assignee)

Comment 2

7 years ago
Crash seems to be sporadic; I was able to reproduce this once, but re-trying with the same build it didn't crash again.

With Firefox 3.6.6 (release version), I didn't get a crash but the font failed to render at all; window remained blank.
(Assignee)

Comment 3

7 years ago
Created attachment 458652 [details] [diff] [review]
patch, v1 - check loca table offsets for validity

This will cause us to reject the font in the testcase as invalid.
Assignee: nobody → jfkthame
Attachment #458652 - Flags: review?(jdaggett)
blocking2.0: --- → ?
Keywords: crash
blocking2.0: ? → final+
(Assignee)

Comment 4

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

Updated

7 years ago
Attachment #458953 - Flags: review?(jdaggett) → review+

Comment 5

7 years ago
Comment on attachment 458652 [details] [diff] [review]
patch, v1 - check loca table offsets for validity

+struct MaxpTable {
+    AutoSwap_PRUint32    version; // CFF: 0x00005000; TrueType: 0x00010000
+    AutoSwap_PRUint16    numGlyphs;
+// truetype version has additional fields that we don't currently use
+};

+            maxpOffset = dirEntry->offset;
+            maxpLen = dirEntry->length;
+            if (maxpLen < sizeof(MaxpTable)) {
+                NS_WARNING("invalid font (maxp table length)");
+                return PR_FALSE;
+            }

Hmmm, seems like it would be pretty easy to add fields to the table
and quietly break CFF font loading.  Maybe call this MaxpTableHeader?
At least a comment noting the dependency of the logic in
ValidateSFNTHeaders() on the fact that MaxpTable only has the first
two fields.

+        if (aLocaLen < (aNumGlyphs + 1) * sizeof(PRUint16)) {
+            return PR_FALSE;
+        }

Although I think sizeof will always have a return type of size_t and
so will coerce the right side of this comparison to a 32-bit value, it
might be better to explicitly cast the (aNumGlyphs + 1) value to
PRUint32, otherwise I could imagine situations where aNumGlyphs =
0x8000 gets you into trouble because an overflow occurs before an
upcast occurs.
Attachment #458652 - Flags: review?(jdaggett) → review+
(Assignee)

Comment 6

7 years ago
Pushed patch (with changes from comment #5) and crashtest:
http://hg.mozilla.org/mozilla-central/rev/a1f30810ad0c
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Reporter)

Comment 7

7 years ago
Reopened - because of new testcase which crashes at the same location.
Whiteboard: rdar://8339272
(Reporter)

Comment 8

7 years ago
Created attachment 468072 [details]
testcase: cmap,glyf

Tag: b'glyf' Checksum: 0x0015049d Offset:  1504/0x000005e0 Length: 25280
Tag: b'cmap' Checksum: 0x0000ddd1 Offset:   268/0x0000010c Length: 1236

Table: b'cmap'
Number of replaced values: 8
Offset:    80/0x000050	Value: ['80', '00', '00', '00']
Offset:   305/0x000131	Value: ['7f', 'ff', 'ff', 'ff']
Offset:   415/0x00019f	Value: ['20', '00']
Offset:   449/0x0001c1	Value: ['00', '00', '00', '00', '00', '00', '00', '01']
Offset:   470/0x0001d6	Value: ['ff']
Offset:   918/0x000396	Value: ['20', '00']
Offset:  1032/0x000408	Value: ['ff', 'ff', 'ff', 'ff']
Offset:  1149/0x00047d	Value: ['7f', 'ff', 'ff', 'ff']
Table: b'glyf'
Number of replaced values: 5
Offset:  6747/0x001a5b	Value: ['7f', 'ff']
Offset: 12306/0x003012	Value: ['7f', 'ff', 'ff', 'ff']
Offset: 13923/0x003663	Value: ['20', '00']
Offset: 17124/0x0042e4	Value: ['20', '00']
Offset: 21776/0x005510	Value: ['00', '00', '00', '00', '00', '00', '00', '01']

Load the provided html file.

Note: FontBook is also affected.
(Reporter)

Comment 9

7 years ago
Created attachment 468073 [details]
callstack: cmap,glyf
(Reporter)

Updated

7 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 10

7 years ago
This will be fixed by the OTS sanitizer (bug 527276).
Depends on: 527276
(Assignee)

Comment 11

7 years ago
Now that OTS has landed, this no longer crashes; the sanitizer safely rejects the invalid font.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Crash Signature: [@TTrueTypeFontHandler::ParseOutline]
(Reporter)

Updated

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