Bug 1248876 (CVE-2016-1977)

Graphite2 Machine::Code::decoder::analysis::set_ref stack out of bounds bit set

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: hofusec, Assigned: jfkthame)

Tracking

({sec-critical})

unspecified
mozilla47
sec-critical
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox44 wontfix, firefox45 fixed, firefox46+ fixed, firefox47+ fixed, firefox-esr3845+ fixed, firefox-esr45 fixed)

Details

(Whiteboard: [adv-main45+][adv-esr38.7+])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
Created attachment 8720162 [details]
poc.zip
(Reporter)

Comment 2

3 years ago
Created attachment 8720163 [details]
asanlogpoc1.txt
(Reporter)

Comment 3

3 years ago
Created attachment 8720164 [details]
asanlogpoc2.txt
tyson: is this a dupe of any of yours? Might be two separate bugs since the symptoms are different.
Group: core-security → gfx-core-security
(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.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 6

3 years ago
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.
(Assignee)

Comment 7

3 years ago
(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

3 years ago
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.
(Assignee)

Comment 9

3 years ago
(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.
(Assignee)

Comment 10

3 years ago
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.
(Reporter)

Comment 11

3 years ago
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

3 years ago
There are two patches that I think fix this problem, The latter is 2b7f42152ec779c6f96ea744d83a7b7a6912473b which should reject the font.
(Assignee)

Comment 13

3 years ago
Created attachment 8722456 [details] [diff] [review]
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.
Attachment #8722456 - Flags: review?(jd.bugzilla)
(Assignee)

Updated

3 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED

Updated

3 years ago
Keywords: sec-high
(Assignee)

Comment 14

3 years ago
Created attachment 8723200 [details] [diff] [review]
Update graphite2 to upstream cset fb32e2e39ccd0980672f55052f75fc10f8736fb2 to pick up recent bugfixes
(Assignee)

Updated

3 years ago
Attachment #8722456 - Attachment is obsolete: true
Attachment #8722456 - Flags: review?(jd.bugzilla)
(Assignee)

Updated

3 years ago
Attachment #8723200 - Flags: review?(jmuizelaar)
Attachment #8723200 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 15

3 years ago
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.)
Attachment #8723200 - Flags: sec-approval?
Flags: needinfo?(sledru)
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.
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox47: --- → affected
status-firefox-esr38: --- → affected
status-firefox-esr45: --- → affected
tracking-firefox46: --- → ?
tracking-firefox47: --- → +
tracking-firefox-esr38: --- → ?
Flags: needinfo?(lhenry)
The patch is too big for the end of the cycle. Can we just cherry-pick the security fixes?
Flags: needinfo?(sledru)
Flags: needinfo?(jfkthame)
(Assignee)

Comment 18

3 years ago
(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.)
Flags: needinfo?(jfkthame)
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.
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.
tracking-firefox46: ? → +
Flags: needinfo?(lhenry)
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

3 years ago
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.
(Assignee)

Comment 23

3 years ago
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
Attachment #8723200 - Flags: approval-mozilla-esr38?
Attachment #8723200 - Flags: approval-mozilla-beta?
Attachment #8723200 - Flags: approval-mozilla-aurora?
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.
Attachment #8723200 - Flags: approval-mozilla-esr38?
Attachment #8723200 - Flags: approval-mozilla-esr38+
Attachment #8723200 - Flags: approval-mozilla-beta?
Attachment #8723200 - Flags: approval-mozilla-beta+
Attachment #8723200 - Flags: approval-mozilla-aurora?
Attachment #8723200 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 25

3 years ago
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.)
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 on attachment 8723200 [details] [diff] [review]
Update graphite2 to upstream cset fb32e2e39ccd0980672f55052f75fc10f8736fb2 to pick up recent bugfixes

sec-approval+
Attachment #8723200 - Flags: sec-approval? → sec-approval+
(Assignee)

Updated

3 years ago
status-firefox45: affected → fixed
status-firefox46: affected → fixed
status-firefox47: affected → fixed
status-firefox-esr38: affected → fixed
we did the m-b => m-r merge for desktop, it is needed on m-r too.
status-firefox45: fixed → affected
This missed the merge to m-r, so copied it over myself:
https://hg.mozilla.org/releases/mozilla-release/rev/86df68803c11
status-firefox45: affected → fixed
status-firefox44: --- → wontfix
status-firefox-esr45: affected → fixed
tracking-firefox-esr38: ? → 45+
Whiteboard: [adv-main45+][adv-esr38.7+]
Depends on: 1252311
https://hg.mozilla.org/mozilla-central/rev/0def34737ac7
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Group: gfx-core-security → core-security-release
Flags: sec-bounty?
Alias: CVE-2016-1977
Flags: sec-bounty? → sec-bounty+
Keywords: sec-high → sec-critical
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.