1018234 - (CVE-2014-1551) Crash in ~FontTableRec() when called by :~gfxMathTable()

Status: VERIFIED FIXED

(Core :: MathML, defect)

Description

[James Kitchener (:jkitch)]

Attachment: testcase.diff

Apply the attached patch and run the MathML reftests.  The crash occurs sometime after the added reftest, when the test specific fonts expire.

The crash is similar to that recorded in bug 961365, but it appears different stuff happens during the call to ReleaseFontTable()

Comment 1

[Frédéric Wang (:fredw)]

@Jonathan: any idea?

Comment 2

[James Kitchener (:jkitch)]

Attachment: testcase

Oops, wrong testcase attached.

https://tbpl.mozilla.org/?tree=Try&rev=26553ed6bf70
Try run.  

Unfortunately the actual crash occurs in closed source DirectWrite code, which I am unable to debug.

scripts-3 has a different mFontFace to the stretchy font loaded in the previous test (both reference the same file).  The crash occurs at the first call to ~FontTableRec, which is deconstructing the same object as that created by gfxDWriteFontEntry::GetFontTable(HB_TAG('M','A','T','H')).   

(The pointers to mFontFace are identical, but I don't have the symbols to determine whether there are any internal changes to mFontFace between construction and destruction).  mContext is unchanged and appears to always be 0x0.

Comment 3

[James Kitchener (:jkitch)]

Attachment: patch

Within gfxDWriteFontEntry mFontFace is reference counted, yet it is passed to FontTableRecord which has no reference counting and seems to live independently of the gfxDWriteFontEntry which created it.  

Stepping through with a debugger, ~gfxDWriteFontEntry() occurs before ~FontTableRec() so it is possible that mFontFace is disposed of before FontTableRec is finished with it.

The attached patch seems to fix the crash for me, however I suggest you look closely to verify that it does what it is supposed to.

Comment 4

[James Kitchener (:jkitch)]

I suspect this to be a use-after-free, but I do not know if it is exploitable.

Comment 5

[Frédéric Wang (:fredw)]

Comment on attachment 8432330 [details] [diff] [review]
patch

Review of attachment 8432330 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks James. Can you move the crash test to layout/mathml/crashtests/?

Also, it seems that you didn't include any changes to the code.

Comment 6

[James Kitchener (:jkitch)]

Attachment: patch

It would help if I could attach the correct patch.

Comment 7

[James Kitchener (:jkitch)]

Attachment: patch

Test is now a crashtest.

See comment 3.

Please double check that I got the reference counting correct.

Comment 8

[James Kitchener (:jkitch)]

Attachment: patch

This now includes the problematic font

Comment 9

[Frédéric Wang (:fredw)]

Comment on attachment 8432350 [details] [diff] [review]
patch

Review of attachment 8432350 [details] [diff] [review]:
-----------------------------------------------------------------

The font still seems to be missing in this patch.

I let Jonathan check the refcounted change as I'm not familiar with this part of the code.

::: layout/mathml/crashtests/1018234-1.html
@@ +37,5 @@
> +
> +    <p>
> +      <math style="font-family: scripts-3;">
> +        <msqrt>
> +        <mspace/ width="5em">

the slash should be at then end of the tag.

Comment 10

[Frédéric Wang (:fredw)]

I also wonder whether the crash is only reproducible with a MATH font or if it could already happen with some testcase calling the SVGGlyphs code instead... That is if we need to take this patch for Gecko >= 31 only or for older versions too...

Comment 11

[James Kitchener (:jkitch)]

https://crash-stats.mozilla.com/report/index/0b79c1a3-4a46-46cc-91e9-2e9452140203

Crash from Firefox 26 with a very similar stack signature (apart from not the gfxMathTable destructor).  The code in question dates back to Fx24.

Comment 12

[Jonathan Kew [:jfkthame]]

Comment on attachment 8432350 [details] [diff] [review]
patch

Review of attachment 8432350 [details] [diff] [review]:
-----------------------------------------------------------------

It's clear the problem here is a bug in the gfxDWrite font backend's management of the font resources and tables, but I'm not yet sure whether this is the correct fix; I'm concerned it might be setting up a circular reference that will mean certain resources are never freed. I'll try to do some tracing and code-reading this afternoon to figure out what's going on and whether this is the right place to fix it.

Leaving r? pending for now, while I poke around...

::: layout/mathml/crashtests/1018234-1.html
@@ +16,5 @@
> +        src: url(./stretchy.otf);
> +      }
> +    </style>
> +    <script type="text/javascript">
> +    

nit: trailing whitespace

@@ +27,5 @@
> +
> +    <p>
> +      <math style="font-family: scripts-1;">
> +        <msub>
> +          <mspace height="2em" width="2em" mathbackground="blue"/> 

