Closed Bug 1349310 (CVE-2017-7778) Opened 3 years ago Closed 3 years ago

Graphite2 lz4::decompress out of bounds write

Categories

(Core :: Graphics: Text, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- wontfix
firefox-esr52 54+ fixed
firefox53 --- wontfix
firefox54 + fixed
firefox55 + fixed

People

(Reporter: hofusec, Assigned: jfkthame, NeedInfo)

References

Details

(Keywords: csectype-bounds, sec-high, testcase, Whiteboard: [gfx-noted][post-critsmash-triage][adv-main54+][adv-esr52.2+])

Attachments

(4 files, 1 obsolete file)

Attached 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"
Attached file firefox.gdb.log
Group: core-security → gfx-core-security
Martin, can you please take a look?
Flags: needinfo?(martin_hosken)
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)
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: 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)
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)
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+
Attachment #8856570 - Attachment is obsolete: true
Here's the corresponding patch for ESR52 consideration.
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?
Attachment #8865387 - Flags: approval-mozilla-esr52?
Track 54+ as sec-high.
Bug 1352745 and bug 1352747 should also be fixed by the update here.
https://hg.mozilla.org/mozilla-central/rev/6422effd1d9a
Status: ASSIGNED → RESOLVED
Closed: 3 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.