1248876 - (CVE-2016-1977) Graphite2 Machine::Code::decoder::analysis::set_ref stack out of bounds bit set

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Holger Fuhrmannek]

Its possible to trigger an out of bounds bit set with special crafted graphite fonts. This gives the ability to corrupt the stack in various ways.
I have tested the pocs with the current nightly asan build. Poc 1 shows the out of bounds access, Poc 2 shows a use after free which can for example triggered with the vulnerability.

Comment 1

[Holger Fuhrmannek]

Attachment: poc.zip

Comment 2

[Holger Fuhrmannek]

Attachment: asanlogpoc1.txt

Comment 3

[Holger Fuhrmannek]

Attachment: asanlogpoc2.txt

Comment 4

[Daniel Veditz [:dveditz]]

tyson: is this a dupe of any of yours? Might be two separate bugs since the symptoms are different.

Comment 5

[Tyson Smith [:tsmith]]

(In reply to Daniel Veditz [:dveditz] from comment #4)
> tyson: is this a dupe of any of yours? Might be two separate bugs since the
> symptoms are different.

This does not match any of my bugs. Also I agree these look like two different bugs.

Comment 6

[martin_hosken]

This one is fun. Have we found a bug in gcc?

If one builds graphite with RelWithDebInfo (or -g -O3) and runs say, gr2fonttest against this font (any test text) and then break in Pass::readRules, it says m_numRules == 2 but the main loop in there will try to run 3 times! The loop counter n is optimised out. If one builds debug or enables the #if code in there, then it runs the appropriate 2 times.

This is easy enough to patch over (enable that and a couple of other #ifndef NDEBUG lines to support rule->idx). But, since it's a public holiday here today, I would like another day to dig into this. If you are in a hurry, let me know and I'll patch it up for you.

Comment 7

[Jonathan Kew [:jfkthame]]

(In reply to martin_hosken from comment #6)
> This one is fun. Have we found a bug in gcc?
> 
> If one builds graphite with RelWithDebInfo (or -g -O3) and runs say,
> gr2fonttest against this font (any test text) and then break in
> Pass::readRules, it says m_numRules == 2 but the main loop in there will try
> to run 3 times!

I suspect if you're stepping in gdb, you may be seeing weird control flow due to optimisation that makes it difficult to count iterations...

FWIW, I tried running "gr2fonttest poc/poc1.ttf" on OS X (clang, non-debug build), and it dies with a segfault during the second iteration of the loop. (As confirmed by adding printf's at the top and bottom of the loop.) Not every time, though; on some runs, it completes with no problems (two full iterations, then exits cleanly). Which makes me wonder if there could be an uninitialized value involved somewhere.

Comment 8

[martin_hosken]

Well the debugger shows it decrementing r each time and then r goes below m_rules which is when the segv happens. On linux inserting any print causes the loop to behave.

So I've refactored the loop slightly. Hopefully this will fix the problem, in 231d35a02d59fcd613c600d3e369695d24ca3b34. Let's see if OSX is happy.

Comment 9

[Jonathan Kew [:jfkthame]]

(In reply to martin_hosken from comment #8)
> Well the debugger shows it decrementing r each time and then r goes below
> m_rules which is when the segv happens. On linux inserting any print causes
> the loop to behave.
> 
> So I've refactored the loop slightly. Hopefully this will fix the problem,
> in 231d35a02d59fcd613c600d3e369695d24ca3b34. Let's see if OSX is happy.

Hmm. Well, so far I haven't seen that crash in a default OS X build.

**However** if I add a single simple
  printf("HERE\n");
as the first statement within the loop, then it segfaults on the second iteration.

And running it under valgrind tells me:

jkew$ valgrind ./gr2fonttest/gr2fonttest poc/poc1.ttf 
==8104== Memcheck, a memory error detector
==8104== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==8104== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==8104== Command: ./gr2fonttest/gr2fonttest poc/poc1.ttf
==8104== 
--8104-- run: /usr/bin/dsymutil "/Users/jkew/mozdev/graphite/BUILD/src/libgraphite2.3.0.1.dylib"
warning: no debug symbols in executable (-arch x86_64)
HERE
HERE
==8104== Invalid read of size 4
==8104==    at 0x257CD: graphite2::Pass::readRules(unsigned char const*, unsigned long, unsigned char const*, unsigned short const*, unsigned short const*, unsigned char const*, unsigned short const*, unsigned char const*, graphite2::Face&, graphite2::passtype, graphite2::Error&) (in /Users/jkew/mozdev/graphite/BUILD/src/libgraphite2.3.0.1.dylib)
==8104==    by 0x2515F: graphite2::Pass::readPass(unsigned char const*, unsigned long, unsigned long, graphite2::Face&, graphite2::passtype, unsigned int, graphite2::Error&) (in /Users/jkew/mozdev/graphite/BUILD/src/libgraphite2.3.0.1.dylib)
==8104==    by 0x2A9A5: graphite2::Silf::readGraphite(unsigned char const*, unsigned long, graphite2::Face&, unsigned int) (in /Users/jkew/mozdev/graphite/BUILD/src/libgraphite2.3.0.1.dylib)
==8104==    by 0x1DE47: graphite2::Face::readGraphite(graphite2::Face::Table const&) (in /Users/jkew/mozdev/graphite/BUILD/src/libgraphite2.3.0.1.dylib)
==8104==    by 0x126AF: (anonymous namespace)::load_face(graphite2::Face&, unsigned int) (in /Users/jkew/mozdev/graphite/BUILD/src/libgraphite2.3.0.1.dylib)
==8104==    by 0x12BDA: gr_make_file_face (in /Users/jkew/mozdev/graphite/BUILD/src/libgraphite2.3.0.1.dylib)
==8104==    by 0x10000297F: Parameters::testFileFont() const (in ./gr2fonttest/gr2fonttest)
==8104==    by 0x10000317D: main (in ./gr2fonttest/gr2fonttest)
==8104==  Address 0x1020417dc is not stack'd, malloc'd or (recently) free'd
==8104== 
==8104== 
==8104== Process terminating with default action of signal 11 (SIGSEGV)
==8104==  Access not within mapped region at address 0x1020417DC
==8104==    at 0x257CD: graphite2::Pass::readRules(unsigned char const*, unsigned long, unsigned char const*, unsigned short const*, unsigned short const*, unsigned char const*, unsigned short const*, unsigned char const*, graphite2::Face&, graphite2::passtype, graphite2::Error&) (in /Users/jkew/mozdev/graphite/BUILD/src/libgraphite2.3.0.1.dylib)
==8104==    by 0x2515F: graphite2::Pass::readPass(unsigned char const*, unsigned long, unsigned long, graphite2::Face&, graphite2::passtype, unsigned int, graphite2::Error&) (in /Users/jkew/mozdev/graphite/BUILD/src/libgraphite2.3.0.1.dylib)
==8104==    by 0x2A9A5: graphite2::Silf::readGraphite(unsigned char const*, unsigned long, graphite2::Face&, unsigned int) (in /Users/jkew/mozdev/graphite/BUILD/src/libgraphite2.3.0.1.dylib)
==8104==    by 0x1DE47: graphite2::Face::readGraphite(graphite2::Face::Table const&) (in /Users/jkew/mozdev/graphite/BUILD/src/libgraphite2.3.0.1.dylib)
==8104==    by 0x126AF: (anonymous namespace)::load_face(graphite2::Face&, unsigned int) (in /Users/jkew/mozdev/graphite/BUILD/src/libgraphite2.3.0.1.dylib)
==8104==    by 0x12BDA: gr_make_file_face (in /Users/jkew/mozdev/graphite/BUILD/src/libgraphite2.3.0.1.dylib)
==8104==    by 0x10000297F: Parameters::testFileFont() const (in ./gr2fonttest/gr2fonttest)
==8104==    by 0x10000317D: main (in ./gr2fonttest/gr2fonttest)

which suggests there's still something fragile here. I don't know why adding the printf() triggers this (obviously, it shouldn't...). Also, sprinkling more printf()s through the loop can perturb the behavior further, either leading to a slightly different valgrind complaint (in some cases, even an "invalid write of size 8") or suppressing it and running cleanly.

Ugh... this doesn't feel good.

Comment 10

[Jonathan Kew [:jfkthame]]

I dug into this a bit more, and AFAICT something is going wrong during the call of

new (m_codes+n*2-2) vm::Machine::Code(false, ac_begin, ac_end, r->preContext, r->sort, *m_silf, face, pt, &prog_pool_free);

which is to be assigned to r->action. What I see in lldb is that the value of |r| (which is held in register r13 during this function) gets modified during that |new|; specifically, it gets an extra bit set in it, so that it changes from

  r13 = 0x0000000100107328

before the |new| is called to

  r13 = 0x0000000102107328

afterwards (but before attempting the r->action=... assignment). As r13 is |r| here, corrupting it like this results in a segfault when trying to write to |r->action|.

So then the question becomes how r13 is getting stomped. Or not exactly stomped, just a single bit gets twiddled.

Inside the |new|, of course, general-purpose registers may be used for other things, and indeed I see r13 getting a completely unrelated value. But when we return, and it is restored from the stack, that pesky extra bit has been set.

Which seems to imply that something inside the |new| (or the vm::Machine::Code constructor) may be damaging the stored register value on the stack.

Comment 11

[Holger Fuhrmannek]

The root of the problem is a out of bounds bit set at "contexts[index + slotref].flags.referenced = true;" in Graphite2 Machine::Code::decoder::analysis::set_ref stack.
contexts is a member of the analysis struct which is a member of the decoder class which is allocated on the stack in the code class constructor. Therefore we have a stack corruption with the possibility to set a bit in stored registers or local variables on the stack. If a bit is already set nothing happen. Also Addresses can be different on different runs so not always a bit set results in a crash. The two pocs change a bit at a different offset so they lead to different crashes.

Comment 12

[martin_hosken]

There are two patches that I think fix this problem, The latter is 2b7f42152ec779c6f96ea744d83a7b7a6912473b which should reject the font.

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: Update graphite2 to upstream cset 2b7f42152ec779c6f96ea744d83a7b7a6912473b to pick up recent bugfixes

This is simply updating to the latest upstream code; includes fixes for a bunch of Tyson's recent fuzz-bugs, in addition to the issue here.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: Update graphite2 to upstream cset fb32e2e39ccd0980672f55052f75fc10f8736fb2 to pick up recent bugfixes

Comment 15

[Jonathan Kew [:jfkthame]]

Comment on attachment 8723200 [details] [diff] [review]
Update graphite2 to upstream cset fb32e2e39ccd0980672f55052f75fc10f8736fb2 to pick up recent bugfixes

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

* Given the patch, it's probably fairly easy to construct a malicious font that causes errors such buffer overruns on unpatched versions. Some flaws could lead to worse issues such as arbitrary code execution, but this would be harder to identify directly from the code fixes in 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 directly; we're just pulling a bunch of fixes that have landed upstream.

Which older supported branches are affected by this flaw?

* All

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

* n/a

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

* Straightforward to backport new library version.

How likely is this patch to cause regressions; how much testing does it need?

* Regressions seem unlikely, the changes are primarily simple checks for potential error conditions.



(Note that the changes here have all landed upstream, so to the extent that they point to possible vulnerabilities, they're already public.)

Comment 16

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

If I give sec-approval+ for this for this cycle, I'd want this on Aurora, Beta, and ESR38 as well. Before I give sec-approval+, I need release management to weigh in on taking this.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

The patch is too big for the end of the cycle. Can we just cherry-pick the security fixes?

Comment 18

[Jonathan Kew [:jfkthame]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #17)
> The patch is too big for the end of the cycle. Can we just cherry-pick the
> security fixes?

This is entirely a collection of security fixes. See upstream log at https://github.com/silnrsi/graphite/commits/master .... everything since release 1.3.5 is in response to the various fuzz-bugs reported recently.

(Note that the patch here resolves a whole set of bugs, not just this one -- see the open dependencies of bug 750695. I don't see any value in trying to cherry-pick certain of the changes while ignoring others.)

Comment 19

[Daniel Veditz [:dveditz]]

Note there are other consumers of graphite; whether we take the fixes or not they'll be public. The patches themselves are already public, just a matter of time before they're announced as security fixes.

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 46 since this is sec-high. Up to Sylvestre whether we take this in 45 (for the RC build on Monday).   Here are my thoughts though. I worry that if we do take it, regressions (and likely having to do an RC2 build), but if we don't take it, we end up pressured to put this in a dot release anyway in which case we'd still have to fix whatever problems might ensue. We also end up potentially having to rebuild 2 different ESRs if there are problems.    Leaving this not fixed will present us with the same problem in mid-beta for 46, but still with the pressure to do a dot release for 45 plus the 2 esrs.  
So, we are trying to weigh the risk of active exploits from the published fix, vs. the risk of regressions and rebuilding very late in the release cycle. 

Al and Dan, can we plan to fix this in 46 (without expecting a dot release for 45?) Seems like we will still want this in both ESRs.

Comment 21

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

If we don't fix it in 45 we *must* fix it in 46. The bugs are effectively public now from the graphite2 side though it isn't clear if anyone is looking or if there is a lot of detail public on their end. They did a whole bunch of security fixes and shipped a release. Graphite has been highlighted recently in the press as a security concern so this has a chance of blowing up into a chemspill easily (as you effectively note). These must go into ESR (both) as well when we ship them on mainline.

Comment 22

[martin_hosken]

Given the fuzz testing seems to be quietening down, I plan to release 1.3.6 on Monday. I would suggest that the risk of regressions in this release is very low in comparison to the number of security bug fixes. I can understand your jitters, but would suggest that you would probably want to cherry pick every single patch in the release anyway. There have been zero functionality changes in this release. Every single commit has been in response to the excellent work done by the Mozilla testing team.

Comment 23

[Jonathan Kew [:jfkthame]]

Comment on attachment 8723200 [details] [diff] [review]
Update graphite2 to upstream cset fb32e2e39ccd0980672f55052f75fc10f8736fb2 to pick up recent bugfixes

[Approval Request Comment]
See above. This updates us to latest graphite2 upstream release, which is a collection of fixes for recently-found fuzz bugs that represent potential security issues.

Requesting approval-mozilla-{aurora,beta,esr} to re-ping all the relevant release drivers. As I think :abillings is suggesting above, I think this should land right away on all branches (from 45 onward, plus ESR38).

Given that the patches included here are all public (upstream), there's no real benefit to holding them back. And if we ship 45 without them, there's a substantial risk someone else will draw attention to them during the next 6 weeks (maybe complete with a public exploit), and we'll have to chemspill anyway.


User impact if declined: possibility for malicious webfonts to crash the browser, perhaps achieve arbitrary code execution, etc.

Fix Landed on Version: approval pending for all branches

Risk to taking this patch (and alternatives if risky): Minimal risk. Each of the changes is a small, localized fix to defend against corrupt or malicious font data. Note also that as the fixes here are specifically in the graphite2 library, there is no risk of regressing any other aspect of behavior; content that does not use graphite fonts will never reach any of this code.

String or UUID changes made by this patch: none

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8723200 [details] [diff] [review]
Update graphite2 to upstream cset fb32e2e39ccd0980672f55052f75fc10f8736fb2 to pick up recent bugfixes

OK, thanks for the arguments. Taking it on all branches then.

Comment 25

[Jonathan Kew [:jfkthame]]

This has now been tagged as release 1.3.6 upstream; I'll update the patch accordingly, so that we're on an official release changeset.

(There are no additional code changes; the only difference is the bugfix field of the version number.)

Comment 26

[Carsten Book [:Tomcat]]

per irc conversation: Jonathan is waiting on sec approval and will land this on his own, so no sheriff uplift needed at that time now

Comment 27

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

Comment on attachment 8723200 [details] [diff] [review]
Update graphite2 to upstream cset fb32e2e39ccd0980672f55052f75fc10f8736fb2 to pick up recent bugfixes

sec-approval+

Comment 28

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0def34737ac7eb9d8260a20212fa963a04dd9ea9
Bug 1248876 - Update graphite2 to upstream release 1.3.6. r=jrmuizel

Comment 29

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f151e0b3b6ee
https://hg.mozilla.org/releases/mozilla-beta/rev/bd8994cd0148
https://hg.mozilla.org/releases/mozilla-esr38/rev/ec9cff7bb543

Comment 30

[Sylvestre Ledru [:Sylvestre]]

we did the m-b => m-r merge for desktop, it is needed on m-r too.

Comment 31

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

This missed the merge to m-r, so copied it over myself:
https://hg.mozilla.org/releases/mozilla-release/rev/86df68803c11

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0def34737ac7