and here

::: layout/mathml/crashtests/crashtests.list
@@ +1,1 @@
> +load 1018234-1.html

I think it'd be preferable to put this at the end of the list - i.e. let's use numerical order rather than lexical.

Comment 13

[Jonathan Kew [:jfkthame]]

(And yes, it's very likely the same issue could affect the SVGGlyphs code, if the same SVG-in-OT font is instantiated multiple times simultaneously.)

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 8432350 [details] [diff] [review]
patch

Review of attachment 8432350 [details] [diff] [review]:
-----------------------------------------------------------------

OK, looks good to me - let's do it.

Comment 15

[James Kitchener (:jkitch)]

Attachment: patch

Comment 16

[James Kitchener (:jkitch)]

Green try:  (Code in question is Windows only).
https://tbpl.mozilla.org/?tree=Try&rev=aea2bb00f9b5

Comment 17

[Ryan VanderMeulen [:RyanVM]]

No sec rating and affects multiple branches = needs security approval to land. Especially with Fx30 going to RC today.
https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 8433299 [details] [diff] [review]
patch

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

I believe this can lead to a use-after-free of a DirectWrite font-face object, but as the DW code is closed-source, it's not clear to me how readily that might translate to an exploit rather than merely a crash.


Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The actual code change doesn't offer much clue, IMO.

The testcase shows how to trigger a crash; whether/how that could be extended to a more serious exploit is unknown. Still, we might want to land only the code patch at first, and defer landing the crashtest.

(The crashtest here depends on MathML OpenType support landed for mozilla-31. I suspect, though, that equivalent examples could probably be constructed for older Gecko versions using non-MathML codepaths.)


Which older supported branches are affected by this flaw? All (on Windows only)


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? The same code change should apply to all; minimal risk.


How likely is this patch to cause regressions; how much testing does it need? Seems unlikely. The only potential issue would be that DirectWrite font-face objects may live a bit longer, but AFAICT the effect is not significant - and anyway, ensuring these objects are not prematurely deleted is the whole point of the patch!

Comment 19

[James Kitchener (:jkitch)]

Attachment: code patch only (without test), for landing

Sorry about that.

The attached patch is for versions before v31.  The other patch won't apply cleanly as the font it copies is not present.  This patch removes the test (which won't detect the problem on those versions anyway).

> I believe this can lead to a use-after-free of a DirectWrite font-face
> object, but as the DW code is closed-source, it's not clear to me how
> readily that might translate to an exploit rather than merely a crash.

mFontFace->ReleaseFontTable() is a virtual method, so I suppose someone could overwrite the vtable.  No idea how feasible that is.

https://crash-stats.mozilla.com/report/index/2d9e99ac-808f-4dc7-967b-d13432140131
Here's a crash with reason EXCEPTION_ILLEGAL_INSTRUCTION.  Examples can also be found for EXCEPTION_ACCESS_VIOLATION_READ/WRITE/EXEC.  I don't have the experience to draw any conclusions from that though.

Comment 20

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

We aren't going to be able to take this on 30. The release builds are either made or in process. So this needs to go to 31 and later. 

We need to figure out a security rating though. A UAF is usually critical but given the lack of confidence in what is actually going on, it isn't clear that is appropriate as a rating.

Comment 21

[Karl Tomlinson (:karlt)]

Calling a virtual function on a deleted object is usually sec-critical
(unless IDWriteFontFace objects at least safely poison their memory on delete and the memory is only ever reallocated for objects with a similar vtable)
http://hg.mozilla.org/releases/mozilla-release/annotate/39faf812aaec/gfx/thebes/gfxDWriteFontList.cpp#l350
bp-0b79c1a3-4a46-46cc-91e9-2e9452140203

