Bug 1349310 (CVE-2017-7778)

Graphite2 lz4::decompress out of bounds write

RESOLVED FIXED in Firefox -esr52

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: hofusec, Assigned: jfkthame, NeedInfo)

Tracking

({csectype-bounds, sec-high, testcase})

unspecified
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox-esr45 wontfix, firefox-esr5254+ fixed, firefox53 wontfix, firefox54+ fixed, firefox55+ fixed)

Details

(Whiteboard: [gfx-noted][post-critsmash-triage][adv-main54+][adv-esr52.2+])

Attachments

(4 attachments, 1 obsolete attachment)

Reporter

Description

2 years ago
Posted file grlz432bit.poc.zip
On 32 bit systems an out of bounds write read/write can occur during the decompressing of a lz4 compressed graphite font table.

valgrind log (with gr2fonttest tool):

Invalid read of size 16
   at 0x4048550: lz4::decompress(void const*, unsigned int, void*, unsigned int) (in /src/libgraphite2.so.3.0.1)
 Address 0x685218d is 16,843,109 bytes inside a block of size 16,843,117 alloc'd
   at 0x402D17C: malloc 
   by 0x4049567: graphite2::Face::Table::decompress() (in /src/libgraphite2.so.3.0.1)

Invalid read of size 16
   at 0x4048555: lz4::decompress(void const*, unsigned int, void*, unsigned int) (in /src/libgraphite2.so.3.0.1)
 Address 0x685219d is 8 bytes after a block of size 16,843,117 alloc'd
   at 0x402D17C: malloc 
   by 0x4049567: graphite2::Face::Table::decompress() (in /src/libgraphite2.so.3.0.1)

Invalid write of size 8
   at 0x404855A: lz4::decompress(void const*, unsigned int, void*, unsigned int) (in /src/libgraphite2.so.3.0.1)
 Address 0x6852195 is 0 bytes after a block of size 16,843,117 alloc'd
   at 0x402D17C: malloc 
   by 0x4049567: graphite2::Face::Table::decompress() (in /src/libgraphite2.so.3.0.1)

Invalid write of size 8
   at 0x404855E: lz4::decompress(void const*, unsigned int, void*, unsigned int) (in /src/libgraphite2.so.3.0.1)
 Address 0x685219d is 8 bytes after a block of size 16,843,117 alloc'd
   at 0x402D17C: malloc 
   by 0x4049567: graphite2::Face::Table::decompress() (in /src/libgraphite2.so.3.0.1)

Invalid read of size 16
   at 0x4048567: lz4::decompress(void const*, unsigned int, void*, unsigned int) (in /src/libgraphite2.so.3.0.1)
 Address 0x68521ad is 16,843,141 bytes inside a block of size 16,846,784 in arena "client"
Reporter

Comment 1

2 years ago
Posted file firefox.gdb.log
Group: core-security → gfx-core-security
Martin, can you please take a look?
Flags: needinfo?(martin_hosken)

Comment 3

2 years ago
Fixed? in 75b83cd
Flags: needinfo?(martin_hosken)
Holger, can you confirm that the upstream fix works for you too?
Flags: needinfo?(hofusec)
We should probably find an owner for this.
Flags: needinfo?(milan)
Priority: -- → P3
Whiteboard: [gfx-noted]
Looks like we have it moving.
Flags: needinfo?(milan)
Depends on: CVE-2017-7772
> Looks like we have it moving.

We need someone to own landing the upstream fix, the regression fixes (see bugs Tyson is going to add to the dependency list), and figure out what release branches might need this also.
Flags: needinfo?(milan)
Depends on: CVE-2017-7773
Jonathan, have you done this for graphite in the past?
Flags: needinfo?(milan) → needinfo?(jfkthame)
Assignee

Comment 9

