1223002 - Graphite 2 instruction parameter validation bypass

Status: VERIFIED FIXED

(Core :: Graphics: Text, defect)

Description

[Holger Fuhrmannek]

Attachment: poc.zip

With special CNTXT_ITEM instructions its possible to circumvent the validation of parameter for internal instructions in the Graphite smart font system.
This gives a huge attack surface which can exploited for example to a reliable jump to an arbitrary address with a special crafted font.
I'm sure its also possible to bypass ASLR with this bug.

With the current nightly 64bit and windows 7 the poc jumps to 0x4141414141414141.

Comment 1

[Tyson Smith [:tsmith]]

Attachment: call_stack.txt

Comment 2

[Jonathan Kew [:jfkthame]]

Martin, here's one that sounds like it wants fixing ASAP.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: Cherry-pick post-1.3.4 bugfixes for graphite2 from upstream

This is fixed upstream, so we just need to take the most recent fixes from there.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Always call ReleaseGrFace to balance GetGrFace, even if the face is null

Running the testcase here revealed a bug on our side when gr_face initialization fails; we end up asserting on shutdown. So let's fix that while we're here.

Comment 5

[martin_hosken]

Given the fixes that have gone into 1.3.4 and this following bug, I would recommend applying these to gecko 44 and even 43. But that's you guys' call :)

Comment 6

[John Daggett (:jtd)]

Comment on attachment 8688503 [details] [diff] [review]
Always call ReleaseGrFace to balance GetGrFace, even if the face is null

Review of attachment 8688503 [details] [diff] [review]:
-----------------------------------------------------------------

Hmmm, calling release on a null face seems sort of weird. So, r+ with a comment explaining the necessity of always calling release.

Comment 7

[Jonathan Kew [:jfkthame]]

I've confirmed that the testcase attached here crashes current FF43 (Beta) and 44 (Developer Edition) releases.[1] It doesn't appear to hurt FF42 (Release) or 38esr, though that doesn't necessarily mean they are immune... it could be that older graphite2 releases would be vulnerable to a slight variation of the example.

[1] E.g. https://crash-stats.mozilla.com/report/index/6ebd7243-196b-4de0-b661-d7c4a2151118

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 8688500 [details] [diff] [review]
Cherry-pick post-1.3.4 bugfixes for graphite2 from upstream

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The patch doesn't directly show how to construct an exploit, but does hint at a specific part of the font tables that might be worth exploring.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No. (We could land the testcase as a crashtest later.)

Which older supported branches are affected by this flaw?
FF43 and FF44.

If not all supported branches, which bug introduced the flaw?
Bug 1200098 (major update of graphite2 library).

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Aurora and Beta are currently on graphite2-1.3.3. To backport, we should take a combined patch with bug 1220591 (update to 1.3.4) plus this bug (post-1.3.4 fixes). I'll post a combined patch.

How likely is this patch to cause regressions; how much testing does it need?
Minimal risk of regressions; this just adds error-checking to the graphite2 code so as to reject bad font tables before anything bad happens.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: Update graphite2 to release 1.3.4 plus post-release bugfixes from upstream

This is the combined backport of bug 1220591 (update to 1.3.4) and this bug, for aurora and beta branches.

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 8689196 [details] [diff] [review]
Update graphite2 to release 1.3.4 plus post-release bugfixes from upstream

Approval Request Comment

(See sec-approval comment above.)

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8688500 [details] [diff] [review]
Cherry-pick post-1.3.4 bugfixes for graphite2 from upstream

sec-approval+ for trunk and other approvals given.

Comment 12

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37722da27765ccd0ed63e7dc4f83241ef156f39c
Bug 1223002 - Cherry-pick post-1.3.4 bugfixes for graphite2 from upstream. r=jdaggett

Comment 13

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/460116fc714a
https://hg.mozilla.org/releases/mozilla-beta/rev/dca2389bacea

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/37722da27765

Comment 15

[Jonathan Kew [:jfkthame]]

Oops, I failed to push the second patch here when landing to inbound. (This part isn't a security issue, it's just so we don't mistakenly think we've leaked a face that we actually failed to create.) Coming up shortly...

Comment 16

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef2ee7ebb2503f1af7699db407dc09777184243a
Bug 1223002 - Always call ReleaseGrFace to balance GetGrFace, even if the face is null. r=jdaggett

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/460116fc714a

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ef2ee7ebb250

Comment 19

[Bogdan Maris, Desktop QA]

Reproduced the initial crash using old Nightly x64 (2015-11-08) on Ubuntu 14.04 64bit (http://pastebin.com/RGLTwbmN console output) and Windows 7 64-bit (bp-08f1ac46-64d5-4658-94bd-fbc582151207), verified that the crash does not reproduce anymore using latest Nightly 45.0a1, latest Developer Edition 44.0a2 and Firefox 43 beta 9 across platforms (Windows 7 64-bit, Ubuntu 14.04 64-bit and Mac OS X 10.11).