(In reply to James Kitchener (:jkitch) from comment #16)
> Green try:

We usually avoid using try or other public media for security bugs, or if necessary obfuscate the patch with other changes and a different bug number, and test without crashtests.  Fortunately, the particular proof-of-concept in the crashtest is only effective on Aurora.

Comment 22

[James Kitchener (:jkitch)]

Once again I apologise.

Comment 24

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

I'm giving sec-approal+ to check this in on June 24 or later (a couple of weeks into the next cycle).

Comment 25

[Mats Palmgren (inactive)]

(In reply to Al Billings [:abillings] from comment #24)
> I'm giving sec-approal+ to check this in on June 24 or later (a couple of
> weeks into the next cycle).

I think we should just land the fix, *not* the testcase, at that time.
Although the test was briefly exposed on Try, there's no reason to expose
it further.  The test should be landed at a later time when the bug
becomes public (as usual).

Comment 26

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

Ok. I agree with just landing the fix. We should hold the test until after release.

Comment 28

[Frédéric Wang (:fredw)]

(In reply to Al Billings [:abillings] from comment #26)
> Ok. I agree with just landing the fix. We should hold the test until after
> release.

Should we upload a new patch for the fix only, or can attachment 8433352 [details] [diff] [review] be taken for the trunk?

Comment 29

[Jonathan Kew [:jfkthame]]

Comment on attachment 8433352 [details] [diff] [review]
code patch only (without test), for landing

This should apply on trunk, as well as any branches where we decide to uplift this.

Comment 30

[Jonathan Kew [:jfkthame]]

Pushed the fix to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdb42fe85f9a

Comment 31

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

https://hg.mozilla.org/mozilla-central/rev/cdb42fe85f9a

Comment 32

[Lukas Blakk [:lsblakk] use ?needinfo]

Please nominate this for Beta/Aurora/ESR24 uplift.

Comment 33

[Jonathan Kew [:jfkthame]]

Comment on attachment 8433352 [details] [diff] [review]
code patch only (without test), for landing

Approval Request Comment

[Feature/regressing bug #]: longstanding bug in DirectWrite font code - may have been introduced by changes in bug 847344 (mozilla-24); I'm not sure if a similar flaw existed before then.

[User impact if declined]: Sporadic crash - generally occurs during deferred deletion of cached objects, so it'll appear rather random.

[Describe test coverage new/current, TBPL]: Existing unit tests exercise this font code heavily, but were not hitting the crash scenario, probably because it's too timing- and usage-pattern dependent. We now have a crashtest here, ready to land after the fix is pushed out. Crash-stats does not show any crashes with this signature since the fix landed on trunk, though the frequency was not really high enough on Nightly builds to make this definitive.

[Risks and why]: Low-risk patch - just adds a strong reference to avoid premature deletion of a DWrite object, no other change in behavior.

[String/UUID change made/needed]: none

Comment 34

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8433352 [details] [diff] [review]
code patch only (without test), for landing

As Lukas said, we want this patch to land in all branches.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/525ebf67bd8f
https://hg.mozilla.org/releases/mozilla-beta/rev/af9d52d05f74
https://hg.mozilla.org/releases/mozilla-esr24/rev/b32413094075

Comment 38

[Kamil Jozwiak [:kjozwiak]]

I couldn't reproduce the original issue from comment #0, I attempted the following:

Built m-c fx using changeset e86b84998b18 (June 23th, 2014):
- applied the patch from comment # 1
- ran "./mach reftest layout/reftests/mathml/reftest.list" and couldn't reproduce any crashes
- applied the patch from comment # 2
- ran "./mach reftest layout/reftests/mathml/reftest.list" and couldn't reproduce any crashes

Built aruora using changeset f118b45daada (June 23th, 2014):
- applied the patch from comment # 1
- ran "./mach reftest layout/reftests/mathml/reftest.list" and couldn't reproduce any crashes
- applied the patch from comment # 2
- ran "./mach reftest layout/reftests/mathml/reftest.list" and couldn't reproduce any crashes

I've also tried running the patch(s) against the latest changesets without any luck. James, am I missing something in the way I'm trying to reproduce? If so, could you please let me know how I can reproduce this? Maybe listing the original changeset that produced this issue?

Comment 39

[James Kitchener (:jkitch)]

Changeset e86b84998b18 crashes for me with the comment 2 testcase applied.

.mozconfig used:
ac_add_options --enable-debug
ac_add_options --disable-optimize

Opt builds are much faster, so a slightly different codepath is invoked (deletion is triggered by shutdown rather than by timeout). However crash reports indicate that the shutdown path can also cause the crashes.

This crash only occurs when DirectWrite is used, so it is limited to Windows.  It does not occur for Windows XP, Vista without the platform update, or when hardware acceleration is disabled.  (I have only tested it with 32 bit builds on 64 bit Windows 7 and 64 bit 8.1, so the requirements may be somewhat stricter).

Comment 40

[Daniel Veditz [:dveditz]]

Adding the in-testsuite? flag to remind us to land the test

Comment 41

[Kamil Jozwiak [:kjozwiak]]

Using the information from comment #39, I reproduced the crash consistently:

- using changeset e86b84998b18 without the patch completed the entire mathml reftest without any crashes
- using changeset e86b84998b18 and the patch from comment #2, the reftest crashes consistently (tried 5 different times with the same crash occurring)

Went through the following verification:

- fx33 [changeset: 7075808c3306] -> Passed without issues
- fx32 [changeset: dfeac3d29b5e] -> Passed without issues
- fx31 [changeset: 6befadcaa685] -> Passed without issues
- fx esr24 [changeset: eced3be3a20e] -> Passed without issues