2 years ago
This updates graphite2 to the latest upstream master as of today (commit 8afc7d00819598663745b4a9f838533601fd0e32), which includes fixes for several recently-reported fuzz bugs. We'll need to go through the various reported bugs to confirm whether each of them is covered by this, and resolve as appropriate; but I don't see any value in our trying to cherry-pick individual commits for each bug rather than just updating to "latest upstream", as there haven't been any significant feature changes recently, it's just bug-fixes.
Attachment #8856570 - Flags: review?(milan)
Assignee

Updated

2 years ago
Assignee: nobody → jfkthame
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8856570 - Flags: review?(milan) → review+
Flags: needinfo?(jfkthame)
Jonathan, are we good to ask for security approval?
Flags: needinfo?(jfkthame)
Assignee

Comment 11

2 years ago
This supersedes the patch above, as there is now a new upstream release we can take instead of an arbitrary commit. We should take this on all supported channels, as it includes fixes for a number of potential security issues uncovered by ongoing fuzzing.
Attachment #8865387 - Flags: review?(milan)
Assignee

Comment 12

2 years ago
Comment on attachment 8865387 [details] [diff] [review]
Update graphite2 to release 1.3.10

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Not exactly, though it's clear a number of issues (input validation, bounds checks, etc) in the graphite library are being fixed.

Which older supported branches are affected by this flaw?
All

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should backport without change to beta. Esr52 is currently on release 1.3.8 + cherry-picked fixes, so a slightly different patch will be needed, but trivial to create.

How likely is this patch to cause regressions; how much testing does it need?
Minimal risk, this update just consists of fixes for potential security issues, not new features.
Flags: needinfo?(jfkthame)
Attachment #8865387 - Flags: sec-approval?
Attachment #8865387 - Flags: approval-mozilla-esr52?
Attachment #8865387 - Flags: approval-mozilla-beta?
Attachment #8865387 - Flags: review?(milan) → review+
Duplicate of this bug: 1363063
Comment on attachment 8865387 [details] [diff] [review]
Update graphite2 to release 1.3.10

sec-approval=dveditz, a=dveditz for beta. Let's land this so we can get as much real-world testing as possible.
Attachment #8865387 - Flags: sec-approval?
Attachment #8865387 - Flags: sec-approval+
Attachment #8865387 - Flags: approval-mozilla-beta?
Attachment #8865387 - Flags: approval-mozilla-beta+
Assignee

Updated

2 years ago
Attachment #8856570 - Attachment is obsolete: true
Assignee

Comment 16

2 years ago
Here's the corresponding patch for ESR52 consideration.
Assignee

Comment 17

2 years ago
Comment on attachment 8865663 [details] [diff] [review]
[esr52 patch] Update graphite2 to release 1.3.10

Just moving the approval flag from the trunk patch to this version, which is the one that applies to the esr52 channel.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: potential vulnerabilities via webfonts
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): minimal risk
String or UUID changes made by this patch: none
Attachment #8865663 - Flags: approval-mozilla-esr52?
Assignee

Updated

2 years ago
Attachment #8865387 - Flags: approval-mozilla-esr52?
Track 54+ as sec-high.
Assignee

Comment 19

2 years ago
Bug 1352745 and bug 1352747 should also be fixed by the update here.
Assignee

Updated

2 years ago
Assignee

Updated

2 years ago
Assignee

Updated

2 years ago
Assignee

Updated

2 years ago
https://hg.mozilla.org/mozilla-central/rev/6422effd1d9a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Group: gfx-core-security → core-security-release
Flags: sec-bounty?
Comment on attachment 8865663 [details] [diff] [review]
[esr52 patch] Update graphite2 to release 1.3.10

sec-high, esr52.2+
Attachment #8865663 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Flags: sec-bounty? → sec-bounty+
Hi Holger, can you verify that this has been fixed?
Flags: needinfo?(hofusec)
Whiteboard: [gfx-noted] → [gfx-noted][post-critsmash-triage]
Whiteboard: [gfx-noted][post-critsmash-triage] → [gfx-noted][post-critsmash-triage][adv-main54+][adv-esr52.2+]
Alias: CVE-2017-7778
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.