1349310 - (CVE-2017-7778) Graphite2 lz4::decompress out of bounds write

Status: RESOLVED FIXED

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

Description

[Holger Fuhrmannek]

Attachment: 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"

Comment 1

[Holger Fuhrmannek]

Attachment: firefox.gdb.log

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Martin, can you please take a look?

Comment 3

[martin_hosken]

Fixed? in 75b83cd

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Holger, can you confirm that the upstream fix works for you too?

Comment 5

[Bas Schouten (:bas.schouten)]

We should probably find an owner for this.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Looks like we have it moving.

Comment 7

[Daniel Veditz [:dveditz]]

> 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.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Jonathan, have you done this for graphite in the past?

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: Update graphite2 to latest upstream code

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.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Jonathan, are we good to ask for security approval?

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: Update graphite2 to release 1.3.10

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.

Comment 12

[Jonathan Kew [:jfkthame]]

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.

Comment 14

[Daniel Veditz [:dveditz]]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6422effd1d9a
https://hg.mozilla.org/releases/mozilla-beta/rev/798ee0e6f73e

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: [esr52 patch] Update graphite2 to release 1.3.10

Here's the corresponding patch for ESR52 consideration.

Comment 17

[Jonathan Kew [:jfkthame]]

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

Comment 18

[Gerry Chang [:gchang]]

Track 54+ as sec-high.

Comment 19

[Jonathan Kew [:jfkthame]]

Bug 1352745 and bug 1352747 should also be fixed by the update here.

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/6422effd1d9a

Comment 21

[Julien Cristau [:jcristau]]

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

sec-high, esr52.2+

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/81b3ce7d37b3

Comment 23

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi Holger, can you verify that this has been fixed?